Re: [Fwd: [PATCH] cr16-elf: position independent code (PIC) support]

[Nick Clifton <nickc at redhat dot com>]

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