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

[Cary Coutant <ccoutant at google dot com>]

> 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.

>   (Output_data_plt_aarch64::do_write): Femove useless got_base. Set
> the got_pov entry to plt0.

"Remove"

> 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.

+template<>
+bool
+rvalue_checkup<0,0>(int64_t) {return true;}

Spaces after the comma, and inside the braces.

+template<>
+uint64_t
+rvalue_bit_select<0,0>(uint64_t x) { return x; }

Likewise.

+  if(code == elfcpp::R_AARCH64_ABS64)

Space between "if" and "(".

+  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.

+ const AArch64_reloc_property*
+  get_reloc_property(unsigned int code) const

Bad indentation here.

+    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.

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.

+  // 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?

+  // 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?

+protected:
+  void
+  do_select_as_default_target()

"protected" should be indented one space.

+    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.

+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?

+  {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.

+    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>(...).

+    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).

+  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.

+      (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:

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