[PATCH] D67504: [ELF] Make MergeInputSection merging aware of output sections

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the updates. I've made a few suggestions about adding comments to define what addSection and recordSection do and when they should be used, and in the case of addSection what they should be called. I also think that there may be a case for a postFinalizeAddSection(InputSection *isec) function to do the actions that we are inlining for the copyRelocations on Relocations.cpp:570. This would make it easier to know what was needed for any other InputSections that needed to be added later.



================
Comment at: ELF/OutputSections.cpp:85
 }
 
+void OutputSection::recordSection(InputSectionBase *isec) {
----------------
The names recordSection() and addSection() are very close, and at first glance it can be difficult to know what the responsibilities of each of the functions are and when it is appropriate to call them.

"Record that isec will be placed in this OutputSection. This does not become permanent until finalizeInputSections is called. It should not be used after finalizeInputSections is called, if you need to add an InputSection post finalizeInputSections, then you must do the following:
- Find or create an InputSectionDescription to hold InputSection.
- Add the InputSection to the InputSectionDesciption::sections
- Det the OutputSection partition.
- Call addSection(isec) to commit the section."



================
Comment at: ELF/OutputSections.cpp:96
+
 void OutputSection::addSection(InputSection *isec) {
   if (!hasInputSections) {
----------------
I don't think that function's name is matching what it does anymore. Maybe something like commitSection. It looks to be doing:
- Initializing or checking flags are consistent with other InputSections.
- Updating the type and flags.
- Updating the alignment.


================
Comment at: ELF/Relocations.cpp:574
+  // sections.
+  osec->addSection(sec);
+  if (osec->sectionCommands.empty() ||
----------------
One thing we might not have done is set osec->partition if it hasn't been done already. This is something that recordSection() does.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67504/new/

https://reviews.llvm.org/D67504