This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Thanks,

On Wed, Jun 14, 2017 at 1:42 AM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
> 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?

Yup, let's change "return  false;" -> "return true;" (and add proper comments).
Also, I assume this relaxation-changing-instructions behavior should
not break erratum 835769? Erratum 843419 must have adrp as its first
insn, whereas 835769 begins with LD/ST.

Otherwise, LGTM

We shall still wait for Cary's approval.

Han

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]