[Fwd: [PATCH] cr16-elf: position independent code (PIC) support]
Nick Clifton
nickc@redhat.com
Tue Nov 25 11:14:00 GMT 2008
Hi Swami,
> Could you please review the patch.
The patch looks very good. There are a few, minor points however:
> +/* Used to mark functions which have had redundant parts of their
> + prologue deleted. */
> +#define CR16_DELETED_PROLOGUE_BYTES 0x2
> + unsigned char flags;
Is there any particular reason for using 0x2 as the value of the deleted
prologue bytes flag ? Also is there any reason that you are using a
flag byte and binary value rather than a boolean or a bitfield ?
> + /* Random linker state flags. */
> +#define CR16_HASH_ENTRIES_INITIALIZED 0x1
> + char flags;
Similar questions.
> +#define elf32_cr16_link_hash_traverse(table, func, info) \
> + (elf_link_hash_traverse \
> + (&(table)->root, \
> + (bfd_boolean (*) PARAMS ((struct elf_link_hash_entry *, PTR))) (func), \
> + (info)))
The "PARAMS" and "PTR" macros are redundant. Please always use function
prototypes and the "void *" type instead. ie:
#define elf32_cr16_link_hash_traverse(table, func, info) \
elf_link_hash_traverse (&(table)->root, \
(bfd_boolean (*) (struct elf_link_hash_entry *, void *)) (func), \
(info))
This applies to the other uses of PTR in the patch as well...
> + /* Added or subtract the offset value */
Comment formatting, plus spelling typo. ie:
+ /* Add or subtract the offset value. */
Similarly:
> + /* Check for Ranga */
Should be:
+ /* Check range. */
> + //if ((long) value < 0x1fa && (long) value > -0x100) //REVISIT: range
> + if ((long) value < 0xfa && (long) value > -0x100)
Please do not use C++ style comments.
> +#if 0
> + /* Try to turn a 16bit immediate address into a 4bit
> + immediate address. */
> + if ((ELF32_R_TYPE (irel->r_info) == (int) R_CR16_IMM20)
Why is this section of code suppressed ? Is it unnecessary, or
something that is broken but which you intend to fix in the future ? If
the code is never going to be used then it should just be removed.
> + /* Set the contents of the .interp section to the interpreter. */
> + if (info->executable)
> + {
> +#if 0
> + s = bfd_get_section_by_name (dynobj, ".interp");
Similar questions.
> + + (h->got.offset &~ 1))
Spacing issue. The above line implies that "&~" is a C operator whereas
in fact it is just two operators juxtaposed. I would suggest this instead:
+ + (h->got.offset & ~1))
> + if (strneq (input_line_pointer, "@cGOT", 5)
> + || strneq (input_line_pointer, "@cgot", 5))
> + {
> + if (GOT_symbol == NULL)
> + GOT_symbol = symbol_find_or_make (GLOBAL_OFFSET_TABLE_NAME);
> +
> + symbol_with_at_gotc = 1;
> + }
> + else if (strneq (input_line_pointer, "@GOT", 4)
> + || strneq (input_line_pointer, "@got", 4))
You should document these new cr16 assembler operand qualifiers in
gas/doc/c-cr16.texi.
> - RELOC_NUMBER (R_CR16_ABS20, 12)
> - RELOC_NUMBER (R_CR16_ABS24, 13)
[snip]
> + RELOC_NUMBER (R_CR16_GOT_REGREL20, 12)
> + RELOC_NUMBER (R_CR16_GOTC_REGREL20, 13)
> + RELOC_NUMBER (R_CR16_ABS20, 14)
> + RELOC_NUMBER (R_CR16_ABS24, 15)
You are changing the numeric values of already established relocations.
This is a bad idea as it means that any CR16 object files or libraries
that use the old values will be incompatible with the updated linker and
with object files/libraries that use the new values. It would be better
to just add the new R_CR16_GOT... relocs to the end of the current list
of relocs.
Cheers
Nick
More information about the Binutils
mailing list