Re: [PATCH] gold: Fix aarch64 fix_errata_and_relocate_erratum_stubs internal error

["Han Shen via binutils" <binutils at sourceware dot org>]

Hi James, thanks for fixing this.

This looks ok to me, except some changes of spaces->tabs, like those
starting from "// Erratum fix is done (or skipped), continue to
relocate erratum".

I'll strip these unrelated changes and submit it shortly.

Thanks again,
Han

gold/
PR gold/21868
* aarch64.cc (AArch64_relobj::try_fix_erratum_843419_optimized):
Add extra view offset argument to function.
(AArch64_relobj::fix_errata_and_relocate_erratum_stubs): Add
extra view offset set to the output offset when the view has
is_input_output_view set, since it has not already been
included. Pass this to try_fix_erratum_843419_optimized.
---
 gold/aarch64.cc | 58 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 58c744967a..87daf9d0a9 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1864,7 +1864,7 @@ class AArch64_relobj : public
Sized_relobj_file<size, big_endian>
   // applied.
   bool
   try_fix_erratum_843419_optimized(
-      The_erratum_stub*,
+      The_erratum_stub*, AArch64_address,
       typename Sized_relobj_file<size, big_endian>::View_size&);

   // Whether a section needs to be scanned for relocation stubs.
@@ -1980,6 +1980,7 @@ AArch64_relobj<size,
big_endian>::fix_errata_and_relocate_erratum_stubs(
 {
   typedef typename elfcpp::Swap<32,big_endian>::Valtype Insntype;
   unsigned int shnum = this->shnum();
+  const Relobj::Output_sections& out_sections(this->output_sections());
   for (unsigned int i = 1; i < shnum; ++i)
     {
       The_stub_table* stub_table = this->stub_table(i);
@@ -1988,43 +1989,59 @@ AArch64_relobj<size,
big_endian>::fix_errata_and_relocate_erratum_stubs(
       std::pair<Erratum_stub_set_iter, Erratum_stub_set_iter>
  ipair(stub_table->find_erratum_stubs_for_input_section(this, i));
       Erratum_stub_set_iter p = ipair.first, end = ipair.second;
+      typename Sized_relobj_file<size, big_endian>::View_size&
+ pview((*pviews)[i]);
+      AArch64_address view_offset = 0;
+      if (pview.is_input_output_view)
+ {
+  // In this case, write_sections has not added the output offset to
+  // the view's address, so we must do so. Currently this only happens
+  // for a relaxed section.
+  unsigned int index = this->adjust_shndx(i);
+  const Output_relaxed_input_section* poris =
+      out_sections[index]->find_relaxed_input_section(this, index);
+  gold_assert(poris != NULL);
+  view_offset = poris->address() - pview.address;
+ }
+
       while (p != end)
  {
   The_erratum_stub* stub = *p;
-  typename Sized_relobj_file<size, big_endian>::View_size&
-    pview((*pviews)[i]);

   // Double check data before fix.
-  gold_assert(pview.address + stub->sh_offset()
+  gold_assert(pview.address + view_offset + stub->sh_offset()
       == stub->erratum_address());

   // Update previously recorded erratum insn with relocated
   // version.
   Insntype* ip =
-    reinterpret_cast<Insntype*>(pview.view + stub->sh_offset());
+    reinterpret_cast<Insntype*>(
+      pview.view + view_offset + stub->sh_offset());
   Insntype insn_to_fix = ip[0];
   stub->update_erratum_insn(insn_to_fix);

   // First try to see if erratum is 843419 and if it can be fixed
   // without using branch-to-stub.
-  if (!try_fix_erratum_843419_optimized(stub, pview))
+  if (!try_fix_erratum_843419_optimized(stub, view_offset, pview))
     {
       // Replace the erratum insn with a branch-to-stub.
       AArch64_address stub_address =
  stub_table->erratum_stub_address(stub);
       unsigned int b_offset = stub_address - stub->erratum_address();
       AArch64_relocate_functions<size, big_endian>::construct_b(
- pview.view + stub->sh_offset(), b_offset & 0xfffffff);
+ pview.view + view_offset + stub->sh_offset(),
+ b_offset & 0xfffffff);
     }

-          // Erratum fix is done (or skipped), continue to relocate erratum
-          // stub. Note, when erratum fix is skipped (either because we
-          // proactively change the code sequence or the code sequence is
-          // changed by relaxation, etc), we can still safely relocate the
-          // erratum stub, ignoring the fact the erratum could never be
-          // executed.
+  // Erratum fix is done (or skipped), continue to relocate erratum
+  // stub. Note, when erratum fix is skipped (either because we
+  // proactively change the code sequence or the code sequence is
+  // changed by relaxation, etc), we can still safely relocate the
+  // erratum stub, ignoring the fact the erratum could never be
+  // executed.
           stub_table->relocate_erratum_stub(
-              stub, pview.view + (stub_table->address() - pview.address));
+    stub,
+    pview.view + view_offset + (stub_table->address() - pview.address));

           // Next erratum stub.
   ++p;
@@ -2042,7 +2059,7 @@ AArch64_relobj<size,
big_endian>::fix_errata_and_relocate_erratum_stubs(
 template<int size, bool big_endian>
 bool
 AArch64_relobj<size, big_endian>::try_fix_erratum_843419_optimized(
-    The_erratum_stub* stub,
+    The_erratum_stub* stub, AArch64_address view_offset,
     typename Sized_relobj_file<size, big_endian>::View_size& pview)
 {
   if (stub->type() != ST_E_843419)
@@ -2052,9 +2069,11 @@ AArch64_relobj<size,
big_endian>::try_fix_erratum_843419_optimized(
   typedef typename elfcpp::Swap<32,big_endian>::Valtype Insntype;
   E843419_stub<size, big_endian>* e843419_stub =
     reinterpret_cast<E843419_stub<size, big_endian>*>(stub);
-  AArch64_address pc = pview.address + e843419_stub->adrp_sh_offset();
+  AArch64_address pc =
+    pview.address + view_offset + e843419_stub->adrp_sh_offset();
   unsigned int adrp_offset = e843419_stub->adrp_sh_offset ();
-  Insntype* adrp_view = reinterpret_cast<Insntype*>(pview.view + adrp_offset);
+  Insntype* adrp_view =
+    reinterpret_cast<Insntype*>(pview.view + view_offset + adrp_offset);
   Insntype adrp_insn = adrp_view[0];

   // If the instruction at adrp_sh_offset is "mrs R, tpidr_el0", it may come
@@ -2070,8 +2089,9 @@ AArch64_relobj<size,
big_endian>::try_fix_erratum_843419_optimized(
   // return true.
   if (!Insn_utilities::is_adrp(adrp_insn) && adrp_offset)
     {
-      Insntype* prev_view
- = reinterpret_cast<Insntype*>(pview.view + adrp_offset - 4);
+      Insntype* prev_view =
+ reinterpret_cast<Insntype*>(
+  pview.view + view_offset + adrp_offset - 4);
       Insntype prev_insn = prev_view[0];

       if (Insn_utilities::is_mrs_tpidr_el0(prev_insn))
-- 
2.14.1

On Mon, Aug 28, 2017 at 8:23 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>> gold/
>>         PR gold/21868
>>         * aarch64.cc (AArch64_relobj::try_fix_erratum_843419_optimized):
>>         Add extra view offset argument to function.
>>         (AArch64_relobj::fix_errata_and_relocate_erratum_stubs): Add
>>         extra view offset set to the output offset when the view has
>>         is_input_output_view set, since it has not already been
>>         included. Pass this to try_fix_erratum_843419_optimized.
>
> This is OK with me if it's OK with Han.
>
> Han, if it looks good to you, can you apply it for James?
>
> Thanks!
>
> -cary



-- 
Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330