[lld][WebAssembly] Fix non-pie dynamic-linking executable #108146

yamt · 2024-09-11T06:13:29Z

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 #109249,
fixes it by checking sym->isShared() explictly.
(as a bonus, you don't need to rely on --unresolved-symbols=import-dynamic
anymore.)

Fixes #107387

llvmbot · 2024-09-11T06:13:59Z

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-wasm

Author: YAMAMOTO Takashi (yamt)

Changes

Fixes #107387

Full diff: https://github.com/llvm/llvm-project/pull/108146.diff

3 Files Affected:

(modified) lld/wasm/Driver.cpp (+2-1)
(modified) lld/wasm/InputChunks.cpp (+1-1)
(modified) lld/wasm/Relocations.cpp (+2-1)

diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index cb8fe2534f5fe7..79cacc63922ee5 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -913,7 +913,8 @@ static void createSyntheticSymbols() {
   }
 
   if (ctx.isPic ||
-      config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) {
+      config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic ||
+      !config->isStatic) {
     // 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.
diff --git a/lld/wasm/InputChunks.cpp b/lld/wasm/InputChunks.cpp
index 975225974aff6e..dbf800422e45b5 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->isDefined() && !sym->hasGOTIndex())
       continue;
 
     LLVM_DEBUG(dbgs() << "gen reloc: type=" << relocTypeToString(rel.Type)
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index 6f33a4f28a9d09..70229179afdcfc 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -146,7 +146,8 @@ void scanRelocations(InputChunk *chunk) {
 
     if (ctx.isPic ||
         (sym->isUndefined() &&
-         config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic)) {
+         config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) ||
+        sym->isShared()) {
       switch (reloc.Type) {
       case R_WASM_TABLE_INDEX_SLEB:
       case R_WASM_TABLE_INDEX_SLEB64:

sbc100 · 2024-09-11T17:27:27Z

lld/wasm/Driver.cpp

@@ -913,7 +913,8 @@ static void createSyntheticSymbols() {
  }

  if (ctx.isPic ||
-      config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) {
+      config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic ||
+      !config->isStatic) {


I wonder if can replace ctx.isPic with !config->isStatic above? Does it make sense to allow -shared + -static? Or -pie + -static?

I wonder if we need a better way to detect if dynamic linking used at all since you can imagine a case where isStatic is true, but we do have dynamic linking. For example:

$ wasm-ld foo.o -Bdynamic -ldylib -Bstatic -lstaticlib

In that case since -Bstatic comes last on the command line the config->isStatic will be false even though we include libdylib.so dyanmically.

Another example:

wasm-ld foo.o libdylib.so libstaticlib.a

Here config->isStatic will remain false even though dynamic linking is used.

Actually it looks like that the latter case should reject the .so file. I see the ELF linker does this:

llvm-project/lld/ELF/Driver.cpp

Lines 341 to 344 in 96b7c64

if (config->isStatic) {

error("attempted static link of dynamic object " + path);

return;

}

. I'll upload a PR for that.

I wonder if can replace ctx.isPic with !config->isStatic above? Does it make sense to allow -shared + -static? Or -pie + -static?

I wonder if we need a better way to detect if dynamic linking used at all since you can imagine a case where isStatic is true, but we do have dynamic linking. For example:

$ wasm-ld foo.o -Bdynamic -ldylib -Bstatic -lstaticlib

In that case since -Bstatic comes last on the command line the config->isStatic will be false even though we include libdylib.so dyanmically.

i guess it's simpler to defer the decision to createApplyDataRelocationsFunction.
how do you think?

i guess it's simpler to defer the decision to createApplyDataRelocationsFunction.

i pushed a commit to implement it.

@sbc100 do you have any more concerns?

sbc100 · 2024-09-18T14:53:47Z

lld/wasm/Writer.cpp

+      make<SyntheticFunction>(nullSignature, "__wasm_apply_data_relocs"));
+  def->markLive();
+
+  createFunction(def, bodyContent);


This change seems to incorporate a couple of different things now. Would you mind separating out the change to "Only create __wasm_apply_data_relocs conditionally". That seems like it could land separately and is somewhat unrelated to fix at hand?

Also, regarding the conditional creation of this symbol, what happens if somebody does --export=__wasm_apply_data_relocs or otherwise references this symbol, but we "skip" creating it?

This change seems to incorporate a couple of different things now. Would you mind separating out the change to "Only create __wasm_apply_data_relocs conditionally". That seems like it could land separately and is somewhat unrelated to fix at hand?

#109249

Also, regarding the conditional creation of this symbol, what happens if somebody does --export=__wasm_apply_data_relocs or otherwise references this symbol, but we "skip" creating it?

is it expected to work at all? do you have real use cases? or is it just a theoretical concern?
i thought __wasm_apply_data_relocs was just for dynamic-linking.

sbc100 · 2024-09-18T14:56:16Z

Could you update the PR description describing what was broken and how this fixes the issue?

Also, can we add a new test or update one of the existing ones to make sure we don't regress again?

yamt · 2024-09-19T07:49:17Z

Could you update the PR description describing what was broken and how this fixes the issue?

Also, can we add a new test or update one of the existing ones to make sure we don't regress again?

i will update if/when #109249 is merged.
i will mark this PR a draft for now.

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 · 2024-10-01T05:20:40Z

updated after [lld][WebAssembly] Avoid emitting empty __wasm_apply_data_relocs function #109249
added a test

sbc100 · 2024-10-01T05:57:38Z

lld/test/wasm/Inputs/lib.s

+	.type	f,@function
+f:
+	.functype	f () -> ()
+	end_function


Rather than creating a new file here you can just pick an existing file such as ret32.s and compile that

ok. i will try.

sbc100 · 2024-10-01T05:59:46Z

lld/wasm/Relocations.cpp

@@ -146,7 +146,8 @@ void scanRelocations(InputChunk *chunk) {

    if (ctx.isPic ||
        (sym->isUndefined() &&
-         config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic)) {
+         config->unresolvedSymbols == UnresolvedPolicy::ImportDynamic) ||
+        sym->isShared()) {


Can you put this new clause on line 147 (e.g. if (ctx.isPic || sym->isShared() ||)

sbc100 · 2024-10-01T06:05:55Z

lld/test/wasm/dylink-non-pie.s

+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.lib.o %p/Inputs/lib.s
+# RUN: wasm-ld -m wasm32 --experimental-pic -shared --no-entry %t.lib.o -o %t.lib.so
+# RUN: llvm-mc -filetype=obj -mattr=+reference-types -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: wasm-ld -m wasm32 --features=mutable-globals -Bdynamic --export-table --growable-table --export-memory --export=__stack_pointer --export=__heap_base --export=__heap_end --entry=_start %t.o %t.lib.so -o %t.wasm


Can you remove any link flags that you don't need here? e.g. all of the --export flags and the --entry flag?

i wanted to mimic the real situation to link a non-pie executable.

sbc100 · 2024-10-01T06:07:08Z

lld/test/wasm/dylink-non-pie.s

+	i32.const	f_p@MBREL
+	i32.add
+	i32.load	0
+	call_indirect	 __indirect_function_table, () -> ()


You don't need the __indirect_function_table symbol to make this test work. You can just do i32.const f_p followed by drop I think.

let me think a bit. thank you.

no functional changes are intended

sbc100 · 2024-10-01T15:40:28Z

lld/test/wasm/dylink-non-pie.s

+
+# CHECK:   - Type:            EXPORT
+# CHECK:       - Name:            __wasm_apply_data_relocs
+# CHECK-NEXT:         Kind:            FUNCTION


Should we be checking for some more things here?

Perhaps we can check that GOT.ret32 is imported? Perhaps we can decompile __wasm_apply_data_relocs and verify that is using GOT.ret32 as expected?

do you know any existing tests doing similar things which i can use as an example?

Yes, there are a few tests that disassembly stuff. Looks for DIS-NEXT in lld/test/wasm.

ok. i will take a look.

sbc100 · 2024-10-01T15:43:26Z

lld/test/wasm/dylink-non-pie.s

+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.lib.o %p/Inputs/ret32.s
+# RUN: wasm-ld -m wasm32 --experimental-pic -shared --no-entry %t.lib.o -o %t.lib.so
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: wasm-ld -m wasm32 -Bdynamic %t.o %t.lib.so -o %t.wasm


Just to confirm, this effectively generates a non-PIC output? And ctx.isPic not set?

I wonder if we can confirm this this test somehow? Perhaps its enough that we successfully use i32.const f_p (which would not work if the output was PIC, right?) instead of i32.const f_p@MBREL

ctx.isPic is not set as neither of pie or shared is set.

the following is the generated output. it looks reasonable to me.

(module $dylink-non-pie.s.tmp.wasm (type (;0;) (func (param f32) (result i32))) (type (;1;) (func)) (import "env" "ret32" (func $ret32 (type 0))) (import "GOT.func" "ret32" (global $ret32 (mut i32))) (func $__wasm_apply_data_relocs (type 1) i32.const 1024 global.get $ret32 i32.store) (func $_start (type 1) i32.const 1024 drop) (table (;0;) 1 1 funcref) (memory (;0;) 2) (global $__stack_pointer (mut i32) (i32.const 66576)) (export "memory" (memory 0)) (export "_start" (func $_start)) (export "__wasm_apply_data_relocs" (func $__wasm_apply_data_relocs)) (data $.data (i32.const 1024) "\00\00\00\00"))

I wonder if we can confirm this this test somehow? Perhaps its enough that we successfully use i32.const f_p (which would not work if the output was PIC, right?) instead of i32.const f_p@MBREL

maybe we can confirm it doesn't have __memory_base/__table_base imports?

…ta_relocs

yamt · 2024-10-10T03:41:52Z

@sbc100 do you have any more concerns?

sbc100 · 2024-10-31T13:44:17Z

lld/test/wasm/dylink-non-pie.s

+	.functype	ret32 (f32) -> (i32)
+	.functype	_start () -> ()
+	.globl	_start
+	.type	_start,@function


this line is not needed I think

sbc100 · 2024-10-31T13:44:34Z

lld/test/wasm/dylink-non-pie.s

+# RUN: llvm-objdump -d --no-show-raw-insn --no-leading-addr %t.wasm | FileCheck %s --check-prefixes DIS
+
+	.functype	ret32 (f32) -> (i32)
+	.functype	_start () -> ()


This line is not needed since its already on line 13

sbc100 · 2024-10-31T13:45:15Z

lld/test/wasm/dylink-non-pie.s

+	end_function
+
+	.section	.data.f_p,"",@
+	.globl	f_p


Does this need to be global symbol? i.e. can you remove this line?

it doesn't seem necessary. removed. thank you.

sbc100

lgtm % nits

yamt · 2024-11-28T08:17:19Z

can this land?

llvmbot added lld lld:wasm labels Sep 11, 2024

sbc100 reviewed Sep 11, 2024

View reviewed changes

yamt force-pushed the nonpie-dynamic-linking branch 2 times, most recently from 82dd674 to 9513512 Compare September 12, 2024 03:28

sbc100 reviewed Sep 18, 2024

View reviewed changes

yamt marked this pull request as draft September 19, 2024 07:49

sbc100 mentioned this pull request Sep 19, 2024

[lld][WebAssembly] Avoid emitting empty __wasm_apply_data_relocs function #109249

Merged

yamt force-pushed the nonpie-dynamic-linking branch from 3559e1f to 91f33a4 Compare October 1, 2024 05:18

yamt added 2 commits October 1, 2024 14:19

add a test

06482d7

yamt force-pushed the nonpie-dynamic-linking branch from 91f33a4 to 06482d7 Compare October 1, 2024 05:19

yamt changed the title ~~[lld][WebAssembly]: Restore non-pie dynamic-linking executable~~ [lld][WebAssembly] Fix non-pie dynamic-linking executable Oct 1, 2024

yamt marked this pull request as ready for review October 1, 2024 05:20

sbc100 reviewed Oct 1, 2024

View reviewed changes

yamt added 3 commits October 1, 2024 17:14

dylink-non-pie.s: use the existing file

fb9a6ab

tweak the order of conditions

9e234c1

no functional changes are intended

lld/test/wasm/dylink-non-pie.s: simplify

972b540

sbc100 reviewed Oct 1, 2024

View reviewed changes

yamt added 2 commits October 2, 2024 09:26

lld/test/wasm/dylink-non-pie.s: check the contents of __wasm_apply_da…

9c486a4

…ta_relocs

lld/test/wasm/dylink-non-pie.s: ensure it doesn't import __memory_base

35ecf24

yamt mentioned this pull request Oct 31, 2024

[WebAssembly] non-pie dynamic-linking executable seems broken (LLVM 19 regression) #107387

Closed

sbc100 reviewed Oct 31, 2024

View reviewed changes

sbc100 approved these changes Oct 31, 2024

View reviewed changes

lld/test/wasm/dylink-non-pie.s: remove a few unnecessary things

156e6f2

sbc100 merged commit 9df375e into llvm:main Jan 3, 2025
8 checks passed

	if (config->isStatic) {
	error("attempted static link of dynamic object " + path);
	return;
	}

[lld][WebAssembly] Fix non-pie dynamic-linking executable #108146

[lld][WebAssembly] Fix non-pie dynamic-linking executable #108146

Uh oh!

Conversation

yamt commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Sep 18, 2024

Uh oh!

yamt commented Sep 19, 2024

Uh oh!

yamt commented Oct 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt commented Sep 11, 2024 •

edited

Loading

llvmbot commented Sep 11, 2024 •

edited

Loading