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

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

Hi Jiong, thanks for the fix.

One question - in your patch, when linker encounters 2 cases described
in your comments, "try_fix_erratum_843419_optimized" returns false,
which makes linker fall back to "replacing the erratum insn with a
branch-to-stub." Whereas the correct behavior should leave the code
untouched, because adrp is replaced by relaxation and shall never
trigger the erratum. What do you think?

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



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