This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [GOLD][PATCH][UPDATED] Added support for R_ARM_V4BX relocation (with interworking)
- From: Viktor Kutuzov <vkutuzov at accesssoftek dot com>
- To: Doug Kwan (關振紱) <dougkwan at google dot com>, Ian Lance Taylor <iant at google dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 19 Jan 2010 16:40:03 -0800
- Subject: RE: [GOLD][PATCH][UPDATED] Added support for R_ARM_V4BX relocation (with interworking)
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)
+ {