[lld][WebAssembly] Fix stub library deps causing LTO archive members to be required post-LTO #101894
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbc100
changed the title
|
@llvm/pr-subscribers-lld Author: Sam Clegg (sbc100) ChangesFixes: emscripten-core/emscripten#16836 Full diff: https://github.com/llvm/llvm-project/pull/101894.diff 4 Files Affected:
diff --git a/lld/test/wasm/lto/stub-library.s b/lld/test/wasm/lto/stub-library.s
index 20e2a62211413..a5c724012faec 100644
--- a/lld/test/wasm/lto/stub-library.s
+++ b/lld/test/wasm/lto/stub-library.s
@@ -1,12 +1,15 @@
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
-# RUN: llvm-as %S/Inputs/foo.ll -o %t1.o
-# RUN: wasm-ld %t.o %t1.o %p/Inputs/stub.so -o %t.wasm
+# RUN: mkdir -p %t
+# RUN: llvm-as %S/Inputs/foo.ll -o %t/foo.o
+# RUN: rm -f %t/libfoo.a
+# RUN: llvm-ar rcs %t/libfoo.a %t/foo.o
+# RUN: wasm-ld %t.o %t/libfoo.a %p/Inputs/stub.so -o %t.wasm
# RUN: obj2yaml %t.wasm | FileCheck %s
-# The function `bar` is declared in stub.so and depends on `foo`, which happens
-# be in an LTO object.
-# This verifies that stub library dependencies (required exports) can be defined
-# in LTO objects.
+## The function `bar` is declared in stub.so and depends on `foo`, which happens
+## be in an LTO object, in an archive file.
+## This verifies that stub library dependencies (required exports) can be defined
+## in LTO objects.
.functype bar () -> ()
.globl _start
diff --git a/lld/test/wasm/stub-library-archive.s b/lld/test/wasm/stub-library-archive.s
index 76483d1463d64..131f21bea4a13 100644
--- a/lld/test/wasm/stub-library-archive.s
+++ b/lld/test/wasm/stub-library-archive.s
@@ -1,6 +1,7 @@
# RUN: split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %t/main.s
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t/foodeps.o %t/foodeps.s
+# RUN: mkdir -p %t
# RUN: rm -f %t/libfoodeps.a
# RUN: llvm-ar rcs %t/libfoodeps.a %t/foodeps.o
# RUN: wasm-ld %t.o %p/Inputs/libstub.so %t/libfoodeps.a -o %t.wasm
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 8c83d17db02f5..f8a163ee39eb7 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -945,10 +945,23 @@ static void processStubLibrariesPreLTO() {
// 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()) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "marking symbol deps: " << name << "\n");
for (const auto dep : deps) {
auto* needed = symtab->find(dep);
if (needed ) {
needed->isUsedInRegularObj = true;
+ // Like with handleLibcall we have extract any archive members
+ // that might need to be exported due to stub library symbol being
+ // used. Without this the LTO object could be extracted during
+ // processStubLibraries, which is too late since LTO has already
+ // beeing performed at that point.
+ if (needed->isLazy() && isa<BitcodeFile>(needed->getFile())) {
+ if (!config->whyExtract.empty())
+ ctx.whyExtractRecords.emplace_back("<stubdep>", needed->getFile(),
+ *needed);
+ cast<LazySymbol>(needed)->extract();
+ }
}
}
}
diff --git a/lld/wasm/InputFiles.cpp b/lld/wasm/InputFiles.cpp
index f3f0ef9a99497..532c2e619e1bb 100644
--- a/lld/wasm/InputFiles.cpp
+++ b/lld/wasm/InputFiles.cpp
@@ -761,7 +761,7 @@ void StubFile::parse() {
}
// Lines starting with # are considered comments
- if (line.starts_with("#"))
+ if (line.starts_with("#") || !line.size())
continue;
StringRef sym;
|
|
@llvm/pr-subscribers-lld-wasm Author: Sam Clegg (sbc100) ChangesFixes: emscripten-core/emscripten#16836 Full diff: https://github.com/llvm/llvm-project/pull/101894.diff 4 Files Affected:
diff --git a/lld/test/wasm/lto/stub-library.s b/lld/test/wasm/lto/stub-library.s
index 20e2a62211413..a5c724012faec 100644
--- a/lld/test/wasm/lto/stub-library.s
+++ b/lld/test/wasm/lto/stub-library.s
@@ -1,12 +1,15 @@
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
-# RUN: llvm-as %S/Inputs/foo.ll -o %t1.o
-# RUN: wasm-ld %t.o %t1.o %p/Inputs/stub.so -o %t.wasm
+# RUN: mkdir -p %t
+# RUN: llvm-as %S/Inputs/foo.ll -o %t/foo.o
+# RUN: rm -f %t/libfoo.a
+# RUN: llvm-ar rcs %t/libfoo.a %t/foo.o
+# RUN: wasm-ld %t.o %t/libfoo.a %p/Inputs/stub.so -o %t.wasm
# RUN: obj2yaml %t.wasm | FileCheck %s
-# The function `bar` is declared in stub.so and depends on `foo`, which happens
-# be in an LTO object.
-# This verifies that stub library dependencies (required exports) can be defined
-# in LTO objects.
+## The function `bar` is declared in stub.so and depends on `foo`, which happens
+## be in an LTO object, in an archive file.
+## This verifies that stub library dependencies (required exports) can be defined
+## in LTO objects.
.functype bar () -> ()
.globl _start
diff --git a/lld/test/wasm/stub-library-archive.s b/lld/test/wasm/stub-library-archive.s
index 76483d1463d64..131f21bea4a13 100644
--- a/lld/test/wasm/stub-library-archive.s
+++ b/lld/test/wasm/stub-library-archive.s
@@ -1,6 +1,7 @@
# RUN: split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %t/main.s
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t/foodeps.o %t/foodeps.s
+# RUN: mkdir -p %t
# RUN: rm -f %t/libfoodeps.a
# RUN: llvm-ar rcs %t/libfoodeps.a %t/foodeps.o
# RUN: wasm-ld %t.o %p/Inputs/libstub.so %t/libfoodeps.a -o %t.wasm
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 8c83d17db02f5..f8a163ee39eb7 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -945,10 +945,23 @@ static void processStubLibrariesPreLTO() {
// 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()) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "marking symbol deps: " << name << "\n");
for (const auto dep : deps) {
auto* needed = symtab->find(dep);
if (needed ) {
needed->isUsedInRegularObj = true;
+ // Like with handleLibcall we have extract any archive members
+ // that might need to be exported due to stub library symbol being
+ // used. Without this the LTO object could be extracted during
+ // processStubLibraries, which is too late since LTO has already
+ // beeing performed at that point.
+ if (needed->isLazy() && isa<BitcodeFile>(needed->getFile())) {
+ if (!config->whyExtract.empty())
+ ctx.whyExtractRecords.emplace_back("<stubdep>", needed->getFile(),
+ *needed);
+ cast<LazySymbol>(needed)->extract();
+ }
}
}
}
diff --git a/lld/wasm/InputFiles.cpp b/lld/wasm/InputFiles.cpp
index f3f0ef9a99497..532c2e619e1bb 100644
--- a/lld/wasm/InputFiles.cpp
+++ b/lld/wasm/InputFiles.cpp
@@ -761,7 +761,7 @@ void StubFile::parse() {
}
// Lines starting with # are considered comments
- if (line.starts_with("#"))
+ if (line.starts_with("#") || !line.size())
continue;
StringRef sym;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
sbc100
force-pushed
the
lto_stubs
branch
2 times, most recently
from
42c33c5 to
c693efe
Compare
kripken
approved these changes
Aug 6, 2024
| ctx.whyExtractRecords.emplace_back(toString(stub_file), | ||
| needed->getFile(), *needed); | ||
| cast<LazySymbol>(needed)->extract(); | ||
| } |
There was a problem hiding this comment.
Perhaps this code could be shared with handleLibcall?
…to be required post-LTO Fixes: emscripten-core/emscripten#16836 Fixes: emscripten-core/emscripten#20275
kripken
approved these changes
Aug 6, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/2546 Here is the relevant piece of the build log for the reference: |
|
This seems to have caused a regression in the LTO versions of emscripten's test_lazy_load_code_unconditional test (see here for a build log). |
|
/cc @kripken |
|
I confirmed the code size regression mentioned in the last comment has been resolved (almost entirely, at least: current sizes are 5174 and 9786, but that's very close to before the regression). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: emscripten-core/emscripten#16836