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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I agree that we'll need something like this for string merging to be used on many embedded systems. I'm a bit concerned about having two semi-parallel vectors sectionBases and sections as we've got some redundancy there and that could lead to problems. At the moment sectionBases is only used for a small part of the link, yet as we don't empty it, it could be used by mistake later on instead of sections. If having two vectors is unavoidable then we should be clear on what the policy is for using one or the other. If we only intend sectionBases to be used for a short time, I'd empty them or prevent them from being used once we're done with them.



================
Comment at: ELF/Driver.cpp:1927
 
+  // Merge MergeInputSections into MergeSyntheticSections.
+  for (BaseCommand *base : script->sectionCommands)
----------------
To me the most important thing here is sectionBases is no longer used after this point.
I suggest something like:
Migrate the sectionBases to sections. This includes merging MergeInputSections into a single MergeSyntheticSection. From this point onwards OutputSection::sections should be used instead of OutputSection::sectionBases should not.

Alternatively explicitly emptying the sectionBases would work.
sec->sectionBases.empty();


================
Comment at: ELF/LinkerScript.h:171
 
+  std::vector<InputSectionBase *> sectionBases;
   std::vector<InputSection *> sections;
----------------
grimar wrote:
> These 2 needs comment then I think.
Agreed. One thing that this does do is introduce a section of Driver.cpp where we should use sectionBases and after the merge only sections. This is difficult to discover and we'll need to be careful to catch people using the wrong one.


================
Comment at: ELF/Relocations.cpp:572
+  osec->recordSection(sec);
+  osec->addSection(sec);
+  if (osec->sectionCommands.empty() ||
----------------
I think it will be worth a comment, to state that we are adding a section after sectionBases has been transferred to sections


================
Comment at: ELF/Relocations.cpp:573
+  osec->addSection(sec);
+  if (osec->sectionCommands.empty() ||
+      !isa<InputSectionDescription>(osec->sectionCommands.back()))
----------------
Unless I'm missing something, won't recordSection(sec) have done the equivalent of:

```
 if (osec->sectionCommands.empty() ||
    !isa<InputSectionDescription>(osec->sectionCommands.back()))
  osec->sectionCommands.push_back(make<InputSectionDescription>(""));
```
So you could replace with?

```
auto *isd = cast<InputSectionDescription>(osec->sectionCommands.back());
isd->sections.push_back(sec);
```
Or if sectionBases is not used beyond this point perhaps don't call osec->recordSection(sec).



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67504