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

[Viktor Kutuzov <vkutuzov at accesssoftek dot com>]

Hello Dough, thank you for review. Please, see my comment below.

Viktor.
________________________________________
From: Doug Kwan (關振紱) [dougkwan@google.com]
Sent: Tuesday, January 19, 2010 3:44 PM
To: Viktor Kutuzov; Ian Lance Taylor
Cc: binutils@sourceware.org
Subject: Re: [GOLD][PATCH][UPDATED] Added support for R_ARM_V4BX relocation     (with interworking)

Please see comments below.

+ protected:
+  // Arm V4BX stubs are created via a stub factory.  So these are protected.
+  Arm_v4bx_stub(const Stub_template* stub_template, const uint32_t reg)
+    : Stub(stub_template), reg_(reg)
+  { gold_assert(reg < 0xf); }

No need to use gold_assert as make_arm_v4bx already has an assertion.

[VK] Ok.

+  friend class Stub_factory;
+
+  // Return the relocation target address of the i-th relocation in the
+  // stub.
+  Arm_address
+  do_reloc_target(size_t)
+  { return NULL; }
^^^ Arm_address is a scalar type.  Please do a "return 0;" or better
"gold_unreachable();" since V4BX stub does not contain any relocation.

[VK] Ok.
 
+ private:
+  // A template to implement do_write.
+  template<bool big_endian>
+  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));

I find the indentation a bit strange.  If Ian is okay with it, it is fine.

[VK] Agree, it is strange. I'll fix it.

+    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_);
+  }

Most, if not all uses of elfcpp::Swap::writeval in gold use the correct
pointer type.  Perhaps you need to add an reinterpret_cast? Ian?

[VK] It is similar to Stub::do_fixed_endian_write(). I used this method as an example.

-  { 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();
+  }

Add a pair of parentheses so that the expression indents nicely with emacs :^)

        return (this->reloc ...
                && this->..
                && this->..);

[VK] That is acceptable with eclipse :)
[VK] Ok. Not a problem.

+  // 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_[stub->reg()] = stub;
+  }

If we add a stub for a register that already has a stub, we are doing something
wrong.  Please change this to

[VK] It isn't something wrong on my sight, but Ok.

@@ -1073,7 +1167,8 @@
   Arm_relobj(const std::string& name, Input_file* input_file, off_t offset,
              const typename elfcpp::Ehdr<32, big_endian>& ehdr)
     : Sized_relobj<32, big_endian>(name, input_file, offset, ehdr),
-      stub_tables_(), local_symbol_is_thumb_function_(),
+      stub_tables_(),
+      local_symbol_is_thumb_function_(),
       attributes_section_data_(NULL), mapping_symbols_info_(),
       section_has_cortex_a8_workaround_(NULL)
   { }

There is no need for this unless you really don't like the formatting :)

[VK] missed rollback, sorry. I'll return back these changes.

+    // Ensure that we have a BX instruction.
+    gold_assert((val & 0x0ffffff0) == 0x012fff10);
+    const uint32_t reg = (val & 0xf);
+    if (is_interworking && reg != 0xf)
+      {
+       Stub_table<big_endian>* stub_table =
+           object->stub_table(relinfo->data_shndx);
+
+       if (stub_table == NULL)
+         return This::STATUS_BAD_RELOC;
Change this to gold_assert.  This should not happend.

[VK] Ok.

+  // Check if we can use V4BX interworking.
+  // The V4BX interworking stub contains BX instruction,
+  // which is not specified for some profiles.
+  if (this->fix_v4bx() == 2 && !this->may_use_blx())
+    gold_error("unable to provide V4BX reloc interworking fix up. "
+       "The target profile does not support BX instruction.");

You need to use the _(...) macro here to enclose the error message.  Please
look at other use of gold_error.

[VK] Ok.

+    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(relinfo, view, object, address,
+                                      (target->fix_v4bx() == 2));
+      break;

This is different from what ld is doing.  It is sufficient to just check
target fix_v4bx() > 0.  If someone tries to use --fix-v4bx-interworking
for a target not supporting BLX, there is already an error.

[VK] Ok, agree.

+  if (r_type == elfcpp::R_ARM_V4BX)
+    {
+      const uint32_t reg = (addend & 0xf);
+      if (this->fix_v4bx() == 2 && reg < 0xf)
+       {
+         // Try looking up an existing stub from a stub table.
+         Stub_table<big_endian>* stub_table =
+           arm_relobj->stub_table(relinfo->data_shndx);

If we cannot find the stub table, something is wrong in gold.  Change this to

        gold_assert(stub_table != NULL);
        if (stub_table->find_arm_v4bx_stub(reg) == NULL)
         ...

[VK] Ok.

+         if (stub_table && stub_table->find_arm_v4bx_stub(reg) == NULL)
+           {