-
Notifications
You must be signed in to change notification settings - Fork 17.2k
[WebAssembly] WASIP3 Library Call Thread Context Support #175800
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
base: main
Are you sure you want to change the base?
Changes from all commits
48a90dd
540d785
8f222c9
e9775de
6d2b464
0bf29ae
4a18333
927daeb
e632709
f44bf29
18a1ee3
8980e38
08a3a16
9092b93
81ffb2b
a9e7310
c369bb2
d368838
61f03e5
c9665c1
07e122d
651c362
0175bd5
9a93078
85fab66
e577185
446d56d
3f10fe8
dbae3ef
e0dcb2a
e8babc7
822ea98
cb2fb8d
6cf7c2a
fa7eea8
cc1ea2f
b9039eb
1b490cd
b55ae63
a1c563b
69654ef
b98b9ab
7121ea5
70b3b93
43f756f
4ffe623
36c39bf
d953510
0656bbf
c5190ff
6e0c9c0
3386a11
5775592
4d7a81e
1001e65
7a44741
4c62c81
5bebe2a
e9fa9fc
10ed134
2ee9845
c6ddead
2bae426
557bfac
61c25c8
0fe07f0
7a58fce
8047c76
24cb457
ff172dc
d11664a
f04b74a
6bdff2c
a637ae1
0bf18a6
6c9c276
f019ea9
de9424c
5954755
1f9e903
f61837a
bde41fc
daf5b03
36ea1df
41f8893
0a8c6ec
90c342f
a42942c
13d3870
f75cb64
e1992e8
c3d46dc
9ecceef
9d296ec
95fc53a
ae859d0
abc859d
3749d9e
0028170
f9f40b7
c8d5420
9908d72
0e1a5a9
e187d8e
a2e3a85
b7bc6a3
a79dc2f
09f0fe3
1ee14c4
af1eb70
c661e66
e00d35f
742ae7c
9b688ca
3570777
45ff0f0
e841994
da1949c
8dfc97c
cdca5aa
7559ec5
4135d3d
a1cacc9
a2367c8
dfbfcd9
5c04563
08080df
92880bd
4f658ae
a3a46eb
87190fe
ed65fda
086608e
f0651c3
a69b43d
7cf2aa7
6b13795
fb482d1
aa5c37f
b92a49c
a50c1e7
b467230
4562305
92c7b06
b2d8f85
5ebc91d
f177461
9a7eac9
7b0520d
880a277
c363a1a
a9c8aa0
d20cda1
bf8aef5
33a19dd
58f864b
4d25f80
ff62651
de0e782
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ class LLVM_LIBRARY_VISIBILITY WebAssemblyTargetInfo : public TargetInfo { | |
| bool HasExtendedConst = false; | ||
| bool HasFP16 = false; | ||
| bool HasGC = false; | ||
| bool HasLibcallThreadContext = false; | ||
| bool HasMultiMemory = false; | ||
| bool HasMultivalue = false; | ||
| bool HasMutableGlobals = false; | ||
|
|
@@ -110,6 +111,8 @@ class LLVM_LIBRARY_VISIBILITY WebAssemblyTargetInfo : public TargetInfo { | |
| PtrDiffType = SignedLong; | ||
| IntPtrType = SignedLong; | ||
| } | ||
| if (T.getOS() == llvm::Triple::WASIp3) | ||
| HasLibcallThreadContext = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic seems to be repeated in WebAssemblySubtarget.cpp. Is the expected/unavoidable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is expected, because both Clang and LLVM need to default the behaviour based on the triple |
||
| } | ||
|
|
||
| StringRef getABI() const override; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,11 @@ static bool WantsPthread(const llvm::Triple &Triple, const ArgList &Args) { | |
| return WantsPthread; | ||
| } | ||
|
|
||
| static bool WantsLibcallThreadContext(const llvm::Triple &Triple, | ||
| const ArgList &Args) { | ||
| return Triple.getOS() == llvm::Triple::WASIp3; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this function or can use the |
||
|
|
||
| void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA, | ||
| const InputInfo &Output, | ||
| const InputInfoList &Inputs, | ||
|
|
@@ -169,6 +174,9 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA, | |
|
|
||
| AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA); | ||
|
|
||
| if (WantsLibcallThreadContext(ToolChain.getTriple(), Args)) | ||
| CmdArgs.push_back("--libcall-thread-context"); | ||
|
|
||
| if (WantsPthread(ToolChain.getTriple(), Args)) | ||
| CmdArgs.push_back("--shared-memory"); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,6 +217,7 @@ | |
| // GENERIC-NOT: #define __wasm_simd128__ 1{{$}} | ||
| // GENERIC-NOT: #define __wasm_tail_call__ 1{{$}} | ||
| // GENERIC-NOT: #define __wasm_wide_arithmetic__ 1{{$}} | ||
| // GENERIC-NOT: #define __wasm_libcall_thread_context__ 1{{$}} | ||
|
|
||
| // RUN: %clang -E -dM %s -o - 2>&1 \ | ||
| // RUN: -target wasm32-unknown-unknown -mcpu=bleeding-edge \ | ||
|
|
@@ -251,3 +252,12 @@ | |
| // RUN: | FileCheck %s -check-prefix=BLEEDING-EDGE-NO-SIMD128 | ||
| // | ||
| // BLEEDING-EDGE-NO-SIMD128-NOT: #define __wasm_simd128__ 1{{$}} | ||
|
|
||
| // RUN: %clang -E -dM %s -o - 2>&1 \ | ||
| // RUN: -target wasm32-wasip3 \ | ||
| // RUN: | FileCheck %s -check-prefix=LIBCALL-THREAD-CONTEXT | ||
| // RUN: %clang -E -dM %s -o - 2>&1 \ | ||
| // RUN: -target wasm64-wasip3 \ | ||
| // RUN: | FileCheck %s -check-prefix=LIBCALL-THREAD-CONTEXT | ||
|
|
||
| // LIBCALL-THREAD-CONTEXT: #define __wasm_libcall_thread_context__ 1{{$}} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be removed too if we remove the macro.
TartanLlama marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s | ||
| # RUN: wasm-ld --libcall-thread-context -o %t.libcall.wasm %t.o | ||
| # RUN: obj2yaml %t.libcall.wasm | FileCheck %s --check-prefix=LIBCALL | ||
| # RUN: wasm-ld -o %t.global.wasm %t.o | ||
| # RUN: obj2yaml %t.global.wasm | FileCheck %s --check-prefix=GLOBAL | ||
|
|
||
| .globl _start | ||
| _start: | ||
| .functype _start () -> () | ||
| end_function | ||
|
|
||
| # LIBCALL: Name: __init_stack_pointer | ||
| # GLOBAL: Name: __stack_pointer |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Test that linking object files with mismatched thread context ABIs fails with an error. | ||
| # The presence of an import of __stack_pointer from the env module should be treated | ||
| # as an indication that the global thread context ABI is being used. | ||
|
|
||
| # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s | ||
| # RUN: not wasm-ld --libcall-thread-context %t.o -o %t.wasm 2>&1 | FileCheck %s | ||
|
|
||
| # CHECK: object file uses globals for thread context, but --libcall-thread-context was specified | ||
|
|
||
| .globl _start | ||
| _start: | ||
| .functype _start () -> () | ||
| end_function | ||
|
|
||
| .globaltype __stack_pointer, i32 | ||
|
|
||
| .globl use_stack_pointer | ||
| use_stack_pointer: | ||
| .functype use_stack_pointer () -> () | ||
| global.get __stack_pointer | ||
| drop | ||
| end_function | ||
|
TartanLlama marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s | ||
|
TartanLlama marked this conversation as resolved.
|
||
| # RUN: wasm-ld --libcall-thread-context --shared-memory -no-gc-sections -o %t.wasm %t.o | ||
| # RUN: obj2yaml %t.wasm | FileCheck %s | ||
| # RUN: llvm-objdump -d --no-print-imm-hex --no-show-raw-insn %t.wasm | FileCheck %s --check-prefix=DIS | ||
|
|
||
| .globl __wasm_get_tls_base | ||
| __wasm_get_tls_base: | ||
| .functype __wasm_get_tls_base () -> (i32) | ||
| i32.const 0 | ||
| end_function | ||
|
|
||
| .globl _start | ||
| _start: | ||
| .functype _start () -> (i32) | ||
| call __wasm_get_tls_base | ||
| i32.const tls1@TLSREL | ||
| i32.add | ||
| i32.load 0 | ||
| call __wasm_get_tls_base | ||
| i32.const tls2@TLSREL | ||
| i32.add | ||
| i32.load 0 | ||
| i32.add | ||
| end_function | ||
|
|
||
| .section .tdata.tls1,"",@ | ||
| .globl tls1 | ||
| tls1: | ||
| .int32 1 | ||
| .size tls1, 4 | ||
|
|
||
| .section .tdata.tls2,"",@ | ||
| .globl tls2 | ||
| tls2: | ||
| .int32 2 | ||
| .size tls2, 4 | ||
|
|
||
| .section .custom_section.target_features,"",@ | ||
| .int8 2 | ||
| .int8 43 | ||
| .int8 11 | ||
| .ascii "bulk-memory" | ||
| .int8 43 | ||
| .int8 7 | ||
| .ascii "atomics" | ||
|
|
||
|
|
||
| # CHECK: GlobalNames: | ||
| # CHECK-NEXT: - Index: 0 | ||
| # CHECK-NEXT: Name: __init_stack_pointer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not quite clear on how these newly-named versions of the old globals are still needed/used. We we document that part somewhere? Maybe it is and I missed it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're used to have a known-good stack and TLS area in case they are needed by task initialization: https://github.com/TartanLlama/wasi-libc/blob/sy/coop-threading/libc-bottom-half/sources/__wasm_init_task.s I'm planning to submit a PR to the tooling conventions repo when this patch is merged, but I can make a draft one now if you like |
||
| # CHECK-NEXT: - Index: 1 | ||
| # CHECK-NEXT: Name: __init_tls_base | ||
| # CHECK-NEXT: - Index: 2 | ||
| # CHECK-NEXT: Name: __tls_size | ||
| # CHECK-NEXT: - Index: 3 | ||
| # CHECK-NEXT: Name: __tls_align | ||
|
|
||
| # DIS-LABEL: <__wasm_init_memory>: | ||
|
|
||
| # DIS-LABEL: <_start>: | ||
| # DIS-EMPTY: | ||
| # DIS-NEXT: call 4 | ||
| # DIS-NEXT: i32.const 0 | ||
| # DIS-NEXT: i32.add | ||
| # DIS-NEXT: i32.load 0 | ||
| # DIS-NEXT: call 4 | ||
| # DIS-NEXT: i32.const 4 | ||
| # DIS-NEXT: i32.add | ||
| # DIS-NEXT: i32.load 0 | ||
| # DIS-NEXT: i32.add | ||
| # DIS-NEXT: end | ||
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 wonder again if we really need this new macro.
Now that this is no longer a wasm feature I think there is even more reason to not do this. Given that even the target triple information is not exposed as builtin macrso I don't see why we should give this special status.
If wasi-libc really needs to know in the source code it can define do something like
-target=wasip3 -DUSE_WASIP3Uh oh!
There was an error while loading. Please reload this page.
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 think since we've decided that this can just be the OS ABI (i.e. the wasip3 OS implies use of the libcall thread context ABI), any code that cares should be able to use the
__wasip3__macro that gets defined when we target that OS.(in other words, target triple information typically is exposed as a builtin macro, e.g. here).
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.
This is still useful as it will enable us to merge code into
wasi-libc's WASIP3 target that will work for both ABIs while the codebase transitions.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 also don't really like having this in with the wasm features, but I'm sympathetic to the need to transition to the final ABI for wasip3. Is it just libraries in wasi-sdk that would need this transition? Maybe we could leave this in LLVM for a release and then remove it (or how long would it need to be?)
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.
Yeah, everything else should just be a recompile, I can only forsee libc needing code changes