[PATCH] D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419

[Rafael Avila de Espindola via llvm-commits]

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith added a comment.
>
> Many thanks for taking the time to review this and some of the other Arm/AArch64 specific stuff, it is much appreciated.
>
> Posting into Phabricator to keep comments together.
>
>>> +  if (Config->EMachine == EM_AARCH64 && Config->FixCortexA53Errata843419) {
>>>  +    Script->assignAddresses();
>>>  +    reportA53Errata843419Fixes();
>>>  +  }
>> 
>> Why do you need assignAddresses()? This is just after a loop that stops
>>  when address don't change.
>
> At the moment, until https://reviews.llvm.org/D37944 Add support for AArch64 Range Thunks lands the assignAddresses won't be entered. Some merge work will be needed here as Thunks and Errata fixes can impact each other.

How? Can't you always put the errata fixes after all regular code?

>>> +// This file implements Section Patching for the purpose of working around
>> Given where this is called you also need to be able to place the
>>  replacement sequence after all other sections or you will need to
>>  reevaluate addresses for thunks, no?
>
> Yes. At time of writing AArch64 doesn't support thunks, my preference would be to land the range thunks support for AArch64 first (https://reviews.llvm.org/D37944) and then merge this in with some test cases involving range extension thunks.

Sure. Please add it as a dependency to this one.

>>> +// - The implementation here only supports one patch, the AArch64 Cortex-53
>>>  +// errata 843419 that affects r0p0, r0p1, r0p2 and r0p4 versions of the core.
>>>  +// To keep the initial version simple there is no support for multiple
>>>  +// architectures or selection of different patches.
>> 
>> BTW, is it public knowledge what uses these versions? I am not sure if
>> that is something that is present in a few early dev boards or in most
>> of the phones on the planet.
>
> Unfortunately I don't have concrete information, we'd need to know for
> each phone what SoC it was using, and for each SoC which rev of the
> Cortex-A53 it was using. What I do know is that it was reproduced in
> real Android software on enough phones to make the NDK enable the
> gold/bfd equivalent option by default.

OK, that is plenty. Just add a comment saying it is enabled by default
on the android ndk.

Cheers,
Rafael