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

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 4 inline comments as done.
peter.smith added a comment.

Thanks very much for the review comments, I'll update the diff shortly.



================
Comment at: ELF/Relocations.cpp:982
+    });
+    if (ThunkBegin == ThunkEnd)
+      continue;
----------------
ruiu wrote:
> Is this the same as `Thunks.empty()`?
Not quite as remove_if won't reduce the size of the vector unless Thunks.erase is called. Given that I managed to forget to consistently use ThunkBegin and ThunkEnd after various review cycles makes me think it is best to call Thunks.erase(). I've made that update as it means that ThunkBegin and ThunkEnd can be removed. 


================
Comment at: ELF/Relocations.cpp:1072-1074
+  forEachExecInputSection(OutputSections, [&](OutputSection *OS,
+                                              std::vector<InputSection *> *ISR,
+                                              InputSection *IS) {
----------------
ruiu wrote:
> It seems `IS` is always a member of `ISR`, but it is not obvious why `forEachExecInputSection` is designed like that. Maybe we should change the type of the function from
> 
> 
>   forEachExecInputSection(ArrayRef<OutputSectionCommand *> OutputSections,
>       std::function<void(OutputSection *, std::vector<InputSection *> *,
>                          InputSection *)> Fn)
> 
> to
> 
>   forEachExecInputSection(ArrayRef<OutputSectionCommand *> OutputSections,
>       std::function<void(OutputSection *, std::vector<InputSection *> *)> Fn)
> 
> and run a for-loop inside the callback function.
I've made the change. The reason I originally had it in forEachExecInputSection() was to reduce the level of indentation needed in the callbacks.
 


================
Comment at: ELF/Relocations.cpp:1134
       if (auto *ISD = dyn_cast<InputSectionDescription>(BC)) {
-        CurTS = nullptr;
-        for (InputSection *IS : ISD->Sections)
+        for (InputSection* IS : ISD->Sections)
           Fn(OS, &ISD->Sections, IS);
----------------
ruiu wrote:
> Unnecessary style change.
I've run clang-format on the file, hopefully there aren't too many just white-space differences.


================
Comment at: ELF/Target.h:75
+  // regular spacings that enable the majority of branches reach the Thunks.
+  uint32_t ThunkSectionSpacing = 0x0;
+
----------------
ruiu wrote:
> nit: just 0 as `GotBaseSymOff` is set to just 0.
Done


https://reviews.llvm.org/D34689