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

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 19 inline comments as done.
peter.smith added a comment.

Thanks very much for the comments. I'll upload a patch shortly with the suggestions applied.



================
Comment at: ELF/ARMErrataFix.cpp:202
+  // or the PLT.
+  auto relIt = llvm::find_if(isec->relocations, [=](const Relocation &r) {
+    return r.offset == off &&
----------------
MaskRay wrote:
> The find_if code sequence is also used in `implementPatch`:
> 
> ```
> auto relIt = llvm::find_if(isec->relocations, [=](const Relocation &r) {
>     return r.offset == patcheeOffset &&
>            (r.type == R_ARM_THM_JUMP19 || r.type == R_ARM_THM_JUMP24 ||
>             r.type == R_ARM_THM_CALL);
>   });
> ```
> 
> Is it necessary to store the iterator in ScanResult? 
I think that would help. I've made it so a pointer to the relocation is stored in ScanResult. As ImplementPatch uses all of ScanResult I've just passed through rather than splitting up the parameters.


================
Comment at: ELF/ARMErrataFix.cpp:227
+  // 0x100 is 1 branch every 4KiB for a 1 MiB range.
+  uint64_t dest = source + isec->getSize() + 0x100;
+  return target->inBranchRange(
----------------
MaskRay wrote:
> `source + isec->getSize() + 0x100` or `isec->getVA() + isec->getSize() + 0x100`?
Thanks for pointing out the mistake. It should be isec->getVA() + isec->getSize() + 0x100


================
Comment at: ELF/ARMErrataFix.cpp:336
+                    [=](const Defined *a, const Defined *b) {
+                      return (isArmMapSymbol(a) && isArmMapSymbol(b)) ||
+                             (isArmMapSymbol(a) && isDataMapSymbol(b)) ||
----------------
MaskRay wrote:
> I think these disjunctions can be simplified to:
> 
> `isThumbMapSymbol(a) == isThumbMapSymbol(b)`
Yes it can, that makes it a lot simpler; thanks.


================
Comment at: ELF/ARMErrataFix.cpp:490
+    auto thumbSym = llvm::find_if(mapSyms, [&](const Defined *ms) {
+      return ms->getName().startswith("$t");
+    });
----------------
MaskRay wrote:
> MaskRay wrote:
> > peter.smith wrote:
> > > MaskRay wrote:
> > > > Above you use `s->getName() == "$t" || s->getName().startswith("$t.");`
> > > I think using ms here is defensible. Beforehand we were processing all symbols, that may or may not be mapping symbols. Here we know that the symbol is a mapping symbol as we filtered them out earlier.
> > > 
> > > I don't mind changing if you'd prefer s instead of ms? 
> > `ms` as the variable name is fine.  Sorry, I should have been clearer. I meant why `.startswith("$t")` is used here. But I see the reason now:
> > 
> > Because the elements contain exclusively mapping symbols:
> > 
> > ```
> >  if (!isArmMapSymbol(def) && !isThumbMapSymbol(def) &&
> >           !isDataMapSymbol(def))
> >         continue;
> > ```
> > 
> > `ms->getName().startswith("$t");` should be sufficient.
> In `init()`
> 
> ```
>     mapSyms.erase(
>         std::unique(mapSyms.begin(), mapSyms.end(), ...),
>       
>         mapSyms.end());
> ```
> 
> You can normalize `mapSyms` to start with a thumb mapping symbol (`erase(begin())` if not thumb).  Then you can do `auto thumbSym = mapSyms.begin();` here.
Thanks for the suggestion, I've implemented that.


================
Comment at: test/ELF/arm-fix-cortex-a8-nopatch.s:36
+ .type target2, %function
+ .local target2
+target2:
----------------
MaskRay wrote:
> `.local` is the default for a defined symbol. Is the directive here to emphasize the symbol is local? Same question goes for other `target*` symbols.
They are definitely intended to be local as the assembler can make more assumptions about resolving fixups without relocations if it is. I can remove them if they are the default as I don't think it is hugely important to emphasise.


================
Comment at: test/ELF/arm-fix-cortex-a8-thunk.s:32
+// THUNK:      00110000 _start:
+// THUNK-NEXT:   110000:      	beq.w	#0 <__ThumbV7PILongThunk_far_away+0x4>
+// THUNK:      00110004 __ThumbV7PILongThunk_far_away:
----------------
MaskRay wrote:
> You can just use spaces, instead of interleaving spaces and tabs before `beq.w`.
> 
> 
> > <__ThumbV7PILongThunk_far_away+0x4>
> 
> Unrelated to this patch, I guess `+0x4` is incorrect. If so, we need an llvm-objdump fix as I mentioned in D66539.
Apologies, forgot to untabify the file before posting. Neither GNU or llvm objdump do a particularly good job with Arm, Thumb branches due to the implicit PC offset.


================
Comment at: test/ELF/arm-fix-cortex-a8-toolarge.s:26
+/// 32-bit Branch spans 2 4KiB regions, preceded by a 32-bit non branch
+/// instruction, expect a patch to be attempted. Unfortunately the branch
+/// cannot reach outside the section so we have to abort the patch.
----------------
MaskRay wrote:
> `expect` -> `expects`?
I've rewritten to "a patch will be attempted".  It is difficult to say in the original, I'd say "We expect" or "The test expects". 


================
Comment at: test/ELF/arm-fix-cortex-a8-toolarge.s:41
+/// 32-bit Branch and link spans 2 4KiB regions, preceded by a 32-bit
+/// non branch instruction, expect a patch to be attempted. Unfortunately the
+/// the BL cannot reach outside the section so we have to abort the patch.
----------------
MaskRay wrote:
> `expect` -> `expects`?
Rewritten to "a patch will be attempted"


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

https://reviews.llvm.org/D67284