- __wasm_init_memory is now the WebAssembly start function instead of being called from __wasm_call_ctors or called directly by the runtime.
- Adds a new synthetic data symbol __wasm_init_memory_flag that is atomically incremented from zero to one by the thread responsible for initializing memory.
- All threads now unconditionally perform data.drop on all passive segments.
- Removes --passive-segments and --active-segments flags and controls segment type based on --shared-memory instead. The deleted flags were only present to ameliorate the upgrade path in Emscripten.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 37683 Build 37682: arc lint + arc unit
Event Timeline
Comment Actions
I don't have the full context/history of this feature, but it mostly looks LGTM to me.
- The tool-convention linking doc says we don't generate the start section for now. I guess we should update that too.
- I'm not sure what the exact condition for the passive segment generation. We should have config->sharedMemory for sure, but in some place we have !config->relocatable and others !config->shared but not both.
- How did we tell which one is the main thread before this patch?
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 92 | So __wasm_init_tls is also called from the start function in passive segments as in __wasm_init_memory, right? | |
| lld/test/wasm/data-segments.ll | ||
| 5 | Now that tests in this file don't have --active-segments and --passive-segments anymore and are instead controlled by --shared-memory, it might be helpful to add whether we have shared memory or not in the comments, like ; atomics, shared memory => error ; atomics, bulk memory, shared memory => passive segments ... The same for other comments below. | |
| lld/test/wasm/shared-memory.yaml | ||
| 3 | Real nit: why remove the whitespace before |? | |
| lld/wasm/Driver.cpp | ||
| 474 | Are we guaranteed not to be relocatable here? | |
| 507–516 | What happens when config->sharedMemory && config->shared? | |
| lld/wasm/MarkLive.cpp | ||
| 88 | WasmSym::initMemory was created like if (config->sharedMemory && !config->shared) {
WasmSym::initMemory = symtab->addSyntheticFunction(
"__wasm_init_memory", WASM_SYMBOL_VISIBILITY_HIDDEN,
make<SyntheticFunction>(nullSignature, "__wasm_init_memory"));
...
}Don't we need the condition !config->shared here too? | |
| lld/wasm/Writer.cpp | ||
| 376 | Nit: It looks we have using namespace llvm above, so we don't need llvm:: | |
| 401 | This may not be related to this CL, but just wondering, what happens we explicitly enable a feature using --feature= option that's in the disallowed set in one of the objects? Do we error out? | |
| 712 |
| |
Comment Actions
Thanks for you comments! I am also planning to update the memory initialization scheme to use wait and notify so that it works with racing modules instead of requiring the runtime to ensure that no module is called into before the main module is finished initializing. In a web context the runtime will have to ensure the main thread wins this race because it is not allowed to wait, but this is still strictly more general than the current scheme. Does that sound good to you?
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 92 | No, __wasm_init_tls requires a runtime-allocated TLS buffer as an argument, so it has to be called from runtime code rather than a start function. | |
| lld/test/wasm/data-segments.ll | ||
| 5 | Good idea, will do. | |
| lld/test/wasm/shared-memory.yaml | ||
| 3 | Hmm, I don't think that was intentional. Thanks! | |
| lld/wasm/Driver.cpp | ||
| 474 | Yes, this function is only called when !config->relocatable. This may have been refactored in the meantime, though. I'll have to rebase and merge. | |
| 507–516 | Only the main module (which I understand is never config->shared?) needs to initialize memory. It would be bad if __wasm_init_memory were run more than once per memory. So shared modules just don't have these symbols. | |
| lld/wasm/MarkLive.cpp | ||
| 88 | Hmm, I guess it works regardless because enqueue silently disregards null arguments. But you're right that the condition should be there for consistency. Will update. | |
Comment Actions
- Address review comments
| lld/wasm/Writer.cpp | ||
|---|---|---|
| 401 | Yes, that would be an error unless --no-check-features is also passed. --feature= is just a more explicit way of passing features and is not meant to be any more privileged than the target features sections in determining what is correct and safe. | |
| 712 | Before this change we relied on the runtime to manually invoke __wasm_init_memory or __wasm_call_ctors (which called __wasm_init_memory) on the main thread. With this change, the code is more robust because it doesn't require the runtime to call anything or even ensure that there is only one thread running during memory initialization. Whichever thread runs first initializes the memory, but is not otherwise marked as the main thread or privileged in any way. If you instantiate a module on a number of threads at once it should not be possible to determine which one initialized the memory. | |
Comment Actions
Can you verify that you can pass all the "wasm2" tests in emscripten with this change?
Comment Actions
Yes, although this CL only affects shared-memory builds, so wasm2 tests are not affected. I have also verified that the pthreads tests in the browser test suite also pass with this change.
So __wasm_init_tls is also called from the start function in passive segments as in __wasm_init_memory, right?