[GOLD] gold aarch64 erratum stubs are created but not always applied

Message ID CAEt-8LBL9Dvoph5OUOP-14n9r-P-XgOAJLin9muTuzdhsNqqOw@mail.gmail.com
State New
Headers show
Series
  • [GOLD] gold aarch64 erratum stubs are created but not always applied
Related show

Commit Message

Peter Smith Nov. 30, 2017, 11:36 a.m.
Hello,

As a result of looking into fixing
https://sourceware.org/bugzilla/show_bug.cgi?id=20765 I checked over
the results of applying --fix-cortex-a53-843419 to a very large
program (gitit) with two stub tables and thousands of erratum fixes. I
noticed that all the erratum_stubs were being created but about 1/3 of
them were being skipped over by
fix_errata_and_relocate_erratum_stubs(). By skipped over I mean no
branch relocation or adrp -> adr transformation was applied to the
erratum address, leaving the erratum_stub unreachable, and with a
branch with a 0 immediate.

The root cause of the skipped over erratum_stubs is
Erratum_stub::invalidate_erratum_stub() that is used to set relobj_ to
NULL when an erratum_stub has been processed. Unfortunately relobj_ is
used in operator<() so altering relobj makes the results from
erratum_stubs_.lower_bound() as used in
find_erratum_stubs_for_input_section() unreliable.

I've proposed a simple fix that adds a new field that can be used in
Erratum_stub::invalidate_erratum_stub(). This preserves the order
relied upon by erratum_stubs_.lower_bound().

In theory this could hit any program with more than one relobj needing
erratum stubs, but in practice I think most small programs will get
away with it. The larger the program the greater the chance that
erratum_stubs_.lower_bound() incorrectly reporting 0 stubs found for
an input section.

This bug will be undetectable on hardware that doesn't exhibit the
erratum, but it may result on the erratum triggering on faulty
hardware even when --fix-cortex-a53-843419 is used.

Peter

Comments

Cary Coutant Nov. 30, 2017, 10:04 p.m. | #1
> As a result of looking into fixing

> https://sourceware.org/bugzilla/show_bug.cgi?id=20765 I checked over

> the results of applying --fix-cortex-a53-843419 to a very large

> program (gitit) with two stub tables and thousands of erratum fixes. I

> noticed that all the erratum_stubs were being created but about 1/3 of

> them were being skipped over by

> fix_errata_and_relocate_erratum_stubs(). By skipped over I mean no

> branch relocation or adrp -> adr transformation was applied to the

> erratum address, leaving the erratum_stub unreachable, and with a

> branch with a 0 immediate.

>

> The root cause of the skipped over erratum_stubs is

> Erratum_stub::invalidate_erratum_stub() that is used to set relobj_ to

> NULL when an erratum_stub has been processed. Unfortunately relobj_ is

> used in operator<() so altering relobj makes the results from

> erratum_stubs_.lower_bound() as used in

> find_erratum_stubs_for_input_section() unreliable.

>

> I've proposed a simple fix that adds a new field that can be used in

> Erratum_stub::invalidate_erratum_stub(). This preserves the order

> relied upon by erratum_stubs_.lower_bound().


Thanks for finding this!

It seems the sole purpose of invalidating the stub is nothing more
than a guard against relocating the same stub twice. Rather than
adding a new field, I've modified your patch to invalidate the stub by
setting erratum_insn_ to invalid_insn. (My first thought was simply to
set the stub type to ST_NONE, but that's a const member in the base
class, so it wasn't worth doing that.)

I've committed the patch below.

-cary


2017-11-30  Peter Smith  <peter.smith@linaro.org>
            Cary Coutant  <ccoutant@gmail.com>

gold/
        PR gold/20765
        * aarch64.cc (Erratum_stub::invalidate_erratum_stub): Use erratum_insn_
        instead of relobj_ to invalidate the stub.
        (Erratum_stub::is_invalidated_erratum_stub): Likewise.


diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 02fabb7749..04da01d51f 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1052,13 +1052,13 @@ public:
   void
   invalidate_erratum_stub()
   {
-     gold_assert(this->relobj_ != NULL);
-     this->relobj_ = NULL;
+     gold_assert(this->erratum_insn_ != invalid_insn);
+     this->erratum_insn_ = invalid_insn;
   }

   bool
   is_invalidated_erratum_stub()
-  { return this->relobj_ == NULL; }
+  { return this->erratum_insn_ == invalid_insn; }

 protected:
   virtual void

Patch

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 74c411d..2e03127 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -953,7 +953,7 @@  public:
     : Stub_base<size, big_endian>(type), relobj_(relobj),
       shndx_(shndx), sh_offset_(sh_offset),
       erratum_insn_(invalid_insn),
-      erratum_address_(this->invalid_address)
+      erratum_address_(this->invalid_address), valid_(true)
   {}
 
   ~Erratum_stub() {}
@@ -1064,13 +1064,13 @@  public:
   void
   invalidate_erratum_stub()
   {
-     gold_assert(this->relobj_ != NULL);
-     this->relobj_ = NULL;
+     gold_assert(this->valid_ == true);
+     this->valid_ = false;
   }
 
   bool
   is_invalidated_erratum_stub()
-  { return this->relobj_ == NULL; }
+  { return this->valid_ == false; }
 
 protected:
   virtual void
@@ -1087,6 +1087,8 @@  private:
   Insntype erratum_insn_;
   // The address of the above insn.
   AArch64_address erratum_address_;
+  // After the erratum stub has been fully processed valid_ will be false
+  bool valid_;
 };  // End of "Erratum_stub".