[PATCH] D34692: [LLD][ELF] Add support for multiple passes to createThunks()

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Mostly nits again. The logic looks good to me, though I'm far from being completely familiar with all parts of the existing code.



================
Comment at: ELF/Relocations.cpp:977
+// are added at link time rather than compile time as the decision on whether
+// a thunk is needed, such as the caller and callee are out of range, can only
+// be made at link time.
----------------
Grammar nit: such as the caller and the callee **being** out of range


================
Comment at: ELF/Relocations.cpp:983
+// the thunk needs to be placed in the program such that the caller can reach
+// the thunk and the thunk can reach the callee, furthermore adding thunks to
+// the program alters addresses, which can mean more thunks etc.
----------------
Grammar nit: the comma should be a semicolon, and there should be a comma after furthermore.


================
Comment at: ELF/Relocations.cpp:1011
+// Thunk placement algorithm:
+// For Mips LA25 ThunkSections the placement is explicit, it has to be before
+// getTargetInputSection()
----------------
Grammar nit: the comma should be either a colon or a semicolon, and there should be a period at the end of the sentence.


================
Comment at: ELF/Relocations.cpp:1022
+//
+// In lld for the first pass we pre-create one or more ThunkSections per
+// InputSectionDescription at Target specific intervals. A ThunkSection is
----------------
Grammar nit: In lld, for the first pass, ...


================
Comment at: ELF/Relocations.cpp:1025
+// placed so that the estimated end of the ThunkSection is within range of the
+// start of the InputSectionDescription or the last ThunkSection. For example:
+// InputSectionDescription
----------------
I think "previous" sounds clearer than "last" here.


================
Comment at: ELF/Relocations.cpp:1039
+// of work. An important principle is that it is not an error if a Thunk cannot
+// be placed in a pre-created ThunkSection, when this happens we create a new
+// ThunkSection placed next to the caller. This allows us to handle the vast
----------------
Grammar nit: in a pre-created ThunkSection; when this happens, ...


================
Comment at: ELF/Relocations.cpp:1055
+
+// Remove ThunkSections that are empty, this should only be the initial set
+// precreated on pass 0.
----------------
Grammar nit: comma should be semicolon.


================
Comment at: ELF/Relocations.cpp:1100
+          // In general place Thunk Sections without specific targets before
+          // non-Thunk Sections
+          return true;
----------------
Nit: period at the end.


================
Comment at: ELF/Relocations.cpp:1259
+// All InputSections that may need a Thunk are reachable from
+// OutputSectionCommands
 //
----------------
Nit: Period at the end.


================
Comment at: ELF/Relocations.cpp:1284
+    // With Thunk Size much smaller than branch range we expect to
+    // converge quickly, if we get to 10 something has gone wrong.
+    fatal("thunk creation not converged");
----------------
Grammar nit: comma ->semicolon.


================
Comment at: ELF/Relocations.h:172
+  // All the ThunkSections that we have created, organised by InputSectionRange
   // will contain a mix of ThunkSections that have been created this pass, and
+  // ThunkSections that have been merged into InputSectionRanges on previous
----------------
I think it would be clearer to put a period at the end of InputSectionRange above, and make this line its own sentence, i.e. "This will contain ...".


================
Comment at: ELF/Relocations.h:178
+
+  // All the ThunkSections that we have created, organised by InputSectionRange
+  std::map<std::vector<InputSection *> *, std::vector<ThunkSection *>>
----------------
"All the ThunkSections that we have created **in the current pass**", perhaps? The comment as it stands doesn't make the difference between `ThunkSections` and `NewThunkSections` apparent.

Nit: period at the end of the sentence.


https://reviews.llvm.org/D34692