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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: ELF/Relocations.cpp:1086
+          if (Off >= Limit) {
+            uint32_t ThunkOff = (PrevIS == nullptr)
+                                    ? IS->OutSecOff
----------------
smeenai wrote:
> 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.
> * 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 think I just figured my own question out. Conceptually, if the end of the //current// `InputSection` is beyond `Limit`, you'll add a `ThunkSection` at the end of the //previous// `InputSection`. For the very first `InputSection`, there's no previous one, in which case treating the start of the first `InputSection` as the end of the non-existent previous `InputSection` makes sense. You probably wanna go back to that behavior, sorry.

Instead of storing the entire `PrevIS`, how about just having a variable that keeps track of the end address of the previous section? I think that'd make the intent more immediately obvious, and its initialization to `ISR->front()->OutSecOff` could have a brief comment explaining the special casing for the first `InputSection`.


https://reviews.llvm.org/D34689