Page MenuHomePhabricator
Differential D84001 Diff 283485 lld/ELF/Writer.cpp

Changeset View

Standalone View

View Options

lld/ELF/Writer.cpp

Show First 20 Lines • Show All 1,604 Lines • ▼ Show 20 Lines while (nonScriptI != e) {
std::rotate(pos, nonScriptI, end); std::rotate(pos, nonScriptI, end);
nonScriptI = end; nonScriptI = end;
} }
script->adjustSectionsAfterSorting(); script->adjustSectionsAfterSorting();
} }
static bool compareByFilePosition(InputSection *a, InputSection *b) { static bool compareByFilePosition(InputSection *a, InputSection *b) {
InputSection *la = a->getLinkOrderDep(); InputSection *la = a->flags & SHF_LINK_ORDER ? a->getLinkOrderDep() : nullptr;
InputSection *lb = b->getLinkOrderDep(); InputSection *lb = b->flags & SHF_LINK_ORDER ? b->getLinkOrderDep() : nullptr;
// SHF_LINK_ORDER sections with non-zero sh_link are ordered before others. // SHF_LINK_ORDER sections with non-zero sh_link are ordered before others.
grimarUnsubmitted
Done
ReplyInline Actions

This comment needs updating.

grimar: This comment needs updating.
if (!la || !lb) if (!la || !lb)
return la && !lb; return la && !lb;
OutputSection *aOut = la->getParent(); OutputSection *aOut = la->getParent();
OutputSection *bOut = lb->getParent(); OutputSection *bOut = lb->getParent();
if (aOut != bOut) if (aOut != bOut)
return aOut->addr < bOut->addr; return aOut->addr < bOut->addr;
return la->outSecOff < lb->outSecOff; return la->outSecOff < lb->outSecOff;
} }
template <class ELFT> void Writer<ELFT>::resolveShfLinkOrder() { template <class ELFT> void Writer<ELFT>::resolveShfLinkOrder() {
for (OutputSection *sec : outputSections) { for (OutputSection *sec : outputSections) {
if (!(sec->flags & SHF_LINK_ORDER)) if (!(sec->flags & SHF_LINK_ORDER))
continue; continue;
// The ARM.exidx section use SHF_LINK_ORDER, but we have consolidated // The ARM.exidx section use SHF_LINK_ORDER, but we have consolidated
// this processing inside the ARMExidxsyntheticsection::finalizeContents(). // this processing inside the ARMExidxsyntheticsection::finalizeContents().
if (!config->relocatable && config->emachine == EM_ARM && if (!config->relocatable && config->emachine == EM_ARM &&
sec->type == SHT_ARM_EXIDX) sec->type == SHT_ARM_EXIDX)
continue; continue;
// Link order may be distributed across several InputSectionDescriptions // Link order may be distributed across several InputSectionDescriptions.
// but sort must consider them all at once. // Sorting is performed separately.
psmithUnsubmitted
Not Done
ReplyInline Actions

I think we need to be careful here. This is not a problem for .ARM.exidx as we handle it as a special case, but if we followed this pattern it could cause problems. For example a typical ld.bfd linker script has:

.ARM.exidx : { *(.arm.exidx) *(.arm.exidx.*) }

The intent is to form a single table with the sorting order considered all at once. The script above would create two InputSectionDescriptions and if sorted independently could be in the wrong order. There may be other similar cases that follow the .name and .name.suffix pattern.

A possible heuristic is that if all the input section descriptions have SHF_LINK_ORDER then consider them all at once. If there is a mix of SHF_LINK_ORDER and non SHF_LINK_ORDER then it is unlikely that the ordering requirements are being used.

psmith: I think we need to be careful here. This is not a problem for .ARM.exidx as we handle it as a…
pccUnsubmitted
Not Done
ReplyInline Actions
.ARM.exidx : { *(.arm.exidx) *(.arm.exidx.*) }

I think this unambiguously means to sort the groups separately. If the intent was to sort them together, it should have been written as

.ARM.exidx : { *(.arm.exidx .arm.exidx.*) }

But it doesn't look like we can expect linker scripts to be doing this in the first place though? Looking at binutils I see things in the default linker scripts like

ld/testsuite/ld-arm/arm.ld:  .ARM.exidx : { *(.ARM.exidx*) }

which would lead to the correct behavior. And looking at the history of that file it's been that way since the beginning. Presumably if people are copy/pasting from binutils into their linker scripts then they would have a similar clause to this which would be handled correctly.

A possible heuristic is that if all the input section descriptions have SHF_LINK_ORDER then consider them all at once. If there is a mix of SHF_LINK_ORDER and non SHF_LINK_ORDER then it is unlikely that the ordering requirements are being used.

I don't think we should work around broken linker scripts like this, at least not without more evidence that it is a problem, since it will complicate cases involving symbol assignments. For example, in this case:

.foo {
 start_foo = .;
  *(.foo)
  stop_foo = .;
  start_bar = .;
  *(.bar)
  stop_bar = .;
}

start_foo/stop_foo and start_bar/stop_bar would need to cover the correct parts of .foo even if .foo or .bar have SHF_LINK_ORDER sections.

pcc: > ```.ARM.exidx : { *(.arm.exidx) *(.arm.exidx.*) }``` I think this unambiguously means to…
std::vector<InputSection **> scriptSections; std::vector<InputSection **> scriptSections;
std::vector<InputSection *> sections; std::vector<InputSection *> sections;
bool started = false, stopped = false;
for (BaseCommand *base : sec->sectionCommands) { for (BaseCommand *base : sec->sectionCommands) {
if (auto *isd = dyn_cast<InputSectionDescription>(base)) { if (auto *isd = dyn_cast<InputSectionDescription>(base)) {
bool hasLinkOrder = false;
scriptSections.clear();
sections.clear();
for (InputSection *&isec : isd->sections) { for (InputSection *&isec : isd->sections) {
if (!(isec->flags & SHF_LINK_ORDER)) { if (isec->flags & SHF_LINK_ORDER) {
if (started)
stopped = true;
} else if (stopped) {
error(toString(isec) + ": SHF_LINK_ORDER sections in " + sec->name +
" are not contiguous");
} else {
started = true;
scriptSections.push_back(&isec);
sections.push_back(isec);
InputSection *link = isec->getLinkOrderDep(); InputSection *link = isec->getLinkOrderDep();
if (link && !link->getParent()) if (link && !link->getParent())
error(toString(isec) + ": sh_link points to discarded section " + error(toString(isec) + ": sh_link points to discarded section " +
toString(link)); toString(link));
hasLinkOrder = true;
} }
scriptSections.push_back(&isec);
sections.push_back(isec);
} }
} else if (started) { if (hasLinkOrder && errorCount() == 0) {
stopped = true;
}
}
if (errorCount())
continue;
llvm::stable_sort(sections, compareByFilePosition); llvm::stable_sort(sections, compareByFilePosition);
for (int i = 0, n = sections.size(); i < n; ++i) for (int i = 0, n = sections.size(); i < n; ++i)
*scriptSections[i] = sections[i]; *scriptSections[i] = sections[i];
} }
} }
}
}
}
grimarUnsubmitted
Done
ReplyInline Actions

Consider reordering to reduce nesting and to remove clear() calls:

// Link order may be distributed across several InputSectionDescriptions.
// Sorting is performed separately.
for (BaseCommand *base : sec->sectionCommands) {
  auto *isd = dyn_cast<InputSectionDescription>(base);
  if (!isd)
    continue;

  bool hasLinkOrder = false;
  std::vector<InputSection **> scriptSections;
  std::vector<InputSection *> sections;
  for (InputSection *&isec : isd->sections) {
    if (isec->flags & SHF_LINK_ORDER) {
      InputSection *link = isec->getLinkOrderDep();
      if (link && !link->getParent())
        error(toString(isec) + ": sh_link points to discarded section " +
              toString(link));
      hasLinkOrder = true;
    }
    scriptSections.push_back(&isec);
    sections.push_back(isec);
  }

  if (!hasLinkOrder || errorCount())
    continue;

  llvm::stable_sort(sections, compareByFilePosition);
  for (int i = 0, n = sections.size(); i < n; ++i)
    *scriptSections[i] = sections[i];
}
grimar: Consider reordering to reduce nesting and to remove `clear()` calls: ``` // Link order may…
grimarUnsubmitted
Done
ReplyInline Actions

Aside: we can replace constructions like:

for (BaseCommand *base : sec->sectionCommands) {
  auto *isd = dyn_cast<InputSectionDescription>(base);
  if (!isd)
    continue;

with just

for (auto *isd: filter<InputSectionDescription>(sec->sectionCommands)) {

here and in many other places in LLD if we land D45166 + D45490.

grimar: Aside: we can replace constructions like: ``` for (BaseCommand *base : sec->sectionCommands)…
static void finalizeSynthetic(SyntheticSection *sec) { static void finalizeSynthetic(SyntheticSection *sec) {
if (sec && sec->isNeeded() && sec->getParent()) if (sec && sec->isNeeded() && sec->getParent())
sec->finalizeContents(); sec->finalizeContents();
} }
// We need to generate and finalize the content that depends on the address of // We need to generate and finalize the content that depends on the address of
// InputSections. As the generation of the content may also alter InputSection // InputSections. As the generation of the content may also alter InputSection
▲ Show 20 Lines • Show All 1,310 Lines • Show Last 20 Lines