-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[lld][WebAssembly] Preserve LTO stub deps for bitcode symbols #173235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When a stub .so file contains ``` A: B ``` And `A` is defined in bitcode that's pulled in for LTO, but both `A` and `B` are removed in `LTO::linkRegularLTO` due to not being dead: https://github.com/llvm/llvm-project/blob/24297bea9672722d8fbaaff137b301b0becaae9c/llvm/lib/LTO/LTO.cpp#L1042-L1054 Then the symbol `A` becomes undefined after LTO, `processStubLibraries` tries to import `A` from JS, and tries to export its dependency `B`: https://github.com/llvm/llvm-project/blob/24297bea9672722d8fbaaff137b301b0becaae9c/lld/wasm/Driver.cpp#L1108-L1109 But `B` is gone, causing this error: ```console wasm-ld: error: ....: undefined symbol: B. Required by A ``` This PR marks `B` as required not only when `A` is null or undefined but also `A` is in bitcode, because bitcode functions can be DCE'd, causing the linker to import the symbol and export its dependencies.
|
@llvm/pr-subscribers-lld-wasm @llvm/pr-subscribers-lld Author: Heejin Ahn (aheejin) ChangesWhen a stub .so file contains And llvm-project/llvm/lib/LTO/LTO.cpp Lines 1042 to 1054 in 24297be
A becomes undefined after LTO, processStubLibraries tries to import A from JS, and tries to export its dependency B: llvm-project/lld/wasm/Driver.cpp Lines 1108 to 1109 in 24297be
B is gone, causing this error:
wasm-ld: error: ....: undefined symbol: B. Required by AThis PR marks Full diff: https://github.com/llvm/llvm-project/pull/173235.diff 3 Files Affected:
diff --git a/lld/test/wasm/lto/Inputs/stub-dependency-main.s b/lld/test/wasm/lto/Inputs/stub-dependency-main.s
new file mode 100644
index 0000000000000..9e84901a6fe80
--- /dev/null
+++ b/lld/test/wasm/lto/Inputs/stub-dependency-main.s
@@ -0,0 +1,4 @@
+.globl _start
+_start:
+ .functype _start () -> ()
+ end_function
diff --git a/lld/test/wasm/lto/stub-dependency.ll b/lld/test/wasm/lto/stub-dependency.ll
new file mode 100644
index 0000000000000..3b2daf39cc03a
--- /dev/null
+++ b/lld/test/wasm/lto/stub-dependency.ll
@@ -0,0 +1,33 @@
+; RUN: llvm-as %s -o %t.bc
+; RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %p/Inputs/stub-dependency-main.s -o %t.o
+; RUN: echo "#STUB" > %t.so
+; RUN: echo "A: B" >> %t.so
+; RUN: wasm-ld %t.o %t.bc %t.so -o %t.wasm
+; RUN: obj2yaml %t.wasm | FileCheck %s
+
+; Test if LTO stub dependencies are preserved if a symbol they depend on is
+; defined in bitcode and DCE'd and become undefined in the LTO process. Here 'B'
+; should be preserved and exported.
+
+target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
+target triple = "wasm32-unknown-unknown"
+
+define void @A() {
+ ret void
+}
+
+define void @B() {
+ ret void
+}
+
+; CHECK: - Type: EXPORT
+; CHECK-NEXT: Exports:
+; CHECK-NEXT: - Name: memory
+; CHECK-NEXT: Kind: MEMORY
+; CHECK-NEXT: Index: 0
+; CHECK-NEXT: - Name: _start
+; CHECK-NEXT: Kind: FUNCTION
+; CHECK-NEXT: Index: 0
+; CHECK-NEXT: - Name: B
+; CHECK-NEXT: Kind: FUNCTION
+; CHECK-NEXT: Index: 1
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 97e50783985a8..71daa30f1dec9 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -1028,7 +1028,11 @@ static void processStubLibrariesPreLTO() {
// If the symbol is not present at all (yet), or if it is present but
// undefined, then mark the dependent symbols as used by a regular
// object so they will be preserved and exported by the LTO process.
- if (!sym || sym->isUndefined()) {
+ // If the symbol is defined and in bitcode, it can be DCE'd during LTO and
+ // become undefined, so mark the dependent symbols as used by a regular
+ // object as well.
+ if (!sym || sym->isUndefined() ||
+ (sym->isDefined() && isa_and_nonnull<BitcodeFile>(sym->getFile()))) {
for (const auto dep : deps) {
auto* needed = symtab->find(dep);
if (needed ) {
|
| .globl _start | ||
| _start: | ||
| .functype _start () -> () | ||
| end_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have lld/test/wasm/Inputs/start.s which has this same content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: b16180f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was merged into the existing stub-library.s anyway: 414e7a4
lld/test/wasm/lto/stub-dependency.ll
Outdated
| ; RUN: llvm-as %s -o %t.bc | ||
| ; RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %p/Inputs/stub-dependency-main.s -o %t.o | ||
| ; RUN: echo "#STUB" > %t.so | ||
| ; RUN: echo "A: B" >> %t.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you checking the stub file on its own rather then using echo to create it here?
Or better still use the split-file thing to create two files from here. See lld/test/wasm/no-strip-segment.s for an example of how to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was merged into stub-library.s, reusing the existing stub.so: 414e7a4
lld/test/wasm/lto/stub-library.s
Outdated
| ## defined in an LTO object. We also test the case where the LTO object is | ||
| ## with an archive file. | ||
| ## The function `quux` is declared in stub.so and depends on `bar` which is | ||
| ## defined in an LTO object as well. In this case, `bar` is DCE'd and becomes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems a little backwards to me. It looks like baz is defined in the stub.so and baz depends on quux but I don't see quux depending on anything at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like baz is both defined in foo.ll and declared in stub.so to be importable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## The function `baz` is declared in stub.so and depends on `quux`, and both | ||
| ## `baz` and `quux` are defined in an LTO object. When `baz` is DCE'd and | ||
| ## becomes undefined in the LTO process, `quux` should still be preserved and | ||
| ## exported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a little confused I'm afarid.
If baz is never actually undefined (i.e. never actually needs to be imported at all) why would we want to use the import from stub.so and therefore why would we ever export quux in this case?
if baz was used in this program wouldn't we pick the one defined internally rather than externally? But baz is never actually needed anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure myself why we should preserve and export it, but it looks that's what processStubLibraries expects:
llvm-project/lld/wasm/Driver.cpp
Lines 1108 to 1109 in 2040b55
| if (sym && sym->isUndefined()) { | |
| depsAdded |= addStubSymbolDeps(stub_file, sym, deps); |
baz is DCE'd and becomes undefined, but processStubLibraries tries to export the deps in case sym && sym->isUndefined(). My fix is a way to avoid this crash. Should we fix this part instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the problem is, processStubLibraries does not distinguish a case where a symbol is used but undefined (so that we need to import it) and a case where the symbol is not used and becomes undefined in the LTO. Is there a good way to add this condition here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the symbol baz is always defined isn't it? So it should never need to be imported at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stub library symbol should not be used in the case that a definition exists in the program being linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was defined and pulled in to LTO but was removed and became undefined in
llvm-project/llvm/lib/LTO/LTO.cpp
Lines 1042 to 1054 in 24297be
| if (LivenessFromIndex && !ThinLTO.CombinedIndex.isGUIDLive(GV->getGUID())) { | |
| if (Function *F = dyn_cast<Function>(GV)) { | |
| if (DiagnosticOutputFile) { | |
| if (Error Err = F->materialize()) | |
| return Err; | |
| OptimizationRemarkEmitter ORE(F, nullptr); | |
| ORE.emit(OptimizationRemark(DEBUG_TYPE, "deadfunction", F) | |
| << ore::NV("Function", F) | |
| << " not added to the combined module "); | |
| } | |
| } | |
| continue; | |
| } |
processStubLibraries sees it as undefined and decides it should be imported
The stub library symbol should not be used in the case that a definition exists in the program being linked.
I agree but that's not how the code here is written. We should either (unnecessarily) make the deps available and export them (this PR) or exclude these cases in processStubLibraries after LTO. I think the latter is better than this PR (because it avoids unnecessary exports) Maybe I should try that (even though I'm not sure how to do it now; I can investigate)
| ret void | ||
| } | ||
|
|
||
| define void @quux() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this file is called just foo its seems maybe odd to defined these other symbols here too? Maybe not too bad though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to reuse existing foo but it didn't fit the case here. Should we rename the file?
When a stub .so file contains
And
Ais defined in bitcode that's pulled in for LTO, but bothAandBare removed inLTO::linkRegularLTOdue to not being dead:llvm-project/llvm/lib/LTO/LTO.cpp
Lines 1042 to 1054 in 24297be
Abecomes undefined after LTO,processStubLibrariestries to importAfrom JS, and tries to export its dependencyB:llvm-project/lld/wasm/Driver.cpp
Lines 1108 to 1109 in 24297be
Bis gone, causing this error:wasm-ld: error: ....: undefined symbol: B. Required by AThis PR marks
Bas required not only whenAis null or undefined but alsoAis in bitcode, because bitcode functions can be DCE'd, causing the linker to import the symbol and export its dependencies.