[PATCH] D34689: [LLD][ELF] Pre-create ThunkSections at Target specific intervals

Shoaib Meenai via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 7 12:18:34 PDT 2017


smeenai added a comment.

Since the branch range we're supporting in this patch is ±16 MiB, in theory the thunk sections could be spaced 32 MiB apart instead of 16, with input sections jumping either forward or backward to thunks, correct? I understand it's probably not worth the additional complexity to support that, and I'm not suggesting changing it; I just wanted to confirm my understanding :)



================
Comment at: ELF/Relocations.cpp:1086
+          if (Off >= Limit) {
+            uint32_t ThunkOff = (PrevIS == nullptr)
+                                    ? IS->OutSecOff
----------------
peter.smith wrote:
> smeenai wrote:
> > 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 :)
> If I've understood correctly, for Off to be >= Limit then IS->getSize() must be >=  (ThunkSectionSpacing - ThunkSectionSize), i.e. a gigantic InputSection. These are difficult to handle, and in some cases impossible (if there is a branch in the middle of a Section so large it can't escape the Section). I've put an answer for the 2nd bullet. 
> 
> > Why do we create a thunk section at the start of the first IS, whereas all other thunk sections are at the end of ISs?
> I don't have a good answer for that so I've made it consistent. Initially the code was written when there was simultaneously OutputSections->Sections and InputSectionDescriptions and I probably haven't been aggressive enough at refactoring the function.
> > How do we handle ISs which require multiple thunk sections?
> At present (with the latest modifications) we'll create a ThunkSection after the InputSection. This will be out of range of branches in the earlier part of the InputSection, but in range of the later part. In later patches (D34691 and D34692) we check to see if a ThunkSection is in range, if it isn't we create a new one prior to the InputSection, this should be in range of branches of the earlier part of the InputSection. We can't do anything if the Section is so large that a branch towards the middle can't escape. This might happen with an LTO build of a very large program that isn't compiled with -ffunction-sections so the whole program ends up in a single .text section, but I think it would be rare.   
>  
> 
> 
Yup, I was thinking of the case where `IS->getSize() >= ThunkSectionSpacing - ThunkSectionSize`. It makes sense that we can't handle that case completely, and a comment stating that would probably be helpful.


================
Comment at: ELF/Relocations.cpp:1103
+ThunkSection *ThunkCreator::addThunkSection(OutputSection *OS,
                                             std::vector<InputSection *> *ISR,
                                             uint64_t Off) {
----------------
peter.smith wrote:
> smeenai wrote:
> > Could this parameter be made a const pointer?
> I don't think so, the ISR is the key in the map which usually means it can be const, but in mergeThunks we insert the ThunkSections into ISR so it needs to be non-const at that point. Let me know if I've missed a way of doing it though.  
You're right; I'd missed that.


================
Comment at: ELF/Relocations.cpp:1090-1091
+        }
+        if (NeedTrailingTS)
+          addThunkSection(OS, ISR, Off);
+      });
----------------
As I understand, the design relies on `ThunkSection`s being used by `InputSection`s prior to them. In that case, don't we always need a final trailing `ThunkSection`, to service the `InputSection`s at the end?

In other words, with the current code, I believe an example layout could end up being something like:

```
InputSection
ThunkSection
InputSection
InputSection
InputSection
ThunkSection
InputSection
InputSection
```

I'm wondering if you need a `ThunkSection` at the end too, to service the last two `InputSection`s in my example.


https://reviews.llvm.org/D34689





More information about the llvm-commits mailing list