Re: [PATCH][gold, aarch64] Skip ERRATUM 843419 fix if the sequences have been relaxed by TLS optimization

[Jiong Wang <jiong dot wang at foss dot arm dot com>]

On 13/06/17 23:17, Han Shen via binutils wrote:
> Whereas the correct behavior should leave the code
> untouched, because adrp is replaced by relaxation and shall never
> trigger the erratum.

Hi Han,

  This is exactly what I want to do.

> What do you think?

  Look more on fix_errata + try_try_fix_erratum_843419_optimized, I think we
should return true as return true means this sequence becomes safe after either
instruction rewrite or double-check that we don't want to install the branch-to-stub,
is this looks correct to you?

> Han
>
> On Thu, Jun 8, 2017 at 3:35 AM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
> > On 07/06/17 10:26, Jiong Wang wrote:
> > > On 18/07/16 17:46, Han Shen wrote:
> > >
> > > > Hi Andrew, thanks for reporting this. Could you send me the objs and
> > > > the command line? (I tried to build hhvm on aarch64 machine, seemed to
> > > > me this needs a few third_packages that need to be installed through
> > > > apt-get  (mysql, for example), since I am not a superuser on the
> > > > machine, it is not easy for me to build the whole thing from scratch
> > > > ...)
> > > >
> > > > On Fri, Jul 15, 2016 at 10:42 PM, Andrew Pinski <pinskia@gmail.com>
> > > > wrote:
> > > > > On Thu, Jul 16, 2015 at 2:44 PM, Han Shen <shenhan@google.com> wrote:
> > > > > > Hi Cary, this is the patch for erratum 843419 fix optimization.
> > > > > >
> > > > > > Usually we apply branch-to-stub fix for all erratum. For 843419, under
> > > > > > some
> > > > > > condition, instead of generating jumps, we re-write 'adrp' with 'adr'
> > > > > > (only
> > > > > > applicable if adrp calculation result fits in adr range), thus break
> > > > > > such
> > > > > > erratum sequence and eliminate performance penalty (2-jump/fix).
> > > > > >
> > > > > > Test - build on x86_64 platform and aarch64 platform using opt and
> > > > > > debug (-O0).
> > > > > >     Pass unit tests.  Pass gold local test suite.  Pass tests from arm.
> > > > > >
> > > > > > Ok for trunk?
> > > > > Hi,
> > > > >     I am getting an internal error some of the time when linking HHVM :
> > > > > Erratum 843419 found and fixed at
> > > > > "../runtime/libhphp_runtime.a(bytecode.cpp.o)", section 10882, offset
> > > > > 0x0000022c.
> > > > > Erratum 843419 found and fixed at
> > > > > "../runtime/libhphp_runtime.a(unique-stubs.cpp.o)", section 7040,
> > > > > offset 0x00000218.
> > > > > Erratum 843419 found and fixed at
> > > > > "../runtime/libhphp_runtime.a(bytecode.cpp.o)", section 10882, offset
> > > > > 0x0000022c.
> > > > > Erratum 843419 found and fixed at
> > > > > "../runtime/libhphp_runtime.a(unique-stubs.cpp.o)", section 7040,
> > > > > offset 0x00000218.
> > > > > /usr/bin/ld.gold: internal error in try_fix_erratum_843419_optimized,
> > > > > at ../../gold/aarch64.cc:2007
> > > > > collect2: error: ld returned 1 exit status
> > > > >
> > > > > Is there anything which you need to debug this issue?
> > > > >
> > > > > Thanks,
> > > > > Andrew
> > > > >
> > > > > > gold/ChangeLog
> > > > > >
> > > > > > 2015-07-15 Han Shen <shenhan@google.com>
> > > > > >
> > > > > >           Optimize erratum 843419 fix.
> > > > > >
> > > > > >           gold/ChangeLog: * aarch64.cc (AArch64_insn_utilities::is_adr):
> > > > > > New
> > > > > >           method.  (AArch64_insn_utilities::aarch64_adr_encode_imm): New
> > > > > > method.
> > > > > >           (AArch64_insn_utilities::aarch64_adrp_decode_imm): New method.
> > > > > >           (E843419_stub): New sub-class of Erratum_stub.
> > > > > >           (AArch64_relobj::try_fix_erratum_843419_optimized): New
> > > > > > method.
> > > > > >           (AArch64_relobj::section_needs_reloc_stub_scanning): Try
> > > > > > optimized fix.
> > > > > >           (AArch64_relobj::create_erratum_stub): Add 1 argument.
> > > > > >           (Target_aarch64::scan_erratum_843419_span): Pass in adrp insn
> > > > > > offset.
> > > > > >
> > > > > > --
> > > > > > Han Shen
> > >
> > > I have encountered the same issue.
> > >
> > > On latest GOLD, the following assertion triggered:
> > >
> > >    gold/aarch64.cc +2016 gold_assert(Insn_utilities::is_adrp(adrp_insn));
> > >
> > > A quick debug shows the offending instruction sequence is like the
> > > following:
> > >
> > >
> > >     91000401     add    x1, x0, #0x1
> > >     d53bd042     mrs    x2, tpidr_el0
> > >     d2a00000     movz    x0, #0x0, lsl #16  <- adrp_sh_offset
> > >     f285cf00     movk    x0, #0x2e78
> > >     8b000040     add    x0, x2, x0
> > >     f9001401     str    x1, [x0,#40]
> > >
> > > This sequence looks like the sequence for TLS local executable mode, So, I
> > > am
> > > wondering if this issue is caused by TLS relaxtaion?
> > >
> > > Before relaxataion they will be adrp + add instructions and the stub was
> > > recorded
> > > at that time.  Then the later relaxation change the instructions and break
> > > the
> > > assumptions.
> >
> >
> > Here is a proposed fix, please have a look, it fixed the HHVM build.
> >
> > This patch simply returns false from the erratum fix function.  From my
> > understanding, this ignores that particular stub.  I feel this is reasonable
> > as
> > after TLS relaxataion the original sequence does not contain adrp
> > instruction
> > anymore.
> >   This patch return false in to cases:
> >    * if the instruction pointed by adrp_sh_offset is "mrs R, tpidr_el0".
> >      IE -> LE relaxation etc. may generate this.
> >    * if the instruction pointed by adrp_sh_offset is not ADRP and the
> > instruction
> >      before adrp_sh_offset is "mrs R, tpidr_el0", LD -> LE relaxation etc may
> >      generate this.
> >
> >
> > gold/
> > 2017-06-08  Jiong Wang  <jiong.wang@arm.com>
> >
> >          * aarch64.cc (Insn_utilities::is_mrs_tpidr_el0): New method.
> >          (AArch64_relobj<size,
> > big_endian>::try_fix_erratum_843419_optimized):
> >          Skip if there is TLS relaxation.