[WebAssembly] Default export tables with --cooperative-threading - #208263
Conversation
|
@llvm/pr-subscribers-lld Author: Alex Crichton (alexcrichton) ChangesThis commit is a change to The other motivation behind this change is that it'll avoid the need to manually specify cc WebAssembly/wasi-libc#808 Full diff: https://github.com/llvm/llvm-project/pull/208263.diff 2 Files Affected:
diff --git a/lld/test/wasm/cooperative-threading-table.s b/lld/test/wasm/cooperative-threading-table.s
new file mode 100644
index 0000000000000..43839984d2ca5
--- /dev/null
+++ b/lld/test/wasm/cooperative-threading-table.s
@@ -0,0 +1,37 @@
+# Test that --cooperative-threading exports the function table by default.
+
+# RUN: llvm-mc -mattr=+call-indirect-overlong -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o
+
+# RUN: wasm-ld --cooperative-threading -o %t.wasm %t.o
+# RUN: obj2yaml %t.wasm | FileCheck %s --check-prefix=EXPORT
+
+# EXPORT: - Type: TABLE
+# EXPORT-NEXT: Tables:
+# EXPORT-NEXT: - Index: 0
+# EXPORT-NEXT: ElemType: FUNCREF
+
+# EXPORT: - Type: EXPORT
+# EXPORT: - Name: __indirect_function_table
+# EXPORT-NEXT: Kind: TABLE
+# EXPORT-NEXT: Index: 0
+
+# When the table is imported instead there is no need to also export it.
+
+# RUN: wasm-ld --cooperative-threading --import-table -o %t2.wasm %t.o
+# RUN: obj2yaml %t2.wasm | FileCheck %s --check-prefix=IMPORT
+
+# IMPORT: - Type: IMPORT
+# IMPORT: - Module: env
+# IMPORT-NEXT: Field: __indirect_function_table
+# IMPORT-NEXT: Kind: TABLE
+# IMPORT-NOT: Kind: TABLE
+
+.globl __indirect_function_table
+.tabletype __indirect_function_table, funcref
+
+.globl _start
+_start:
+ .functype _start () -> ()
+ i32.const 1
+ call_indirect __indirect_function_table, () -> ()
+ end_function
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 9a2e3a82a9279..c213d7ca0b0f3 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -759,6 +759,14 @@ static void setConfigs() {
if (ctx.arg.sharedMemory)
error("--cooperative-threading is incompatible with --shared-memory");
ctx.arg.libcallThreadContext = true;
+
+ // Cooperative threading requires the table is either imported or exported
+ // or otherwise there's no way for embedders to read spawned functions from
+ // the table. If we've gotten this far and the table isn't otherwise
+ // imported (e.g in `isPic` mode) then export the table instead to ensure
+ // that it's visible to the outside world.
+ if (!ctx.arg.importTable)
+ ctx.arg.exportTable = true;
}
}
|
|
@llvm/pr-subscribers-lld-wasm Author: Alex Crichton (alexcrichton) ChangesThis commit is a change to The other motivation behind this change is that it'll avoid the need to manually specify cc WebAssembly/wasi-libc#808 Full diff: https://github.com/llvm/llvm-project/pull/208263.diff 2 Files Affected:
diff --git a/lld/test/wasm/cooperative-threading-table.s b/lld/test/wasm/cooperative-threading-table.s
new file mode 100644
index 0000000000000..43839984d2ca5
--- /dev/null
+++ b/lld/test/wasm/cooperative-threading-table.s
@@ -0,0 +1,37 @@
+# Test that --cooperative-threading exports the function table by default.
+
+# RUN: llvm-mc -mattr=+call-indirect-overlong -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o
+
+# RUN: wasm-ld --cooperative-threading -o %t.wasm %t.o
+# RUN: obj2yaml %t.wasm | FileCheck %s --check-prefix=EXPORT
+
+# EXPORT: - Type: TABLE
+# EXPORT-NEXT: Tables:
+# EXPORT-NEXT: - Index: 0
+# EXPORT-NEXT: ElemType: FUNCREF
+
+# EXPORT: - Type: EXPORT
+# EXPORT: - Name: __indirect_function_table
+# EXPORT-NEXT: Kind: TABLE
+# EXPORT-NEXT: Index: 0
+
+# When the table is imported instead there is no need to also export it.
+
+# RUN: wasm-ld --cooperative-threading --import-table -o %t2.wasm %t.o
+# RUN: obj2yaml %t2.wasm | FileCheck %s --check-prefix=IMPORT
+
+# IMPORT: - Type: IMPORT
+# IMPORT: - Module: env
+# IMPORT-NEXT: Field: __indirect_function_table
+# IMPORT-NEXT: Kind: TABLE
+# IMPORT-NOT: Kind: TABLE
+
+.globl __indirect_function_table
+.tabletype __indirect_function_table, funcref
+
+.globl _start
+_start:
+ .functype _start () -> ()
+ i32.const 1
+ call_indirect __indirect_function_table, () -> ()
+ end_function
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 9a2e3a82a9279..c213d7ca0b0f3 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -759,6 +759,14 @@ static void setConfigs() {
if (ctx.arg.sharedMemory)
error("--cooperative-threading is incompatible with --shared-memory");
ctx.arg.libcallThreadContext = true;
+
+ // Cooperative threading requires the table is either imported or exported
+ // or otherwise there's no way for embedders to read spawned functions from
+ // the table. If we've gotten this far and the table isn't otherwise
+ // imported (e.g in `isPic` mode) then export the table instead to ensure
+ // that it's visible to the outside world.
+ if (!ctx.arg.importTable)
+ ctx.arg.exportTable = true;
}
}
|
af99609 to
57523ce
Compare
This commit is a change to `wasm-ld`'s behavior when the `--cooperative-threading` flag is passed to the linker. The change here is to by default work as if `--export-table` was passed as well. This is required conventionally on this target because the table is where function pointers are read from in the component model `thread.new-indirect` intrinsic. If the table is not exported then there's no way to turn the core module into a component so it's effectively required. This behavior only applies to when the table isn't otherwise imported, for example in shared libraries. The other motivation behind this change is that it'll avoid the need to manually specify `-Wl,--export-table` when compiling for the `wasm32-wasip3` target. This additionally avoids the need for the Clang driver to figure out if flags like `--import-table` were otherwise passed. Basically it seemed best to put this in `wasm-ld` itself to avoid as little juggling of pieces as necessary. cc WebAssembly/wasi-libc#808
57523ce to
8afd860
Compare
| // imported (e.g in `isPic` mode) then export the table instead to ensure | ||
| // that it's visible to the outside world. | ||
| if (!ctx.arg.importTable) | ||
| ctx.arg.exportTable = true; |
There was a problem hiding this comment.
I wonder if it would make more sense to do this in the clang driver? Or in the wask-sdk clang driver config file?
Also, couldn't you imagine a scheme which did not involve exporting the table but instead exporting a spawn_fibre function that take a table index itself? i.e. is it really true that the embedder always need to read from the table or can just use function pointers in some cases?
There was a problem hiding this comment.
I thought about putting in the driver, yeah, but I feel that here in wasm-ld is the best place. One consequence of a driver is that --export-table as a flag is incompatible with -shared, --import-table, and --relocatable. For all of these flags the driver would have to figure out what's going on and whether --export-table otherwise needs to be passed. Given that I don't think the *.cfg file will work, and while the driver in theory could work it'd also be something I'd have to replicate in rustc, too. Given all that it's how I figured that here in wasm-ld was probably the best place to put this since it's got the most context.
For a different scheme, that's theoretically possible, yeah, but the current definition of the threading intrinsics is that the new-thread intrinsic takes a table + index and spawns the thread based on that. This is intended to be somewhat future-compatible with a world where an intrinsic takes a funcref directly, but that's not possible to call from C right now hence the interim table+index intrinsic.
There was a problem hiding this comment.
Hmm, yes I the driver would need more smarts in that case to avoid adding --export-table when it shouldn't.
I'm not convinced the linker should be charge or implementing the specifics of the wasi scheme like this, but maybe its the only good option in this case.
On the other hand is it too late to reconsider how fibers are launched in the pre-funcref world. Wouldn't calling start_fibre_indirect be better some ways (since it would avoid exposing the entire table to host)?
There was a problem hiding this comment.
Coop threading hasn't shipped yet so it's still possible to change details like this, but personally I'd say that this is the next-best design before a true funcref-like design. I'd prefer, for example, to call an imported function with a funcref parameter ("go run this in a thread") as the desired end state which would also keep the table entirely encapsulated.
If you'd like though I can file an issue on WebAssembly/component-model and cc you on that for discussion, too.
|
@sbc100 did you have other thoughts on this PR? We'd ideally like to get this PR (and #208332 + #208597) landed in time for the LLVM 23 release to ensure WASIp3 is ready for LLVM 23 with wasi-sdk/Rust/etc. I think we'll probably need to backport now that the release branch has been created as well, and would you be ok helping us out with that? |
…208263) This commit is a change to `wasm-ld`'s behavior when the `--cooperative-threading` flag is passed to the linker. The change here is to by default work as if `--export-table` was passed as well. This is required conventionally on this target because the table is where function pointers are read from in the component model `thread.new-indirect` intrinsic. If the table is not exported then there's no way to turn the core module into a component so it's effectively required. This behavior only applies to when the table isn't otherwise imported, for example in shared libraries. The other motivation behind this change is that it'll avoid the need to manually specify `-Wl,--export-table` when compiling for the `wasm32-wasip3` target. This additionally avoids the need for the Clang driver to figure out if flags like `--import-table` were otherwise passed. Basically it seemed best to put this in `wasm-ld` itself to avoid as little juggling of pieces as necessary. cc WebAssembly/wasi-libc#808
2 participants
This commit is a change to
wasm-ld's behavior when the--cooperative-threadingflag is passed to the linker. The change here is to by default work as if--export-tablewas passed as well. This is required conventionally on this target because the table is where function pointers are read from in the component modelthread.new-indirectintrinsic. If the table is not exported then there's no way to turn the core module into a component so it's effectively required. This behavior only applies to when the table isn't otherwise imported, for example in shared libraries.The other motivation behind this change is that it'll avoid the need to manually specify
-Wl,--export-tablewhen compiling for thewasm32-wasip3target. This additionally avoids the need for the Clang driver to figure out if flags like--import-tablewere otherwise passed. Basically it seemed best to put this inwasm-lditself to avoid as little juggling of pieces as necessary.cc WebAssembly/wasi-libc#808