Repository navigation
wasm2c: use _addcarry_u64 only on MSVC x64, and test a 64-bit memory with MSVC - #2901
Conversation
0b99108 to
d8765dc
Compare
|
@sbc100 Should I sqash this two commits? |
|
We auto-squash on commit. Would you mind sending a separate PR for the skip removal? |
with no extra issue opening? |
|
Yes, no need to for a a separate issue. It seems like memory64 has worked on windows x64 for while but we forgot to remove this block. So fixing that by removing that block seems separable from how we make non-x64 work. |
yes I can, I did few launches of the CI on my own fork to check it before fix |
`run-spec-wasm2c.py` skips a test on Windows when memory64 is enabled. That skip was written when memory64 was opt-in (`--enable-memory64`, #2086). Since #2837 memory64 is on by default (`--disable-memory64`), so the condition holds for almost every test and 260 of the 262 wasm2c tests are skipped on `windows-latest`: the C that wasm2c writes is not compiled by cl.exe. This removes the skip. Split out of #2901 at @sbc100's request.
…with MSVC _addcarry_u64 is an intrinsic of the x64 targets: a module with a 64-bit memory fails to link with MSVC for x86 and ARM64 (LNK2019, unresolved external symbol _addcarry_u64). Fall back to the portable branch there. The problem was not seen because run-spec-wasm2c.py skipped the test whenever memory64 is enabled on Windows. That skip was written for the opt-in --enable-memory64; once memory64 became the default it holds for almost every test, so the C that wasm2c writes was not compiled by cl.exe at all. Remove the skip, add a test of a 64-bit memory, and a CI job that builds wabt for Windows x86 and ARM64 and runs it.
…nstead of a new test
91c7f26 to
3d6ca47
Compare
|
@sbc100 rebased |
|
@o-murphy Thanks for the fix. Would you mind rebasing and squashing into one commit actually. It makes it bit easier for us to land. |
Yes, I will |
Thanks, once you push this, we can land it |
checked_add_u64inwasm2c.declarations.cuses_addcarry_u64for every MSVCtarget, but that intrinsic exists only on x64. A module with a 64-bit memory that
loads or stores fails to link with MSVC for x86 and for ARM64:
This restricts the branch to
_M_X64; the other MSVC targets use the existingportable
#elsebranch.The problem was not caught by CI because
run-spec-wasm2c.pyskips the testwhenever memory64 is enabled on Windows. That skip was written for the opt-in
--enable-memory64(#2086). After #2837 made memory64 the default and renamed theflag to
--disable-memory64, the condition holds for almost every test, so 260 ofthe 262 wasm2c tests are skipped on
windows-latest, and the C that wasm2c writesis not compiled by cl.exe at all. This PR:
test/wasm2c/msvc-compile.txt, a spec test of a module with a 64-bit memory(a store and load, and an out-of-bounds access that traps);
-A Win32) and ARM64(
windows-11-arm) and runs that test (-DWABT_RUN_TESTS_ARGS=msvc-compile).The filter has no slash because the paths have backslashes on Windows.
Regenerated:
src/prebuilt/wasm2c_source_declarations.ccand the expected output ofthe tests in
test/wasm2c/that contain the template.Testing: on GitHub Actions (
windows-latest) the new test fails without the changewith the two
LNK2019errors above, on x86 and on ARM64, and passes with it.Fixes #2885