Page MenuHomePhabricator

[ELF] Make MergeInputSection merging aware of output sections
ClosedPublic

Authored by MaskRay on Sep 12 2019, 8:52 AM.

Details

Summary

Fixes PR38748

mergeSections() calls getOutputSectionName() to get output section
names. Two MergeInputSections may be merged even if they are made
different by SECTIONS commands.

This patch moves mergeSections() after processSectionCommands() and
addOrphanSections() to fix the issue. The new pass is renamed to
OutputSection::finalizeInputSections().

processSectionCommands() and addorphanSections() are changed to add
sections to InputSectionDescription::sectionBases.

finalizeInputSections() merges MergeInputSections and migrates
sectionBases to sections.

For the -r case, we drop an optimization that tries keeping sh_entsize
non-zero. This is for the simplicity of addOrphanSections(). The
updated merge-entsize2.s reflects the change.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Sep 12 2019, 8:52 AM
MaskRay updated this revision to Diff 219936.Sep 12 2019, 9:14 AM

Delete unrelated InputSectionIterator (I tried changing InputSectionDescription::sections from an InputSection* vector to InputSectionBase* vector, but the result is not clean - many subsequent passes assume the elements of sections are InputSections. The size of OutputSection is not a big concern, so I added another field instead)

Fix a comment.

Add llvm::erase_if(inputSections, [](InputSectionBase *s) { return isa<MergeInputSection>(s); }); to fix an assetion failure

MaskRay added a comment.EditedSep 12 2019, 8:44 PM

Two passes assign InputSectionBases to OutputSections:

  • processSectionCommands() calls createInputSectionList to append elements to InputSectionDescription::sectionBases
  • addOrphanSections() calls OutputSection::recordSections() to append elements to InputSectionDescription::sectionBases The change also removes an use of SectionBase::assigned, which will enable a further simplication: D67531.

After an InputSectionBase is added to InputSectionDescription::sectionBases: if it is an InputSection, it will go directly to InputSectionDescription::sections; if it is a MergeInputSection, it will be merged with other MergeInputSections to form a MergeSyntheticSection, then the MergeSyntheticSection (a second-order derived class of InputSection) will be appended to InputSectionDescription::sections.

Generally the approach looks good to me. Lets see what other think.

ELF/LinkerScript.h
171 ↗(On Diff #219936)

These 2 needs comment then I think.

test/ELF/linkerscript/merge-output-sections.s
23 ↗(On Diff #219936)

Can you use --implicit-check-not=section instead (here and above)?

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.

ELF/Driver.cpp
1927 ↗(On Diff #219936)

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();

ELF/LinkerScript.h
171 ↗(On Diff #219936)

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.

ELF/Relocations.cpp
572 ↗(On Diff #219936)

I think it will be worth a comment, to state that we are adding a section after sectionBases has been transferred to sections

573 ↗(On Diff #219936)

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).

MaskRay updated this revision to Diff 220108.Sep 13 2019, 8:09 AM
MaskRay marked 8 inline comments as done.
MaskRay retitled this revision from [ELF] Make SHF_MERGE merging aware of output sections to [ELF] Make MergeInputSection merging aware of output sections.
MaskRay edited the summary of this revision. (Show Details)

Thanks for the review!

Applied suggestions.

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.

ELF/OutputSections.cpp
85 ↗(On Diff #220108)

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."
96 ↗(On Diff #220108)

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.
ELF/Relocations.cpp
574 ↗(On Diff #220108)

One thing we might not have done is set osec->partition if it hasn't been done already. This is something that recordSection() does.

MaskRay updated this revision to Diff 220489.Sep 17 2019, 5:58 AM
MaskRay marked 4 inline comments as done.

Add comments

MaskRay added a subscriber: pcc.Sep 17 2019, 6:07 AM
MaskRay added inline comments.
ELF/Driver.cpp
1927 ↗(On Diff #219936)

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.

ELF/OutputSections.cpp
85 ↗(On Diff #220108)

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).

ELF/Relocations.cpp
573 ↗(On Diff #219936)

Omitting osec->recordSection(sec) should be better.

574 ↗(On Diff #220108)

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.

Thanks for the update. I don't have any more comments at the moment, I'm in favour of making the change. It would be useful to get the perspective of someone coming to the patch for the first time to see if it was easy for them to understand. A possible alternative would be to filter out the MergeInputSections into a separate list. Assign all the other InputSections as before. Then work out where the MergeSyntheticSections would go, create them and insert them into the OutputSections. However I don't think that would be obviously any better.

Thanks for the update. I don't have any more comments at the moment, I'm in favour of making the change. It would be useful to get the perspective of someone coming to the patch for the first time to see if it was easy for them to understand. A possible alternative would be to filter out the MergeInputSections into a separate list. Assign all the other InputSections as before. Then work out where the MergeSyntheticSections would go, create them and insert them into the OutputSections. However I don't think that would be obviously any better.

Thanks for the review. The problem with 2 lists (one for MergeInputSections that will be merged into MergeSyntheticSections, the other for InputSections) is that the relative positions are significant in the output. If we have 2 lists, sortInputSectionswill become more complex. LinkerScript.cpp:395

// Sort sections as instructed by SORT-family commands and --sort-section
// option. Because SORT-family commands can be nested at most two depth
// (e.g. SORT_BY_NAME(SORT_BY_ALIGNMENT(.text.*))) and because the command
...
static void sortInputSections(MutableArrayRef<InputSectionBase *> vec,
                              const SectionPattern &pat) {

... It would be useful to get the perspective of someone coming to the patch for the first time to see if it was easy for them to understand. ...

@ruiu @grimar 🤔

grimar accepted this revision.Tue, Sep 24, 2:21 AM

LGTM

This revision is now accepted and ready to land.Tue, Sep 24, 2:21 AM
This revision was automatically updated to reflect the committed changes.

This change has caused some regressions in some of our local testing. I've filed the bug PR43461 (https://bugs.llvm.org/show_bug.cgi?id=43461) for the issue.