Re: GOLD: RFA: Add support for RX target

[Ian Lance Taylor <ian at airs dot com>]

Nick Clifton <nickc@redhat.com> writes:

> ./ChangeLog
> 2010-12-13  Nick Clifton  <nickc@redhat.com>
>
> 	* configure.ac: Add RX to list of targets for which
> 	--enable-gold is valid.
>         * configure: Regenerate.
>
> elfcpp/ChangeLog
> 2010-12-13  Nick Clifton  <nickc@redhat.com>
>
> 	* rx.h: New file.  Contains ELF definitions specific to the
> 	Renesas RX architecture.
>         * elfcpp.h (enum EM): Add EM_RX.
>
> gold/ChangeLog
> 2010-12-13  Nick Clifton  <nickc@redhat.com>
>
> 	* rx.cc: New file.  Adds support for Renesas RX architecture.
>         * Makefile.am (TARGETSOURCES): Add rx.cc.
>         (ALL_TARGETOBJS): Add rx.$(OBJEXT)
>         * Makefile.in: Regenerate.
>         * configure.ac: Add RX as a default target.
>         * configure: Regenerate.
>         * configure.tgt: Add rx-*-* as a supported target.


> + #include <cstdio>
> + #include <cstring>
> + #include <limits>
> + #include <cstdio>
> + #include <string>
> + #include <algorithm>
> + #include <map>
> + #include <utility>
> + #include <set>

You only have to #include <cstdio> once.


> + inline bool
> + Target_rx::Relocate::relocate(
> +     const Relocate_info<32, false>*  relinfo,
> +     Target_rx*                       target,
> +     Output_section *                 /* output_section */,
> +     size_t                           relnum,
> +     const elfcpp::Rela<32, false>&   rela,
> +     unsigned int                     r_type,
> +     const Sized_symbol<32>*          /* gsym */,
> +     const Symbol_value<32>*          psymval,
> +     unsigned char*                   view,
> +     RX_address                       address,
> +     section_size_type                /* view_size */)
> + {
> +   typedef RX_relocation_functions Reloc;
> +   elfcpp::Elf_Xword addend = rela.get_r_addend();
> + 
> +   if (target->uses_bigendian_data ())

s/ ()/()/


> + void
> + Target_rx::relocate_section(
> +     const Relocate_info<32, false>*  relinfo,
> +     unsigned int                     sh_type,
> +     const unsigned char*             prelocs,
> +     size_t                           reloc_count,
> +     Output_section*                  output_section,
> +     bool                             needs_special_offset_handling,
> +     unsigned char*                   view,
> +     RX_address                       address,
> +     section_size_type                view_size,
> +     const Reloc_symbol_changes*      reloc_symbol_changes)
> + {
> +   typedef Target_rx::Relocate Rx_relocate;
> + 
> +   gold_assert(sh_type == elfcpp::SHT_RELA);
> + 
> +   // See if we are relocating a relaxed input section.  If so, the view
> +   // covers the whole output section and we need to adjust accordingly.
> +   if (needs_special_offset_handling)
> +     {
> +       const Output_relaxed_input_section* poris =
> + 	output_section->find_relaxed_input_section(relinfo->object,
> + 						   relinfo->data_shndx);
> +       if (poris != NULL)
> + 	{
> + 	  RX_address section_address = poris->address();
> + 	  section_size_type section_size = poris->data_size();
> + 
> + 	  gold_assert((section_address >= address)
> + 		      && ((section_address + section_size)
> + 			  <= (address + view_size)));
> + 
> + 	  off_t offset = section_address - address;
> + 	  view += offset;
> + 	  address += offset;
> + 	  view_size = section_size;
> + 	}
> +     }
> + 
> +   if (uses_bigendian_data ())

This line should be
  if (this->uses_bigendian_data())

> +     {
> +       // This is a copy of the relocate_section() function in target-reloc.h,
> +       // except with the additional calls to convert_host to fix up the
> +       // extracted information.
> +       // FIXME: There ought to be a better way to do this.

Yes, there ought to be a better way to do this.  If your code is
correct, then it looks like the problem is that you have a little-endian
object but the reloc fields are written with big-endian data.  The
comment at the top of the file suggests that the symbol table is also
written with big-endian data, in which case I think this code is
insufficient.

Anyhow, if the reloc fields are consistenly written big-endian, then the
right approach is not to call convert_host, but instead to change

> +       typedef Reloc_types<elfcpp::SHT_RELA, 32, false>::Reloc Reltype;

to

> +       typedef Reloc_types<elfcpp::SHT_RELA, 32, true>::Reloc Reltype;

That will cause the reloc code to fetch values big-endian rather than
little-endian.

Now, if you need to have a little-endian Object with a big-endian
Reltype, then I think the right approach is to split relocate_section in
target-reloc.h so that it calls another template which takes, as a
parameter, the reloc type.  The normal case would instantiate this
template with Reloc_types<sh_type, size, big_endian>::Reloc.  The RX
case would call the new template with Reloc_types<sh_type, size,
true>::Reloc.

However, I'm not sure this is the right approach, because I'm not sure
it will handle the symbol table correctly.  It seems to me that you are
going to need use a big-endian Object rather than a little-endian
Object.  And if you do that, you might as well templatize the whole
Target, as Target_arm does.  Then instead of worrying about
relocate_section, you would handle the required conversions entirely in
the Relocate class.


> + // Process relocations for gc.
> + 
> + void
> + Target_rx::gc_process_relocs(Symbol_table*             /*symtab*/,
> + 			     Layout*                   /*layout*/,
> + 			     Sized_relobj<32, false>*  /*object*/,
> + 			     unsigned int              /*data_shndx*/,
> + 			     unsigned int              ,
> + 			     const unsigned char*      /*prelocs*/,
> + 			     size_t                    /*reloc_count*/,
> + 			     Output_section*           /*output_section*/,
> + 			     bool                      /*needs_special_offset_handling*/,
> + 			     size_t                    /*local_symbol_count*/,
> + 			     const unsigned char*      /*plocal_symbols*/)
> + {
> + #if 0
> +   typedef Target_rx RX;
> +   typedef Target_rx::Scan Scan;
> + 
> +   gold::gc_process_relocs<32, false, RX, elfcpp::SHT_RELA, Scan>
> +     (symtab, layout, this, object, data_shndx, prelocs,
> +      reloc_count, output_section, needs_special_offset_handling,
> +      local_symbol_count, plocal_symbols);
> + #endif
> + }

Should probably give an error here for now.

> + static const char *
> + flag_meaning(elfcpp::Elf_Word flags, elfcpp::Elf_Word other_flags)

I would prefer that this be a static function in Target_rx rather than a
file static function.  It amounts to the same thing in the end.

> + {
> +   switch (flags)
> +     {
> +     case elfcpp::E_FLAG_RX_64BIT_DOUBLES:
> +       return "uses 64-bit doubles";
> +     case elfcpp::E_FLAG_RX_DSP:
> +       return "uses DSP instructions";
> +     case elfcpp::E_FLAG_RX_64BIT_DOUBLES | elfcpp::E_FLAG_RX_DSP:
> +       switch (other_flags)
> + 	{
> + 	case 0:
> + 	  return "uses 64-bit doubles & DSP instructions";
> + 	case elfcpp::E_FLAG_RX_64BIT_DOUBLES:
> + 	  return "uses DSP instructions";
> + 	case elfcpp::E_FLAG_RX_DSP:
> + 	  return "uses 64-bit doubles";
> + 	default:
> + 	  gold_unreachable();

If I understand this correctly, this should not be gold_unreachable,
because the object file could contain any random data.  It should print
an error about unrecognized flags, or something like that, rather than
crash the linker.

> + 	}
> +     case 0:
> +       switch (other_flags)
> + 	{
> + 	case elfcpp::E_FLAG_RX_64BIT_DOUBLES | elfcpp::E_FLAG_RX_DSP:
> + 	case elfcpp::E_FLAG_RX_64BIT_DOUBLES:
> + 	  return "uses 32-bit doubles";
> + 	case elfcpp::E_FLAG_RX_DSP:
> + 	  return "synthesies DSP instructions";
> + 	default:
> + 	  gold_unreachable();
> + 	}
> +     default:
> +       gold_unreachable();

Same for these two calls to gold_unreachable.

> + bool
> + Target_rx::check_type_and_flags (int type, elfcpp::Elf_Word flags, const char * name)

s/ (/(/
s/char */char*/

Also the line might be too long.

> + {
> +   static const char * saved_name = NULL;

s/char */char*/

However, don't use a static variable here.  It should be a member of
Target_rx instead.

> +   if (are_processor_specific_flags_set())

Write as
    if (this->are_processor_specific_flags_set())

> +     {
> +       static elfcpp::Elf_Word previous_flags = -1;

Don't use a static variable here.  It should be a member of Target_rx
instead.

> +       elfcpp::Elf_Word set_flags = processor_specific_flags();

Use "this->".

> +       if (set_flags != flags
> + 	  && previous_flags != flags)
> + 	{
> + 	  // Make sure that we do not complain about this particular
> + 	  // discrepancy more than once in a row.
> + 	  previous_flags = flags;
> + 
> + 	  gold_error(_("%s: %s whereas %s does not"),	
> + 		     name, flag_meaning(flags, set_flags), saved_name);

This looks kind of hard to internationalize correctly.

> +   else if (flags)
> +     {
> +       set_processor_specific_flags(flags);

Use "this->".

> + // Use do_make_elf_object to override the same function in the base class.
> + // We need to checks the ELF header flags before allowing the object to be
> + // made.
> + Object*
> + Target_rx::do_make_elf_object(const std::string&              name,
> + 			      Input_file*                     input_file,
> + 			      off_t                           offset,
> + 			      const elfcpp::Ehdr<32, false>&  ehdr)
> + {
> +   if (! check_type_and_flags (ehdr.get_e_type(),
> + 			      ehdr.get_e_flags(),
> + 			      name.c_str ()))
> +     return NULL;

s/! check_type_and_flags/!this->check_type_and_flags/

> + 
> +   Sized_relobj<32, false>* obj =
> +     new Sized_relobj<32, false>(name, input_file, offset, ehdr);
> +   obj->setup();
> +   return obj;
> + }
> + 
> + Object*
> + Target_rx::do_make_elf_object(const std::string&             name,
> + 			      Input_file*                    input_file,
> + 			      off_t                          offset,
> + 			      const elfcpp::Ehdr<32, true>&  ehdr)
> + {
> +   if (! check_type_and_flags (ehdr.get_e_type(),
> + 			      ehdr.get_e_flags(),
> + 			      name.c_str ()))
> +     return NULL;

s/! check_type_and_flags/!this->check_type_and_flags/

> + 
> +   Sized_relobj<32, true>* obj =
> +     new Sized_relobj<32, true>(name, input_file, offset, ehdr);
> +   obj->setup();
> +   return obj;
> + }

Hmmm, I see, so you do have a big-endian Sized_relobj.  But
Target_rx::relocate_section takes a little-endian Relocate_info.  I
guess I don't see how you are avoiding a type error when
Sized_relobj<size, big_endian>::do_relocate_sections calls
target->relocate_section.

Ian