[PATCH] D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 6 20:29:23 PDT 2019
MaskRay added a comment.
I have read through the file but haven't gone through the logic in a debugger. Some suggestions and questions inlined.
================
Comment at: ELF/ARMErrataFix.cpp:42
+// - There is a 32-bit Thumb-2 branch instruction with an address of the form
+// xxxxxxFFE. The first 2 bytes of the instruction are in 4KiB region 1, the
+// second 2 bytes are in region 2.
----------------
What are "region 1" and "region 2"?
================
Comment at: ELF/ARMErrataFix.cpp:63
+
+class lld::elf::Patch657417Section : public SyntheticSection {
+public:
----------------
Omit `lld::elf`. This is not needed because of `using namespace lld::elf;` above. I'm thinking whether it is better to use
```
// delete use namespace lld;
namespace lld {
namespace elf {
...
}
}
```
for new files.
================
Comment at: ELF/ARMErrataFix.cpp:80
+ Symbol *patchSym;
+ // A decoding of the branch instruction at patcheeOffset
+ uint32_t instr;
----------------
Missing full stop.
================
Comment at: ELF/ARMErrataFix.cpp:87
+
+// Return true if the half-word is the first half of a 32-bit instruction.
+// armv7ar architecture reference manual 32-bit instruction encoding
----------------
> Return true if the half-word is the first half of a 32-bit instruction.
Do you mean the first half word?
================
Comment at: ELF/ARMErrataFix.cpp:88
+// Return true if the half-word is the first half of a 32-bit instruction.
+// armv7ar architecture reference manual 32-bit instruction encoding
+// First halfword of a 32-bit instruction is of form:
----------------
32-bit Thumb instruction encoding? (The heading as used in the manual)
================
Comment at: ELF/ARMErrataFix.cpp:91
+// | 1 1 1 | op1 (2) | op2 (7) | x (4) |
+// With op1 == 0b00
+static bool is32bitInstruction(uint16_t hw) {
----------------
op1 != 0b00 ?
op1 == 0b00 encodes a 16-bit Thumb instruction.
================
Comment at: ELF/ARMErrataFix.cpp:156
+ else {
+ write16le(buf, 0xf000);
+ write16le(buf + 2, 0x9000);
----------------
Use one write32le?
================
Comment at: ELF/ARMErrataFix.cpp:181
+// the source.
+static bool branchToFirstRegion(const InputSection *isec, uint64_t off,
+ uint32_t instr) {
----------------
> branchToFirstRegion
Is "first region" the term used to refer to `destAddr < sourceAddr && (destAddr & 0xfffff000) == (sourceAddr & 0xfffff000);`
================
Comment at: ELF/ARMErrataFix.cpp:231
+ uint64_t isecAddr = isec->getVA(0);
+ // Advance Off so that (ISAddr + Off) modulo 0x1000 is at least 0xffa. We
+ // need to check for a 32-bit instruction immediately before a 32-bit branch
----------------
ISAddr + Off -> isecAddr + off
================
Comment at: ELF/ARMErrataFix.cpp:248
+ const uint8_t *buf = isec->data().begin();
+ // ARMv7-A Thumb 32-bit instructions are encoded as pairs of l6-bit
+ // little-endian halfwords.
----------------
l6 -> 16
================
Comment at: ELF/ARMErrataFix.cpp:269
+ }
+ off += 0x1000;
+ return scanRes;
----------------
Say `off % 0x1000 = 0xffc` before the assignment. The erratum sequence in the next page starts at 0xffa. This increment will skip that erratum sequence.
Is this a possible scenario?
================
Comment at: ELF/ARMErrataFix.cpp:286
+ auto isArmMapSymbol = [](const Symbol *b) {
+ return b->getName() == "$a" || b->getName().startswith("$a.");
+ };
----------------
In include/llvm/Object/ELFObjectFile.h and llvm-objdump, we just use `startswith("$a")`. Is there a reason to check `"$a."`? Below, you just use `return ms->getName().startswith("$t");`
================
Comment at: ELF/ARMErrataFix.cpp:297
+ for (InputFile *file : objectFiles) {
+ auto *f = cast<ObjFile<ELF32LE>>(file);
+ for (Symbol *b : f->getLocalSymbols()) {
----------------
Should there be a check to reject ELF32BE beforehand?
It can be placed in Driver.cpp:checkOptions.
================
Comment at: ELF/ARMErrataFix.cpp:314
+ std::vector<const Defined *> &mapSyms = kv.second;
+ if (mapSyms.size() <= 1)
+ continue;
----------------
`if (mapSyms.size() <= 1) continue` can be deleted.
================
Comment at: ELF/ARMErrataFix.cpp:342
+ // Set the outSecOff of patches to the place where we want to insert them.
+ // We use a similar strategy to Thunk placement, using 1 MiB as the range of
+ // the Thumb-2 conditional branch with a contingency accounting for thunk
----------------
initial Thunk placement?
================
Comment at: ELF/ARMErrataFix.cpp:344
+ // the Thumb-2 conditional branch with a contingency accounting for thunk
+ // generation
+ auto patchIt = patches.begin();
----------------
Missing full stop.
================
Comment at: ELF/ARMErrataFix.cpp:350
+ if (isecLimit > patchUpperBound) {
+ while (patchIt != patchEnd) {
+ if ((*patchIt)->getBranchAddr() - outSecAddr >= prevIsecLimit)
----------------
Or use `for (; patchIt != patchEnd; ++patchIt)`
================
Comment at: ELF/ARMErrataFix.cpp:351
+ while (patchIt != patchEnd) {
+ if ((*patchIt)->getBranchAddr() - outSecAddr >= prevIsecLimit)
+ break;
----------------
Is it guaranteed that getBranchAddr() is monotonically increasing?
================
Comment at: ELF/ARMErrataFix.cpp:363
+
+ // merge all patch sections. We use the outSecOff assigned above to
+ // determine the insertion point. This is ok as we only merge into an
----------------
Capitalize.
================
Comment at: ELF/ARMErrataFix.cpp:370
+ auto mergeCmp = [](const InputSection *a, const InputSection *b) {
+ if (a->outSecOff < b->outSecOff)
+ return true;
----------------
```
if (a->outSecOff != b->outSecOff)
return a->outSecOff < b->outSecOff;
return isa<Patch657417Section>(a) && !isa<Patch657417Section>(b);
```
================
Comment at: ELF/ARMErrataFix.cpp:503
+bool ARMErr657417Patcher::createFixes() {
+ if (initialized == false)
+ init();
----------------
!initialized
================
Comment at: ELF/ARMErrataFix.h:27
+public:
+ // return true if Patches have been added to the OutputSections.
+ bool createFixes();
----------------
Capitalize
================
Comment at: ELF/ARMErrataFix.h:42
+ // the ranges of code and data in an executable InputSection.
+ std::map<InputSection *, std::vector<const Defined *>> sectionMap;
+
----------------
`std::map -> DenseMap`
`std::map` may give a false impression that the key is ordered. InputSection's are allocated from a BumpPtrAllocator. While the allocator is called "bump", that just refers to the allocation strategy within a slab. When a new slab is allocated by malloc, it is not guaranteed the address will monotonically increase.
================
Comment at: ELF/Writer.cpp:1573
- if (config->fixCortexA53Errata843419) {
+ if (config->fixCortexA53Errata843419 || config->fixCortexA8) {
if (changed)
----------------
Alternatively,
```
if (config->fixCortexA53Errata843419) {
if (changed)
script->assignAddresses();
changed |= a64p.createFixes();
}
if (config->fixCortexA8) {
if (changed)
script->assignAddresses();
changed |= a32p.createFixes();
}
```
if you think it is clearer.
================
Comment at: test/ELF/arm-fix-cortex-a8-recognize.s:4
+// RUN: ld.lld --fix-cortex-a8 -verbose %t.o -o %t2 2>&1 | FileCheck %s
+// RUN: llvm-objdump -d %t2 -start-address=0x1a004 --stop-address=0x1a024 --no-show-raw-insn | FileCheck --check-prefix=CHECK-PATCHES %s
+// RUN: llvm-objdump -d %t2 -start-address=0x12ffa --stop-address=0x13002 --no-show-raw-insn | FileCheck --check-prefix=CALLSITE1 %s
----------------
`-start-address` -> `--start-address`
================
Comment at: test/ELF/arm-fix-cortex-a8-recognize.s:14
+
+// CHECK: ld.lld: detected cortex-a8-657419 erratum sequence starting at 12FFE in unpatched output.
+// CHECK-NEXT: ld.lld: detected cortex-a8-657419 erratum sequence starting at 13FFE in unpatched output.
----------------
Align `ld.lld`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67284/new/
https://reviews.llvm.org/D67284
More information about the llvm-commits
mailing list