Re: [GOLD][PATCH] Added support for R_ARM_V4BX relocation (with interworking)

[=?big5?b?RG91ZyBLd2FuICjD9q62vHcp?= <dougkwan at google dot com>]

+  void inline
+  do_fixed_endian_v4bx_write(unsigned char* view, section_size_type)
+  {+    const Insn_template* insns = this->stub_template()->insns();
+    elfcpp::Swap<32, big_endian>::writeval(view, insns[0].data() + (this->reg_
<< 16));

Please reformat to fit in 80 columns.

+    view += insns[0].size();
+    elfcpp::Swap<32, big_endian>::writeval(view, insns[1].data() + this->reg_);
+    view += insns[1].size();
+    elfcpp::Swap<32, big_endian>::writeval(view, insns[2].data() + this->reg_);
+  }
+
+  // A register index (r0-r14), which is associated with the stub.
+  uint32_t reg_;
+};
+
 // Stub factory class.

 class Stub_factory
@@ -782,6 +838,15 @@
                              source, destination, original_insn);
   }

@@ -808,7 +873,7 @@
   { }

   ~Stub_table()
-  { }
+  { this->remove_all_arm_v4bx_stubs(); }

There is no need for this.  Currently we do not destroy stub tables.  If we are
to destory stub tables, you need to also add code to release other stubs to
avoid memory leak.

   // Owner of this stub table.
   Arm_input_section<big_endian>*
@@ -818,7 +883,9 @@
   // Whether this stub table is empty.
   bool
   empty() const
-  { return this->reloc_stubs_.empty() && this->cortex_a8_stubs_.empty(); }
+  { return this->reloc_stubs_.empty()
+       && this->cortex_a8_stubs_.empty()
+       && this->arm_v4bx_stubs_.empty(); }

Please reformat this to be consistent.  If a method needs more than one line,
begin with a line with just a single left brace and end this with another line
with just a right brace.

   // Return the current data size.
   off_t
@@ -845,10 +912,25 @@
     this->cortex_a8_stubs_.insert(value);
   }

+  // Add an ARM V4BX relocation stub. A register index will be retrieved
+  // from the stub.
+  void
+  add_arm_v4bx_stub(Arm_v4bx_stub* stub)
+  {
+    gold_assert(stub);
+    if (this->find_arm_v4bx_stub(stub->reg()) == NULL)
+      this->arm_v4bx_stubs_.insert(
+         std::pair<uint32_t, Arm_v4bx_stub*>(stub->reg(), stub));
+  }

Would it be simpler to write

     if (...)
        this->arm_v4bx_stubs_[stub_regs()] = stub;

instead?
+
   // Remove all Cortex-A8 stubs.
   void
   remove_all_cortex_a8_stubs();

+  // Remove all ARM V4BX stubs.
+  void
+  remove_all_arm_v4bx_stubs();
+

We may not need this.  Please ask Ian.

   // Look up a relocation stub using KEY.  Return NULL if there is none.
   Reloc_stub*
   find_reloc_stub(const Reloc_stub::Key& key) const
@@ -857,6 +939,15 @@
     return (p != this->reloc_stubs_.end()) ? p->second : NULL;
   }

+  // Look up an arm v4bx relocation stub using the register index.
+  // Return NULL if there is none.
+  Arm_v4bx_stub*
+  find_arm_v4bx_stub(const uint32_t reg) const
+  {
+    typename Arm_v4bx_stub_list::const_iterator p =
this->arm_v4bx_stubs_.find(reg);
+    return (p != this->arm_v4bx_stubs_.end()) ? p->second : NULL;
+  }
+

please format to fit in 80 columns.

Since we can only have at most 15 v4BX stubs, would it simplify things if we
use std::vector<Arm_v4bx_stub*> to implement arm_v4bx_stubs_?  You can
set the of vector to 15 in Stub_table::Stub_table.

   // Relocate stubs in this stub table.
   void
   relocate_stubs(const Relocate_info<32, big_endian>*,
@@ -1097,6 +1193,22 @@
     this->stub_tables_[shndx] = stub_table;
   }

+  // Return the stub table to save a common interworking veneer code.
+  Stub_table<big_endian>*
+  glue_owner_stub_table() const
+  {
+    return this->glue_owner_stub_table_;
+  }
+
+  // Set the stub table to save a common interworking veneer code.
+  void
+  set_glue_owner_stub_table(Stub_table<big_endian>* stub_table)
+  {
+    gold_assert(stub_table != NULL);
+    if (this->glue_owner_stub_table_ == NULL)
+      this->glue_owner_stub_table_ = stub_table;
+  }
+
   // Whether a local symbol is a THUMB function.  R_SYM is the symbol table
   // index.  This is only valid after do_count_local_symbol is called.
   bool
@@ -1685,6 +1799,14 @@
   fix_cortex_a8() const
   { return this->fix_cortex_a8_; }

+  // Whether we fix R_ARM_V4BX relocation.
+  // 0 - do not fix
+  // 1 - replace with MOV instruction (armv4 target)
+  // 2 - make interworking veneer (>= armv4t targets only)
+  short
+  fix_v4bx() const
+  { return this->fix_v4bx_; }
+

Any reason why we want to use short instead of int?

   // Scan a span of THUMB code section for Cortex-A8 erratum.
   void
   scan_span_for_cortex_a8_erratum(Arm_relobj<big_endian>*, unsigned int,
@@ -2048,6 +2170,8 @@
   bool fix_cortex_a8_;
   // Map addresses to relocs for Cortex-A8 erratum.
   Cortex_a8_relocs_info cortex_a8_relocs_info_;
+  // Whether we need to fix code for V4BX relocations.
+  short fix_v4bx_;
 };

 template<bool big_endian>
@@ -3713,6 +3888,19 @@
   this->cortex_a8_stubs_.clear();
 }

+// Remove all ARM V4BX relocation stubs.
+
+template<bool big_endian>
+void
+Stub_table<big_endian>::remove_all_arm_v4bx_stubs()
+{
+  for (Arm_v4bx_stub_list::iterator p = this->arm_v4bx_stubs_.begin();
+       p != this->arm_v4bx_stubs_.end();
+       ++p)
+    delete p->second;
+  this->arm_v4bx_stubs_.clear();
+}
+
 // Relocate one stub.  This is a helper for Stub_table::relocate_stubs().

We may not need this.

@@ -3898,6 +4117,18 @@
       arm_relobj->mark_section_for_cortex_a8_workaround(stub->shndx());
     }

+  for (typename Arm_v4bx_stub_list::const_iterator p =
this->arm_v4bx_stubs_.begin();

Reformat to fit in 80 column.

+      p != this->arm_v4bx_stubs_.end();
+      ++p)
+    {
+    Arm_v4bx_stub* stub = p->second;
+      const Stub_template* stub_template = stub->stub_template();
+      uint64_t stub_addralign = stub_template->alignment();
+      off = align_address(off, stub_addralign);
+      stub->set_offset(off);
+      off += stub_template->size();
+    }
+
   gold_assert(off <= this->prev_data_size_);
 }

@@ -4080,6 +4311,9 @@
          Arm_relobj<big_endian>* arm_relobj =
            Arm_relobj<big_endian>::as_arm_relobj(p->relobj());
          arm_relobj->set_stub_table(p->shndx(), stub_table);
+         // Only a stub table for the first passed input code segment will be
+         // used.
+         arm_relobj->set_glue_owner_stub_table(stub_table);

I don't think this is the right approach.  There is only one glue owner stub
table per input object.  There is not guarantee that the glue onwer stub
table will be within the valid branch range.   There is even no guarantee that
the glue owner stub table and the section using it will be in memory at the
same time if overlays is allowed.  Why don't use just use the per output
section stub table just like other stubs?

        }
       prev_p = p++;
     }
@@ -4626,7 +4860,8 @@

       if (arm_input_section != NULL
          && arm_input_section->is_stub_table_owner()-         &&
!arm_input_section->stub_table()->empty())
+         && (!arm_input_section->stub_table()->empty()
+             || (this->glue_owner_stub_table_ != NULL && !this->glue_owner_stub
_table_->empty())))

Please reformat to fit in 80 columns.
        {
          // We cannot discard a section if it owns a stub table.
          Output_section* os = this->output_section(i);
@@ -4840,6 +5075,18 @@
        return RelocFuncs::thumb32_cond_branch_offset(upper_insn, lower_insn);
       }

+    // This is specific relocation, which does not have a real addend and
+    // using to replace a whole ARM instruction. We return an instruction
+    // "under" the relocation instead of regular addend to give an ability
+    // extracting the used register index within the instruction and create
+    // an appropriate stub.
+    case elfcpp::R_ARM_V4BX:
+      {
+       typedef typename elfcpp::Swap<32, big_endian>::Valtype Valtype;
+       const Valtype* wv = reinterpret_cast<const Valtype*>(view);
+       return elfcpp::Swap<32, big_endian>::readval(wv);
+      }
+
     default:
       gold_unreachable();
     }

Please move this code out into the part that handles the R_ARM_V4BX in
the beginning of scan_reloc_of_stubs.   The register number if not an
addend.

@@ -6090,6 +6346,15 @@
                                                    address, thumb_bit);
       break;

+    case elfcpp::R_ARM_V4BX:
+      if (target->fix_v4bx() == 1
+         || !target->may_use_blx()
+         || (target->fix_v4bx() == 2 && target->may_use_blx()))
+       reloc_status =
+         Arm_relocate_functions::v4bx(view, object, address,
+                                      (target->fix_v4bx() == 2));
+      break;
+
     case elfcpp::R_ARM_TARGET1:
       // This should have been mapped to another type already.
       // Fall through.
@@ -6238,6 +6503,7 @@
     case elfcpp::R_ARM_MOVT_PREL:
     case elfcpp::R_ARM_THM_MOVW_PREL_NC:
     case elfcpp::R_ARM_THM_MOVT_PREL:
+    case elfcpp::R_ARM_V4BX:
       return 4;

     case elfcpp::R_ARM_TARGET1:
@@ -7446,6 +7712,9 @@
       // THUMB branches.
       destination = value + addend + 4;
       break;
+    case elfcpp::R_ARM_V4BX:
+      destination = 0;
+      break;
     default:
       gold_unreachable();

Please remove this. See my note below.

     }
@@ -7493,6 +7762,23 @@
        new Cortex_a8_reloc(stub, r_type,
                            destination | (target_is_thumb ? 1 : 0));
     }
+
+  // Process the R_ARM_V4BX relocations.
+  const uint32_t reg = (addend & 0xf);
+  if (r_type == elfcpp::R_ARM_V4BX && this->fix_v4bx() == 2 && reg < 0xf)
+    {
+      Stub_table<big_endian>* stub_table =
+         arm_relobj->glue_owner_stub_table();
+      // Locate stub by the register index.
+      if (stub_table && stub_table->find_arm_v4bx_stub(reg) == NULL)
+        {
+         // create a new stub and add it to stub table.
+         Arm_v4bx_stub* stub =
+           this->stub_factory().make_arm_v4bx_stub(reg);
+         gold_assert(stub != NULL);
+         stub_table->add_arm_v4bx_stub(stub);
+        }

Move this code to the entry of scan_reloc_for_reloc and exit the function
after processing R_ARM_V4BX.  There is no reason why the rest of the code
need to see R_ARM_V4BX. So something like:

    if (r_type == elfcpp::R_ARM_V4BX)
      {
        if (this->fix_v4b4() == 2 ...)

        return;
      }


 }