In particular, we should apply the -undefined behavior to all
such symbols, include those that are specified via the command line
(i.e. -e, -u, and -exported_symbol). ld64 supports this too.
Details
- Reviewers
thakis gkm - Group Reviewers
Restricted Project - Commits
- rG2516b0b5261d: [lld-macho] Treat undefined symbols uniformly
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Interesting! Do you know of anything that actually does this? Why? Every use case I can think of would better use -U main instead of -undefined dynamic_lookup.
(Change lgtm in any case.)
lld/MachO/Driver.cpp | ||
---|---|---|
1152–1153 | I think this needs basically the same change. -u _foo -undefined dynamic_lookup forces a dynamically-looked-up symbol foo to exist at load time with ld64. (Can't really think of a use case for that either, but since it's next to the entry point right now and behaves the same way right now, this should probably receive the same treatment in this change.) |
Bah, I accidentally removed you as the reviewer when doing arc diff --verbatim. I've made a bunch of changes since you stamped it though, might be worth to have another quick look...
Interesting! Do you know of anything that actually does this?
Nope, it was just something that occurred to me to test as I was reviewing our entry symbol handling...
lld/MachO/Driver.cpp | ||
---|---|---|
1152–1153 | good point... looks like -exported_symbol needs the same treatment too. I've verified that ld64 does apply the same behavior to them, and this diff has been expanded accordingly. |
Thanks! Still lg, but more suggestions:
lld/MachO/Driver.cpp | ||
---|---|---|
1146–1150 | Could we keep this here so all the treatUndefinedSymbol()s are in one place? Or is there something that forces this to move around? if (config->outputType == MH_EXECUTE && isa<Undefined>(config->entry)) { treatUndefinedSymbol(cast<Undefined(config->entry), "the entry point"); if (isa<Undefined>(config->entry)) return false; } | |
lld/MachO/SymbolTable.cpp | ||
197 | nit: s/&/source/ And if we're doing capturing, do [source, sym] and remove the arg, to make things self-consistent. (& works here, but with & it's easy to accidentally capture a stack variable, and if the closure escapes that's a recipe for subtle bugs, and compilers don't yet warn on default-capturing locals in escaping closures. So I try to not use default capture.) | |
205 | nit: this reads imho easier like so: std::string message = "undefined symbol: " + toString(sym); if (!source.empty()) message += "\n>>> referenced by " + source.str(); else message += "\n>>> referenced by " + toString(sym.getFile()); return message; That was source doesn't point to different things based on control flow, and it's fairly linear and straight-forward. (The "referenced by" string is duplicated, but right on top of each other, so it's obvious it's the same) (It allocates more, but only in a code path that emits diags, so it doesn't normally run anyways.) |
- move undefined check for entry back into Driver.cpp
- return all undefined symbol errors from -u, not just the first one, similar to what we do for -exported_symbol
lld/MachO/SymbolTable.cpp | ||
---|---|---|
197 | fair enough. while at it I renamed message to msg so it doesn't shadow the message defined in lld/Common. |
lg with writer.cpp change undone
lld/MachO/SymbolTable.cpp | ||
---|---|---|
197 | meh, the local message string shadows too. i'd keep the old name (but up to you) | |
198 | Since you're doing returns in the conditional now: style guide says no else after return :P | |
lld/MachO/Writer.cpp | ||
1062 | ...this change is no longer needed now that this is done in driver, rigth? |
Could we keep this here so all the treatUndefinedSymbol()s are in one place? Or is there something that forces this to move around?