Re: [gold][aarch64]patch2: link helloworld

[Jing Yu <jingyu at google dot com>]

On Tue, Aug 5, 2014 at 10:49 AM, Cary Coutant <ccoutant@google.com> wrote:
>> 2014-07-29  Jing Yu <jingyu@google.com>
>>                   Han Shen <shenhan@google.com>
>>
>> * elfcpp/aarch64.h(enum): Replace withdrawn with R_AARCH64_withdrawn
>> * gold/Makefile.am(HFILES): Add aarch64-reloc-property.h
>
> The ChangeLog entry needs to be split into two, one for
> elfcpp/ChangeLog, and one for gold/ChangeLog. Remove the "elfcpp/" and
> "gold/" from the file names on each line. Please put periods at the
> end of each entry.
Done.
>
>>   (Output_data_plt_aarch64::do_write): Femove useless got_base. Set
>> the got_pov entry to plt0.
>
> "Remove"
Done.
>
>> diff --git a/gold/Makefile.in b/gold/Makefile.in
>
> No need to include generated files in the patch.
>
> +  return (low_bound <= x && x < up_bound)
> +    && ((x & extra_alignment_requirement) == 0);
>
> Put another set of parens around the entire expression, and indent the
> second line so the "&&" is aligned properly.
Done.
>
> +template<>
> +bool
> +rvalue_checkup<0,0>(int64_t) {return true;}
>
> Spaces after the comma, and inside the braces.
Done.
>
> +template<>
> +uint64_t
> +rvalue_bit_select<0,0>(uint64_t x) { return x; }
>
> Likewise.
Done.
>
> +  if(code == elfcpp::R_AARCH64_ABS64)
>
> Space between "if" and "(".
Done.
>
> +  typedef uint64_t (*rvalue_bit_select_func_p)(uint64_t);
>
> This typedef uses the "_p" convention for predicate functions, but the
> function doesn't return bool.
Done.
>
> + const AArch64_reloc_property*
> +  get_reloc_property(unsigned int code) const
>
> Bad indentation here.
Done.
>
> +    unsigned int idx = code_to_array_index(code);
> +    gold_assert(idx < Property_table_size);
>
> The assert here seems overcautious -- code_to_array_index() already
> does the same assert.
Done.

>
> What happens if you receive an object file with a bad relocation type?
> It would be preferable to issue a regular error on bad input, rather
> than an internal error. You should assert only when a logic error in
> gold's own code could lead to a false condition.
Yes, added gold_error when encountering an unexpected reloc type.

>
> +  // The Parse_expression class is used to convert relocation operations in
> +  // aarch64-reloc.def into S-expression strings, which are parsed again to
> +  // build actual expression trees.  We do not build the expression trees
> +  // directly because the parser for operations in arm-reloc.def is simpler
> +  // this way.  Conversion from S-expressions to trees is simple.
>
> So you parse the "S + A" forms from aarch64-reloc.def into a tree form
> (letting the compiler do the actual parsing), then convert that back
> to a different string-based "( + S A )" form during initialization.
> Then, when it's time to apply the relocation, you plan to parse the "(
> + S A )" form into a tree form again, right?
>
> Don't take this as an objection, but why not just put the S-expression
> form directly into the .def file, and save a bit of initialization
> time? Alternatively, parsing the two forms doesn't seem appreciably
> different, so why not just parse the original form when it's time to
> apply relocations? Or, why not just save the tree form after the first
> parse?
>
> In Relocate::relocate(), none of the relocations implemented so far
> use these expressions -- are you planning to take advantage of these
> expressions to simplify the logic later, or will they continue to go
> unused?

Ok, removed this part totally, including string s_expression member
variable, since we probably won't use this approach.

>
> +  // Map aarch64 rtypes into range(0,300) as following
> +  //   256 ~ 313 -> 0 ~ 57
> +  //   512 ~ 573 -> 128 ~ 189
> +  //   1024 ~ 1032 -> 256 ~ 264
>
> Why bother to reserve space in your array for the dynamic relocations?
Yeah, that's correct. Removed mapping from 1024-1032.

>
> +protected:
> +  void
> +  do_select_as_default_target()
>
> "protected" should be indented one space.
Done.

>
> +    offset = this->first_plt_entry_offset() +
> +      this->count_ * this->get_plt_entry_size();
>
> Parenthesize the expression and indent the second line under the open paren.
Done.

>
> +static const AArch64_howto aarch64_howto[AArch64_reloc_property::INST_NUM] =
> +{
> +  {0, -1, -1}, // DATA
> +  {0x1fffe0, 5, -1}, // MOVW  [20:5]-imm16
> +  {0xffffe0, 5, -1}, // LD    [23:5]-imm19
> +  {0x60ffffe0, 29, 5}, // ADR   [30:29]-immlo  [23:5]-immhi
> +  {0x60ffffe0, 29, 5}, // ADR   [30:29]-immlo  [23:5]-immhi
>
> Should this be ADRP?
Yes. Done.
>
> +  {0x3ffc00, 10, -1}, // ADD  [21:10]-imm12
> +  {0x3ffc00, 10, -1}, // LDST  [21:10]-imm12
> +  {0x7ffe0, 5, -1}, // TBZNZ  [18:5]-imm14
> +  {0xffffe0, 5, -1}, // CONDB  [23:5]-imm19
> +  {0x3ffffff, 0, -1}, // B [25:0]-imm26
> +  {0x3ffffff, 0, -1}, // CALL [25:0]-imm26
> +};
>
> +  // Page(expr) = expr & ~0xFFF
> +
> +  static inline typename elfcpp::Swap<size, big_endian>::Valtype
> +  Page(typename elfcpp::Elf_types<size>::Elf_Addr address)
> +  {
> +    return (address >> 12) << 12;
> +  }
>
> Why write it as a mask operation in the comment, but use shifting in
> the implementation? I'd prefer the mask operation, and the comment
> ought to be in English.
Changed the implementation and comment.

>
> +    elfcpp::Swap<valsize, big_endian>::writeval(wv,
> +      (Valtype)(val | (immed << doffset)));
>
> +    elfcpp::Swap<valsize, big_endian>::writeval(wv,
> +      (Valtype)(val | (immed1 << doffset1) | (immed2 << doffset2)));
>
> +    elfcpp::Swap_unaligned<valsize, big_endian>::writeval(view, (Valtype)x);
>
> (... and several more places ...)
>
> Use static_cast<Valtype>(...).
Done.

>
> +    typename elfcpp::Elf_types<size>::Elf_Addr x =
> +      psymval->value(object, addend);
>
> It's customary to indent lines by four spaces in cases like this
> (e.g., when you begin the entire rhs of an assignment on a new line).
Done.

>
> +  int got_base = target->got_ != NULL
> +       ? target->got_->current_data_size() >= 0x8000 ? 0x8000 : 0
> +       : 0;
>
> Parenthesize the expression and indent subsequent lines under the open paren.

Done.
>
> +      (size == 64
> +       ? (big_endian ? "elf64-bigaarch64"
> +  : "elf64-littleaarch64")
> +       : (big_endian ? "elf32-bigaarch64"
> +  : "elf32-littleaarch64")),
> +      (size == 64
> +       ? (big_endian ? "aarch64_elf64_be_vec"
> +  : "aarch64_elf64_le_vec")
> +       : (big_endian ? "aarch64_elf32_be_vec"
> +  : "aarch64_elf32_le_vec")))
>
> I think this would be much easier to read if you specialized each of
> the four constructors, like this:
>
Done.

> template<int size, bool big_endian>
> class Target_selector_aarch64 : public Target_selector
> {
>  public:
>   Target_selector_aarch64();
>
>   virtual Target*
>   do_instantiate_target()
>   { return new Target_aarch64<size, big_endian>(); }
> };
>
> template<>
> Target_selector_aarch64<32, true>::Target_selector_aarch64()
>   : Target_selector(elfcpp::EM_AARCH64, 32, true,
>                     "elf32-bigaarch64", "aarch64_elf32_be_vec")
> { }
>
> template<>
> Target_selector_aarch64<32, false>::Target_selector_aarch64()
>   : Target_selector(elfcpp::EM_AARCH64, 32, false,
>                     "elf32-littleaarch64", "aarch64_elf32_le_vec")
> { }
>
> template<>
> Target_selector_aarch64<64, true>::Target_selector_aarch64()
>   : Target_selector(elfcpp::EM_AARCH64, 64, true,
>                     "elf64-bigaarch64", "aarch64_elf64_be_vec")
> { }
>
> template<>
> Target_selector_aarch64<64, false>::Target_selector_aarch64()
>   : Target_selector(elfcpp::EM_AARCH64, 64, false,
>                     "elf64-littleaarch64", "aarch64_elf64_le_vec")
> { }
>
> -cary

Thanks for the review!
Attached the updated patch.

elfcpp/Changelog:
2014-08-07  Jing Yu <jingyu@google.com>
                       Han Shen <shenhan@google.com>

* aarch64.h(enum): Replace withdrawn with R_AARCH64_withdrawn.


gold/Changelog:
2014-08-07  Jing Yu <jingyu@google.com>
                      Han Shen <shenhan@google.com>

* Makefile.am(HFILES): Add aarch64-reloc-property.h.
  (DEFFILES): add aarch64-reloc.def.
  (TARGETSOURCES): Add aarch64-reloc-property.cc.
  (ALL_TARGETOBJS): Add aarch64-reloc-property.$(OBJEXT).
* Makefile.in: Regenerate.
* aarch64-reloc-property.cc: New file.
* aarch64-reloc-property.h: New file.
* aarch64-reloc.def: New file.
* aarch64.cc: Include aarch64-reloc-property.h. Replace spaces
with tab to make the format consistent.
  (Output_data_got_aarch64::symbol_table_): New method.
  (Target_aarch64::do_plt_address_for_global): New method.
  (Target_aarch64::do_plt_address_for_local): New method.
  (Target_aarch64::do_select_as_default_target): New method.
  (Target_aarch64::do_make_data_plt): New method.
  (Target_aarch64::make_data_plt): New method.
  (Output_data_plt_aarch64::has_irelative_section): New method.
  (Output_data_plt_aarch64::address_for_global): New method.
  (Output_data_plt_aarch64::address_for_local): New method.
  (Output_data_plt_aarch64::irelative_rel_): New parameter.
  (Output_data_plt_aarch64::add_entry): Implement contents.
  (Output_data_plt_aarch64::set_final_data_size): Fix typo.
  (Output_data_plt_aarch64::do_write): Remove useless got_base. Set
the got_pov entry to plt0.
  (Output_data_plt_aarch64_standard::do_fill_first_plt_entry):
Implement contents.
  (Output_data_plt_aarch64_standard::do_fill_plt_entry): Implement.
  (AArch64_howto): New struct.
  (aarch64_howto[]): New static const array.
  (AArch64_relocate_functions): New class
  (Target_aarch64::Scan::get_reference_flags): Remove method.
  (Target_aarch64::Scan::local): Implement to support a few relocations.
  (Target_aarch64::Scan::global): Implement to support a few relocations.
  (Target_aarch64::make_plt_section): Implement contents.
  (Target_aarch64::make_plt_entry): Implement contents.
  (Target_aarch64::do_finalize_sections): Implement contents.
  (Target_aarch64::Relocate::relocate): Implement a few relocations.
  (Target_aarch64::relocate_section): Implement contents.

Attachment: gold.patch2
Description: Binary data