[PATCH v4] gold: Add Mips64 support.

[Cary Coutant]

>         * mips.cc: Add Mips64 support.

The ChangeLog should include entries for each new class or function,
and for each changed function. A blanket note like this is used only
when the entire source file is brand new.

>         * output.h (Output_reloc<SHT_REL>::get_address): Change from private to public.
>         (Output_reloc<SHT_REL>::get_symbol_index): Likewise.
>         (Output_data_reloc_base::Sort_relocs_comparison): Change from private to protected.
>         (Output_data_reloc_base::relocs_): Likewise.


+  if (section_type == elfcpp::SHT_NOBITS)
+    return false;
+
+  if (section_flags & elfcpp::SHF_WRITE)
+    return false;

Just out of curiosity... Why would a NOBITS section ever not have
SHF_WRITE set? Is there any reason to test for NOBITS?


+  // Whether this is a STT_SECTION symbol.
+  bool is_section_symbol_;

I suggest moving is_section_symbol_ above shndx_ (or moving shndx_
above tls_type_) to avoid an extra 8 bytes of padding.


  protected:
+  // The name of the options section.
+  const char* mips_elf_options_section_name()
+  { return this->is_newabi() ? ".MIPS.options" : ".options"; }

This should probably be a private method.


+       if (!this->target_->is_output_n64())
+         p->write(pov);
+       else
+         {
+           elfcpp::Mips64_rel_write<big_endian> orel(pov);
+           orel.put_r_offset(p->get_address());
+           orel.put_r_sym(p->get_symbol_index());
+           orel.put_r_ssym(0);
+           orel.put_r_type(p->type());
+           if (p->type() == elfcpp::R_MIPS_REL32)
+             orel.put_r_type2(elfcpp::R_MIPS_64);
+           else
+             orel.put_r_type2(elfcpp::R_MIPS_NONE);
+           orel.put_r_type3(elfcpp::R_MIPS_NONE);
+         }

As I understand it, N64 is the *only* supported 64-bit ABI. A much
less expensive test would be "if (size != 64)", which can be evaluated
at compile time when the template is instantiated. With that change,
you wouldn't need to pass "target" to the Mips_output_data_reloc
constructor.

I wasn't happy with the fact that you had to copy the implementation
of Output_data_reloc_base::do_write(), so I tried a few different
approaches, and finally settled on factoring out the implementation of
that function into a template called
do_write_generic<Output_reloc_writer>(Output_file*). The standard
implementation of do_write() uses a simple Output_reloc_writer class
that simply calls Output_reloc::write() to do the work. (This is all
inlined, so the extra abstraction costs nothing.) With that change,
the Mips_output_data_reloc class can become this:

++// MIPS-specific relocation writer.
++
++template<int sh_type, bool dynamic, int size, bool big_endian>
++struct Mips_output_reloc_writer;
++
++template<int sh_type, bool dynamic, bool big_endian>
++struct Mips_output_reloc_writer<sh_type, dynamic, 32, big_endian>
++{
++  typedef Output_reloc<sh_type, dynamic, 32, big_endian> Output_reloc_type;
++  typedef std::vector<Output_reloc_type> Relocs;
++
++  static void
++  write(typename Relocs::const_iterator p, unsigned char* pov)
++  { p->write(pov); }
++};
++
++template<int sh_type, bool dynamic, bool big_endian>
++struct Mips_output_reloc_writer<sh_type, dynamic, 64, big_endian>
++{
++  typedef Output_reloc<sh_type, dynamic, 64, big_endian> Output_reloc_type;
++  typedef std::vector<Output_reloc_type> Relocs;
++
++  static void
++  write(typename Relocs::const_iterator p, unsigned char* pov)
++  {
++    elfcpp::Mips64_rel_write<big_endian> orel(pov);
++    orel.put_r_offset(p->get_address());
++    orel.put_r_sym(p->get_symbol_index());
++    orel.put_r_ssym(0);
++    orel.put_r_type(p->type());
++    if (p->type() == elfcpp::R_MIPS_REL32)
++      orel.put_r_type2(elfcpp::R_MIPS_64);
++    else
++      orel.put_r_type2(elfcpp::R_MIPS_NONE);
++    orel.put_r_type3(elfcpp::R_MIPS_NONE);
++  }
++};
++
++template<int sh_type, bool dynamic, int size, bool big_endian>
++class Mips_output_data_reloc : public Output_data_reloc<sh_type, dynamic,
++                                                        size, big_endian>
++{
++ public:
++  Mips_output_data_reloc(bool sort_relocs)
++    : Output_data_reloc<sh_type, dynamic, size, big_endian>(sort_relocs)
++  { }
++
++ protected:
++  // Write out the data.
++  void
++  do_write(Output_file* of)
++  {
++    typedef Mips_output_reloc_writer<sh_type, dynamic, size,
big_endian> Writer;
++    this->template do_write_generic<Writer>(of);
++  }
++};

I've committed that change along with your patch to output.h that
moves Output_reloc::get_address() and Output_reloc::get_symbol_index()
to the public interface (see separate email).


   put_r_info(Reltype_write* new_reloc, Reltype* reloc, unsigned int r_sym)
   {
     unsigned int r_type = elfcpp::elf_r_type<32>(reloc->get_r_info());
-    new_reloc->put_r_info(elfcpp::elf_r_info<64>(r_sym, r_type));
+    new_reloc->put_r_info(elfcpp::elf_r_info<32>(r_sym, r_type));

Is this a bug fix to the 32-bit support? If so, it should probably go
in separately. (That patch is preapproved.)


+    ei_class_ = size == 64 ? elfcpp::ELFCLASS64 : elfcpp::ELFCLASS32;

I don't see the point of this data member -- it will always track the
"size" template parameter for the target. Since Target_mips<32,...>
and Target_mips<64,...> are distinct targets, you will never be called
upon to merge them, and merge_processor_specific_flags() will never
see an inconsistency (since a target mismatch will have already been
detected in make_elf_sized_object).


   rel16(unsigned char* view, const Mips_relobj<size, big_endian>* object,
         const Symbol_value<size>* psymval, Mips_address addend_a,
+        bool extract_addend, unsigned int r_type, bool calculate_only,
+        Valtype &calculated_value)

In gold, we try to adhere to a convention where reference parameters
are always const. If you're going to modify a reference parameter, it
should be a pointer. (Also, space goes *after* the '&'.)


+    if (calculate_only)
+      calculated_value = x;
+    else
+      elfcpp::Swap<16, big_endian>::writeval(wv, val);
+
     mips_reloc_shuffle(view, r_type, false);

Do you really want to call mips_reloc_shuffle() if calculate_only is
true? If so, a comment would help, but it seems to me that since you
haven't actually written anything to the view yet, it's not really
doing anything useful.


-    return (Bits<16>::has_overflow32(x)
-            ? This::STATUS_OVERFLOW
-            : This::STATUS_OKAY);
+    return check_overflow<16>(x);

I observe that the returned status is never used if calculate_only is
true (which makes sense). Perhaps you just want to return immediately
after calculating the intermediate result rather than waste time
checking for overflow.


-  0x8df90000,           // l[wd] $25, %lo(.got.plt entry)($15)
+  0x01f90000,           // l[wd] $25, %lo(.got.plt entry)($15)

As above, if this is a bug fix for 32-bit, it should go in separately
(also preapproved).


I see the following pattern in several places:

+  if (sh_type == elfcpp::SHT_REL)
+    {
+      ...
+    }
+  else if (sh_type == elfcpp::SHT_RELA)
+    {
+      ...
+    }

I suggest adding:

++  else
++    gold_unreachable();


+  // The first operation in a record which references a symbol uses the symbol
+  // implied by r_sym. The next operation in a record which references a symbol
+  // uses the special symbol value given by the r_ssym field. A third operation
+  // in a record which references a symbol will assume a NULL symbol,
+  // i.e. value zero.

This comment (which agrees with what I read in the psABI) does not
describe what the code below it does. It looks to me like the first
relocation (r_types[0]) always uses r_sym, the second (r_types[1])
always uses r_ssym, and the third (r_types[2]) always uses NULL.
Perhaps a TODO comment is needed here?

-cary