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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: ELF/ARMErrataFix.cpp:202
+  // or the PLT.
+  auto relIt = llvm::find_if(isec->relocations, [=](const Relocation &r) {
+    return r.offset == off &&
----------------
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? 


================
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(
----------------
`source + isec->getSize() + 0x100` or `isec->getVA() + isec->getSize() + 0x100`?


================
Comment at: ELF/ARMErrataFix.cpp:263
+  const uint8_t *buf = isec->data().begin();
+  // ARMv7-A Thumb 32-bit instructions are encoded 2 consecutive l6-bit
+  // little-endian halfwords.
----------------
`l6 -> 16` or just delete `l6-bit`.


================
Comment at: ELF/ARMErrataFix.cpp:336
+                    [=](const Defined *a, const Defined *b) {
+                      return (isArmMapSymbol(a) && isArmMapSymbol(b)) ||
+                             (isArmMapSymbol(a) && isDataMapSymbol(b)) ||
----------------
I think these disjunctions can be simplified to:

`isThumbMapSymbol(a) == isThumbMapSymbol(b)`


================
Comment at: ELF/ARMErrataFix.cpp:479
+  for (InputSection *isec : isd.sections) {
+    //  LLD doesn't use the erratum sequence in SyntheticSections.
+    if (isa<SyntheticSection>(isec))
----------------
Superfluous space in the comment.


================
Comment at: ELF/ARMErrataFix.cpp:483
+    // Use sectionMap to make sure we only scan Thumb code and not Arm or inline
+    // data. We have already sorted MapSyms in ascending order and removed
+    // consecutive mapping symbols of the same type. Our range of executable
----------------
`MapSyms` -> `mapSyms`


================
Comment at: ELF/ARMErrataFix.cpp:490
+    auto thumbSym = llvm::find_if(mapSyms, [&](const Defined *ms) {
+      return ms->getName().startswith("$t");
+    });
----------------
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.


================
Comment at: test/ELF/arm-fix-cortex-a8-nopatch.s:36
+ .type target2, %function
+ .local target2
+target2:
----------------
`.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.


================
Comment at: test/ELF/arm-fix-cortex-a8-nopatch.s:123
+
+// CALLSITE7:    19002:            bl      #-4
----------------
Add a line: `CALLSITE7:      00019002 target7`


================
Comment at: test/ELF/arm-fix-cortex-a8-recognize.s:202
+// CHECK-PATCHES-NEXT:    1a020:        b       #-52
+
----------------
Delete the trailing empty line.


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


================
Comment at: test/ELF/arm-fix-cortex-a8-toolarge.s:3
+// RUN: llvm-mc -filetype=obj -triple=armv7a-linux-gnueabihf --arm-add-build-attributes %s -o %t.o
+// RUN: ld.lld --fix-cortex-a8 -verbose %t.o -o %t2 2>&1 | FileCheck %s
+/// Test that we warn, but don't attempt to patch when it is impossible to
----------------
`%t2` -> `/dev/null` because it is not used.

PS: I usually use `%t`/`%t.so` as the executable/DSO name for the object file `%t.o`. The suffix (usually empty, `1` or `2`) indicates their relations.


================
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.
----------------
`expect` -> `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.
----------------
`expect` -> `expects`?


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

https://reviews.llvm.org/D67284