[lld][ELF] Add --why-live flag (inspired by Mach-O) #127112
Conversation
mysterymath
force-pushed
the
lld-why-live
branch
from
4c37d6a to
0f1b043
Compare
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Daniel Thornburgh (mysterymath) ChangesThis prints the stack of reasons that symbols that match the given glob(s) survived GC. It has no effect unless section GC occurs. A symbol may be live intrisically, because referenced by another symbol or section, or because part of a live section. Sections have similar reasons. This implementation does not require -ffunction-sections or -fdata-sections to produce readable results, althought it does tend to work better (as does GC). Full diff: https://github.com/llvm/llvm-project/pull/127112.diff 5 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index b2859486d58e9..12164f5999343 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -223,6 +223,7 @@ struct Config {
llvm::StringRef thinLTOCacheDir;
llvm::StringRef thinLTOIndexOnlyArg;
llvm::StringRef whyExtract;
+ llvm::SmallVector<llvm::GlobPattern, 0> whyLive;
llvm::StringRef cmseInputLib;
llvm::StringRef cmseOutputLib;
StringRef zBtiReport = "none";
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 13e8f8ce6df20..db0b2ea8afcf0 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1472,6 +1472,15 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
ctx.arg.warnSymbolOrdering =
args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
ctx.arg.whyExtract = args.getLastArgValue(OPT_why_extract);
+ for (opt::Arg *arg : args.filtered(OPT_why_live)) {
+ StringRef value(arg->getValue());
+ if (Expected<GlobPattern> pat = GlobPattern::create(arg->getValue())) {
+ ctx.arg.whyLive.emplace_back(std::move(*pat));
+ } else {
+ ErrAlways(ctx) << arg->getSpelling() << ": " << pat.takeError();
+ continue;
+ }
+ }
ctx.arg.zCombreloc = getZFlag(args, "combreloc", "nocombreloc", true);
ctx.arg.zCopyreloc = getZFlag(args, "copyreloc", "nocopyreloc", true);
ctx.arg.zForceBti = hasZOption(args, "force-bti");
diff --git a/lld/ELF/MarkLive.cpp b/lld/ELF/MarkLive.cpp
index b6c22884d9176..8e9e385bc26dc 100644
--- a/lld/ELF/MarkLive.cpp
+++ b/lld/ELF/MarkLive.cpp
@@ -29,9 +29,11 @@
#include "Target.h"
#include "lld/Common/CommonLinkerContext.h"
#include "lld/Common/Strings.h"
+#include "llvm/ADT/DenseMapInfoVariant.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Object/ELF.h"
#include "llvm/Support/TimeProfiler.h"
+#include <variant>
#include <vector>
using namespace llvm;
@@ -42,6 +44,10 @@ using namespace lld;
using namespace lld::elf;
namespace {
+
+// Something that can be the most proximate reason that something else is alive.
+typedef std::variant<InputSectionBase *, Symbol *> LiveReason;
+
template <class ELFT> class MarkLive {
public:
MarkLive(Ctx &ctx, unsigned partition) : ctx(ctx), partition(partition) {}
@@ -50,7 +56,10 @@ template <class ELFT> class MarkLive {
void moveToMain();
private:
- void enqueue(InputSectionBase *sec, uint64_t offset);
+ void enqueue(InputSectionBase *sec, uint64_t offset = 0,
+ Symbol *sym = nullptr,
+ std::optional<LiveReason> reason = std::nullopt);
+ void printWhyLive(Symbol *s) const;
void markSymbol(Symbol *sym);
void mark();
@@ -70,6 +79,12 @@ template <class ELFT> class MarkLive {
// There are normally few input sections whose names are valid C
// identifiers, so we just store a SmallVector instead of a multimap.
DenseMap<StringRef, SmallVector<InputSectionBase *, 0>> cNamedSections;
+
+ // The most proximate reason that something is live. If something doesn't have
+ // a recorded reason, it is either dead, intrinsically live, or an
+ // unreferenced symbol in a live section. (These cases are trivially
+ // detectable and need not be stored.)
+ DenseMap<LiveReason, LiveReason> whyLive;
};
} // namespace
@@ -101,6 +116,12 @@ void MarkLive<ELFT>::resolveReloc(InputSectionBase &sec, RelTy &rel,
Symbol &sym = sec.file->getRelocTargetSym(rel);
sym.used = true;
+ LiveReason reason;
+ if (!ctx.arg.whyLive.empty()) {
+ Defined *reasonSym = sec.getEnclosingSymbol(rel.r_offset);
+ reason = reasonSym ? LiveReason(reasonSym) : LiveReason(&sec);
+ }
+
if (auto *d = dyn_cast<Defined>(&sym)) {
auto *relSec = dyn_cast_or_null<InputSectionBase>(d->section);
if (!relSec)
@@ -119,17 +140,29 @@ void MarkLive<ELFT>::resolveReloc(InputSectionBase &sec, RelTy &rel,
// group/SHF_LINK_ORDER rules (b) if the associated text section should be
// discarded, marking the LSDA will unnecessarily retain the text section.
if (!(fromFDE && ((relSec->flags & (SHF_EXECINSTR | SHF_LINK_ORDER)) ||
- relSec->nextInSectionGroup)))
- enqueue(relSec, offset);
+ relSec->nextInSectionGroup))) {
+ Symbol *canonicalSym = d;
+ if (!ctx.arg.whyLive.empty() && d->isSection()) {
+ if (Symbol *s = relSec->getEnclosingSymbol(offset))
+ canonicalSym = s;
+ else
+ canonicalSym = nullptr;
+ }
+ enqueue(relSec, offset, canonicalSym, reason);
+ }
return;
}
- if (auto *ss = dyn_cast<SharedSymbol>(&sym))
- if (!ss->isWeak())
+ if (auto *ss = dyn_cast<SharedSymbol>(&sym)) {
+ if (!ss->isWeak()) {
cast<SharedFile>(ss->file)->isNeeded = true;
+ if (!ctx.arg.whyLive.empty())
+ whyLive.try_emplace(&sym, reason);
+ }
+ }
for (InputSectionBase *sec : cNamedSections.lookup(sym.getName()))
- enqueue(sec, 0);
+ enqueue(sec, 0, nullptr, reason);
}
// The .eh_frame section is an unfortunate special case.
@@ -187,7 +220,8 @@ static bool isReserved(InputSectionBase *sec) {
}
template <class ELFT>
-void MarkLive<ELFT>::enqueue(InputSectionBase *sec, uint64_t offset) {
+void MarkLive<ELFT>::enqueue(InputSectionBase *sec, uint64_t offset,
+ Symbol *sym, std::optional<LiveReason> reason) {
// Usually, a whole section is marked as live or dead, but in mergeable
// (splittable) sections, each piece of data has independent liveness bit.
// So we explicitly tell it which offset is in use.
@@ -201,15 +235,71 @@ void MarkLive<ELFT>::enqueue(InputSectionBase *sec, uint64_t offset) {
return;
sec->partition = sec->partition ? 1 : partition;
+ if (!ctx.arg.whyLive.empty() && reason) {
+ if (sym) {
+ // If a specific symbol is referenced, that makes it alive. It may in turn
+ // make its section alive.
+ whyLive.try_emplace(sym, *reason);
+ whyLive.try_emplace(sec, sym);
+ } else {
+ // Otherwise, the reference generically makes the section live.
+ whyLive.try_emplace(sec, *reason);
+ }
+ }
+
// Add input section to the queue.
if (InputSection *s = dyn_cast<InputSection>(sec))
queue.push_back(s);
}
+// Print the stack of reasons that the given symbol is live.
+template <class ELFT> void MarkLive<ELFT>::printWhyLive(Symbol *s) const {
+ // Skip dead symbols. A symbol is dead if it belongs to a dead section.
+ if (auto *d = dyn_cast<Defined>(s)) {
+ auto *reason = dyn_cast_or_null<InputSectionBase>(d->section);
+ if (reason && !reason->isLive())
+ return;
+ }
+
+ auto msg = Msg(ctx);
+ msg << "live symbol: " << toStr(ctx, *s);
+
+ LiveReason cur = s;
+ while (true) {
+ auto it = whyLive.find(cur);
+ // If there is a specific reason this object is live...
+ if (it != whyLive.end()) {
+ cur = it->second;
+ } else {
+ // This object is live, but it has no tracked reason. It is either
+ // intrinsically live or an unreferenced symbol in a live section. Return
+ // in the first case.
+ if (!std::holds_alternative<Symbol *>(cur))
+ return;
+ auto *d = dyn_cast<Defined>(std::get<Symbol *>(cur));
+ if (!d)
+ return;
+ auto *reason = dyn_cast_or_null<InputSectionBase>(d->section);
+ if (!reason)
+ return;
+ cur = LiveReason{reason};
+ }
+
+ msg << "\n>>> kept live by ";
+ if (std::holds_alternative<Symbol *>(cur)) {
+ auto *s = std::get<Symbol *>(cur);
+ msg << toStr(ctx, *s);
+ } else {
+ auto *s = std::get<InputSectionBase *>(cur);
+ msg << toStr(ctx, s);
+ }
+ }
+}
+
template <class ELFT> void MarkLive<ELFT>::markSymbol(Symbol *sym) {
if (auto *d = dyn_cast_or_null<Defined>(sym))
if (auto *isec = dyn_cast_or_null<InputSectionBase>(d->section))
- enqueue(isec, d->value);
+ enqueue(isec, d->value, sym);
}
// This is the main function of the garbage collector.
@@ -256,7 +346,7 @@ template <class ELFT> void MarkLive<ELFT>::run() {
}
for (InputSectionBase *sec : ctx.inputSections) {
if (sec->flags & SHF_GNU_RETAIN) {
- enqueue(sec, 0);
+ enqueue(sec, 0, nullptr, std::nullopt);
continue;
}
if (sec->flags & SHF_LINK_ORDER)
@@ -295,7 +385,7 @@ template <class ELFT> void MarkLive<ELFT>::run() {
// Preserve special sections and those which are specified in linker
// script KEEP command.
if (isReserved(sec) || ctx.script->shouldKeep(sec)) {
- enqueue(sec, 0);
+ enqueue(sec);
} else if ((!ctx.arg.zStartStopGC || sec->name.starts_with("__libc_")) &&
isValidCIdentifier(sec->name)) {
// As a workaround for glibc libc.a before 2.34
@@ -323,11 +413,20 @@ template <class ELFT> void MarkLive<ELFT>::mark() {
resolveReloc(sec, rel, false);
for (InputSectionBase *isec : sec.dependentSections)
- enqueue(isec, 0);
+ enqueue(isec, 0, nullptr, &sec);
// Mark the next group member.
if (sec.nextInSectionGroup)
- enqueue(sec.nextInSectionGroup, 0);
+ enqueue(sec.nextInSectionGroup, 0, nullptr, &sec);
+ }
+
+ if (!ctx.arg.whyLive.empty()) {
+ for (Symbol *sym : ctx.symtab->getSymbols()) {
+ if (llvm::any_of(ctx.arg.whyLive, [sym](const llvm::GlobPattern &pat) {
+ return pat.match(sym->getName());
+ }))
+ printWhyLive(sym);
+ }
}
}
@@ -353,7 +452,7 @@ template <class ELFT> void MarkLive<ELFT>::moveToMain() {
continue;
if (ctx.symtab->find(("__start_" + sec->name).str()) ||
ctx.symtab->find(("__stop_" + sec->name).str()))
- enqueue(sec, 0);
+ enqueue(sec);
}
mark();
diff --git a/lld/ELF/Options.td b/lld/ELF/Options.td
index c31875305952f..babc84f345b95 100644
--- a/lld/ELF/Options.td
+++ b/lld/ELF/Options.td
@@ -559,6 +559,12 @@ defm wrap : Eq<"wrap", "Redirect symbol references to __wrap_symbol and "
"__real_symbol references to symbol">,
MetaVarName<"<symbol>">;
+defm why_live
+ : EEq<"why-live",
+ "Report a chain of references preventing garbage collection for "
+ "each symbol matching <glob>">,
+ MetaVarName<"<glob>">;
+
def z: JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,
HelpText<"Linker option extensions">;
diff --git a/lld/test/ELF/why-live.s b/lld/test/ELF/why-live.s
new file mode 100644
index 0000000000000..12d373cd78d28
--- /dev/null
+++ b/lld/test/ELF/why-live.s
@@ -0,0 +1,132 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -n -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo -e ".globl test_shared\n .section .test_shared,\"ax\",@progbits\n test_shared: jmp test_shared" |\
+# RUN: llvm-mc -n -filetype=obj -triple=x86_64 -o %t.shared.o
+# RUN: ld.lld -shared %t.shared.o -o %t.so
+
+## Simple live section
+.globl _start
+.section ._start,"ax",@progbits
+_start:
+jmp test_simple
+jmp .Llocal
+jmp .Llocal_within_symbol
+jmp test_shared
+.size _start, .-_start
+
+.globl test_simple
+.section .test_simple,"ax",@progbits
+test_simple:
+jmp test_simple
+jmp test_from_unsized
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_simple | FileCheck %s --check-prefix=SIMPLE
+
+# SIMPLE: live symbol: test_simple
+# SIMPLE-NEXT: >>> kept live by _start
+
+## Live only by being a member of .test_simple
+.globl test_incidental
+test_incidental:
+jmp test_incidental
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_incidental | FileCheck %s --check-prefix=INCIDENTAL
+
+# INCIDENTAL: live symbol: test_incidental
+# INCIDENTAL-NEXT: >>> kept live by {{.*}}.o:(.test_simple)
+# INCIDENTAL-NEXT: >>> kept live by test_simple
+# INCIDENTAL-NEXT: >>> kept live by _start
+
+## Reached from a reference in section .test_simple directly, since test_simple is an unsized symbol.
+.globl test_from_unsized
+.section .test_from_unsized,"ax",@progbits
+test_from_unsized:
+jmp test_from_unsized
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_from_unsized | FileCheck %s --check-prefix=FROM-UNSIZED
+
+# FROM-UNSIZED: live symbol: test_from_unsized
+# FROM-UNSIZED-NEXT: >>> kept live by {{.*}}.o:(.test_simple)
+# FROM-UNSIZED-NEXT: >>> kept live by test_simple
+# FROM-UNSIZED-NEXT: >>> kept live by _start
+
+## Symbols in dead sections are dead and not reported.
+.globl test_dead
+.section .test_dead,"ax",@progbits
+test_dead:
+jmp test_dead
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_dead | count 0
+
+## Undefined symbols are considered live, since they are not in dead sections.
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_undef -u test_undef | FileCheck %s --check-prefix=UNDEFINED
+
+# UNDEFINED: live symbol: test_undef
+# UNDEFINED-NOT: >>>
+
+## Defined symbols without input section parents are live.
+.globl test_absolute
+test_absolute = 1234
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_absolute | FileCheck %s --check-prefix=ABSOLUTE
+
+# ABSOLUTE: live symbol: test_absolute
+# ABSOLUTE-NOT: >>>
+
+## Retained sections are intrinsically live, and they make contained symbols live.
+.globl test_retained
+.section .test_retained,"axR",@progbits
+test_retained:
+jmp test_retained
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_retained | FileCheck %s --check-prefix=RETAINED
+
+# RETAINED: live symbol: test_retained
+# RETAINED-NEXT: >>> kept live by {{.*}}:(.test_retained)
+
+## Relocs that reference offsets from sections (e.g., from local symbols) are considered to point to the section if no enclosing symbol exists.
+
+.globl test_section_offset
+.section .test_section_offset,"ax",@progbits
+test_section_offset:
+jmp test_section_offset
+.Llocal:
+jmp test_section_offset
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_section_offset | FileCheck %s --check-prefix=SECTION-OFFSET
+
+# SECTION-OFFSET: live symbol: test_section_offset
+# SECTION-OFFSET-NEXT: >>> kept live by {{.*}}:(.test_section_offset)
+# SECTION-OFFSET-NEXT: >>> kept live by _start
+
+## Relocs that reference offsets from sections (e.g., from local symbols) are considered to point to the enclosing symbol if one exists.
+
+.globl test_section_offset_within_symbol
+.section .test_section_offset_within_symbol,"ax",@progbits
+test_section_offset_within_symbol:
+jmp test_section_offset_within_symbol
+.Llocal_within_symbol:
+jmp test_section_offset_within_symbol
+.size test_section_offset_within_symbol, .-test_section_offset_within_symbol
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_section_offset_within_symbol | FileCheck %s --check-prefix=SECTION-OFFSET-WITHIN-SYMBOL
+
+# SECTION-OFFSET-WITHIN-SYMBOL: live symbol: test_section_offset_within_symbol
+# SECTION-OFFSET-WITHIN-SYMBOL-NEXT: >>> kept live by _start
+
+## Shared symbols
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections %t.so --why-live=test_shared | FileCheck %s --check-prefix=SHARED
+
+# SHARED: live symbol: test_shared
+# SHARED-NEXT: >>> kept live by _start
+
+## Globs match multiple cases. Multiple --why-live flags union.
+
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections %t.so --why-live=test_s* | FileCheck %s --check-prefix=MULTIPLE
+# RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections %t.so --why-live=test_simple --why-live=test_shared | FileCheck %s --check-prefix=MULTIPLE
+
+# MULTIPLE-DAG: live symbol: test_simple
+# MULTIPLE-DAG: live symbol: test_shared
|
lld/ELF/MarkLive.cpp
Outdated
| @@ -101,6 +116,12 @@ void MarkLive<ELFT>::resolveReloc(InputSectionBase &sec, RelTy &rel, | |||
| Symbol &sym = sec.file->getRelocTargetSym(rel); | |||
| sym.used = true; | |||
|
|
|||
| LiveReason reason; | |||
| if (!ctx.arg.whyLive.empty()) { | |||
| Defined *reasonSym = sec.getEnclosingSymbol(rel.r_offset); | |||
There was a problem hiding this comment.
getEnclosingSymbol iterates all symbols in the file, which is very slow. This perhaps should only be run after we get a patch from a root to the --why-live specified symbol.
|
Apologies for the delay in responding. I've just started looking through the code. I think it would be helpful to update the description with a mini specification of what you intend the output to be. There will have been some design decisions made on what to include in the output. At the moment this needs to be reverse engineered out of the code and tests. For example:
|
There was a problem hiding this comment.
Thanks for working on this. I've got some subjective suggestions about the external interface.
- I recommend adding the object and section information as well as the symbol (possibly via a verbose mode). This particularly useful when there are multiple symbols and sections with the same name.
- For the root symbol, I recommend adding information about the source if its easily available. For example _start is the entry point. This could be particularly useful if a symbol is unexpectedly made live via a linker script reference.
Finally, one that you may want to exclude, is any Arm/AArch64 mapping symbol from being included in the why-live output. These often are defined at the same address as function and data symbols, but you'll almost certainly want to prefer the function and data symbol.
lld/ELF/MarkLive.cpp
Outdated
| whyLive.try_emplace(sec, sym); | ||
| } else { | ||
| // Otherwise, the reference generically makes the section live. | ||
| whyLive.try_emplace(sec, *reason); |
There was a problem hiding this comment.
Could there be a case where the first match we see doesn't have a symbol, but a later chain of relocations would?
In that case we could prefer to update sec -> *reason to sec -> new-symbol
There was a problem hiding this comment.
I tried this, but updating liveness reason beyond the first encountered can cause the liveness reason to become circular. The first encountered reason is immune to this, since it is part of the acyclic region of the reference graph close to the GC roots.
There's probably a way around this, but it seems like a big lift for the benefit, particularly in the first patch.
lld/test/ELF/why-live.s
Outdated
| @@ -0,0 +1,132 @@ | |||
| # REQUIRES: x86 | |||
|
|
|||
| # RUN: llvm-mc -n -filetype=obj -triple=x86_64 %s -o %t.o | |||
There was a problem hiding this comment.
I think it will be worth adding some tests for symbols/sections that are kept via linker-script KEEP, referenced from a linker script expression.
The referenced from a linker script expression is something that might surprise some users. It would be useful to see what that prints.
Although it could be an extension in a later patch, I think we could give more information about the top level symbol. For example
>>> kept live by _start (crt0.o(.text.start); entry point)
Could also mention exported symbol, retained, referenced from linker script etc.
There was a problem hiding this comment.
I added to the whyLive interface a place for string fields; accordingly, all liveness edges have recorded reasons, including the top level. The interface is such that it's difficult to forget to provide a reason.
With this change, linker script cases don't vary from the ones tested semantically; it's just that different reason strings are used. We could exhaustively test all the reason strings, but that doesn't seem very likely to catch additional issues given the nature of the interface.
An unspecified path (the first one we find); updated the description accordingly. When the user reaches for
See above.
See below.
I'm just scanning the linker's symbol table, so IIUC that shouldn't include
Yes, that works. If there is an enclosing sized symbol at that location, then that is reported. Otherwise, the reference is considered a generic reference to the section. This case is tested by
Yes, enclosing objects are preferred. Updated the description.
Not at the moment. One can do this by naming any one global symbol defined in the section. If such a symbol is directly referenced, then that reference will be reported. Otherwise, it will be reported alive by virtue of being part of the section, and some reference to the section will be reported as the reason the section is alive.
Not at present. |
|
Actually, I think I was wrong about |
I think it should be fairly straightforward to add at least some information, if annoying to test. And I'd agree, it would be generally quite useful.
Only sized or directly referenced symbols can be reported as liveness reasons. Given that the mapping symbols mark positions, I'd assume they wouldn't have either of those characteristics. Is that correct? |
There was a problem hiding this comment.
I think we need Mach-O style template <bool RecordWhyLive> to prevent overhead with the regular --gc-sections code path.
https://reviews.llvm.org/D69607 is a previous attempt adding the option.
Yes that is correct. They aren't directly called and they have no size. |
It seems prudent to collect some evidence that this might be a performance problem before moving to a less readable implementation. I'm planning to do a simple link of clang with |
I finally got back around to this, and I tried the above link of clang. Running a Student's t test on 5 trials yields a mean regression of 1.5%, and a very low p value for the means being the same (0.00024). That's definitely way too much, so I'll go through and add the templates. I'm surprised it was this big of a difference, but I'm still glad I measured. |
I've upload a performance fix using a TrackWhyLive template. I've verified that the compiler is able to reason that the extra args aren't used in the version of MarkLive where why-live isn't tracked, and 10 trials of the clang link shows no statistically significant difference in performance. |
|
I've updated this patch to handle reporting and querying local symbols, but noticed something surprising as I was doing so. MarkLive computes a section offset for each relocation using the effective addend. This is used with However, the JMP instructions I were using on x86-64 have addends that actually point 4 bytes before the effective target of the jump. It doesn't seem generally true that the effective addend for a reloc actually points at the address that is actually reference (that is, the one that needs to be kept by GC due to the reloc.) This discrepancy might cause an incorrect symbol to be attributed as live, or more concerningly, the wrong SHF_MERGE section to be marked live. I'm unsure whether this can cause issues with SHF_MERGE in practice, but it definitely interacts poorly with For now, I've just changed the tests to use |
lld/ELF/MarkLive.cpp
Outdated
| @@ -101,6 +116,12 @@ void MarkLive<ELFT>::resolveReloc(InputSectionBase &sec, RelTy &rel, | |||
| Symbol &sym = sec.file->getRelocTargetSym(rel); | |||
| sym.used = true; | |||
|
|
|||
| LiveReason reason; | |||
| if (!ctx.arg.whyLive.empty()) { | |||
| Defined *reasonSym = sec.getEnclosingSymbol(rel.r_offset); | |||
lld/ELF/MarkLive.cpp
Outdated
| whyLive.try_emplace(sec, sym); | ||
| } else { | ||
| // Otherwise, the reference generically makes the section live. | ||
| whyLive.try_emplace(sec, *reason); |
There was a problem hiding this comment.
I tried this, but updating liveness reason beyond the first encountered can cause the liveness reason to become circular. The first encountered reason is immune to this, since it is part of the acyclic region of the reference graph close to the GC roots.
There's probably a way around this, but it seems like a big lift for the benefit, particularly in the first patch.
lld/test/ELF/why-live.s
Outdated
| @@ -0,0 +1,132 @@ | |||
| # REQUIRES: x86 | |||
|
|
|||
| # RUN: llvm-mc -n -filetype=obj -triple=x86_64 %s -o %t.o | |||
There was a problem hiding this comment.
I added to the whyLive interface a place for string fields; accordingly, all liveness edges have recorded reasons, including the top level. The interface is such that it's difficult to forget to provide a reason.
With this change, linker script cases don't vary from the ones tested semantically; it's just that different reason strings are used. We could exhaustively test all the reason strings, but that doesn't seem very likely to catch additional issues given the nature of the interface.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Will take a look this week. Apologies for the delay. |
lld/ELF/MarkLive.cpp
Outdated
| using SecOffset = std::pair<InputSectionBase *, unsigned>; | ||
|
|
||
| // Something that can have an independent reason for being live. | ||
| using LiveObject = std::variant<InputSectionBase *, Symbol *, SecOffset>; |
There was a problem hiding this comment.
In the context of a linker, using object and obj could be easily thought of as a relocatable object.
Perhaps LiveItem or LiveEntity instead? With item or entity in LiveReason below.
There was a problem hiding this comment.
Done. (LiveItem)
| std::optional<LiveObject> obj; | ||
| StringRef desc; | ||
| }; | ||
|
|
There was a problem hiding this comment.
It looks like only a subset of the functions use TrackWhyLive (3 at rough count), just in case we're trying to minimise code-size, we could split the non parameter using functions into a base class. It may not be worth it for just (ELFT * TrackWhyLive) instantiations though.
There was a problem hiding this comment.
Acknowledged. I played around with this a bit, but I wasn't able to come up with something without a readability hit. I think there's things we can do if we end up with code size issues in lld overall, but otherwise, the presence of the TrackWhyLive==true variants shouldn't upset instruction cache, since they'll be essentially dead for most links.
lld/ELF/MarkLive.cpp
Outdated
| @@ -201,45 +238,126 @@ void MarkLive<ELFT>::enqueue(InputSectionBase *sec, uint64_t offset) { | |||
| return; | |||
| sec->partition = sec->partition ? 1 : partition; | |||
|
|
|||
| if (TrackWhyLive) { | |||
| if (sym) { | |||
| // If a specific symbol is referenced, that makes it alive. It may in turn | |||
There was a problem hiding this comment.
It looks like if the symbol is reference it will make its section live. The comment says "It may". Is that intentional?
Strangely I've never thought of it as alive, just live. I guess alive makes more sense in the sense of dead code.
There was a problem hiding this comment.
Given the semantics of "first reference wins as a liveness reason", pretty much all of these could say "may". I think it's clearer to remove "may" and replace "makes" with "keeps".
Surprisingly both "live" and "alive" were already present in this file. I tried to always use "live", but it looks like I missed one ;)
lld/ELF/MarkLive.cpp
Outdated
| } else { | ||
| // This object is live, but it has no tracked reason. It must be an | ||
| // unreferenced symbol in a live section or a symbol with no section. | ||
| const auto getParentSec = [&]() -> InputSectionBase * { |
There was a problem hiding this comment.
It looks like this lambda is only used once. Could it be worth inlining it?
Something like:
InputSectionBase *sec = nullptr;
if (auto *d = dyn_cast<Defined>(std::get<Symbol *>(cur))
sec = dyn_cast_or_null<InputSectionBase>(d->section);
There was a problem hiding this comment.
Ah thanks, that's a cleaner pattern. Done.
lld/ELF/MarkLive.cpp
Outdated
| else | ||
| msg << std::get<InputSectionBase *>(cur); | ||
| } | ||
| if (!reason.obj) { |
There was a problem hiding this comment.
Unless I've missed an update to reason.obj, this looks like it could be just else {
There was a problem hiding this comment.
Good catch; made this a guard clause to remove a level of nesting.
lld/ELF/MarkLive.cpp
Outdated
|
|
||
| for (InputSectionBase *sec : cNamedSections.lookup(sym.getName())) | ||
| enqueue(sec, 0); | ||
| enqueue(sec, 0, nullptr, reason); |
There was a problem hiding this comment.
I found the nullptr and std::nullopt callsites a bit more difficult to reason about, I had to keep referring back to the definition to remember what it meant.
Just thinking if there's an easy way to help out. One way would be to make a constant for nullptr like noSym and noObj (or noItem). Possibly a comment like nullptr /* no symbol */. This isn't a strong opinion.
There was a problem hiding this comment.
The /*sym=*/ pattern is pretty common for things like this; I've done so throughout.
lld/ELF/MarkLive.cpp
Outdated
| auto msg = Msg(ctx); | ||
|
|
||
| const auto printSymbol = [&](Symbol *s) { | ||
| if (s->isLocal()) |
There was a problem hiding this comment.
It may be worth printing the file for globals as well. It isn't needed to disambiguate the function, but it means I have to search in my source code for it if I want to find it.
Not a strong opinion at first as I expect we can introduce some form of user policy for how much information to print if it is requested by users.
There was a problem hiding this comment.
Eh, sure. I've mentally gone back and forth about this this whole patch, but I'm still very ambivalent. Since I can't strongly justify the difference, having one way of printing is simpler than two (in addition to the advantages you mentioned). Done.
mysterymath
force-pushed
the
lld-why-live
branch
from
fa04ac8 to
c34ae3c
Compare
| relSec->nextInSectionGroup))) { | ||
| Symbol *canonicalSym = d; | ||
| if (TrackWhyLive && d->isSection()) { | ||
| if (Symbol *s = relSec->getEnclosingSymbol(offset)) |
There was a problem hiding this comment.
As mentioned, getEnclosingSymbol iterates all symbols in the file, which is very slow. This perhaps should only be run after we get a path from a root to the --why-live specified symbol and know that the information is needed.
There was a problem hiding this comment.
I think it's possible, but it AFAICT it would be quite complicated.
The other instance of getEnclosingSymbol was easy to defer, since it only affected the right hand side of the whyLive map. Deferring determination of identity on the left hand side of the map is different. In particular, it wouldn't be clear whether a given SecOffset actually does match the --why-live glob without resolving it. It also makes it difficult to maintain the "first reference wins" invariant that makes the whyLive graph acyclic. Multiple SecOffsets could end up resolving to the same symbol, and the resolved reason would need to be the first of all such references in the order of discovery.
I've added some comments to document the invariant and the reasoning for leaving this in. I'd expect that walking the symbols for a referenced section wouldn't be too bad in the usual --function-sections --data-sections case, but if that's wrong, we could build something like the following:
- Keep an order-of-discovery counter for each deferred
SecOffset - Walk all symbols and collect sections that contain symbols that match globs and unresolved
SecOffsets - For each such section:
- Collect the unresolved offsets and sort them
- Walk the symbols in value order and resolve the offsets in one pass
- While printing, if a section is encountered that has unresolved
SecOffsets, do the above routine too.
I suppose we could also just throw up our hands and build a symbol interval map for each section when doing whyLive. That would probably be dramatically simpler.
lld/docs/ld.lld.1
Outdated
| @@ -751,6 +751,8 @@ Report unresolved symbols as warnings. | |||
| Force load of all members in a static library. | |||
| .It Fl -why-extract Ns = Ns Ar file | |||
| Print to a file about why archive members are extracted. | |||
| .It Fl --why-live Ns = Ns Ar glob | |||
lld/test/ELF/why-live.s
Outdated
|
|
||
| # RUN: ld.lld %t.o %t.so -o /dev/null --gc-sections --why-live=test_incidental | FileCheck %s --check-prefix=INCIDENTAL | ||
|
|
||
| # INCIDENTAL: live symbol: {{.*}}.o:(test_incidental) |
There was a problem hiding this comment.
If you use rm -rf %t && split-file %s %t && cd %t. You can write a.o instead of the vague {{.*}}.o. You could also omit -o /dev/null
There was a problem hiding this comment.
Thanks, nice. Done. I probably should have done this when I ended up with a .so anyway.
| // If a symbol is referenced in a live section, it is used. | ||
| Symbol &sym = sec.file->getRelocTargetSym(rel); | ||
| sym.used = true; | ||
|
|
||
| LiveReason reason; | ||
| if (TrackWhyLive) |
There was a problem hiding this comment.
I had originally written it this way, but upon reflection I didn't really want the "doesn't compile the contents" behavior, in case it masks errors in the contents. (Similar to the old C preference of if to #if to avoid bitrot.) Assuming that a compiler worth its salt can delete the if, is there a strong reason to use if constexpr here?
lld/test/ELF/why-live.s
Outdated
| # FROM-UNSIZED-NEXT: >>> referenced by: {{.*}}.o:(.test_simple) | ||
| # FROM-UNSIZED-NEXT: >>> contained live symbol: {{.*}}.o:(test_simple) | ||
| # FROM-UNSIZED-NEXT: >>> referenced by: {{.*}}.o:(_start) (entry point) | ||
| # FROM-UNSIZED-NOT: >>> |
There was a problem hiding this comment.
could replace these -NOT: >>> with -EMPTY:
|
@MaskRay, is this ready to land? |
lld/ELF/MarkLive.cpp
Outdated
| @@ -42,16 +44,30 @@ using namespace lld; | |||
| using namespace lld::elf; | |||
|
|
|||
| namespace { | |||
| template <class ELFT> class MarkLive { | |||
|
|
|||
There was a problem hiding this comment.
delete the blank line after namespace {
lld/test/ELF/why-live.test
Outdated
|
|
||
| ## Globs match multiple cases. Multiple --why-live flags union. | ||
|
|
||
| # RUN: ld.lld a.o a.so --gc-sections --why-live=test_s* | FileCheck %s --check-prefix=MULTIPLE |
There was a problem hiding this comment.
perhaps add another test_s* to show the behavior when a symbol is matched by multiple patterns.
We might need XXX-NOT: live
This prints the stack of reasons that symbols that match the given glob(s) survived GC. It has no effect unless section GC occurs. A symbol may be live intrisically, because referenced by another symbol or section, or because part of a live section. Sections have similar reasons. This implementation does not require -ffunction-sections or -fdata-sections to produce readable results, althought it does tend to work better (as does GC).
- Explicitly track when explicitly live - This prevents e.g. _start from being reported live as part of .start. - Use quad for more control over reloc offsets
- Prefer "using" - Defer lookup of sec+offset liveness reasons to printing time - Indent instructions - Avoid unnecessary toStr() - Provide description strings to print with liveness reasons
- LiveObject -> LiveItem - nit: alive -> live - Prefer "keeps live" to "(may) make live" - Inline getParentSec lambda - Always print symbol file - reason.item guard clause - Use /*offset=*/ and /*sym=*/ for enqueue - Update ld.lld.1
- Add comments clarifying invariants and getEnclosinSymbol rationale - Remove extraneous "-" from ld.lld.1 - Replace "-NOT: >>>" with "-EMPTY:" - Use split-file to simplify paths
- Delete blank line after "namespace {"
- Extend glob test to cover multiple pattern matches; add NOT
mysterymath
force-pushed
the
lld-why-live
branch
from
f4fd53b to
5938210
Compare
lld/test/ELF/why-live.test
Outdated
|
|
||
| ## Globs match multiple cases. Multiple --why-live flags union. | ||
|
|
||
| # RUN: ld.lld a.o a.so --gc-sections --why-live=test_se* --why-live=test_se* | FileCheck %s --check-prefix=MULTIPLE |
There was a problem hiding this comment.
When * is used in an argument, quote it.
This prints a stack of reasons that symbols that match the given glob(s) survived GC. It has no effect unless section GC occurs.
This implementation does not require -ffunction-sections or -fdata-sections to produce readable results, althought it does tend to work better (as does GC).
Details about the semantics:
STB_LOCAL) are supported.STT_SECTION):