[lld][WebAssembly] Avoid emitting empty __wasm_apply_data_relocs function #109249
Conversation
This file contains 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
|
@llvm/pr-subscribers-lld-wasm Author: YAMAMOTO Takashi (yamt) Changes… phase Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations:
Full diff: https://github.com/llvm/llvm-project/pull/109249.diff 10 Files Affected:
diff --git a/lld/test/wasm/data-segments.ll b/lld/test/wasm/data-segments.ll
index 9354e6c8e4d2b0..670ac3c1f373fa 100644
--- a/lld/test/wasm/data-segments.ll
+++ b/lld/test/wasm/data-segments.ll
@@ -113,7 +113,7 @@
; PASSIVE-NEXT: Name: __wasm_init_memory
; PASSIVE-PIC: - Type: START
-; PASSIVE-PIC-NEXT: StartFunction: 3
+; PASSIVE-PIC-NEXT: StartFunction: 2
; PASSIVE-PIC-NEXT: - Type: DATACOUNT
; PASSIVE-PIC-NEXT: Count: 3
; PASSIVE-PIC-NEXT: - Type: CODE
@@ -125,9 +125,6 @@
; PASSIVE-PIC-NEXT: Locals: []
; PASSIVE-PIC-NEXT: Body: {{.*}}
; PASSIVE-PIC-NEXT: - Index: 2
-; PASSIVE-PIC-NEXT: Locals: []
-; PASSIVE-PIC-NEXT: Body: 0B
-; PASSIVE-PIC-NEXT: - Index: 3
; PASSIVE-PIC-NEXT: Locals:
; PASSIVE32-PIC-NEXT: - Type: I32
; PASSIVE64-PIC-NEXT: - Type: I64
@@ -152,8 +149,6 @@
; PASSIVE-PIC-NEXT: - Index: 1
; PASSIVE-PIC-NEXT: Name: __wasm_init_tls
; PASSIVE-PIC-NEXT: - Index: 2
-; PASSIVE-PIC-NEXT: Name: __wasm_apply_data_relocs
-; PASSIVE-PIC-NEXT: - Index: 3
; PASSIVE-PIC-NEXT: Name: __wasm_init_memory
; no data relocations.
@@ -161,8 +156,6 @@
; DIS-EMPTY:
; DIS-NEXT: end
-; In PIC mode __wasm_apply_data_relocs is export separatly to __wasm_call_ctors
-; PIC-DIS: <__wasm_apply_data_relocs>:
; PIC-DIS-EMPTY:
; DIS-LABEL: <__wasm_init_memory>:
diff --git a/lld/test/wasm/shared-weak-symbols.s b/lld/test/wasm/shared-weak-symbols.s
index 90de006353b3d3..df049ce4600fe4 100644
--- a/lld/test/wasm/shared-weak-symbols.s
+++ b/lld/test/wasm/shared-weak-symbols.s
@@ -30,7 +30,7 @@ call_weak:
# ASM: 10 80 80 80 80 00 call 0
drop
call hidden_weak_func
-# ASM: 10 84 80 80 80 00 call 4
+# ASM: 10 83 80 80 80 00 call 3
end_function
# ASM-NEXT: 0b end
@@ -62,15 +62,12 @@ call_weak:
# CHECK-NEXT: - Name: __wasm_call_ctors
# CHECK-NEXT: Kind: FUNCTION
# CHECK-NEXT: Index: 1
-# CHECK-NEXT: - Name: __wasm_apply_data_relocs
-# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 2
# CHECK-NEXT: - Name: weak_func
# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 3
+# CHECK-NEXT: Index: 2
# CHECK-NEXT: - Name: call_weak
# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 5
+# CHECK-NEXT: Index: 4
# CHECK-NEXT: - Type: CODE
# CHECK: - Type: CUSTOM
@@ -81,10 +78,8 @@ call_weak:
# CHECK-NEXT: - Index: 1
# CHECK-NEXT: Name: __wasm_call_ctors
# CHECK-NEXT: - Index: 2
-# CHECK-NEXT: Name: __wasm_apply_data_relocs
-# CHECK-NEXT: - Index: 3
# CHECK-NEXT: Name: weak_func
-# CHECK-NEXT: - Index: 4
+# CHECK-NEXT: - Index: 3
# CHECK-NEXT: Name: hidden_weak_func
-# CHECK-NEXT: - Index: 5
+# CHECK-NEXT: - Index: 4
# CHECK-NEXT: Name: call_weak
diff --git a/lld/test/wasm/tls-export.s b/lld/test/wasm/tls-export.s
index 1f64be607abb23..619f9d2df312ab 100644
--- a/lld/test/wasm/tls-export.s
+++ b/lld/test/wasm/tls-export.s
@@ -40,9 +40,6 @@ tls1:
# CHECK-NEXT: - Name: __wasm_call_ctors
# CHECK-NEXT: Kind: FUNCTION
# CHECK-NEXT: Index: 0
-# CHECK-NEXT: - Name: __wasm_apply_data_relocs
-# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 1
# CHECK-NEXT: - Name: tls1
# CHECK-NEXT: Kind: GLOBAL
# CHECK-NEXT: Index: 2
diff --git a/lld/test/wasm/tls-non-shared-memory.s b/lld/test/wasm/tls-non-shared-memory.s
index a2e2257cc9392e..1754fd6254bb80 100644
--- a/lld/test/wasm/tls-non-shared-memory.s
+++ b/lld/test/wasm/tls-non-shared-memory.s
@@ -127,9 +127,6 @@ tls1:
# PIE-NEXT: - Name: memory
# PIE-NEXT: Kind: MEMORY
# PIE-NEXT: Index: 0
-# PIE-NEXT: - Name: __wasm_apply_data_relocs
-# PIE-NEXT: Kind: FUNCTION
-# PIE-NEXT: Index: 1
# PIE-NEXT: - Type:
# .tdata and .data are combined into single segment in PIC mode.
diff --git a/lld/test/wasm/tls-relocations.s b/lld/test/wasm/tls-relocations.s
index ebe83227631f47..7260d72535a009 100644
--- a/lld/test/wasm/tls-relocations.s
+++ b/lld/test/wasm/tls-relocations.s
@@ -66,7 +66,7 @@ tls_sym:
# ASM-NEXT: i32.const 16
# ASM-NEXT: memory.init 0, 0
# call to __wasm_apply_tls_relocs
-# ASM-NEXT: call 4
+# ASM-NEXT: call 3
# ASM-NEXT: end
# ASM: <__wasm_apply_tls_relocs>:
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 2de7dcaeb43d47..289c1217ff5ead 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -917,17 +917,6 @@ static void createSyntheticSymbols() {
is64 ? i64ArgSignature : i32ArgSignature,
"__wasm_init_tls"));
}
-
- if (ctx.isPic ||
- config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) {
- // For PIC code, or when dynamically importing addresses, we create
- // synthetic functions that apply relocations. These get called from
- // __wasm_call_ctors before the user-level constructors.
- WasmSym::applyDataRelocs = symtab->addSyntheticFunction(
- "__wasm_apply_data_relocs",
- WASM_SYMBOL_VISIBILITY_DEFAULT | WASM_SYMBOL_EXPORTED,
- make<SyntheticFunction>(nullSignature, "__wasm_apply_data_relocs"));
- }
}
static void createOptionalSymbols() {
diff --git a/lld/wasm/InputChunks.cpp b/lld/wasm/InputChunks.cpp
index 975225974aff6e..f350d4d1c04d6a 100644
--- a/lld/wasm/InputChunks.cpp
+++ b/lld/wasm/InputChunks.cpp
@@ -378,7 +378,7 @@ void InputChunk::generateRelocationCode(raw_ostream &os) const {
uint64_t offset = getVA(rel.Offset) - getInputSectionOffset();
Symbol *sym = file->getSymbol(rel);
- if (!ctx.isPic && sym->isDefined())
+ if (!ctx.isPic && !sym->hasGOTIndex())
continue;
LLVM_DEBUG(dbgs() << "gen reloc: type=" << relocTypeToString(rel.Type)
diff --git a/lld/wasm/Symbols.cpp b/lld/wasm/Symbols.cpp
index f74699d0763fd9..b2bbd11c53ef23 100644
--- a/lld/wasm/Symbols.cpp
+++ b/lld/wasm/Symbols.cpp
@@ -80,7 +80,6 @@ namespace wasm {
DefinedFunction *WasmSym::callCtors;
DefinedFunction *WasmSym::callDtors;
DefinedFunction *WasmSym::initMemory;
-DefinedFunction *WasmSym::applyDataRelocs;
DefinedFunction *WasmSym::applyGlobalRelocs;
DefinedFunction *WasmSym::applyTLSRelocs;
DefinedFunction *WasmSym::applyGlobalTLSRelocs;
diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h
index 2ba575fddc8796..5ce3ecbc4ab194 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -591,18 +591,14 @@ struct WasmSym {
// Function that calls the libc/etc. cleanup function.
static DefinedFunction *callDtors;
- // __wasm_apply_data_relocs
- // Function that applies relocations to data segment post-instantiation.
- static DefinedFunction *applyDataRelocs;
-
// __wasm_apply_global_relocs
// Function that applies relocations to wasm globals post-instantiation.
// Unlike __wasm_apply_data_relocs this needs to run on every thread.
static DefinedFunction *applyGlobalRelocs;
// __wasm_apply_tls_relocs
- // Like applyDataRelocs but for TLS section. These must be delayed until
- // __wasm_init_tls.
+ // Like __wasm_apply_data_relocs but for TLS section. These must be
+ // delayed until __wasm_init_tls.
static DefinedFunction *applyTLSRelocs;
// __wasm_apply_global_tls_relocs
diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index 681f6a137ceac4..70daf8e6502874 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -1145,6 +1145,8 @@ void Writer::createSyntheticInitFunctions() {
static WasmSignature nullSignature = {{}, {}};
+ createApplyDataRelocationsFunction();
+
// Passive segments are used to avoid memory being reinitialized on each
// thread's instantiation. These passive segments are initialized and
// dropped in __wasm_init_memory, which is registered as the start function
@@ -1467,15 +1469,29 @@ void Writer::createApplyDataRelocationsFunction() {
{
raw_string_ostream os(bodyContent);
writeUleb128(os, 0, "num locals");
+ uint64_t off = os.tell();
for (const OutputSegment *seg : segments)
if (!config->sharedMemory || !seg->isTLS())
for (const InputChunk *inSeg : seg->inputSegments)
inSeg->generateRelocationCode(os);
+ if (off == os.tell()) {
+ LLVM_DEBUG(dbgs() << "skipping empty __wasm_apply_data_relocs\n");
+ return;
+ }
writeU8(os, WASM_OPCODE_END, "END");
}
- createFunction(WasmSym::applyDataRelocs, bodyContent);
+ // __wasm_apply_data_relocs
+ // Function that applies relocations to data segment post-instantiation.
+ static WasmSignature nullSignature = {{}, {}};
+ auto def = symtab->addSyntheticFunction(
+ "__wasm_apply_data_relocs",
+ WASM_SYMBOL_VISIBILITY_DEFAULT | WASM_SYMBOL_EXPORTED,
+ make<SyntheticFunction>(nullSignature, "__wasm_apply_data_relocs"));
+ def->markLive();
+
+ createFunction(def, bodyContent);
}
void Writer::createApplyTLSRelocationsFunction() {
@@ -1771,8 +1787,6 @@ void Writer::run() {
if (!config->relocatable) {
// Create linker synthesized functions
- if (WasmSym::applyDataRelocs)
- createApplyDataRelocationsFunction();
if (WasmSym::applyGlobalRelocs)
createApplyGlobalRelocationsFunction();
if (WasmSym::applyTLSRelocs)
|
|
@llvm/pr-subscribers-lld Author: YAMAMOTO Takashi (yamt) Changes… phase Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations:
Full diff: https://github.com/llvm/llvm-project/pull/109249.diff 10 Files Affected:
diff --git a/lld/test/wasm/data-segments.ll b/lld/test/wasm/data-segments.ll
index 9354e6c8e4d2b0..670ac3c1f373fa 100644
--- a/lld/test/wasm/data-segments.ll
+++ b/lld/test/wasm/data-segments.ll
@@ -113,7 +113,7 @@
; PASSIVE-NEXT: Name: __wasm_init_memory
; PASSIVE-PIC: - Type: START
-; PASSIVE-PIC-NEXT: StartFunction: 3
+; PASSIVE-PIC-NEXT: StartFunction: 2
; PASSIVE-PIC-NEXT: - Type: DATACOUNT
; PASSIVE-PIC-NEXT: Count: 3
; PASSIVE-PIC-NEXT: - Type: CODE
@@ -125,9 +125,6 @@
; PASSIVE-PIC-NEXT: Locals: []
; PASSIVE-PIC-NEXT: Body: {{.*}}
; PASSIVE-PIC-NEXT: - Index: 2
-; PASSIVE-PIC-NEXT: Locals: []
-; PASSIVE-PIC-NEXT: Body: 0B
-; PASSIVE-PIC-NEXT: - Index: 3
; PASSIVE-PIC-NEXT: Locals:
; PASSIVE32-PIC-NEXT: - Type: I32
; PASSIVE64-PIC-NEXT: - Type: I64
@@ -152,8 +149,6 @@
; PASSIVE-PIC-NEXT: - Index: 1
; PASSIVE-PIC-NEXT: Name: __wasm_init_tls
; PASSIVE-PIC-NEXT: - Index: 2
-; PASSIVE-PIC-NEXT: Name: __wasm_apply_data_relocs
-; PASSIVE-PIC-NEXT: - Index: 3
; PASSIVE-PIC-NEXT: Name: __wasm_init_memory
; no data relocations.
@@ -161,8 +156,6 @@
; DIS-EMPTY:
; DIS-NEXT: end
-; In PIC mode __wasm_apply_data_relocs is export separatly to __wasm_call_ctors
-; PIC-DIS: <__wasm_apply_data_relocs>:
; PIC-DIS-EMPTY:
; DIS-LABEL: <__wasm_init_memory>:
diff --git a/lld/test/wasm/shared-weak-symbols.s b/lld/test/wasm/shared-weak-symbols.s
index 90de006353b3d3..df049ce4600fe4 100644
--- a/lld/test/wasm/shared-weak-symbols.s
+++ b/lld/test/wasm/shared-weak-symbols.s
@@ -30,7 +30,7 @@ call_weak:
# ASM: 10 80 80 80 80 00 call 0
drop
call hidden_weak_func
-# ASM: 10 84 80 80 80 00 call 4
+# ASM: 10 83 80 80 80 00 call 3
end_function
# ASM-NEXT: 0b end
@@ -62,15 +62,12 @@ call_weak:
# CHECK-NEXT: - Name: __wasm_call_ctors
# CHECK-NEXT: Kind: FUNCTION
# CHECK-NEXT: Index: 1
-# CHECK-NEXT: - Name: __wasm_apply_data_relocs
-# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 2
# CHECK-NEXT: - Name: weak_func
# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 3
+# CHECK-NEXT: Index: 2
# CHECK-NEXT: - Name: call_weak
# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 5
+# CHECK-NEXT: Index: 4
# CHECK-NEXT: - Type: CODE
# CHECK: - Type: CUSTOM
@@ -81,10 +78,8 @@ call_weak:
# CHECK-NEXT: - Index: 1
# CHECK-NEXT: Name: __wasm_call_ctors
# CHECK-NEXT: - Index: 2
-# CHECK-NEXT: Name: __wasm_apply_data_relocs
-# CHECK-NEXT: - Index: 3
# CHECK-NEXT: Name: weak_func
-# CHECK-NEXT: - Index: 4
+# CHECK-NEXT: - Index: 3
# CHECK-NEXT: Name: hidden_weak_func
-# CHECK-NEXT: - Index: 5
+# CHECK-NEXT: - Index: 4
# CHECK-NEXT: Name: call_weak
diff --git a/lld/test/wasm/tls-export.s b/lld/test/wasm/tls-export.s
index 1f64be607abb23..619f9d2df312ab 100644
--- a/lld/test/wasm/tls-export.s
+++ b/lld/test/wasm/tls-export.s
@@ -40,9 +40,6 @@ tls1:
# CHECK-NEXT: - Name: __wasm_call_ctors
# CHECK-NEXT: Kind: FUNCTION
# CHECK-NEXT: Index: 0
-# CHECK-NEXT: - Name: __wasm_apply_data_relocs
-# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 1
# CHECK-NEXT: - Name: tls1
# CHECK-NEXT: Kind: GLOBAL
# CHECK-NEXT: Index: 2
diff --git a/lld/test/wasm/tls-non-shared-memory.s b/lld/test/wasm/tls-non-shared-memory.s
index a2e2257cc9392e..1754fd6254bb80 100644
--- a/lld/test/wasm/tls-non-shared-memory.s
+++ b/lld/test/wasm/tls-non-shared-memory.s
@@ -127,9 +127,6 @@ tls1:
# PIE-NEXT: - Name: memory
# PIE-NEXT: Kind: MEMORY
# PIE-NEXT: Index: 0
-# PIE-NEXT: - Name: __wasm_apply_data_relocs
-# PIE-NEXT: Kind: FUNCTION
-# PIE-NEXT: Index: 1
# PIE-NEXT: - Type:
# .tdata and .data are combined into single segment in PIC mode.
diff --git a/lld/test/wasm/tls-relocations.s b/lld/test/wasm/tls-relocations.s
index ebe83227631f47..7260d72535a009 100644
--- a/lld/test/wasm/tls-relocations.s
+++ b/lld/test/wasm/tls-relocations.s
@@ -66,7 +66,7 @@ tls_sym:
# ASM-NEXT: i32.const 16
# ASM-NEXT: memory.init 0, 0
# call to __wasm_apply_tls_relocs
-# ASM-NEXT: call 4
+# ASM-NEXT: call 3
# ASM-NEXT: end
# ASM: <__wasm_apply_tls_relocs>:
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 2de7dcaeb43d47..289c1217ff5ead 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -917,17 +917,6 @@ static void createSyntheticSymbols() {
is64 ? i64ArgSignature : i32ArgSignature,
"__wasm_init_tls"));
}
-
- if (ctx.isPic ||
- config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) {
- // For PIC code, or when dynamically importing addresses, we create
- // synthetic functions that apply relocations. These get called from
- // __wasm_call_ctors before the user-level constructors.
- WasmSym::applyDataRelocs = symtab->addSyntheticFunction(
- "__wasm_apply_data_relocs",
- WASM_SYMBOL_VISIBILITY_DEFAULT | WASM_SYMBOL_EXPORTED,
- make<SyntheticFunction>(nullSignature, "__wasm_apply_data_relocs"));
- }
}
static void createOptionalSymbols() {
diff --git a/lld/wasm/InputChunks.cpp b/lld/wasm/InputChunks.cpp
index 975225974aff6e..f350d4d1c04d6a 100644
--- a/lld/wasm/InputChunks.cpp
+++ b/lld/wasm/InputChunks.cpp
@@ -378,7 +378,7 @@ void InputChunk::generateRelocationCode(raw_ostream &os) const {
uint64_t offset = getVA(rel.Offset) - getInputSectionOffset();
Symbol *sym = file->getSymbol(rel);
- if (!ctx.isPic && sym->isDefined())
+ if (!ctx.isPic && !sym->hasGOTIndex())
continue;
LLVM_DEBUG(dbgs() << "gen reloc: type=" << relocTypeToString(rel.Type)
diff --git a/lld/wasm/Symbols.cpp b/lld/wasm/Symbols.cpp
index f74699d0763fd9..b2bbd11c53ef23 100644
--- a/lld/wasm/Symbols.cpp
+++ b/lld/wasm/Symbols.cpp
@@ -80,7 +80,6 @@ namespace wasm {
DefinedFunction *WasmSym::callCtors;
DefinedFunction *WasmSym::callDtors;
DefinedFunction *WasmSym::initMemory;
-DefinedFunction *WasmSym::applyDataRelocs;
DefinedFunction *WasmSym::applyGlobalRelocs;
DefinedFunction *WasmSym::applyTLSRelocs;
DefinedFunction *WasmSym::applyGlobalTLSRelocs;
diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h
index 2ba575fddc8796..5ce3ecbc4ab194 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -591,18 +591,14 @@ struct WasmSym {
// Function that calls the libc/etc. cleanup function.
static DefinedFunction *callDtors;
- // __wasm_apply_data_relocs
- // Function that applies relocations to data segment post-instantiation.
- static DefinedFunction *applyDataRelocs;
-
// __wasm_apply_global_relocs
// Function that applies relocations to wasm globals post-instantiation.
// Unlike __wasm_apply_data_relocs this needs to run on every thread.
static DefinedFunction *applyGlobalRelocs;
// __wasm_apply_tls_relocs
- // Like applyDataRelocs but for TLS section. These must be delayed until
- // __wasm_init_tls.
+ // Like __wasm_apply_data_relocs but for TLS section. These must be
+ // delayed until __wasm_init_tls.
static DefinedFunction *applyTLSRelocs;
// __wasm_apply_global_tls_relocs
diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index 681f6a137ceac4..70daf8e6502874 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -1145,6 +1145,8 @@ void Writer::createSyntheticInitFunctions() {
static WasmSignature nullSignature = {{}, {}};
+ createApplyDataRelocationsFunction();
+
// Passive segments are used to avoid memory being reinitialized on each
// thread's instantiation. These passive segments are initialized and
// dropped in __wasm_init_memory, which is registered as the start function
@@ -1467,15 +1469,29 @@ void Writer::createApplyDataRelocationsFunction() {
{
raw_string_ostream os(bodyContent);
writeUleb128(os, 0, "num locals");
+ uint64_t off = os.tell();
for (const OutputSegment *seg : segments)
if (!config->sharedMemory || !seg->isTLS())
for (const InputChunk *inSeg : seg->inputSegments)
inSeg->generateRelocationCode(os);
+ if (off == os.tell()) {
+ LLVM_DEBUG(dbgs() << "skipping empty __wasm_apply_data_relocs\n");
+ return;
+ }
writeU8(os, WASM_OPCODE_END, "END");
}
- createFunction(WasmSym::applyDataRelocs, bodyContent);
+ // __wasm_apply_data_relocs
+ // Function that applies relocations to data segment post-instantiation.
+ static WasmSignature nullSignature = {{}, {}};
+ auto def = symtab->addSyntheticFunction(
+ "__wasm_apply_data_relocs",
+ WASM_SYMBOL_VISIBILITY_DEFAULT | WASM_SYMBOL_EXPORTED,
+ make<SyntheticFunction>(nullSignature, "__wasm_apply_data_relocs"));
+ def->markLive();
+
+ createFunction(def, bodyContent);
}
void Writer::createApplyTLSRelocationsFunction() {
@@ -1771,8 +1787,6 @@ void Writer::run() {
if (!config->relocatable) {
// Create linker synthesized functions
- if (WasmSym::applyDataRelocs)
- createApplyDataRelocationsFunction();
if (WasmSym::applyGlobalRelocs)
createApplyGlobalRelocationsFunction();
if (WasmSym::applyTLSRelocs)
|
sbc100
approved these changes
Sep 19, 2024
lld/wasm/InputChunks.cpp
Outdated
| @@ -378,7 +378,7 @@ void InputChunk::generateRelocationCode(raw_ostream &os) const { | |||
| uint64_t offset = getVA(rel.Offset) - getInputSectionOffset(); | |||
|
|
|||
| Symbol *sym = file->getSymbol(rel); | |||
| if (!ctx.isPic && sym->isDefined()) | |||
| if (!ctx.isPic && !sym->hasGOTIndex()) | |||
There was a problem hiding this comment.
Is this change needed as part of this change? I think this line could probably use a comment. i.e. Why are we skipping these relocations?
There was a problem hiding this comment.
Should this change be differed to #108146 since it seems like a separate semantic change perhaps?
There was a problem hiding this comment.
this change is necessary because this function is now called regardless of unresolved policy.
There was a problem hiding this comment.
I'm finding this clauses a little hard to grok without a lot of introspection.
Maybe we could add a comment here? Something like:
// Runtime relocations are needed when we don't know the address of a symbol statically.
Or better still, we could add a variable to make it more clear what is going on:
bool requiresRuntimeReloc = ctx.isPic || sym->hasGOTIndex();
if (!requiresRuntimeReloc)
continue;
There was a problem hiding this comment.
I'm finding this clauses a little hard to grok without a lot of introspection.
i agree!
Maybe we could add a comment here? Something like:
// Runtime relocations are needed when we don't know the address of a symbol statically.Or better still, we could add a variable to make it more clear what is going on:
bool requiresRuntimeReloc = ctx.isPic || sym->hasGOTIndex(); if (!requiresRuntimeReloc) continue;
applied. thank you.
lld/wasm/Writer.cpp
Outdated
| @@ -1467,15 +1469,29 @@ void Writer::createApplyDataRelocationsFunction() { | |||
| { | |||
| raw_string_ostream os(bodyContent); | |||
| writeUleb128(os, 0, "num locals"); | |||
| uint64_t off = os.tell(); | |||
| for (const OutputSegment *seg : segments) | |||
| if (!config->sharedMemory || !seg->isTLS()) | |||
| for (const InputChunk *inSeg : seg->inputSegments) | |||
| inSeg->generateRelocationCode(os); | |||
There was a problem hiding this comment.
Would it be cleaner to return a boolean from generateRelocationCode?
i.e.
wroteRelocations |= inSeg->generateRelocationCode(os);
...
if (!wroteRelocations) {
return;
}
Not a big deal but it seems more explictly/obvious.
|
How about making the title |
|
I'm still not clear about the second motivation. i.e. I'm not clear on why this is needed for the followup change. But I'll see if I can figure it out. |
…ction Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations: * This possibly reduces the module size * This is also a preparation to fix llvm#107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)
yamt
force-pushed
the
__wasm_apply_data_relocs
branch
from
346f8de
to
50fd3a8
Compare
done |
yamt
changed the title
from sbc100 llvm#109249 (comment)
sbc100
changed the title
sbc100
approved these changes
Sep 20, 2024
|
can this land? |
yamt
added a commit
to yamt/llvm-project
that referenced
this pull request
Oct 1, 2024
The commit 22b7b84 made the symbols provided by shared libraries "defined", and thus effectively made it impossible to generate non-pie dynamically linked executables using --unresolved-symbols=import-dynamic. This commit, based on llvm#109249, fixes it by checking sym->isShared() explictly. (as a bonus, you don't need to rely on --unresolved-symbols=import-dynamic anymore.) Fixes llvm#107387
yamt
added a commit
to yamt/llvm-project
that referenced
this pull request
Oct 1, 2024
The commit 22b7b84 made the symbols provided by shared libraries "defined", and thus effectively made it impossible to generate non-pie dynamically linked executables using --unresolved-symbols=import-dynamic. This commit, based on llvm#109249, fixes it by checking sym->isShared() explictly. (as a bonus, you don't need to rely on --unresolved-symbols=import-dynamic anymore.) Fixes llvm#107387
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Oct 2, 2024
…tion (llvm#109249) Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations: * This possibly reduces the module size * This is also a preparation to fix llvm#107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Oct 2, 2024
…tion (llvm#109249) Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations: * This possibly reduces the module size * This is also a preparation to fix llvm#107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Oct 3, 2024
…tion (llvm#109249) Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations: * This possibly reduces the module size * This is also a preparation to fix llvm#107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)
xgupta
pushed a commit
to xgupta/llvm-project
that referenced
this pull request
Oct 4, 2024
…tion (llvm#109249) Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary. Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools) Motivations: * This possibly reduces the module size * This is also a preparation to fix llvm#107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)
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.
Instead of always generating __wasm_apply_data_relocs when relevant options like -pie and -shared are specified, generate it only when the relevant relocations are actually necessary.
Note: omitting empty __wasm_apply_data_relocs is not a problem because the export is optional in the spec (DynamicLinking.md) and all runtime linker implementations I'm aware of implement it that way. (emscripten, toywasm, wasm-tools)
Motivations:
This possibly reduces the module size
This is also a preparation to fix [WebAssembly] non-pie dynamic-linking executable seems broken (LLVM 19 regression) #107387, for which it isn't obvious if we need these relocations at the time of createSyntheticSymbols. (unless we introduce a new explicit option like --non-pie-dynamic-link.)