[PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Vladimir Radosavljevic]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

This patch adds support for more relocations in relocatable link.  It also
introduces get_lo16_rel_addend method for finding partnering LO16 relocation
because there is a problem with current implementation when using --threads
option.

Regards,
Vladimir

ChangeLog -

	PR gold/21152
	* target.h (Sized_target::relocate_special_relocatable): Add
	reloc_count parameter.
	* arm.cc (Target_arm::relocate_special_relocatable): Likewise.
	* target-reloc.h (relocate_relocs): Pass reloc_count to
	relocate_special_relocatable.
	* mips.cc (Mips_scan_relocatable_relocs::local_section_strategy):
	Return RELOC_SPECIAL for more relocations.
	(Symbol_visitor_check_symbols::operator()): Check for is_output_pic
	rather then checking output_is_position_independent option.
	(Target_mips::is_output_pic): New method.
	(Mips_relocate_functions::get_lo16_rel_addend): Likewise.
	(Target_mips::set_gp): Add case for relocatable link.
	(Target_mips::relocate_special_relocatable): Add reloc_count
	parameter.  Add support for RELA type of relocation sections.
	Add support for more relocations.  Remove unused code.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: relocatable.patch --]
[-- Type: text/x-patch; name="relocatable.patch", Size: 17703 bytes --]

diff --git a/gold/arm.cc b/gold/arm.cc
index ff472ea..c675065 100644
--- a/gold/arm.cc
+++ b/gold/arm.cc
@@ -2341,6 +2341,7 @@ class Target_arm : public Sized_target<32, big_endian>
 			       unsigned int sh_type,
 			       const unsigned char* preloc_in,
 			       size_t relnum,
+			       size_t reloc_count,
 			       Output_section* output_section,
 			       typename elfcpp::Elf_types<32>::Elf_Off
                                  offset_in_output_section,
@@ -10428,6 +10429,7 @@ Target_arm<big_endian>::relocate_special_relocatable(
     unsigned int sh_type,
     const unsigned char* preloc_in,
     size_t relnum,
+    size_t,
     Output_section* output_section,
     typename elfcpp::Elf_types<32>::Elf_Off offset_in_output_section,
     unsigned char* view,
diff --git a/gold/mips.cc b/gold/mips.cc
index 95bf6db..52edeac 100644
--- a/gold/mips.cc
+++ b/gold/mips.cc
@@ -2858,12 +2858,45 @@ class Mips_scan_relocatable_relocs :
   local_section_strategy(unsigned int r_type, Relobj* object)
   {
     if (Classify_reloc::sh_type == elfcpp::SHT_RELA)
-      return Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA;
+      {
+        switch (r_type)
+          {
+          case elfcpp::R_MIPS_GPREL32:
+          case elfcpp::R_MIPS_GPREL16:
+          case elfcpp::R_MIPS_LITERAL:
+          case elfcpp::R_MICROMIPS_GPREL16:
+          case elfcpp::R_MICROMIPS_GPREL7_S2:
+          case elfcpp::R_MICROMIPS_LITERAL:
+          case elfcpp::R_MIPS16_GPREL:
+            return Relocatable_relocs::RELOC_SPECIAL;
+
+          default:
+            return Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA;
+          }
+      }
     else
       {
         switch (r_type)
           {
           case elfcpp::R_MIPS_26:
+          case elfcpp::R_MIPS_HI16:
+          case elfcpp::R_MIPS_LO16:
+          case elfcpp::R_MIPS_GOT16:
+          case elfcpp::R_MIPS_GPREL32:
+          case elfcpp::R_MIPS_GPREL16:
+          case elfcpp::R_MIPS_LITERAL:
+          case elfcpp::R_MICROMIPS_26_S1:
+          case elfcpp::R_MICROMIPS_HI16:
+          case elfcpp::R_MICROMIPS_LO16:
+          case elfcpp::R_MICROMIPS_GOT16:
+          case elfcpp::R_MICROMIPS_GPREL16:
+          case elfcpp::R_MICROMIPS_GPREL7_S2:
+          case elfcpp::R_MICROMIPS_LITERAL:
+          case elfcpp::R_MIPS16_26:
+          case elfcpp::R_MIPS16_HI16:
+          case elfcpp::R_MIPS16_LO16:
+          case elfcpp::R_MIPS16_GOT16:
+          case elfcpp::R_MIPS16_GPREL:
             return Relocatable_relocs::RELOC_SPECIAL;
 
           default:
@@ -3048,7 +3081,7 @@ class Symbol_visitor_check_symbols
         // stub.
         if (parameters->options().relocatable())
           {
-            if (!parameters->options().output_is_position_independent())
+            if (!this->target_->is_output_pic())
               mips_sym->set_pic();
           }
         else if (mips_sym->has_nonpic_branches())
@@ -3411,6 +3444,7 @@ class Target_mips : public Sized_target<size, big_endian>
                                unsigned int sh_type,
                                const unsigned char* preloc_in,
                                size_t relnum,
+                               size_t reloc_count,
                                Output_section* output_section,
                                typename elfcpp::Elf_types<size>::Elf_Off
                                  offset_in_output_section,
@@ -3596,6 +3630,11 @@ class Target_mips : public Sized_target<size, big_endian>
   is_output_n64() const
   { return size == 64; }
 
+  // Whether the output contains position independent code.
+  bool
+  is_output_pic() const
+  { return (this->processor_specific_flags() & elfcpp::EF_MIPS_PIC) != 0; }
+
   // Whether the output uses NEWABI.  This is valid only after
   // merge_obj_e_flags() is called.
   bool
@@ -4294,6 +4333,52 @@ class Mips_relocate_functions : public Relocate_functions<size, big_endian>
   }
 
  public:
+  // Find partnering LO16 relocation and extract addend from the instruction.
+  // Return true on success or false if the LO16 could not be found.
+
+  static bool
+  get_lo16_rel_addend(unsigned int sh_type, const unsigned char* prelocs,
+                      size_t relnum, size_t reloc_count,
+                      unsigned int hi16_r_type, unsigned int hi16_r_sym,
+                      unsigned char* view, Mips_address* addend)
+  {
+    gold_assert(sh_type == elfcpp::SHT_REL);
+
+    typedef typename Mips_reloc_types<elfcpp::SHT_REL, size, big_endian>::Reloc
+        Reltype;
+    const int reloc_size =
+        Mips_classify_reloc<elfcpp::SHT_REL, size, big_endian>::reloc_size;
+
+    // Start finding lo16 part from the next relocation.
+    prelocs += reloc_size;
+    for (size_t i = relnum + 1; i < reloc_count; ++i, prelocs += reloc_size)
+      {
+        Reltype reloc(prelocs);
+        unsigned int r_sym = Mips_classify_reloc<elfcpp::SHT_REL, size,
+                                                 big_endian>::get_r_sym(&reloc);
+        unsigned int r_type = Mips_classify_reloc<elfcpp::SHT_REL, size,
+                                                  big_endian>::
+                                                  get_r_type(&reloc);
+
+        if (hi16_r_sym == r_sym
+            && is_matching_lo16_reloc(hi16_r_type, r_type))
+          {
+            Mips_address offset = reloc.get_r_offset();
+            view += offset;
+
+            mips_reloc_unshuffle(view, r_type, false);
+            Valtype32* wv = reinterpret_cast<Valtype32*>(view);
+            Valtype32 val = elfcpp::Swap<32, big_endian>::readval(wv);
+            mips_reloc_shuffle(view, r_type, false);
+
+            *addend = Bits<16>::sign_extend32(val & 0xffff);
+            return true;
+          }
+      }
+
+    return false;
+  }
+
   //   R_MIPS16_26 is used for the mips16 jal and jalx instructions.
   //   Most mips16 instructions are 16 bits, but these instructions
   //   are 32 bits.
@@ -8438,6 +8523,23 @@ Target_mips<size, big_endian>::set_gp(Layout* layout, Symbol_table* symtab)
                                       0, false, false));
       this->gp_ = gp;
     }
+
+  if (parameters->options().relocatable())
+    {
+      // If gp is NULL, set it to the default value.
+      if (gp == NULL)
+        gp = static_cast<Sized_symbol<size>*>(symtab->define_as_constant(
+                                      "_gp", NULL, Symbol_table::PREDEFINED,
+                                      MIPS_GP_OFFSET, 0,
+                                      elfcpp::STT_OBJECT,
+                                      elfcpp::STB_GLOBAL,
+                                      elfcpp::STV_DEFAULT,
+                                      0, false, false));
+      // Don't add _gp to the final symtab, because the value of the _gp symbol
+      // will be stored into .reginfo/.MIPS.options section.
+      gp->set_symtab_index(-1U);
+      this->gp_ = gp;
+    }
 }
 
 // Set the dynamic symbol indexes.  INDEX is the index of the first
@@ -8576,7 +8678,6 @@ Target_mips<size, big_endian>::make_plt_entry(Symbol_table* symtab,
   this->plt_->add_entry(gsym, r_type);
 }
 
-
 // Get the .MIPS.stubs section, creating it if necessary.
 
 template<int size, bool big_endian>
@@ -10221,35 +10322,48 @@ Target_mips<size, big_endian>::relocate_special_relocatable(
     unsigned int sh_type,
     const unsigned char* preloc_in,
     size_t relnum,
+    size_t reloc_count,
     Output_section* output_section,
     typename elfcpp::Elf_types<size>::Elf_Off offset_in_output_section,
     unsigned char* view,
-    Mips_address view_address,
+    Mips_address,
     section_size_type,
     unsigned char* preloc_out)
 {
-  // We can only handle REL type relocation sections.
-  gold_assert(sh_type == elfcpp::SHT_REL);
-
-  typedef typename Reloc_types<elfcpp::SHT_REL, size, big_endian>::Reloc
-    Reltype;
-  typedef typename Reloc_types<elfcpp::SHT_REL, size, big_endian>::Reloc_write
-    Reltype_write;
-
   typedef Mips_relocate_functions<size, big_endian> Reloc_funcs;
-
   const Mips_address invalid_address = static_cast<Mips_address>(0) - 1;
 
   Mips_relobj<size, big_endian>* object =
     Mips_relobj<size, big_endian>::as_mips_relobj(relinfo->object);
   const unsigned int local_count = object->local_symbol_count();
 
-  Reltype reloc(preloc_in);
-  Reltype_write reloc_write(preloc_out);
+  unsigned int r_sym;
+  unsigned int r_type;
+  Mips_address r_addend;
+  Mips_address offset;
 
-  elfcpp::Elf_types<32>::Elf_WXword r_info = reloc.get_r_info();
-  const unsigned int r_sym = elfcpp::elf_r_sym<size>(r_info);
-  const unsigned int r_type = elfcpp::elf_r_type<size>(r_info);
+  if (sh_type == elfcpp::SHT_RELA)
+    {
+      const Relatype rela(preloc_in);
+      offset = rela.get_r_offset();
+      r_sym = Mips_classify_reloc<elfcpp::SHT_RELA, size, big_endian>::
+          get_r_sym(&rela);
+      r_type = Mips_classify_reloc<elfcpp::SHT_RELA, size, big_endian>::
+          get_r_type(&rela);
+      r_addend = rela.get_r_addend();
+    }
+  else if (sh_type == elfcpp::SHT_REL)
+    {
+      const Reltype rel(preloc_in);
+      offset = rel.get_r_offset();
+      r_sym = Mips_classify_reloc<elfcpp::SHT_REL, size, big_endian>::
+          get_r_sym(&rel);
+      r_type = Mips_classify_reloc<elfcpp::SHT_REL, size, big_endian>::
+          get_r_type(&rel);
+      r_addend = 0;
+    }
+  else
+    gold_unreachable();
 
   // Get the new symbol index.
   // We only use RELOC_SPECIAL strategy in local relocations.
@@ -10270,7 +10384,6 @@ Target_mips<size, big_endian>::relocate_special_relocatable(
   // Get the new offset--the location in the output section where
   // this relocation should be applied.
 
-  Mips_address offset = reloc.get_r_offset();
   Mips_address new_offset;
   if (offset_in_output_section != invalid_address)
     new_offset = offset + offset_in_output_section;
@@ -10285,19 +10398,6 @@ Target_mips<size, big_endian>::relocate_special_relocatable(
       new_offset = new_sot_offset;
     }
 
-  // In an object file, r_offset is an offset within the section.
-  // In an executable or dynamic object, generated by
-  // --emit-relocs, r_offset is an absolute address.
-  if (!parameters->options().relocatable())
-    {
-      new_offset += view_address;
-      if (offset_in_output_section != invalid_address)
-        new_offset -= offset_in_output_section;
-    }
-
-  reloc_write.put_r_offset(new_offset);
-  reloc_write.put_r_info(elfcpp::elf_r_info<32>(new_symndx, r_type));
-
   // Handle the reloc addend.
   // The relocation uses a section symbol in the input file.
   // We are adjusting it to use a section symbol in the output
@@ -10306,23 +10406,127 @@ Target_mips<size, big_endian>::relocate_special_relocatable(
   // file to refer to that same address.  This adjustment to
   // the addend is the same calculation we use for a simple
   // absolute relocation for the input section symbol.
-  Valtype calculated_value = 0;
+  Valtype x = 0;
   const Symbol_value<size>* psymval = object->local_symbol(r_sym);
-
+  bool extract_addend = sh_type == elfcpp::SHT_REL;
   unsigned char* paddend = view + offset;
   typename Reloc_funcs::Status reloc_status = Reloc_funcs::STATUS_OKAY;
+
+  Reloc_funcs::mips_reloc_unshuffle(paddend, r_type, false);
   switch (r_type)
     {
     case elfcpp::R_MIPS_26:
-      reloc_status = Reloc_funcs::rel26(paddend, object, psymval,
-          offset_in_output_section, true, 0, sh_type == elfcpp::SHT_REL, NULL,
-          false /*TODO(sasa): cross mode jump*/, r_type, this->jal_to_bal(),
-          false, &calculated_value);
+    case elfcpp::R_MICROMIPS_26_S1:
+    case elfcpp::R_MIPS16_26:
+      gold_assert(extract_addend);
+      reloc_status = Reloc_funcs::rel26(paddend, object, psymval, new_offset,
+                                        true, 0, true, NULL, false, r_type,
+                                        false, false, &x);
+      break;
+
+    case elfcpp::R_MIPS_HI16:
+    case elfcpp::R_MIPS_GOT16:
+    case elfcpp::R_MICROMIPS_HI16:
+    case elfcpp::R_MICROMIPS_GOT16:
+    case elfcpp::R_MIPS16_HI16:
+    case elfcpp::R_MIPS16_GOT16:
+      {
+        gold_assert(extract_addend);
+        Valtype32* wv = reinterpret_cast<Valtype32*>(paddend);
+        Valtype32 val = elfcpp::Swap<32, big_endian>::readval(wv);
+        Valtype addend_lo;
+
+        bool found = Reloc_funcs::get_lo16_rel_addend(sh_type, preloc_in,
+                                                      relnum, reloc_count,
+                                                      r_type, r_sym, view,
+                                                      &addend_lo);
+        if (!found)
+          {
+            gold_error(_("%s: Can't find matching LO16 reloc for relocation %u "
+                         "against local symbol %u at 0x%lx in section %s"),
+                       object->name().c_str(), r_type, r_sym,
+                       (unsigned long) offset,
+                       object->section_name(relinfo->data_shndx).c_str());
+            return;
+          }
+
+        Valtype addend = ((val & 0xffff) << 16) + addend_lo;
+        Valtype value = psymval->value(object, addend);
+        x = ((value + 0x8000) >> 16) & 0xffff;
+        val = Bits<32>::bit_select32(val, x, 0xffff);
+
+        elfcpp::Swap<32, big_endian>::writeval(wv, val);
+        reloc_status = Reloc_funcs::STATUS_OKAY;
+        break;
+      }
+
+    case elfcpp::R_MIPS_LO16:
+    case elfcpp::R_MICROMIPS_LO16:
+    case elfcpp::R_MIPS16_LO16:
+      {
+        gold_assert(extract_addend);
+        Valtype32* wv = reinterpret_cast<Valtype32*>(paddend);
+        Valtype32 val = elfcpp::Swap<32, big_endian>::readval(wv);
+        Valtype addend = Bits<16>::sign_extend32(val & 0xffff);
+
+        x = psymval->value(object, addend);
+        val = Bits<32>::bit_select32(val, x, 0xffff);
+
+        elfcpp::Swap<32, big_endian>::writeval(wv, val);
+        reloc_status = Reloc_funcs::STATUS_OKAY;
+        break;
+      }
+
+    case elfcpp::R_MIPS_GPREL16:
+    case elfcpp::R_MIPS_LITERAL:
+    case elfcpp::R_MICROMIPS_GPREL16:
+    case elfcpp::R_MICROMIPS_GPREL7_S2:
+    case elfcpp::R_MICROMIPS_LITERAL:
+    case elfcpp::R_MIPS16_GPREL:
+      reloc_status = Reloc_funcs::relgprel(paddend, object, psymval,
+                                           this->gp_value(), r_addend,
+                                           extract_addend, true, r_type,
+                                           !extract_addend, &x);
+      break;
+
+    case elfcpp::R_MIPS_GPREL32:
+      reloc_status = Reloc_funcs::relgprel32(paddend, object, psymval,
+                                             this->gp_value(), r_addend,
+                                             extract_addend, !extract_addend,
+                                             &x);
       break;
 
     default:
       gold_unreachable();
     }
+  Reloc_funcs::mips_reloc_shuffle(paddend, r_type, false);
+
+  if (sh_type == elfcpp::SHT_RELA)
+    {
+      typedef typename Mips_reloc_types<elfcpp::SHT_RELA, size,
+                                        big_endian>::Reloc_write Relatype_write;
+      Relatype rela(preloc_in);
+      Relatype_write rela_write(preloc_out);
+
+      rela_write.put_r_offset(new_offset);
+      Mips_classify_reloc<elfcpp::SHT_RELA, size, big_endian>::
+          put_r_info(&rela_write, &rela, new_symndx);
+      Mips_classify_reloc<elfcpp::SHT_RELA, size, big_endian>::
+          put_r_addend(&rela_write, x);
+    }
+  else if (sh_type == elfcpp::SHT_REL)
+    {
+      typedef typename Mips_reloc_types<elfcpp::SHT_REL, size,
+                                        big_endian>::Reloc_write Reltype_write;
+      Reltype rel(preloc_in);
+      Reltype_write rel_write(preloc_out);
+
+      rel_write.put_r_offset(new_offset);
+      Mips_classify_reloc<elfcpp::SHT_REL, size, big_endian>::
+          put_r_info(&rel_write, &rel, new_symndx);
+    }
+  else
+    gold_unreachable();
 
   // Report any errors.
   switch (reloc_status)
@@ -10330,11 +10534,11 @@ Target_mips<size, big_endian>::relocate_special_relocatable(
     case Reloc_funcs::STATUS_OKAY:
       break;
     case Reloc_funcs::STATUS_OVERFLOW:
-      gold_error_at_location(relinfo, relnum, reloc.get_r_offset(),
+      gold_error_at_location(relinfo, relnum, offset,
                              _("relocation overflow"));
       break;
     case Reloc_funcs::STATUS_BAD_RELOC:
-      gold_error_at_location(relinfo, relnum, reloc.get_r_offset(),
+      gold_error_at_location(relinfo, relnum, offset,
         _("unexpected opcode while processing relocation"));
       break;
     default:
diff --git a/gold/target-reloc.h b/gold/target-reloc.h
index c8b86c6..536118e 100644
--- a/gold/target-reloc.h
+++ b/gold/target-reloc.h
@@ -767,7 +767,8 @@ relocate_relocs(
 	  Sized_target<size, big_endian>* target =
 	    parameters->sized_target<size, big_endian>();
 	  target->relocate_special_relocatable(relinfo, Classify_reloc::sh_type,
-					       prelocs, i, output_section,
+					       prelocs, i, reloc_count,
+					       output_section,
 					       offset_in_output_section,
 					       view, view_address,
 					       view_size, pwrite);
diff --git a/gold/target.h b/gold/target.h
index 5ca8435..e43623a 100644
--- a/gold/target.h
+++ b/gold/target.h
@@ -993,6 +993,7 @@ class Sized_target : public Target
 			       unsigned int /* sh_type */,
 			       const unsigned char* /* preloc_in */,
 			       size_t /* relnum */,
+			       size_t /* reloc_count */,
 			       Output_section* /* output_section */,
 			       typename elfcpp::Elf_types<size>::Elf_Off
                                  /* offset_in_output_section */,

Re: [PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Cary Coutant]

> This patch adds support for more relocations in relocatable link.  It also
> introduces get_lo16_rel_addend method for finding partnering LO16 relocation
> because there is a problem with current implementation when using --threads
> option.

What problem are you referring to? I'm guessing that you're thinking
you can't use the approach of pushing HI16 relocations onto a list
like Scan::local does with target->got16_addends_, because
relocate_special_relocatable can be called in multiple threads, and
thus cannot update the non-thread-specific list. Well, got16_addends_
ought to be a member of the Target::Scan class, which would make it
thread specific, but it doesn't really matter that much since
Scan::local() and Scan::global() always run single threaded. The same
idea, however, could be used if needed in Target::Relocate, but that
doesn't help with relocate_relocs since there is no Target::Scan or
Target::Relocate equivalent class. It also doesn't help because here
you're trying to look ahead to the paired LO16 while processing a
HI16, while in Scan::local(), it only needs to look back to the paired
HI16 while processing a LO16.

I'm not happy with the potentially quadratic behavior introduced by
get_lo16_rel_addend(), but I guess my real issue isn't with your patch
so much as with the MIPS ABI itself. When the ISA doesn't provide a
sufficiently large operand field for a full embedded addend, ELF
expects you to use RELA relocations. With REL relocations, the linker
is forced to pull ugly stunts like this.

Rather than change the prototype for relocate_special_relocatable(), I
think you'd be better off reimplementing the whole of
relocate_relocs(), similar to what the powerpc port did. If you do
that, you'll be able to make a first pass over the relocs and build a
table of LO16 relocs that you can consult efficiently when processing
the HI16 relocs on the second pass.

-cary

RE: [PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Vladimir Radosavljevic]

Hi Cary,

> What problem are you referring to? I'm guessing that you're thinking
> you can't use the approach of pushing HI16 relocations onto a list
> like Scan::local does with target->got16_addends_, because
> relocate_special_relocatable can be called in multiple threads, and
> thus cannot update the non-thread-specific list. Well, got16_addends_
> ought to be a member of the Target::Scan class, which would make it
> thread specific, but it doesn't really matter that much since
> Scan::local() and Scan::global() always run single threaded. The same
> idea, however, could be used if needed in Target::Relocate, but that
> doesn't help with relocate_relocs since there is no Target::Scan or
> Target::Relocate equivalent class. It also doesn't help because here
> you're trying to look ahead to the paired LO16 while processing a
> HI16, while in Scan::local(), it only needs to look back to the paired
> HI16 while processing a LO16.

Yes, I'm referring to the problem that you described.
What do you think about moving these lists into the class Mips_relobj?  If I do
that, there is no need to reimplement the whole relocate_relocs() and the same
approach from Target::Relocate can be used (look back to the paired HI16 while
processing a LO16).  Report if there is no matching LO16 reloc can be added at
the end of the Target_mips::relocate_relocs for relocatable link.

> I'm not happy with the potentially quadratic behavior introduced by
> get_lo16_rel_addend(), but I guess my real issue isn't with your patch
> so much as with the MIPS ABI itself.

Relocation section is sorted so each HI16 relocation is followed immediately
by an associated LO16 entry (there can be more than one HI16 reloc for one
LO16), so this approach shouldn't have an impact on performance.

Regards,
Vladimir

Re: [PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Cary Coutant]

> Yes, I'm referring to the problem that you described.
> What do you think about moving these lists into the class Mips_relobj?  If I do
> that, there is no need to reimplement the whole relocate_relocs() and the same
> approach from Target::Relocate can be used (look back to the paired HI16 while
> processing a LO16).  Report if there is no matching LO16 reloc can be added at
> the end of the Target_mips::relocate_relocs for relocatable link.

If I'm reading your patch correctly, your new
relocate_special_relocatable doesn't need to look back when processing
the LO16 relocs, but does need to look forward when processing the
HI16 relocs. So the lists used by Target::Scan::local() wouldn't help
here. (Did you mean Target::Scan instead of Target::Relocate? I don't
see any need to find paired relocs in Target::Relocate.)

I did think about putting the info in Mips_relobj, but then you'll
need an ugly static_cast to convert the Sized_relobj_file* in relinfo.
But I don't think that would even help you here, since you need to
look forward. Right?

I think for now, you could go ahead and use get_lo16_rel_addend() as
you've written it, but instead of changing the function prototype,
just calculate reloc_count from what you have in relinfo, as in
Relocate::relocate().

>> I'm not happy with the potentially quadratic behavior introduced by
>> get_lo16_rel_addend(), but I guess my real issue isn't with your patch
>> so much as with the MIPS ABI itself.
>
> Relocation section is sorted so each HI16 relocation is followed immediately
> by an associated LO16 entry (there can be more than one HI16 reloc for one
> LO16), so this approach shouldn't have an impact on performance.

If this is true, you could limit the forward scan to just a single
lookahead. But wouldn't such a restriction severely constrain
optimization? (I can imagine optimizations that would separate the
HI16 and LO16 relocations by an indefinite number of instructions, and
even some that might migrate a LO16 above its paired HI16.)

I should warn you that I'll be gone for the next three weeks, with
limited access to email. Sorry we couldn't get this patch in before I
left.

You should apply for write permission to the binutils repo. Write to
overseers@sourceware.org and name me as your sponsor. Thanks for all
your work keeping the MIPS port up to date!

-cary

RE: [PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Vladimir Radosavljevic]

Hi Cary,

> If I'm reading your patch correctly, your new
> relocate_special_relocatable doesn't need to look back when processing
> the LO16 relocs, but does need to look forward when processing the
> HI16 relocs. So the lists used by Target::Scan::local() wouldn't help
> here. (Did you mean Target::Scan instead of Target::Relocate? I don't
> see any need to find paired relocs in Target::Relocate.)

I was referring to lists (hi16_relocs, got16_relocs and pchi16_relocs) in
Mips_relocate_functions which are used in Target::Relocate.
Look ahead to the paired LO16 while processing a HI16, or look back to the
paired HI16 while processing a LO16 are two approaches for computing AHL addend
for HI16 relocs.  If you think that 'look ahead' is a better approach, we can
remove lists in Mips_relocate_functions and use get_lo16_rel_addend also in
Target::Relocate::relocate, or if you think that 'look back' is a better
approach, we can move those lists into the class Mips_relobj to fix problem with
--threads and update current patch to use them.
What do you think we should do?

Sorry for not giving more information about this in the patch description!

> If this is true, you could limit the forward scan to just a single
> lookahead. But wouldn't such a restriction severely constrain
> optimization? (I can imagine optimizations that would separate the
> HI16 and LO16 relocations by an indefinite number of instructions, and
> even some that might migrate a LO16 above its paired HI16.)

Example of assembly file:
lw $2, %lo(sym)
lui $2, %hi(sym)
lui $2, %hi(sym)

Relocation section:
00000008  00000805 R_MIPS_HI16       00000000   sym
00000004  00000805 R_MIPS_HI16       00000000   sym
00000000  00000806 R_MIPS_LO16       00000000   sym

As you can see, assembler is sorting relocation section so HI16 relocs come
before their matching LO16 pair, and we can rely on that in the linker for
computing AHL addends.  ABI specification says that each HI16 relocation is
followed immediately by an associated LO16 entry, but in practice more HI16
relocations can have one matching LO16, and because of that we can't limit
the forward scan to just a single lookahead.

> You should apply for write permission to the binutils repo. Write to
> overseers@sourceware.org and name me as your sponsor. Thanks for all
> your work keeping the MIPS port up to date!

I have filled out the form (https://sourceware.org/cgi-bin/pdw/ps_form.cgi),
because I didn't have account on sourceware.org.
Thanks for all your reviews!

Regards,
Vladimir

RE: [PATCH][gold] PR 21152: Mips: Handle more relocations in relocatable link

[Maciej W. Rozycki]

On Tue, 21 Mar 2017, Vladimir Radosavljevic wrote:

> > If this is true, you could limit the forward scan to just a single
> > lookahead. But wouldn't such a restriction severely constrain
> > optimization? (I can imagine optimizations that would separate the
> > HI16 and LO16 relocations by an indefinite number of instructions, and
> > even some that might migrate a LO16 above its paired HI16.)
> 
> Example of assembly file:
> lw $2, %lo(sym)
> lui $2, %hi(sym)
> lui $2, %hi(sym)
> 
> Relocation section:
> 00000008  00000805 R_MIPS_HI16       00000000   sym
> 00000004  00000805 R_MIPS_HI16       00000000   sym
> 00000000  00000806 R_MIPS_LO16       00000000   sym
> 
> As you can see, assembler is sorting relocation section so HI16 relocs come
> before their matching LO16 pair, and we can rely on that in the linker for
> computing AHL addends.  ABI specification says that each HI16 relocation is
> followed immediately by an associated LO16 entry, but in practice more HI16
> relocations can have one matching LO16, and because of that we can't limit
> the forward scan to just a single lookahead.

 NB having multiple HI16 relocs associated with a single LO16 reloc is a 
GNU extension (helping with some cases of optimisation, also imperfect 
ones), however the reverse, that is having multiple LO16 relocs associated 
with a single HI16 reloc, is actually set out there in the SVR4 MIPS psABI 
document, addressing the more common case like:

	lui	$1, %hi(sym)
	lw	$2, %lo(sym)($1)
	lw	$3, %lo(sym + 4)($1)

used to fetch a 64-bit data quantity (of course this LUI and hence its 
associated reloc may also be reordered, e.g. typically scheduled into a 
branch delay slot elsewhere).  Observe that the in-place addend difference 
of 4 between the two LO16 relocs does not matter here for a possible 
borrow from the HI16 part as long as `sym' is naturally aligned.

 The choice of REL as the reloc encoding format was indeed unfortunate 
with the original o32 MIPS ABI, and rectified in the later n64/n32 ABIs, 
which use RELA instead.

  Maciej