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

[Hán Shěn (沈涵)]

Hi Cary, thanks a lot for the review. Some comments below.
Re-run and passed all the tests.

For the ease of review, I attached 2 diffs, one is against trunk, the
other is against my previous patch.

Thanks,
Han

On Wed, Apr 15, 2015 at 3:41 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> +    // "<" 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 '<'.

Done.

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

Done.

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

Thanks!

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

Yes, exactly, this is an unnecessary binary search.

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

Done, yes, that's much clearer.

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

Done.

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

Removed this part.

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

Done.

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

'output_address' is output section's output_address, so I use
'output_address + span_start' here.

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

Same as above, 'output_address' is output section's output_address, so
I use 'output_address + span_start' here.
If there a consensus/guideline on whether to use '0xfff' or '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;

Need to update offset before continue, so I reverse the condition and
wrap everything below inside.

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

I use 'output_address + span_start + offset' here.

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

Done.

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

Done.

>
> -cary

-- 
Han Shen

On Wed, Apr 15, 2015 at 3:41 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> +    // "<" 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



-- 
Han Shen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: erratum-patch2.patch
Type: text/x-patch
Size: 23089 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20150416/cfb0bd16/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: erratum-increment.patch
Type: text/x-patch
Size: 8570 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20150416/cfb0bd16/attachment-0001.bin>