Support wasm64 #444
Support wasm64 #444
Conversation
trcrsired
changed the title
|
@yamt Hello. Can you start the CI for me? Thank you |
|
@abrown Hi. Could you explain why CI is not automatic but needs action from the maintainers? |
|
I have removed all signal apis from the upstream file and regenerated api.h and __wasi_realfile.c. |
Add noexcept to wasi header to avoid abi bugs with C++
|
Add CI for wasm64-wasi + wasm64-wasi-threads |
Makefile
Outdated
| @@ -22,6 +22,8 @@ BUILD_LIBC_TOP_HALF ?= yes | |||
| # The directory where we will store intermediate artifacts. | |||
| OBJDIR ?= build/$(TARGET_TRIPLE) | |||
|
|
|||
| BUILD_WASI64 ?= no | |||
There was a problem hiding this comment.
Maybe BUILD_WASM64 would better match the target name?
There was a problem hiding this comment.
But WASI64 is not a thing as far as I know. It doesn't mean much to me as the reader.. whereas WASM64 is the name of the llvm target.
There was a problem hiding this comment.
but wasm64 is not wasi64, because you also have wasm64-emscripten.
There was a problem hiding this comment.
But WASI64 is not a thing as far as I know. It doesn't mean much to me as the reader.. whereas WASM64 is the name of the llvm target.
what about just allowing users to set TRIPLE?
| // TODO: Encoding this in witx. | ||
| #define __WASI_DIRCOOKIE_START (UINT64_C(0)) | ||
| typedef __SIZE_TYPE__ __wasi_size_t; | ||
|
|
||
| _Static_assert(sizeof(__wasi_size_t) == 4, "witx calculated size"); |
There was a problem hiding this comment.
Rather than removing these lines perhaps change the 4 to something like WASM_PTR_SIZE?
There was a problem hiding this comment.
Well the Rust generated script does not work here.
Also, personally, i would like to remove all these _Static_assert because C and C++ are designed in this way to ensure maximum portability. Better not assume anything.
In fact, in C and C++, intxx_t, uintxx_t, uintptr_t do not necessarily exist either, the standard wants you to use uint_leastxx_t.
There was a problem hiding this comment.
You can use __SIZEOF_POINTER__ perhaps which is built into clang. Or you can use ifdef __wasm64__ to set a custom WASM_PTR_SIZE within this file.
Note that this header is specifically not designed to be flexible or portable its very much only designed to be included by a conforming version of clang and it defines as strict/inflexible ABI (which is why all those static asserts exists).
There was a problem hiding this comment.
ret.push_str(&format!( "_Static_assert(sizeof(__wasi_{}_t) == {}, \"witx calculated size\");\n", ident_name(name), dest.mem_size_align().size )); ret.push_str(&format!( "_Static_assert(_Alignof(__wasi_{}_t) == {}, \"witx calculated align\");\n", ident_name(name), dest.mem_size_align().align ));
it is generated by this code, but the values are not correct for 64 bit so i have to disable all of them.
|
|
||
| #ifdef __wasm64__ | ||
| #define a_cas_p a_cas_p | ||
| static inline void *a_cas_p(volatile void *p, void *t, void *s) |
There was a problem hiding this comment.
Why is this needed for wasm64 but not for wasm32?
There was a problem hiding this comment.
because a_cas_p is defined by the clang front end for wasm32 but not wasm64
There was a problem hiding this comment.
c_cas_p doesn't sounds like something that clang itself would ever define...
There was a problem hiding this comment.
well, the problem is that musl does not need c_cas_p on 32 bit platform, but only on 64 bit platform and i am not 100% confident of the correctness here.
|
This PR would effectively create a new wasm64 preview1 ABI, as well as a new wasi-threads ABI. Those are major new interfaces, that could eventually impact a large number of tools and engines. Since this is an official WebAssembly organization project, we should discuss this in a WASI Subgroup meeting before proceeding with this PR. |
Parameters changing back to use signed integer in __wasilibc_real.c rename libc-top-half/musl/arch/wasm32 to libc-top-half/musl/arch/wasm rename libc-top-half/musl/src/thread/wasm32 to libc-top-half/musl/src/thread/wasm rename wasi_thread_start.s to wasi_thread_start.S since .s does not support macros
Even so, it's still a major expansion in the WASI preview1 surface area, so we should get Subgroup guidance before this could be merged. |
|
changed to _wasm64 based on feedback |
|
Hi, I tried your awesome wasm64 support patch with some wasi-threads samples, and have the following result: wasm-ld: error: Global type mismatch: __stack_pointer
>>> defined as var i64 in <internal>
>>> defined as var i32 in /opt/wasi-sdk/build/wasi-sdk-21.4ga1b80857c490+m/bin/../share/wasi-sysroot/lib/wasm64-wasi-threads/libc.a(__init_tls.o)It seems that this line of code in this directory also needs to be modified(i64 when it's wasm64). After I modify it and recompile it, it gets rid of the linking error. What do you think? |
|
@trcrsired what's the status of the work? I seriously consider using wasm64 in one of my projects as that'd solve a few problems; if the work is abandoned, I'm happy to pick it up and drive further (i.e. discuss with WASI subgroup, finalize the PR). |
|
Since there wasn't any progress on that PR for the last 1.5 month, I'm planning to close on the discussion and finalize the PR. I've requested a slot in the next WASI SG meeting; hopefully we can discuss all the concerns and agree on the next steps: WebAssembly/meetings#1479 |
I need to make changes to the "c_header.rs" file because there are numerous instances where "size_t" is mistakenly identified as "uint32_t," which is incompatible with wasm64.