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