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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a subscriber: pcc.
MaskRay added inline comments.


================
Comment at: ELF/Driver.cpp:1927
 
+  // Merge MergeInputSections into MergeSyntheticSections.
+  for (BaseCommand *base : script->sectionCommands)
----------------
peter.smith wrote:
> 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();
> Alternatively explicitly emptying the sectionBases would work.

In finalizeInputSections(): `cmd->sectionBases.clear();`

This has already been done. I agree this point should be highlighted, and thanks for the wording!

> it could be used by mistake later on instead of sections.

`cmd->sectionBases.clear();` should detect such mistakes immediately.


================
Comment at: ELF/OutputSections.cpp:85
 }
 
+void OutputSection::recordSection(InputSectionBase *isec) {
----------------
peter.smith wrote:
> 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."
> 
Thanks a lot for the suggestion!

> Delete the OutputSection partition.

I'll delete this step.  `partition = isec->partition;` is here because LinkerScript.cpp:addInputSec ~L641 checks whether an existing OutputSection can be reused by a new orphan section.

```
  TinyPtrVector<OutputSection *> &v = map[outsecName];
  for (OutputSection *sec : v) {
    if (sec->partition != isec->partition) /// checks partition
      continue;
    sec->recordSection(isec);
    return nullptr;
  }
```

So we have to initialize the partition early.

In Relocations.cpp, I'll reorder `osec->commitSection(sec);` to be the last step to keep consistent with the comment. (`osec->commitSection(sec);` can be either the first or the last step).


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

```
// Linker generated sections which can be used as inputs and are not specific to
// a partition.
struct InStruct {
  InputSection *armAttributes;
  BssSection *bss;
  BssSection *bssRelRo;
```

I think copy relocations from all paritions share the same .bss or .bss.rel.o in the main partition. Though, I'm not sure if the ld.so implementation has considered the interaction between partitions and copy relocations. @pcc In any case, the patch shouldn't change the behavior here.


================
Comment at: ELF/Relocations.cpp:573
+  osec->addSection(sec);
+  if (osec->sectionCommands.empty() ||
+      !isa<InputSectionDescription>(osec->sectionCommands.back()))
----------------
peter.smith wrote:
> 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).
> 
Omitting `osec->recordSection(sec)` should be better.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67504