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

Thu Sep 7 08:24:50 PDT 2017

peter.smith added a comment.

I've added some inline comments to show what I changed.

================
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
----------------
smeenai wrote:
> 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?
I've reworded the sentence a bit. I've done some work downstream to support the older architectures (needs D36823 before I can upstream)  so I've got a bit more information on what changes are needed.

================
Comment at: ELF/Arch/ARM.cpp:77
+  // to 4MiB
+  ThunkSectionSpacing = 0x1000000;
+  // The pre-created ThunkSections are inserted such that the end of the
----------------
smeenai wrote:
> Is there a general preference for expressing such constants in hex? For me, `16 * 1024 * 1024` is much more obviously 16 MiB.
I don't know if there is a preferred style in lld. From looking around it seems to be mostly decimal and sometimes hex. I've not seen the form 16 * 1024 * 1024 in the lld codebase. I've left the constants as they are for now but I'll happily change them if the consensus is to express them in another way.

================
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
----------------
smeenai wrote:
> 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?
Thanks for the suggestions. I've moved a description of the two parameters to the top and I've put in a diagram. 

================
Comment at: ELF/Relocations.cpp:1066-1069
+  bool NeedTrailingTS;
+  uint32_t Off;
+  uint32_t Limit;
+  InputSection *PrevIS = nullptr;
----------------
smeenai wrote:
> Can't all of these be moved inside the lambda?
Yes they can, I've rewritten the function to account for your suggestions. Thanks very much I think that this simplifies it a bit.

================
Comment at: ELF/Relocations.cpp:1086
+          if (Off >= Limit) {
+            uint32_t ThunkOff = (PrevIS == nullptr)
+                                    ? IS->OutSecOff
----------------
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.   

================
Comment at: ELF/Relocations.cpp:1103
+ThunkSection *ThunkCreator::addThunkSection(OutputSection *OS,
                                             std::vector<InputSection *> *ISR,
                                             uint64_t Off) {
----------------
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.  

================
Comment at: ELF/Relocations.h:165-166
   // passes
   std::map<std::vector<InputSection *> *, std::vector<ThunkSection *>>
       ThunkSections;
 };
----------------
ruiu wrote:
> This map is essentially a map from an `InputSectionDescription` to its thunks, right? Does it make more sense to add `std::vector<ThunkSection *>` to `InputSectionDescription`?
The reasons I had to keep the mapping local were:
- The mapping is only needed when generating Thunks and as most Targets don't need them I wanted to avoid modifying the shared data structures that are visible to the end of the link.
- When there are multiple passes we need to remember not just the Thunks we've added but the ones that we have just added in the latest pass (See D34692) we would either need a map or to add yet another vector to InputSectionDescription.

I'm happy to change it if you'd prefer to store the fields in InputSectionDescription and remove the maps.

https://reviews.llvm.org/D34689