[PATCH] D34689: [LLD][ELF] Pre-create ThunkSections at Target specific intervals
Shoaib Meenai via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 6 14:45:25 PDT 2017
smeenai added a comment.
Sorry about all the nits, but I figured I'd try to get all the mechanical issues out of the way, at least :)
================
Comment at: ELF/Arch/ARM.cpp:69-71
+ // ARM B, BL, BLX range 32MiB
+ // Thumb B.W, BL, BLX range 16MiB
+ // Thumb B<cc>.W range 1MiB
----------------
These are the ranges in both the forward and backward direction, right? As in e.g. a Thumb BLX can branch 16 MiB forward or backward? It would probably be helpful to make that explicit (basically an ASCII-friendly equivalent to ±).
================
Comment at: ELF/Arch/ARM.cpp:72
+ // Thumb B<cc>.W range 1MiB
+ // If branch cannot reach a pre-created ThunkSection a new one will be created
+ // so we can handle the rare case of Thumb 2 conditional branch.
----------------
Grammar nit: If **a** branch cannot reach.
================
Comment at: ELF/Arch/ARM.cpp:73
+ // If branch cannot reach a pre-created ThunkSection a new one will be created
+ // so we can handle the rare case of Thumb 2 conditional branch.
+ // FIXME: lld assumes a CPU with support for ARMv6T2 and above encodings.
----------------
Grammar nit: should either be "the rare case of **a** Thumb 2 conditional branch" or "the rare case of Thumb 2 conditional branch**es**".
================
Comment at: ELF/Arch/ARM.cpp:74-75
+ // so we can handle the rare case of Thumb 2 conditional branch.
+ // FIXME: lld assumes a CPU with support for ARMv6T2 and above encodings.
+ // If support is added for ARMv6T2 then when in use this spacing should drop
+ // to 4MiB
----------------
This is confusing me slightly. It says LLD is assuming support for ARMv6T2 and above, so that includes ARMv6T2, but then it says "if support is added for ARMv6T2", which contradicts the previous sentence. Is there a typo somewhere?
================
Comment at: ELF/Arch/ARM.cpp:76
+ // If support is added for ARMv6T2 then when in use this spacing should drop
+ // to 4MiB
+ ThunkSectionSpacing = 0x1000000;
----------------
Nit: period at end of comment.
================
Comment at: ELF/Arch/ARM.cpp:77
+ // to 4MiB
+ ThunkSectionSpacing = 0x1000000;
+ // The pre-created ThunkSections are inserted such that the end of the
----------------
Is there a general preference for expressing such constants in hex? For me, `16 * 1024 * 1024` is much more obviously 16 MiB.
================
Comment at: ELF/Arch/ARM.cpp:78
+ ThunkSectionSpacing = 0x1000000;
+ // The pre-created ThunkSections are inserted such that the end of the
+ // precreated ThunkSection is almost certain to be within range a branch
----------------
If I'm understanding this correctly, the indented layout is:
```
|-------------------------------------------------------|
| Section: ThunkSectionSpacing - ThunkSectionSize bytes |
|-------------------------------------------------------|
| ThunkSection: ThunkSectionSize bytes |
|-------------------------------------------------------|
| Section: ThunkSectionSpacing - ThunkSectionSize bytes |
|-------------------------------------------------------|
| ThunkSection: ThunkSectionSize bytes |
|-------------------------------------------------------|
| ... |
```
When I was reading this originally, I got confused because I thought the intended layout was instead:
```
|-------------------------------------------------------|
| Section: ThunkSectionSpacing bytes |
|-------------------------------------------------------|
| ThunkSection: ThunkSectionSize bytes |
|-------------------------------------------------------|
| Section: ThunkSectionSpacing bytes |
|-------------------------------------------------------|
| ThunkSection: ThunkSectionSize bytes |
|-------------------------------------------------------|
| ... |
```
In hindsight, the name `ThunkSectionSpacing` makes the intended layout pretty clear, but it took me a while to grasp that. Maybe an explicit comment to that effect or an ASCII diagram could help?
================
Comment at: ELF/Arch/ARM.cpp:79
+ // The pre-created ThunkSections are inserted such that the end of the
+ // precreated ThunkSection is almost certain to be within range a branch
+ // from the start of the Section, or immediately following the previous
----------------
Grammar nit: within range **of** a branch
================
Comment at: ELF/Arch/ARM.cpp:80
+ // precreated ThunkSection is almost certain to be within range a branch
+ // from the start of the Section, or immediately following the previous
+ // ThunkSection. Allow for 16384 12 byte Thunks per ThunkSectionSpacing
----------------
Is the second part of the sentence supposed to imply that the start of the Section will be immediately after the end of the previous ThunkSection? It's not really clear to me.
================
Comment at: ELF/Arch/ARM.cpp:81
+ // from the start of the Section, or immediately following the previous
+ // ThunkSection. Allow for 16384 12 byte Thunks per ThunkSectionSpacing
+ ThunkSectionSize = 0x30000;
----------------
Nit: period at the end of comment.
================
Comment at: ELF/Arch/ARM.cpp:82
+ // ThunkSection. Allow for 16384 12 byte Thunks per ThunkSectionSpacing
+ ThunkSectionSize = 0x30000;
}
----------------
Same here; `16384 * 12` would be clearer to me.
================
Comment at: ELF/Relocations.cpp:981
+ // Remove ThunkSections that contain no Thunks
+ llvm::erase_if(Thunks,
----------------
Nit: period at the end of the comment.
================
Comment at: ELF/Relocations.cpp:1021
+ // FIXME: When range extension thunks are supported we will need to check
+ // that the ThunkSection is in range of the caller
+ if (!ThunkSections[ISR].empty())
----------------
Period nit.
================
Comment at: ELF/Relocations.cpp:1027
+ // where no pre-created ThunkSections are in range by creating a new one in
+ // range for now it is unreachable
+ llvm_unreachable("Must have created at least one ThunkSection per ISR");
----------------
There's a run-on sentence; it should probably be "by creating a new one in range**;** for now, it is unreachable."
================
Comment at: ELF/Relocations.cpp:1062
+// offsets that are multiples of a Target specific branch range.
+// For an InputSectionRange that is smaller than the range then a single
+// ThunkSection at the end of the range will do.
----------------
Grammar nit: For an InputSectionRange that is smaller than the range, a single ThunkSection at the end of the range will do.
================
Comment at: ELF/Relocations.cpp:1066-1069
+ bool NeedTrailingTS;
+ uint32_t Off;
+ uint32_t Limit;
+ InputSection *PrevIS = nullptr;
----------------
Can't all of these be moved inside the lambda?
================
Comment at: ELF/Relocations.cpp:1074
+ OutputSections,
+ [&](OutputSection *OS, std::vector<InputSection *> *ISR) {
+ for (InputSection *IS : *ISR) {
----------------
Could `ISR` be made a const pointer?
================
Comment at: ELF/Relocations.cpp:1077
+ if (ISR != PrevISR) {
+ NeedTrailingTS = true;
+ Off = 0;
----------------
If I'm understanding this code correctly, this `if` will only be entered in the first iteration of the loop. If that's the case, it's probably cleaner to just pull the body out and place it above the loop? That should also allow you to get rid of `PrevISR`.
(If `ISR` can be empty, you'd also need to check for that, of course.)
================
Comment at: ELF/Relocations.cpp:1078
+ NeedTrailingTS = true;
+ Off = 0;
+ Limit = IS->OutSecOff +
----------------
Isn't this assignment dead, given that you unconditionally assign `Off` on line 1084?
================
Comment at: ELF/Relocations.cpp:1086
+ if (Off >= Limit) {
+ uint32_t ThunkOff = (PrevIS == nullptr)
+ ? IS->OutSecOff
----------------
What happens if e.g. you have a single `IS` in this `ISR`, and its `Off >= Limit`? If I'm understanding this code correctly, you'll create a thunk at the start of the `IS` (i.e. `IS->OutSecOff`), and you won't create one at the end (because `NeedTrailingTS` will get set to false), but isn't the one at the end also needed?
More generally, I'm not sure about:
* Why do we create a thunk section at the start of the first `IS`, whereas all other thunk sections are at the end of `IS`s?
* How do we handle `IS`s which require multiple thunk sections?
I feel like I'm missing something pretty obvious, but it doesn't hurt to ask :)
================
Comment at: ELF/Relocations.cpp:1096-1097
+
+ if (ISR->back() == IS && NeedTrailingTS)
+ addThunkSection(OS, ISR, Off);
+ }
----------------
Remove the check for `ISR->back() == IS` and move this after the loop?
================
Comment at: ELF/Relocations.cpp:1103
+ThunkSection *ThunkCreator::addThunkSection(OutputSection *OS,
std::vector<InputSection *> *ISR,
uint64_t Off) {
----------------
Could this parameter be made a const pointer?
================
Comment at: ELF/Relocations.cpp:1135
for (BaseCommand *BC : OS->Commands)
if (auto *ISD = dyn_cast<InputSectionDescription>(BC)) {
+ Fn(OS, &ISD->Sections);
----------------
You can get rid of the braces on this `if` now.
https://reviews.llvm.org/D34689
More information about the llvm-commits
mailing list