Re: [gold][aarch64] Patch for erratum-843419 (1 of 2 - report erratum occurrences)

[Cary Coutant <ccoutant at gmail dot com>]

+    // "<" comparator used in ordered_map container.
+    bool
+    operator < (const Mapping_symbol_position& p) const

For consistency, please write this as "operator<(", without spaces
around the '<'.

+  // Only erratum-fixing work needs mapping symbols, so skip this
time consuming
+  // processing if not fixing erratum.
+  if (!parameters->options().fix_cortex_a53())
+    return ;

Extra space.

+  // Read the symbol table section header.
+  const unsigned int symtab_shndx = this->symtab_shndx();
+  elfcpp::Shdr<size, big_endian>
+      symtabshdr(this, this->elf_file()->section_header(symtab_shndx));
+  gold_assert(symtabshdr.get_sh_type() == elfcpp::SHT_SYMTAB);

It seems wasteful to set up these views of the symbol table, when
do_count_local_symbols in the parent class has already done all that.
Both arm and mips targets also do something like this, so a refactor
of the code could benefit all three targets. I won't ask you to do this,
but I'll make a note to look at this myself later.

+  // Read the local symbols.
+  const int sym_size =elfcpp::Elf_sizes<size>::sym_size;

Space after '='.

+  while (p != this->mapping_symbol_info_.end() &&
+        p->first.shndx_ == shndx)
+    {
+      typename Mapping_symbol_info::const_iterator next =
+       this->mapping_symbol_info_.upper_bound(p->first);

Isn't this equivalent to "next = p; ++next"? With map, you'll only
have a single entry per position, and you know that p->first is
already in the map. It seems to me that you're doing an unnecessary
binary search for every element of the map.

I'd suggest doing something like:

      typename Mapping_symbol_info::const_iterator prev = p;
      ++p;

At the top of the loop, remove "p = next;" at the bottom, then replace
"p" with "prev" and "next" with "p" in the body of the loop.

+      // The 3rd insn is a load or store instruction from the "Load/store
+      // register (unsigned immediate)" encoding class, using Rn as the
+      // base address register.
+      if (Insn_utilities::aarch64_ldst_uimm(insn3) &&
+         Insn_utilities::aarch64_rn(insn3)
+         == Insn_utilities::aarch64_rd(insn1))

Add parentheses around the '==' operation, and indent accordingly.

+  // Erratum sequence is at least 3 insns.
+  if (span_end - span_start < 3 * Insn_utilities::BYTES_PER_INSN)
+    return ;

Extra space. This test is redundant -- you'll drop out of the for loop
below immediately if the span isn't long enough. It's also wrong -- it
will fail if there are exactly 3 instructions before the end.

+  Insntype* ip = reinterpret_cast<Insntype*>(input_view + span_start);
+  unsigned int insn_index = 0;
+  for (section_size_type i = span_start; i < span_end;
+       i += Insn_utilities::BYTES_PER_INSN, ++insn_index)
+    {
+      if (span_end - i < 3 * Insn_utilities::BYTES_PER_INSN)
+       break;

Should be "<=".

Given the constraints on the address, it's inefficient to loop over
every address. I'd suggest instead something like this:

  if (output_address & 0x03 != 0)
    return;
  section_size_type offset = 0;
  section_size_type span_length = span_end - span_start;
  // The first instruction must be at page offset 0xFF8 or 0xFFC.
  unsigned int page_offset = output_address & 0xfff;
  if (page_offset < 0xff8)
    offset = 0xff8 - page_offset;
  while (offset + 3 * Insn_utilities::BYTES_PER_INSN <= span_length)
    {
      Insntype* ip = reinterpret_cast<Insntype*>(input_view + offset);
      Insntype insn1 = ip[0];
      if (!Insn_utilities::is_adrp(insn1))
        continue;
      Insntype insn2 = ip[1];
      Insntype insn3 = ip[2];

      // ...

      // Advance to next candidate instruction.
      // We only consider instruction sequences starting at
      // a page offset of 0xff8 or 0xffc.
      page_offset = (output_address + offset) & 0xfff;
      if (page_offset == 0xff8)
        offset += 4;
      else  // (page_offset == 0xffc)
        offset += 0xffc;
    }

(I used '4' instead of Insn_utilities::BYTES_PER_INSN here, because we
explicitly want to advance from 0xff8 to 0xffc.)

+      if (is_erratum_843419_sequence(insn1, insn2, insn3))
+       do_report = true;
+      else if (span_end - i > 16)

Should be '>='. Do you want "4 * Insn_utilities::BYTES_PER_INSN"
instead of 16? (I'm not sold on the usefulness of the symbolic constant;
it seems we're pretty hard-wired to a value of 4.)

+  return ;

Extra space.

+  DEFINE_bool(fix_cortex_a53, options::TWO_DASHES, '\0', false,
+      N_("(AArch64 only) Scan and fix binaries for Cortex-A53 erratums."),
+      N_("(AArch64 only) Do not scan for Cortex-A53 erratums."));

The plural of "erratum" is "errata".

-cary