Page MenuHomePhabricator

Replace MergeOutputSection with synthetic input section MergeSection.
ClosedPublic

Authored by rafael on Jan 31 2017, 8:25 AM.

Details

Reviewers
ruiu
grimar
Summary

With a synthetic merge section we can have, for example, a single .rodata section with stings, fixed sized constants and non merge constants.

I can be simplified further by not setting Entsize, but that is probably better done is a followup patch.

This should allow some cleanup is the linker script code now that every output section command maps to just one output section.

Diff Detail

Event Timeline

rafael created this revision.Jan 31 2017, 8:25 AM
grimar accepted this revision.Jan 31 2017, 8:42 AM

LGTM with single comment, though is up to you and Rui I think to select what is looking better there.

ELF/InputSection.cpp
131

By the way, diff5 of my patch solved this in a bit different way:

template <class ELFT>
void OutputSection<ELFT>::addSection(InputSectionData *C) {
...
  if (auto *Sec = dyn_cast<MergeSyntheticSection<ELFT>>(C))
    Sec->setOutputSection(this);

setOutputSection just assigned OutSec member. I see 2 positive things of doing that:

  1. We use public member OutSec in many places I think, having another getOutputSection() is a bit confuzing
  2. It just simplifies things.

Rui did not like that piece of code it seems, though probably it was better than one more method,

This revision is now accepted and ready to land.Jan 31 2017, 8:42 AM
ruiu edited edge metadata.Jan 31 2017, 2:07 PM

Please update the commit message to describe this patch.

ELF/InputSection.cpp
123

s/0 /0/

but probably writing it in three lines would be easier to read.

ELF/Writer.cpp
153

I think this code has a subtle bug. S->Flags can be uint64_t, but SHF_GROUP and SHF_COMPRESSED are of unsigned int. I think this needs explicit casts.

return S->Flags & ~(uintX_t)SHF_GROUP & ~(uintX_t)SHF_COMPRESSED;
157

Update this comment -- this function does not add sections to the end.

grimar added inline comments.Feb 1 2017, 1:39 AM
ELF/Writer.cpp
157

Ah, I did not notice it at first. That is done to not allow .comment section move and do not break tests I guess.

I think that logic is fine, though we have at least one place where write in comment
that synthetics are placed to the end:

static void removeUnusedSyntheticSections(std::vector<OutputSectionBase *> &V) {
  // Input synthetic sections are placed after all regular ones. We iterate over
  // them all and exit at first non-synthetic.
  for (InputSectionBase<ELFT> *S : llvm::reverse(Symtab<ELFT>::X->Sections)) {

That place above does not affect merge synthetic sections, because them are always !empty(),
but probably also needs some comment update.

As an alternative - in the following patch we probably can make .comment section to be synthetic from start
and change this method to what it did initially: place them to the end.

rafael edited the summary of this revision. (Show Details)Feb 1 2017, 6:24 AM
rafael updated this revision to Diff 86625.Feb 1 2017, 6:40 AM
ruiu accepted this revision.Feb 1 2017, 5:38 PM

LGTM

ELF/Writer.cpp
1025

s/All Input/All input/

grimar closed this revision.Aug 17 2017, 2:33 AM

r294005