[WebAssembly] Generate a call to __wasm_apply_global_tls_relocs in __wasm_init_memory #149832
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-wasm Author: None (Arshia001) ChangesMotivationWe recently implemented the WebAssembly exception handling proposal in Wasmer 6.0. As a result, we can now take advantage of clang's support for compiling SjLj and C++ exceptions to WASM EH. This PR fixes a wasm-ld issue that breaks the use of C++ exception handling in WASI(X) modules. Note: I use WASI(X) to mean either wasi preview 1 or WASIX modules. Error detailsWhen compiling C++ code that uses exceptions, clang generates a TLS initialization happens in two separate places; for the "main thread", As it stands, It is important to note that exception handling code generated by the compiler uses This PR allows a call to But how does emscripten work if this is broken?Good question! Emscripten calls This is a workaround that we can use in WASIX as well. However, as far as I understand, the current behavior is wasm-ld is broken, since Full diff: https://github.com/llvm/llvm-project/pull/149832.diff 1 Files Affected:
diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index b704677d36c93..3cd6a73fb1a31 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -1366,6 +1366,15 @@ void Writer::createInitMemoryFunction() {
writeUleb128(os, s->index, "segment index immediate");
writeU8(os, 0, "memory index immediate");
}
+
+ // After initializing the TLS segment, we also need to apply TLS
+ // relocations in the same way __wasm_init_tls does.
+ if (ctx.arg.sharedMemory && s->isTLS() &&
+ ctx.sym.applyGlobalTLSRelocs) {
+ writeU8(os, WASM_OPCODE_CALL, "CALL");
+ writeUleb128(os, ctx.sym.applyGlobalTLSRelocs->getFunctionIndex(),
+ "function index");
+ }
}
}
|
There was a problem hiding this comment.
According to the comment for createApplyGlobalTLSRelocationsFunction it cannot be called during the start function: (
llvm-project/lld/wasm/Writer.cpp
Lines 1520 to 1523 in 6fa759b
I don't remember exactly why this is...
lld/wasm/Writer.cpp
Outdated
|
|
||
| // After initializing the TLS segment, we also need to apply TLS | ||
| // relocations in the same way __wasm_init_tls does. | ||
| if (ctx.arg.sharedMemory && s->isTLS() && |
There was a problem hiding this comment.
ctx.arg.sharedMemory is probably redundant here since without it applyGlobalTLSRelocs would never be created.
|
@sbc100 thanks for the review!
I can't think of anything, except the fact that it needs In the meantime, do you have other suggestions on how to fix this? I suppose making |
|
This is the change that introduced the comment: https://github.com/llvm/llvm-project/blob/ef8c9135efcb3847fc0e5bbdb55eae18751090df/lld/wasm/Writer.cpp Looking over the code, it seems that back then, llvm-project/lld/wasm/Writer.cpp Lines 1116 to 1138 in ef8c913 Around a year later, static allocation of the TLS section was added in: llvm-project/lld/wasm/Writer.cpp Lines 1190 to 1208 in 74f9841 But |
…nsFunction` * Remove redundant condition when generating call to `__wasm_apply_global_tls_relocs` in `lld::wasm::Writer::createInitMemoryFunction`
|
@sbc100 I believe we're at the one-week ping threshold :) |
|
I'm hoping to get some more time to look into this soon. My main concern is around the timing of application of relocations. The dynamic linking scenario its generally not safe to apply relocations until all libraries have been loaded (i.e. all symbols have been resolved). At last that is true for relocations in general. Perhaps its true that TLS relocations always resolve to internal locations? In which case this might be safe. But this is the reason way |
|
The dynamic linking aspect is very important to us as well, since we just so happen to have released a dynamic linker less than a month ago. Now I'm wondering whether our handling of TLS symbols is correct, at least when a symbol with the same name is exported from multiple side modules... Gonna have to look at it tomorrow. Another interesting case is the catching of C++ exceptions across module boundaries. Note, my entire description of the issue was based on a single module with local-exec TLS. I assume the behaviour will be different with global-dynamic. In the meantime, I'll wait for your review with a healthy dose of excitement. |
|
Taking another look at the code it seem like this should actually be safe since |
| @@ -1366,6 +1366,14 @@ void Writer::createInitMemoryFunction() { | |||
| writeUleb128(os, s->index, "segment index immediate"); | |||
| writeU8(os, 0, "memory index immediate"); | |||
| } | |||
|
|
|||
| // After initializing the TLS segment, we also need to apply TLS | |||
There was a problem hiding this comment.
How about "After initializing the TLS segment and setting __tls_base we can call __wasm_apply_global_tls_relocs"
|
Note that you will still need to call I suppose we could have I assume you are calling |
Yes, this already happens in both wasix-libc and wasi-libc as part of the thread creation routine.
Edit: NVM, my mistake.
Edit: NVM, my mistake
Yes, at the very last stage when every module is already loaded in and instantiated, since we need everything to be resolved. I did a bit more digging, and the same problem also exists in DL modules; however, there the problem doesn't show itself (at least in anything we've compiled). Breakdown of the situation:
While this problem also exists in DL modules, I will again stress the fact that As long as |
|
Just to clarify did you mean to write |
|
I agree that any relocation functions that is guaranteed to only refer to DSO-local symbols are safe to be called from the start function (once Assuming that However, IIUC there is a difference between |
I did not, in the sense that I didn't know there are two variations. Let me go over the code one more time. |
|
Yes, I did mean |
|
Side note: since I didn't know about |
|
But this creates a second problem:
Edit 2: that doesn't work for side modules though. Better to do it in the linker. |
Motivation
We recently implemented the WebAssembly exception handling proposal in Wasmer 6.0. As a result, we can now take advantage of clang's support for compiling SjLj and C++ exceptions to WASM EH. This PR fixes a wasm-ld issue that breaks the use of C++ exception handling in WASI(X) modules.
Note: I use WASI(X) to mean either wasi preview 1 or WASIX modules.
Error details
When compiling C++ code that uses exceptions, clang generates a
GOT.data.internal.__wasm_lpad_contextglobal, which points to the wasm landing pad context that's shared between compiler code and libunwind. This global is initialized in the__wasm_apply_global_tls_relocsfunction.TLS initialization happens in two separate places; for the "main thread",
__wasm_init_memoryruns as the(start)function of the WASM module, initializing all memory segments (including TLS), while also initializing the main thread's__tls_baseto the space reserved for it by the compiler, and signalling this fact to other threads via an atomic. Other threads need to run__wasm_init_tlsafter getting their respective__tls_baseglobal initialized externally.As it stands,
__wasm_apply_global_tls_relocsis only called through__wasm_init_tls, meaning if code doesn't call__wasm_init_tls, any globals that are initialized in__wasm_apply_global_tls_relocsdo not get initialized. This is the case for the main thread.It is important to note that exception handling code generated by the compiler uses
GOT.data.internal.__wasm_lpad_context, while the code in_Unwind_CallPersonalitygoes through__tls_base + offsetdirectly. BecauseGOT.data.internal.__wasm_lpad_contextis not initialized in the main thread, the compiler and_Unwind_CallPersonalitydo not agree on where the landing pad context is stored. This results inscan_eh_tabnot getting the correct LSDA pointer. Exception handling is then completely broken; the catch-all block runs for every exception due to a lack of any type information at runtime.This PR allows a call to
__wasm_apply_global_tls_relocsto be generated in__wasm_init_memoryif needed, which should fix the value ofGOT.data.internal.__wasm_lpad_contextin modules' main threads. Interestingly, through all of our recent work on dynamic linking and PIC modules, we never encountered__wasm_apply_global_tls_relocs, and I don't know if it's used for anything besidesGOT.data.internal.__wasm_lpad_context.But how does emscripten work if this is broken?
Good question! Emscripten calls
__wasm_init_tlsredundantly for main threads, and thus initializes the TLS area twice. This has no observable effect besides being slower, and does indeed fix C++ exception handling.This is a workaround that we can use in WASIX as well. However, as far as I understand, the current behavior is wasm-ld is broken, since
__wasm_init_memoryand__wasm_init_tlsshould behave similarly with respect to TLS initialization, but feel free to disagree with me here.