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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/ARM.cpp:64
   NeedsThunks = true;
+  // Pre-created ThunkSections are spaced roughly 16Mb apart on ARM. This is to
+  // match the most common expected case of a Thumb 2 encoded BL, BLX or B.W
----------------
By "b", I believe you actually meant "B" (i.e. not bit but byte). Probably the best way of representing it is 16 MiB.


================
Comment at: ELF/Relocations.cpp:982
+    });
+    if (ThunkBegin == ThunkEnd)
+      continue;
----------------
Is this the same as `Thunks.empty()`?


================
Comment at: ELF/Relocations.cpp:1072-1074
+  forEachExecInputSection(OutputSections, [&](OutputSection *OS,
+                                              std::vector<InputSection *> *ISR,
+                                              InputSection *IS) {
----------------
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.


================
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);
----------------
Unnecessary style change.


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


https://reviews.llvm.org/D34689