-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lld][WebAssembly] Fix check for implicitly exported mutable globals #160966
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
This change is designed to avoid exporting mutable globals in the case when mutable globals are not available. However, it was being applied in all cases even when mutable globals was enabled.
@llvm/pr-subscribers-lld-wasm @llvm/pr-subscribers-lld Author: Sam Clegg (sbc100) ChangesThis change is designed to avoid exporting mutable globals in the case when mutable globals are not available. However, it was being applied in all cases even when mutable globals was enabled. Full diff: https://github.com/llvm/llvm-project/pull/160966.diff 2 Files Affected:
diff --git a/lld/test/wasm/mutable-global-exports.s b/lld/test/wasm/mutable-global-exports.s
index 4ffaf0a6cbaf0..1c10e92083b5c 100644
--- a/lld/test/wasm/mutable-global-exports.s
+++ b/lld/test/wasm/mutable-global-exports.s
@@ -73,6 +73,9 @@ _start:
# CHECK-ALL-NEXT: - Name: __wasm_call_ctors
# CHECK-ALL-NEXT: Kind: FUNCTION
# CHECK-ALL-NEXT: Index: 0
+# CHECK-ALL-NEXT: - Name: __stack_pointer
+# CHECK-ALL-NEXT: Kind: GLOBAL
+# CHECK-ALL-NEXT: Index: 0
# CHECK-ALL-NEXT: - Name: _start
# CHECK-ALL-NEXT: Kind: FUNCTION
# CHECK-ALL-NEXT: Index: 1
diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index 0d36893653110..9a5b56fc52e2f 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -784,6 +784,9 @@ void Writer::calculateExports() {
unsigned globalIndex =
out.importSec->getNumImportedGlobals() + out.globalSec->numGlobals();
+ bool hasMutableGlobals =
+ out.targetFeaturesSec->features.count("mutable-globals") > 0;
+
for (Symbol *sym : symtab->symbols()) {
if (!sym->isExported())
continue;
@@ -801,7 +804,8 @@ void Writer::calculateExports() {
}
export_ = {name, WASM_EXTERNAL_FUNCTION, f->getExportedFunctionIndex()};
} else if (auto *g = dyn_cast<DefinedGlobal>(sym)) {
- if (g->getGlobalType()->Mutable && !g->getFile() && !g->forceExport) {
+ if (!hasMutableGlobals && g->getGlobalType()->Mutable && !g->getFile() &&
+ !g->isExportedExplicit()) {
// Avoid exporting mutable globals are linker synthesized (e.g.
// __stack_pointer or __tls_base) unless they are explicitly exported
// from the command line.
|
So before we were basically just never exporting the stack pointer when EXPORT_ALL was set, and now we should be when mutable globals is available? |
Yes, exactly. |
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 guess the stack pointer is very rarely used outside of the module, and EXPORT_ALL is also uncommon, so nobody noticed.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/12727 Here is the relevant piece of the build log for the reference
|
…lvm#160966) This check is designed to avoid exporting mutable globals in the case when mutable globals are not available. However, it was being applied in all cases even when mutable globals was enabled. This error is particularly bad since mutable-globals have been enabled in default CPU for a while now, meaning that this condition should not be firing in the wild very often.
This check is designed to avoid exporting mutable globals in the case when mutable globals are not available. However, it was being applied in all cases even when mutable globals was enabled.
This error is particularly bad since mutable-globals have been enabled in default CPU for a while now, meaning that this condition should not be firing in the wild very often.