[LLD] Implement --enable-non-contiguous-regions #90007
Conversation
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Daniel Thornburgh (mysterymath) ChangesWhen enabled, input sections that would otherwise overflow a memory region are instead spilled to the next matching output section. This feature parallels the one in GNU LD, but there are some differences from its documented behavior:
The implementation places stubs at possible spill locations, and replaces them with the original input section when effecting spills. Spilling decisions occur after address assignment. Sections are spilled in reverse order of assignment, with each spill naively decreasing the size of the affected memory regions. This continues until the memory regions are brought back under size. Spilling anything causes another pass of address assignment, and this continues to fixed point. Spilling after rather than during assignment allows the algorithm to consider the size effects of unspillable input sections that appear later in the assignment. Otherwise, such sections (e.g. thunks) may force an overflow, even if spilling something earlier could have avoided it. A few notable feature interactions occur:
Patch is 34.34 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90007.diff 17 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 33bfa42b0fcbf0..bb5d7001f59fbb 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -237,6 +237,7 @@ struct Config {
bool emitLLVM;
bool emitRelocs;
bool enableNewDtags;
+ bool enableNonContiguousRegions;
bool executeOnly;
bool exportDynamic;
bool fixCortexA53Errata843419;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index a5b47f020f8726..915184c1aa2632 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1250,6 +1250,8 @@ static void readConfigs(opt::InputArgList &args) {
config->emitRelocs = args.hasArg(OPT_emit_relocs);
config->enableNewDtags =
args.hasFlag(OPT_enable_new_dtags, OPT_disable_new_dtags, true);
+ config->enableNonContiguousRegions =
+ args.hasArg(OPT_enable_non_contiguous_regions);
config->entry = args.getLastArgValue(OPT_entry);
errorHandler().errorHandlingScript =
@@ -3077,7 +3079,7 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
// sectionBases.
for (SectionCommand *cmd : script->sectionCommands)
if (auto *osd = dyn_cast<OutputDesc>(cmd))
- osd->osec.finalizeInputSections();
+ osd->osec.finalizeInputSections(script.get());
}
// Two input sections with different output sections should not be folded.
diff --git a/lld/ELF/ICF.cpp b/lld/ELF/ICF.cpp
index bfc605c793a92c..677f325e854065 100644
--- a/lld/ELF/ICF.cpp
+++ b/lld/ELF/ICF.cpp
@@ -75,6 +75,7 @@
#include "ICF.h"
#include "Config.h"
#include "InputFiles.h"
+#include "InputSection.h"
#include "LinkerScript.h"
#include "OutputSections.h"
#include "SymbolTable.h"
diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index fa48552b8f7a12..aa5e8f11177fd4 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -9,6 +9,7 @@
#include "InputSection.h"
#include "Config.h"
#include "InputFiles.h"
+#include "LinkerScript.h"
#include "OutputSections.h"
#include "Relocations.h"
#include "SymbolTable.h"
@@ -161,6 +162,7 @@ uint64_t SectionBase::getOffset(uint64_t offset) const {
}
case Regular:
case Synthetic:
+ case Spill:
return cast<InputSection>(this)->outSecOff + offset;
case EHFrame: {
// Two code paths may reach here. First, clang_rt.crtbegin.o and GCC
@@ -309,6 +311,12 @@ std::string InputSectionBase::getObjMsg(uint64_t off) const {
.str();
}
+SpillInputSection::SpillInputSection(InputSectionBase *source,
+ InputSectionDescription *isd)
+ : InputSection(source->file, source->flags, source->type, source->addralign,
+ {}, source->name, SectionBase::Spill),
+ isd(isd) {}
+
InputSection InputSection::discarded(nullptr, 0, 0, 0, ArrayRef<uint8_t>(), "");
InputSection::InputSection(InputFile *f, uint64_t flags, uint32_t type,
diff --git a/lld/ELF/InputSection.h b/lld/ELF/InputSection.h
index 1fb7077ca435bd..3d75a9633a3ccf 100644
--- a/lld/ELF/InputSection.h
+++ b/lld/ELF/InputSection.h
@@ -48,7 +48,7 @@ template <class ELFT> struct RelsOrRelas {
// sections.
class SectionBase {
public:
- enum Kind { Regular, Synthetic, EHFrame, Merge, Output };
+ enum Kind { Regular, Synthetic, EHFrame, Merge, Output, Spill };
Kind kind() const { return (Kind)sectionKind; }
@@ -382,7 +382,8 @@ class InputSection : public InputSectionBase {
static bool classof(const SectionBase *s) {
return s->kind() == SectionBase::Regular ||
- s->kind() == SectionBase::Synthetic;
+ s->kind() == SectionBase::Synthetic ||
+ s->kind() == SectionBase::Spill;
}
// Write this section to a mmap'ed file, assuming Buf is pointing to
@@ -425,6 +426,25 @@ class InputSection : public InputSectionBase {
template <class ELFT> void copyShtGroup(uint8_t *buf);
};
+// A marker for a potential spill location for another input section. This
+// broadly acts as if it were the original section until address assignment.
+// Then it is either replaced with the real input section or removed.
+class SpillInputSection : public InputSection {
+public:
+ // The containing input section description; used to quickly replace this stub
+ // with the actual section.
+ InputSectionDescription *isd;
+
+ // Next spill location for the same source input section.
+ SpillInputSection *next = nullptr;
+
+ SpillInputSection(InputSectionBase *source, InputSectionDescription *cmd);
+
+ static bool classof(const SectionBase *sec) {
+ return sec->kind() == InputSectionBase::Spill;
+ }
+};
+
static_assert(sizeof(InputSection) <= 160, "InputSection is too big");
class SyntheticSection : public InputSection {
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index f815b3ac6feeda..41827015868496 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -304,6 +304,9 @@ getChangedSymbolAssignment(const SymbolAssignmentMap &oldValues) {
void LinkerScript::processInsertCommands() {
SmallVector<OutputDesc *, 0> moves;
for (const InsertCommand &cmd : insertCommands) {
+ if (config->enableNonContiguousRegions)
+ error("INSERT cannot be used with --enable-non-contiguous-regions");
+
for (StringRef name : cmd.names) {
// If base is empty, it may have been discarded by
// adjustOutputSections(). We do not handle such output sections.
@@ -484,12 +487,13 @@ static void sortInputSections(MutableArrayRef<InputSectionBase *> vec,
}
// Compute and remember which sections the InputSectionDescription matches.
-SmallVector<InputSectionBase *, 0>
-LinkerScript::computeInputSections(const InputSectionDescription *cmd,
- ArrayRef<InputSectionBase *> sections) {
+SmallVector<InputSectionBase *, 0> LinkerScript::computeInputSections(
+ const InputSectionDescription *cmd, ArrayRef<InputSectionBase *> sections,
+ const OutputSection &outCmd) {
SmallVector<InputSectionBase *, 0> ret;
SmallVector<size_t, 0> indexes;
DenseSet<size_t> seen;
+ DenseSet<InputSectionBase *> spills;
auto sortByPositionThenCommandLine = [&](size_t begin, size_t end) {
llvm::sort(MutableArrayRef<size_t>(indexes).slice(begin, end - begin));
for (size_t i = begin; i != end; ++i)
@@ -505,12 +509,33 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
size_t sizeBeforeCurrPat = ret.size();
for (size_t i = 0, e = sections.size(); i != e; ++i) {
- // Skip if the section is dead or has been matched by a previous input
- // section description or a previous pattern.
+ // Skip if the section is dead or has been matched by a previous pattern
+ // in this input section description.
InputSectionBase *sec = sections[i];
- if (!sec->isLive() || sec->parent || seen.contains(i))
+ if (!sec->isLive() || seen.contains(i))
continue;
+ if (sec->parent) {
+ // Skip if not allowing multiple matches.
+ if (!config->enableNonContiguousRegions)
+ continue;
+
+ // Disallow spilling into /DISCARD/; special handling would be needed
+ // for this in address assignment, and the semantics are nebulous.
+ if (outCmd.name == "/DISCARD/")
+ continue;
+
+ // Skip if the section's first match was /DISCARD/; such sections are
+ // always discarded.
+ if (sec->parent->name == "/DISCARD/")
+ continue;
+
+ // Skip if the section was already matched by a different input section
+ // description within this output section.
+ if (sec->parent == &outCmd)
+ continue;
+ }
+
// For --emit-relocs we have to ignore entries like
// .rela.dyn : { *(.rela.data) }
// which are common because they are in the default bfd script.
@@ -530,6 +555,8 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
continue;
ret.push_back(sec);
+ if (sec->parent)
+ spills.insert(sec);
indexes.push_back(i);
seen.insert(i);
}
@@ -555,6 +582,28 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
// Matched sections after the last SORT* are sorted by (--sort-alignment,
// input order).
sortByPositionThenCommandLine(sizeAfterPrevSort, ret.size());
+
+ // Replace matches after the first with potential spill sections.
+ if (!spills.empty()) {
+ for (InputSectionBase *&sec : ret) {
+ if (!spills.contains(sec))
+ continue;
+
+ SpillInputSection *sis = make<SpillInputSection>(
+ sec, const_cast<InputSectionDescription *>(cmd));
+
+ // Append the spill input section to the list for the input section,
+ // creating it if necessary.
+ auto res = spillLists.try_emplace(sec, SpillList{sis, sis});
+ if (!res.second) {
+ SpillInputSection *&tail = res.first->second.tail;
+ tail = tail->next = sis;
+ }
+
+ sec = sis;
+ }
+ }
+
return ret;
}
@@ -577,7 +626,7 @@ void LinkerScript::discardSynthetic(OutputSection &outCmd) {
part.armExidx->exidxSections.end());
for (SectionCommand *cmd : outCmd.commands)
if (auto *isd = dyn_cast<InputSectionDescription>(cmd))
- for (InputSectionBase *s : computeInputSections(isd, secs))
+ for (InputSectionBase *s : computeInputSections(isd, secs, outCmd))
discard(*s);
}
}
@@ -588,7 +637,7 @@ LinkerScript::createInputSectionList(OutputSection &outCmd) {
for (SectionCommand *cmd : outCmd.commands) {
if (auto *isd = dyn_cast<InputSectionDescription>(cmd)) {
- isd->sectionBases = computeInputSections(isd, ctx.inputSections);
+ isd->sectionBases = computeInputSections(isd, ctx.inputSections, outCmd);
for (InputSectionBase *s : isd->sectionBases)
s->parent = &outCmd;
ret.insert(ret.end(), isd->sectionBases.begin(), isd->sectionBases.end());
@@ -644,6 +693,9 @@ void LinkerScript::processSectionCommands() {
// Process OVERWRITE_SECTIONS first so that it can overwrite the main script
// or orphans.
+ if (config->enableNonContiguousRegions && !overwriteSections.empty())
+ error("OVERWRITE_SECTIONS cannot be used with "
+ "--enable-non-contiguous-regions");
DenseMap<CachedHashStringRef, OutputDesc *> map;
size_t i = 0;
for (OutputDesc *osd : overwriteSections) {
@@ -911,6 +963,13 @@ void LinkerScript::diagnoseMissingSGSectionAddress() const {
error("no address assigned to the veneers output section " + sec->name);
}
+void LinkerScript::copySpillList(InputSectionBase *dst, InputSectionBase *src) {
+ auto i = spillLists.find(src);
+ if (i == spillLists.end())
+ return;
+ spillLists.try_emplace(dst, i->second);
+}
+
// This function searches for a memory region to place the given output
// section in. If found, a pointer to the appropriate memory region is
// returned in the first member of the pair. Otherwise, a nullptr is returned.
@@ -1066,8 +1125,16 @@ void LinkerScript::assignOffsets(OutputSection *sec) {
// Handle a single input section description command.
// It calculates and assigns the offsets for each section and also
// updates the output section size.
- for (InputSection *isec : cast<InputSectionDescription>(cmd)->sections) {
+
+ DenseSet<InputSection*> spills;
+ auto §ions = cast<InputSectionDescription>(cmd)->sections;
+ for (InputSection *isec : sections) {
assert(isec->getParent() == sec);
+
+ // Skip all possible spills.
+ if (isa<SpillInputSection>(isec))
+ continue;
+
const uint64_t pos = dot;
dot = alignToPowerOf2(dot, isec->addralign);
isec->outSecOff = dot - sec->addr;
@@ -1364,6 +1431,107 @@ const Defined *LinkerScript::assignAddresses() {
return getChangedSymbolAssignment(oldValues);
}
+static bool isRegionOverflowed(MemoryRegion *mr) {
+ if (!mr)
+ return false;
+ return mr->curPos - mr->getOrigin() > mr->getLength();
+}
+
+// Spill input sections in reverse order of address assignment to (potentially)
+// bring memory regions out of overflow. The size savings of a spill can only be
+// estimated, since general linker script arithmetic may occur afterwards.
+// Under-estimates may cause unnecessary spills, but over-estimates can always
+// be corrected on the next pass.
+bool LinkerScript::spillSections() {
+ if (!config->enableNonContiguousRegions)
+ return false;
+
+ bool spilled = false;
+ for (SectionCommand *cmd : reverse(sectionCommands)) {
+ auto *od = dyn_cast<OutputDesc>(cmd);
+ if (!od)
+ continue;
+ OutputSection *osec = &od->osec;
+ if (!osec->size || !osec->memRegion)
+ continue;
+
+ DenseSet<InputSection *> spills;
+ for (SectionCommand *cmd : reverse(osec->commands)) {
+ if (!isRegionOverflowed(osec->memRegion) &&
+ !isRegionOverflowed(osec->lmaRegion))
+ break;
+
+ auto *is = dyn_cast<InputSectionDescription>(cmd);
+ if (!is)
+ continue;
+ for (InputSection *isec : reverse(is->sections)) {
+ // Potential spill locations cannot be spilled.
+ if (isa<SpillInputSection>(isec))
+ continue;
+
+ // Find the next spill location.
+ auto it = spillLists.find(isec);
+ if (it == spillLists.end())
+ continue;
+
+ spilled = true;
+ SpillList &list = it->second;
+
+ SpillInputSection *spill = list.head;
+ if (!spill->next)
+ spillLists.erase(isec);
+ else
+ list.head = spill->next;
+
+ spills.insert(isec);
+
+ // Replace the next spill location with the spilled section and adjust
+ // its properties to match the new location.
+ *llvm::find(spill->isd->sections, spill) = isec;
+ isec->parent = spill->parent;
+ // The alignment of the spill section may have diverged from the
+ // original, but correct assignment requires the spill's alignment,
+ // not the original.
+ isec->addralign = spill->addralign;
+
+ // Record the reduction in overage.
+ osec->memRegion->curPos -= isec->getSize();
+ if (osec->lmaRegion)
+ osec->lmaRegion->curPos -= isec->getSize();
+ if (!isRegionOverflowed(osec->memRegion) &&
+ !isRegionOverflowed(osec->lmaRegion))
+ break;
+ }
+ // Remove any spilled sections.
+ if (!spills.empty())
+ llvm::erase_if(is->sections, [&](InputSection *isec) {
+ return spills.contains(isec);
+ });
+ }
+ }
+
+ return spilled;
+}
+
+// Erase any potential spill sections that were not used.
+void LinkerScript::eraseSpillSections() {
+ if (spillLists.empty())
+ return;
+
+ // Collect the set of input section descriptions that contain potential
+ // spills.
+ DenseSet<InputSectionDescription *> isds;
+ for (const auto &[_, list] : spillLists)
+ for (SpillInputSection *s = list.head; s; s = s->next)
+ isds.insert(s->isd);
+
+ for (InputSectionDescription *isd : isds)
+ llvm::erase_if(isd->sections,
+ [](InputSection *s) { return isa<SpillInputSection>(s); });
+
+ spillLists.clear();
+}
+
// Creates program headers as instructed by PHDRS linker script command.
SmallVector<PhdrEntry *, 0> LinkerScript::createPhdrs() {
SmallVector<PhdrEntry *, 0> ret;
diff --git a/lld/ELF/LinkerScript.h b/lld/ELF/LinkerScript.h
index fa7c6eb9c0d8f7..920eadfdb82b3d 100644
--- a/lld/ELF/LinkerScript.h
+++ b/lld/ELF/LinkerScript.h
@@ -10,6 +10,7 @@
#define LLD_ELF_LINKER_SCRIPT_H
#include "Config.h"
+#include "InputSection.h"
#include "Writer.h"
#include "lld/Common/LLVM.h"
#include "lld/Common/Strings.h"
@@ -287,7 +288,8 @@ class LinkerScript final {
SmallVector<InputSectionBase *, 0>
computeInputSections(const InputSectionDescription *,
- ArrayRef<InputSectionBase *>);
+ ArrayRef<InputSectionBase *>,
+ const OutputSection& outCmd);
SmallVector<InputSectionBase *, 0> createInputSectionList(OutputSection &cmd);
@@ -312,6 +314,15 @@ class LinkerScript final {
uint64_t dot;
+ // List of potential spill locations (SpillInputSection) for an input
+ // section.
+ struct SpillList {
+ // Never nullptr.
+ SpillInputSection *head;
+ SpillInputSection *tail;
+ };
+ llvm::DenseMap<InputSectionBase*, SpillList> spillLists;
+
public:
OutputDesc *createOutputSection(StringRef name, StringRef location);
OutputDesc *getOrCreateOutputSection(StringRef name);
@@ -325,6 +336,7 @@ class LinkerScript final {
void addOrphanSections();
void diagnoseOrphanHandling() const;
void diagnoseMissingSGSectionAddress() const;
+ void copySpillList(InputSectionBase *dst, InputSectionBase *src);
void adjustOutputSections();
void adjustSectionsAfterSorting();
@@ -333,6 +345,8 @@ class LinkerScript final {
bool shouldKeep(InputSectionBase *s);
const Defined *assignAddresses();
+ bool spillSections();
+ void eraseSpillSections();
void allocateHeaders(SmallVector<PhdrEntry *, 0> &phdrs);
void processSectionCommands();
void processSymbolAssignments();
diff --git a/lld/ELF/Options.td b/lld/ELF/Options.td
index 72eaf157a181cf..0e5d15124d260b 100644
--- a/lld/ELF/Options.td
+++ b/lld/ELF/Options.td
@@ -196,6 +196,9 @@ def emit_relocs: F<"emit-relocs">, HelpText<"Generate relocations in output">;
def enable_new_dtags: F<"enable-new-dtags">,
HelpText<"Enable new dynamic tags (default)">;
+def enable_non_contiguous_regions : FF<"enable-non-contiguous-regions">,
+ HelpText<"Spill input sections to later matching output sections to avoid memory region overflow">;
+
def end_group: F<"end-group">,
HelpText<"Ignored for compatibility with GNU unless you pass --warn-backrefs">;
diff --git a/lld/ELF/OutputSections.cpp b/lld/ELF/OutputSections.cpp
index eadab9d745d687..1a6c5b7a09f4ce 100644
--- a/lld/ELF/OutputSections.cpp
+++ b/lld/ELF/OutputSections.cpp
@@ -186,7 +186,7 @@ static MergeSyntheticSection *createMergeSynthetic(StringRef name,
// new synthetic sections at the location of the first input section
// that it replaces. It then finalizes each synthetic section in order
// to compute an output offset for each piece of each input section.
-void OutputSection::finalizeInputSections() {
+void OutputSection::finalizeInputSections(LinkerScript *script) {
std::vector<MergeSyntheticSection *> mergeSections;
for (SectionCommand *cmd : commands) {
auto *isd = dyn_cast<InputSectionDescription>(cmd);
@@ -226,6 +226,8 @@ void OutputSection::finalizeInputSections() {
i = std::prev(mergeSections.end());
syn->entsize = ms->entsize;
isd->sections.push_back(syn);
+ if (script)
+ script->copySpillList(syn, ms);
}
(*i)->addSection(ms);
}
diff --git a/lld/ELF/OutputSections.h b/lld/ELF/OutputSections.h
index 421a0181feb5df..78fede48a23f25 100644
--- a/lld/ELF/OutputSections.h
+++ b/lld/ELF/OutputSections.h
@@ -75,7 +75,7 @@ class OutputSection final : public SectionBase {
void recordSection(InputSectionBase *isec);
void commitSection(InputSection *isec);
- void finalizeInputSections();
+ void finalizeInputSections(LinkerScript *script = nullptr);
// The following members are normally only used in linker scripts.
MemoryRegion *memRegion = nullptr;
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 7b9ada40c0f67b..298c714adb3b43 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -4074,6 +4074,13 @@ static bool isDuplicateArmExidxSec(InputSection *prev, InputSection *cur) {
// InputSection with the highest address and any InputSections that have
// mergeable .ARM.exidx table entries are removed from it.
void ARMExidxSyntheticSection::finalizeContents() {
+ // Ensure that any fixed-point iterations after the first see the original set
+ // of sections.
+ if (!originalExecutableSections.empty())
+ executableSections = originalExecutableSections;
+ else if (config->enableNonContiguousRegions)
+ originalExecutableSections = executableSections;
+
// The executableSections and exidxSections that we use to derive the final
// contents of this SyntheticSection are populated before
// processSectionCommands() and ICF. A /DISCARD/ entry in SECTIONS command or
diff --git a/lld/ELF/SyntheticSections.h b/lld/ELF/SyntheticSections.h
index...
[truncated]
|
mysterymath
force-pushed
the
lld-section-packing
branch
from
7f94e71 to
dcc6a12
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mysterymath
force-pushed
the
lld-section-packing
branch
from
dcc6a12 to
3005ffd
Compare
When enabled, input sections that would otherwise overflow a memory region are instead spilled to the next matching output section. This feature parallels the one in GNU LD, but there are some differences from its documented behavior: - /DISCARD/ only matches previously-unmatched sections (i.e., the flag does not affect it). - If a section fails to fit at any of its matches, the link fails instead of discarding the section. - The flag --enable-non-contiguous-regions-warnings is not implemented, as it exists to warn about such occurrences. The implementation places stubs at possible spill locations, and replaces them with the original input section when effecting spills. Spilling decisions occur after address assignment. Sections are spilled in reverse order of assignment, with each spill naively decreasing the size of the affected memory regions. This continues until the memory regions are brought back under size. Spilling anything causes another pass of address assignment, and this continues to fixed point. Spilling after rather than during assignment allows the algorithm to consider the size effects of unspillable input sections that appear later in the assignment. Otherwise, such sections (e.g. thunks) may force an overflow, even if spilling something earlier could have avoided it. A few notable feature interactions occur: - Stubs affect alignment, ONLY_IF_RO, etc, broadly as if a copy of the input section were actually placed there. - SHF_MERGE synthetic sections use the spill list of their first contained input section (the one that gives the section its name). - ICF occurs oblivious to spill sections; spill lists for merged-away sections become inert and are removed after assignment. - SHF_LINK_ORDER and .ARM.exidx are ordered according to the final section ordering, after all spilling has completed. - INSERT BEFORE/AFTER and OVERWRITE_SECTIONS are explicitly disallowed.
mysterymath
force-pushed
the
lld-section-packing
branch
from
3005ffd to
be9868b
Compare
| @@ -196,6 +196,9 @@ def emit_relocs: F<"emit-relocs">, HelpText<"Generate relocations in output">; | |||
| def enable_new_dtags: F<"enable-new-dtags">, | |||
| HelpText<"Enable new dynamic tags (default)">; | |||
|
|
|||
| def enable_non_contiguous_regions : FF<"enable-non-contiguous-regions">, | |||
There was a problem hiding this comment.
Would it be worth silently accepting
--enable-non-contiguous-regions-warnings for command-line compatibility with GNU ld. I think this is safe because LLD won't encounter the case where it has to give a warning.
There was a problem hiding this comment.
Ah, good idea; removes a needless compatibility ding. Done.
There was a problem hiding this comment.
We used to add some compatibility options or aliases, but mostly stopped doing do.
--enable-non-contiguous-regions is itself quite niche. --enable-non-contiguous-regions-warnings seems more niche and probably doesn't justify an ignored option.
There was a problem hiding this comment.
Done; altered the docs and commit message to match.
| @@ -196,6 +196,9 @@ def emit_relocs: F<"emit-relocs">, HelpText<"Generate relocations in output">; | |||
| def enable_new_dtags: F<"enable-new-dtags">, | |||
| HelpText<"Enable new dynamic tags (default)">; | |||
|
|
|||
| def enable_non_contiguous_regions : FF<"enable-non-contiguous-regions">, | |||
| HelpText<"Spill input sections to later matching output sections to avoid memory region overflow">; | |||
|
|
|||
There was a problem hiding this comment.
Can we add an entry to the LLD man page located in docs/ld.lld.1 too?
lld/ELF/LinkerScript.cpp
Outdated
| @@ -555,6 +583,28 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd, | |||
| // Matched sections after the last SORT* are sorted by (--sort-alignment, | |||
| // input order). | |||
| sortByPositionThenCommandLine(sizeAfterPrevSort, ret.size()); | |||
|
|
|||
| // Replace matches after the first with potential spill sections. | |||
There was a problem hiding this comment.
Could be worth expanding on this a bit to remind us that this is for enable-non-contiguous regions.
Perhaps something like:
When --enable-contiguous-regions is on sections may match an InputSectionDescription in more than one OutputSection. The
first match against is recorded as an InputSection, subsequent matches are recorded as potential spill sections.
There was a problem hiding this comment.
Tweaked the wording to be a bit more imperative; done.
lld/ELF/LinkerScript.cpp
Outdated
| @@ -1066,8 +1126,16 @@ void LinkerScript::assignOffsets(OutputSection *sec) { | |||
| // Handle a single input section description command. | |||
| // It calculates and assigns the offsets for each section and also | |||
| // updates the output section size. | |||
| for (InputSection *isec : cast<InputSectionDescription>(cmd)->sections) { | |||
|
|
|||
| DenseSet<InputSection *> spills; | |||
There was a problem hiding this comment.
I can't see this DenseSet being used? If I've not missed something, can it be removed?
There was a problem hiding this comment.
Thanks, left over from a previous algorithm.
lld/ELF/LinkerScript.cpp
Outdated
| assert(isec->getParent() == sec); | ||
|
|
||
| // Skip all possible spills. |
There was a problem hiding this comment.
Would "Skip all potential spill locations." be better here. Just to keep the language consistent with previous uses.
lld/ELF/LinkerScript.cpp
Outdated
| spilled = true; | ||
| SpillList &list = it->second; | ||
|
|
||
| SpillInputSection *spill = list.head; |
There was a problem hiding this comment.
Would it be worth using potentialSpill here. We have spill spills, spillLists all with fairly close names. I found it hard to keep track of what was what on first reading.
There was a problem hiding this comment.
I think spills is the misnamed one here; spill is a potential spill that is currently being taken, but spills isn't a list of such things; it's a list of spilled input sections. Added a comment for the variable and renamed it to spilledInputSections.
lld/ELF/LinkerScript.cpp
Outdated
| continue; | ||
|
|
||
| spilled = true; | ||
| SpillList &list = it->second; |
There was a problem hiding this comment.
list is very generic. Would it be worth something like
isecSpillLocations or isecSpillLocList?
There was a problem hiding this comment.
Hmm, this is just a stylistic preference on my end, but I tend to use small anaphora (it for iterator, list for a list, etc.) for tightly-scoped variables that temporarily give names to objects before disappearing. Here there's only one list in question (the spill list for the input section), and the scope spans only five lines.
As with all things stylistic, there's tons of nuance, and if you did find this difficult to read, knowing more about why would help me refine the rule.
There was a problem hiding this comment.
It was a similar situation to a previous comment, there were several other lists, spills and spilllist and I found myself having to double check which was which.
I don't have a particularly strong opinion though, so feel free to ignore me here.
lld/ELF/LinkerScript.cpp
Outdated
| // its properties to match the new location. | ||
| *llvm::find(spill->isd->sections, spill) = isec; | ||
| isec->parent = spill->parent; | ||
| // The alignment of the spill section may have diverged from the |
There was a problem hiding this comment.
Presumably this might be increased due to some linker script expression like subalign?
There was a problem hiding this comment.
Yep, that's exactly the case that came up when hammering on the tests. Probably worth explicitly calling out in a comment.
lld/ELF/LinkerScript.cpp
Outdated
| spills.insert(isec); | ||
|
|
||
| // Replace the next spill location with the spilled section and adjust | ||
| // its properties to match the new location. |
There was a problem hiding this comment.
When spilling we are moving OutputSections and this might affect the flags of the output section.
Do we account for splill placements that might have awkward combinations of flags?
I can't at a glance see commitSection() being called https://github.com/llvm/llvm-project/blob/main/lld/ELF/OutputSections.cpp#L116 but I could be missing something.
There was a problem hiding this comment.
This was one of the semantic caveats taken to keep the model of how this works simple. The spill input sections work throughout the whole link as if they were additional copies of the referenced input section in those output sections. This includes calculations for the flags of the output sections, program header generation, etc. The one caveat is they're not included in the list of all input sections, so they're not in scope for ICF, GC, etc.
It's not until the final address assignment in fixed point iteration that the copies drop out. Afterwards, the input sections are seen in their final positions, and the spills have been removed, albeit leaving some of their effects behind.
This isn't as simple as possible; the simplest semantics would be for the whole link to behave "as if" the sections had always been matched to their final positions. However, due to circular dependencies in the link, this would require either running huge swaths of the linker in a loop to fixed point or being able to undo and redo chains of decision-making between output section matching and assignment. The former seems broadly like a non-starter, but the latter we may be able to do piecemeal if the semantics above don't actually end up being sufficient.
| isec->addralign = spill->addralign; | ||
|
|
||
| // Record the reduction in overage. | ||
| osec->memRegion->curPos -= isec->getSize(); |
There was a problem hiding this comment.
In theory this could be a potentially large underestimate if isec has a high alignment and there is a lot of alignment padding.
There was a problem hiding this comment.
That's true; this is definitely a best-effort satisficing type of algorithm.
The situation could be improved by running the spilling both forwards during assignment and backwards afterwards (as opposed to just backwards).
This could be improved further by binary searching for the earliest "spill point" in the address assignment that allows the first memory region to eventually fit, then actualizing all potential spills for that region after that point, then binary searching for the spill point for the next overflowing region, etc. Overall that would take O(m log n) time, where m = |regions| and n = |sections|.
Alternatively, we may be able to handle alignment more specifically and accurately determine its effects.
At the extreme end, we could get a kind of optimality by backtracking whenever a forward assignment overflows and performing the last spill opportunity, for O(n^2) time. That might not actually be too bad in practice.
Anyway, I do think that there's potential room to grow here, but my goal was to find the simplest algorithm that might satisfy most embedded projects. I had originally only had forward spilling (inline with assignment), but late unspillable sections cropped up too many times, and backwards spilling is resistant to them without being more complicated.
There was a problem hiding this comment.
Agreed it is best to keep it simple at first. I'm hoping that the majority of cases where a really highly aligned section is used will be specific to a single region.
- Add no-op `--enable-non-contiguous-regions-warnings`. - Add help text to LLD man page. - Additional implementation comments. - Remove dead variable. - Remove release note merge chaff. - Rename `isRegionOverflowed` to `hasRegionOverflowed`. - Rename variable `is` to `isd` for InputSectionDescription.
There was a problem hiding this comment.
Thanks very much for the update. Only a few small comments on the tests.
I definitely would like to see this feature added as the equivalent feature in Arm's proprietary toolchain has been widely used. Given that the equivalent GNU ld feature is new, I don't think we'll see existing widespread use but I think it could pick up in the future.
| .fnstart | ||
| bx lr | ||
| .save {r9, lr} | ||
| .setfp r9, sp, #0 |
There was a problem hiding this comment.
I think it could be worth making the unwind instructions identical for at least two of the sections. The .exidx table compression merges identical entries. One of things I was worried about with this patch was whether the table compression would still work properly when the order of sections could change.
I think that there could be corner cases where the table compression changes causes the sizes of the .ARM.exidx to change significantly, which then affects thunks, which then affects spills.
For example if we have sections A, B, C with .ARM.exidx A', B' and C', but with A' and B' identical, but C' different. If on pass 1 we have order A, B, C then A' and B' will be compressed. However if spills cause the order to be A, C, B then the table can no longer be compressed.
However to make a test that actually fails to converge would be difficult and not representative of real world usage. For example .ARM.exidx is ro-data so it tends to be all before or all after.
There was a problem hiding this comment.
I think it could be worth making the unwind instructions identical for at least two of the sections. The .exidx table compression merges identical entries. One of things I was worried about with this patch was whether the table compression would still work properly when the order of sections could change.
Ah, good point. I had just copied this from another test, since I haven't learned the details of .ARM.exidx's encoding yet. I've made two of the sets of assembly declarations identical and updated the assertions. Found that llvm-readobj had a nice pretty-printer for the exidx section, and the results seem plausible.
However to make a test that actually fails to converge would be difficult and not representative of real world usage.
Even if the .ARM.exidx alternatively grows or shrinks on different passes, I'd think it still wouldn't affect convergence. At least, it shouldn't on the basis of spilling alone, since performing a spill is still an irrevocable action.
| @@ -0,0 +1,54 @@ | |||
| # If a spilling reorders input sections, the .ARM.exidx table must be rebuilt | |||
There was a problem hiding this comment.
I think you'll need a REQUIRES arm for the test.
| @@ -0,0 +1,54 @@ | |||
| # If a spilling reorders input sections, the .ARM.exidx table must be rebuilt | |||
There was a problem hiding this comment.
There's also an unwritten convention in LLD to use an extra comment character for non FileCheck commands. For example:
## If a spilling reorders ...
It has been some time since that was started so could be worth looking at recent commits to see if it is still in operation.
There was a problem hiding this comment.
Hmm, I did make the relatively unusual choice here not to comment the lit and filecheck commands, since this is a split-file test. That doesn't seem to be the norm, so I've gone back through and adjusted them accordingly.
| SPILL: .first_chance PROGBITS 0000000000000000 001000 000001 | ||
| SPILL-NEXT: .last_chance PROGBITS 0000000000000002 001002 000002 | ||
|
|
||
| # A spill off the end must still fails the link. |
There was a problem hiding this comment.
Typo?
must still fail the link.
- Update example to demonstrate merging - Make test require ARM - Two hashes for comments; one for non-split-file content - Fixed typo
There was a problem hiding this comment.
Thanks for the updated tests. I've checked that with a slightly different .ARM.exidx (changing to r7 rather than r8), this isn't prematurely compressed in the first pass.
Given MaskRay's comment in https://discourse.llvm.org/t/rfc-lld-enable-non-contiguous-regions/76513/17 it will be worth running this past him first before merging.
| @@ -0,0 +1,279 @@ | |||
| # REQUIRES: x86 | |||
|
|
|||
| # RUN: split-file %s %t | |||
There was a problem hiding this comment.
RUN: rm -rf %t && split-file %s %t && cd %t
The convention is to remove %t to avoid prevent lingering files. cd %t allows to remove %t/ below, making RUN lines cleaner.
There was a problem hiding this comment.
Thanks, that does make the tests much easier to read.
| continue; | ||
|
|
||
| if (sec->parent) { |
There was a problem hiding this comment.
If this is moved before ret.push_back(sec);, we can save one if (sec->parent) check and group enableNonContiguousRegions code together.
lld/ELF/LinkerScript.cpp
Outdated
|
|
||
| PotentialSpillSection *pss = make<PotentialSpillSection>( | ||
| sec, const_cast<InputSectionDescription *>(cmd)); | ||
|
|
There was a problem hiding this comment.
The code style is pretty dense. Variables are not followed by a blank line.
auto [it, inserted] = .. is preferred for newer code
There was a problem hiding this comment.
Nice pattern! Done.
lld/ELF/LinkerScript.cpp
Outdated
|
|
||
| spilled = true; | ||
| PotentialSpillList &list = it->second; | ||
|
|
There was a problem hiding this comment.
This blank line and the next one are unneeded.
The code style is pretty dense. We usually insert blank lines for a larger block of code in the absence of comments. We usually reorder comments to make a clock block without blank line longer.
There was a problem hiding this comment.
If I've understood you correctly, it's desirable to have code in LLD as dense as is readable?
I've reordered the blocks based on topic: potential spill list manipulation and manipulating the input section. That should help make things denser, since the existing comments can be altered to cover more code.
lld/ELF/LinkerScript.cpp
Outdated
| } | ||
|
|
||
| // Remove any spilled input sections to complete their move. | ||
| if (!spilledInputSections.empty()) |
There was a problem hiding this comment.
spilled = true can be moved here to remove one line from the above code that requires more attention.
lld/ELF/Writer.cpp
Outdated
| } else if (spilled) { | ||
| // Spilling can change relative section order, so recompute anything that | ||
| // depends on it. | ||
| for (Partition &part : partitions) |
There was a problem hiding this comment.
Create a lambda to avoid duplicating the few lines. We might add more in the future and could accidentally break --enable-non-contiguous-regions
|
|
||
| ## A spill off the end must still fail the link. | ||
|
|
||
| # RUN: not ld.lld -T %t/spill-fail.ld %t/spill.o -o %t/spill-fail --enable-non-contiguous-regions 2>&1 |\ |
There was a problem hiding this comment.
For not ld.lld, we use -o /dev/null. But if rm -rf %t && split-file %s %t && cd %t is used, -o can be omitted.
ditto elsewhere
There was a problem hiding this comment.
Done via the latter.
| # RUN: split-file %s %t | ||
| # RUN: llvm-mc -n -filetype=obj -triple=x86_64 %t/spill.s -o %t/spill.o | ||
|
|
||
| ## An input section must spill to a later match if the region of its first match |
There was a problem hiding this comment.
There are many "must"s in comments. Our convention states what happens and omits "must". Very occasionally we use "should"
There was a problem hiding this comment.
Done; this is terser than I was able to come up with, which I appreciate.
|
|
||
| ## A spill must use the alignment of the later match. | ||
|
|
||
| # RUN: ld.lld -T %t/spill-align.ld %t/spill.o -o %t/spill-align --enable-non-contiguous-regions |
There was a problem hiding this comment.
This subalign test can be merged into the basic spill.ld test.
We try to cover every case while using a smaller number of tests.
|
|
||
| # OVERWRITE_SECTIONS: error: OVERWRITE_SECTIONS cannot be used with --enable-non-contiguous-regions | ||
|
|
||
| # SHF_LINK_ORDER must be reordered if spilling changes relative section order. |
There was a problem hiding this comment.
Thanks. Hah, I missed this while scanning because it was all caps after the hash.
| .byte 1 | ||
|
|
||
| .section .link_order.b,"ao",@progbits,.b | ||
| .byte 2 |
There was a problem hiding this comment.
I'd add another link order section to make the ordering requirement clearer.
There was a problem hiding this comment.
I'm not sure exactly what you mean here; the order swaps from the one given in the input file (.link_order.a, .link_order.b) to respond to the final order of input sections due to spilling: (.b, .a). Is there a combination of link order sections and spilling sections that would be less ambiguous or clearer?
There was a problem hiding this comment.
The idea is to have 3 SHF_LINK_ORDER sections to make sorting requirement clearer.
With two sections it's a bit less clear whether sorting is intended (it could be a reversed order).
There was a problem hiding this comment.
Ah, gotcha. Done.
lld/docs/ReleaseNotes.rst
Outdated
| region would overflow. This reduces the toil of manually packing regions | ||
| (typical for embedded). It also makes full LTO feasible in such cases, since | ||
| IR merging currently prevents the linker script from referring to input | ||
| files. |
There was a problem hiding this comment.
(`#90007 <https://github.com/llvm/llvm-project/pull/90007>`_)
lld/ELF/InputSection.cpp
Outdated
| @@ -309,6 +310,12 @@ std::string InputSectionBase::getObjMsg(uint64_t off) const { | |||
| .str(); | |||
| } | |||
|
|
|||
| PotentialSpillSection::PotentialSpillSection(InputSectionBase *source, | |||
There was a problem hiding this comment.
We prefer const & when a parameter is non-null.
lld/ELF/InputSection.h
Outdated
| @@ -48,7 +48,7 @@ template <class ELFT> struct RelsOrRelas { | |||
| // sections. | |||
| class SectionBase { | |||
| public: | |||
| enum Kind { Regular, Synthetic, EHFrame, Merge, Output }; | |||
| enum Kind { Regular, Synthetic, EHFrame, Merge, Output, Spill }; | |||
There was a problem hiding this comment.
Regular, Synthetic, Spill, EHFrame, Merge, Output may be more efficient.
Ensure classof tested kinds are contiguous
- Remove split-file directory and cd into it. - Consolidate logic in computeInputSections(). - Make spill logic denser and group comments. - Create finalizeOrderDependentContent lambda. - Spill zero-size sections. - Remove warnings flag. - Make potentialSpillLists public; remove copySpillLists. - implicit-check-not: error: - Reword test descriptions to remove "must". - Merge spill and subalign test. - Missing hash before SHF_LINK_ORDER comment. - Add PR reference to release notes - Take potentialSpillSection constructor params by ref
lld/ELF/LinkerScript.cpp
Outdated
| continue; | ||
| PotentialSpillList &list = it->second; | ||
| PotentialSpillSection *spill = list.head; | ||
| if (!spill->next) |
There was a problem hiding this comment.
Prefer if (xxx) to if (!xxx) by swapping then/else branches.
|
|
||
| ## An additional match in /DISCARD/ has no effect. | ||
|
|
||
| # RUN: not ld.lld -T no-spill-into-discard.ld spill.o -o no-spill-into-discard --enable-non-contiguous-regions 2>&1 |\ |
There was a problem hiding this comment.
not ld.lld commands can omit -o (default: a.out) if cd %t has been invoked.
|
|
||
| ## An error is reported for OVERWRITE_SECTIONS. | ||
|
|
||
| # RUN: not ld.lld -T overwrite-sections.ld spill.o -o overwrite-sections --enable-non-contiguous-regions 2>&1 |\ |
| .byte 1 | ||
|
|
||
| .section .link_order.b,"ao",@progbits,.b | ||
| .byte 2 |
There was a problem hiding this comment.
The idea is to have 3 SHF_LINK_ORDER sections to make sorting requirement clearer.
With two sections it's a bit less clear whether sorting is intended (it could be a reversed order).
lld/ELF/OutputSections.cpp
Outdated
| @@ -226,6 +226,13 @@ void OutputSection::finalizeInputSections() { | |||
| i = std::prev(mergeSections.end()); | |||
| syn->entsize = ms->entsize; | |||
| isd->sections.push_back(syn); | |||
| // The merge synthetic section inherits the potential spill locations of | |||
| // its first contained section. | |||
| if (script) { | |||
There was a problem hiding this comment.
Remove the if. `script is non-null
- Swap true and false branches to remove condition negation - No -o for failure tests - Add a third link order section to show both normal sorting and reordering due to spilling.
This reverts commit 6731144.
Reverts #90007 Broke in merging I think.
|
Before merging, it's always beneficial to test it locally. |
Yep, sorry about that. I was desirous to use the Github UI to squash and rebase the change, but perhaps its best to do such things manually if they can't be separated from the action of committing. I guess I could have rebased the whole stack onto main and tested locally before hitting the big button; that would have caught this too. |
When enabled, input sections that would otherwise overflow a memory region are instead spilled to the next matching output section. This feature parallels the one in GNU LD, but there are some differences from its documented behavior: - /DISCARD/ only matches previously-unmatched sections (i.e., the flag does not affect it). - If a section fails to fit at any of its matches, the link fails instead of discarding the section. - The flag --enable-non-contiguous-regions-warnings is not implemented, as it exists to warn about such occurrences. The implementation places stubs at possible spill locations, and replaces them with the original input section when effecting spills. Spilling decisions occur after address assignment. Sections are spilled in reverse order of assignment, with each spill naively decreasing the size of the affected memory regions. This continues until the memory regions are brought back under size. Spilling anything causes another pass of address assignment, and this continues to fixed point. Spilling after rather than during assignment allows the algorithm to consider the size effects of unspillable input sections that appear later in the assignment. Otherwise, such sections (e.g. thunks) may force an overflow, even if spilling something earlier could have avoided it. A few notable feature interactions occur: - Stubs affect alignment, ONLY_IF_RO, etc, broadly as if a copy of the input section were actually placed there. - SHF_MERGE synthetic sections use the spill list of their first contained input section (the one that gives the section its name). - ICF occurs oblivious to spill sections; spill lists for merged-away sections become inert and are removed after assignment. - SHF_LINK_ORDER and .ARM.exidx are ordered according to the final section ordering, after all spilling has completed. - INSERT BEFORE/AFTER and OVERWRITE_SECTIONS are explicitly disallowed.
This fixes the new test linkerscript/enable-non-contiguous-regions.test from #90007 in -stdlib=libc++ -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG builds. adjustOutputSections does not discard the output section .potential_a because it contained .a (which would be spilled to .actual_a). .potential_a and .bc have the same address and will cause an assertion failure.
When enabled, input sections that would otherwise overflow a memory region are instead spilled to the next matching output section.
This feature parallels the one in GNU LD, but there are some differences from its documented behavior:
/DISCARD/ only matches previously-unmatched sections (i.e., the flag does not affect it).
If a section fails to fit at any of its matches, the link fails instead of discarding the section.
The flag --enable-non-contiguous-regions-warnings is not implemented, as it exists to warn about such occurrences.
The implementation places stubs at possible spill locations, and replaces them with the original input section when effecting spills. Spilling decisions occur after address assignment. Sections are spilled in reverse order of assignment, with each spill naively decreasing the size of the affected memory regions. This continues until the memory regions are brought back under size. Spilling anything causes another pass of address assignment, and this continues to fixed point.
Spilling after rather than during assignment allows the algorithm to consider the size effects of unspillable input sections that appear later in the assignment. Otherwise, such sections (e.g. thunks) may force an overflow, even if spilling something earlier could have avoided it.
A few notable feature interactions occur:
Stubs affect alignment, ONLY_IF_RO, etc, broadly as if a copy of the input section were actually placed there.
SHF_MERGE synthetic sections use the spill list of their first contained input section (the one that gives the section its name).
ICF occurs oblivious to spill sections; spill lists for merged-away sections become inert and are removed after assignment.
SHF_LINK_ORDER and .ARM.exidx are ordered according to the final section ordering, after all spilling has completed.
INSERT BEFORE/AFTER and OVERWRITE_SECTIONS are explicitly disallowed.