-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[lld][WebAssembly] Do not relocate ABSOLUTE symbols #153763
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
@llvm/pr-subscribers-lld-wasm Author: YAMAMOTO Takashi (yamt) ChangesFixes #153537 Full diff: https://github.com/llvm/llvm-project/pull/153763.diff 1 Files Affected:
diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp
index e1192706ea913..6c1c4391ea0d7 100644
--- a/lld/wasm/SyntheticSections.cpp
+++ b/lld/wasm/SyntheticSections.cpp
@@ -442,6 +442,8 @@ void GlobalSection::generateRelocationCode(raw_ostream &os, bool TLS) const {
for (const Symbol *sym : internalGotSymbols) {
if (TLS != sym->isTLS())
continue;
+ if (sym->flags & WASM_SYMBOL_ABSOLUTE)
+ continue;
if (auto *d = dyn_cast<DefinedData>(sym)) {
// Get __memory_base
@@ -503,7 +505,8 @@ void GlobalSection::writeBody() {
bool useExtendedConst = false;
uint32_t globalIdx;
int64_t offset;
- if (ctx.arg.extendedConst && ctx.isPic) {
+ if (ctx.arg.extendedConst && ctx.isPic &&
+ (sym->flags & WASM_SYMBOL_ABSOLUTE) == 0) {
if (auto *d = dyn_cast<DefinedData>(sym)) {
if (!sym->isTLS()) {
globalIdx = ctx.sym.memoryBase->getGlobalIndex();
|
@llvm/pr-subscribers-lld Author: YAMAMOTO Takashi (yamt) ChangesFixes #153537 Full diff: https://github.com/llvm/llvm-project/pull/153763.diff 1 Files Affected:
diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp
index e1192706ea913..6c1c4391ea0d7 100644
--- a/lld/wasm/SyntheticSections.cpp
+++ b/lld/wasm/SyntheticSections.cpp
@@ -442,6 +442,8 @@ void GlobalSection::generateRelocationCode(raw_ostream &os, bool TLS) const {
for (const Symbol *sym : internalGotSymbols) {
if (TLS != sym->isTLS())
continue;
+ if (sym->flags & WASM_SYMBOL_ABSOLUTE)
+ continue;
if (auto *d = dyn_cast<DefinedData>(sym)) {
// Get __memory_base
@@ -503,7 +505,8 @@ void GlobalSection::writeBody() {
bool useExtendedConst = false;
uint32_t globalIdx;
int64_t offset;
- if (ctx.arg.extendedConst && ctx.isPic) {
+ if (ctx.arg.extendedConst && ctx.isPic &&
+ (sym->flags & WASM_SYMBOL_ABSOLUTE) == 0) {
if (auto *d = dyn_cast<DefinedData>(sym)) {
if (!sym->isTLS()) {
globalIdx = ctx.sym.memoryBase->getGlobalIndex();
|
0196c11
to
9aa3bf1
Compare
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 we add a test for this? Perhaps one of the existing tests for internal GOT symbols could have a reference to an absolute symbol (the page size symbol maybe?) added?
lld/wasm/SyntheticSections.cpp
Outdated
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.
Maybe do this 2 lines up before the TLS check?
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.
why? just because it can be slightly cheaper?
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
af06b91
to
4766075
Compare
i added a test. |
4766075
to
08fde96
Compare
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.
lgtm % comment
This causes a bunch of test failures on the emscripten waterfall, mostly lsan tests. I'm guessing that not all symbols we add with I think we should revert until we can figure out the best approach here. |
Reverts #153763 This caused a bunch of failures on the emscripten waterfall, specifically most of the lsan tests started failing.
…s" (#154371) Reverts llvm/llvm-project#153763 This caused a bunch of failures on the emscripten waterfall, specifically most of the lsan tests started failing.
where can i see them? |
Looks like all the tests that use lsan + dynamic linking are reporting false leaks:
At least I assume that are false positives, since they go away when this change is reverted. My guess is that we are using |
|
Fixes #153759