[PATCH] D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 4 inline comments as done.
peter.smith added a comment.
Herald added a subscriber: dmgreen.

In D67284#1669708 <https://reviews.llvm.org/D67284#1669708>, @MaskRay wrote:

> LGTM, just a few nits and a requested test.
>
> > I will add a case to make sure the 0xffc is handled in a similar way to 0xffe and will add to the nopatch test case.


Thanks for the review. I've made the updates and will commit shortly, if there is anything else then we should be able to fix it post-commit. There are a couple of test cases in arm-fix-cortex-a8-nopatch.s (CALLSITE6 and CALLSITE7) that check for those cases, but it probably wasn't clear enough. I've expanded the comment to explain what they are checking for.



================
Comment at: ELF/ARMErrataFix.cpp:246
+  uint64_t isecAddr = isec->getVA(0);
+  // Advance Off so that (isecAddr + off) modulo 0x1000 is at least 0xffa. We
+  // need to check for a 32-bit instruction immediately before a 32-bit branch
----------------
MaskRay wrote:
> is at least 0xffa -> is 0xffa?
> 
> I haven't verified, but `off = alignTo(isecAddr+off, 0x1000, 0xffa) - isecAddr;` probably works.
Yes, that cleans it up a lot. Thanks for the suggestion.


================
Comment at: ELF/ARMErrataFix.cpp:373
+  for (const InputSection *isec : isd.sections) {
+    isecLimit = isec->outSecOff + isec->getSize();
+    if (isecLimit > patchUpperBound) {
----------------
MaskRay wrote:
> isecLimit can be defined here, i.e. remove the definition above.
If you mean do something like:
```
uint64_t isecLimit = isec->outSecOff + isec->getSize();
```
I don't think that will work due to the use of isecLimit outside the for loop on line 384:
```
for (; patchIt != patchEnd; ++patchIt)
    (*patchIt)->outSecOff = isecLimit;
``` 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67284/new/

https://reviews.llvm.org/D67284