wabt
wabt copied to clipboard
WebAssembly
Fix architecture checks
This PR consists of two tightly related changes. It depends on #1968
1. Fix x87 math detection
TARGET_ARCH was only used to determine whether to add gcc-specific SSE math flags (-msse2 -mfpmath=sse). The new approach simply assumes gcc compatibility with its __i386__ and __SSE2_MATH__ symbols, which are defined precisely when we are targeting x86-32 with SSE2 math enabled. If those macros are defined, then we conclude all is well. Otherwise, we
add the flags if we know the compiler is gcc or clang (and will thus accept them) and issue a warning if the compiler is unknown.
Fixes #1709 Fixes #1688
2. Use standard modules to test endianness
CMake prior to v3.20 provides a module TestBigEndian which we can use to set the WABT_BIG_ENDIAN define. As of 3.20, the CMAKE_<LANG>_BYTE_ORDER variable should be preferred. Leaving this as a note for the future.
I did test on x86 cross compilation just using -march=i686 and it worked. I can't speak to big endian architectures without emulating them, but I definitely trust CMake's dedicated module for that more than the prior ad-hoc architecture grepping.
It would definitely be wise to test these scenarios on qemu using Docker. I could put together a PR that adds such workflows, but it would take me a significant amount of time, and I caution that the resulting workflows will be very slow.
I can't claim to speak for everybody, but my own tentative view is that if you can test it once on an emulated big-endian machine and once on an actual Mac with Apple Silicon and just show that it works, that might be good enough at this stage. We can defer the issue of getting those in the CI workflow.
Here's a bit more elaboration on my reasoning...
Looking just at the first commit, all I did was change the logic that sets WABT_BIG_ENDIAN from the bespoke logic coded before this PR to the logic provided by CMake itself. My impression is that CMake, with its very many customers, knows how to detect endianness at least as well as the status quo.
If we accept that change, we realize that all that remains is some logic to decide whether or not to apply the flags -msse2 -mfpmath=sse. First, observe that most of that code is dead. Second, observe these flags "belong" to GCC and other compilers like Clang emulate them. The latter flag is documented to set __SSE2_MATH__ and both compilers are documented to set __i386__ on any x86-32 arch, including i686.
For compilers we don't detect, the best we can really do is hope that the user set appropriate flags (via, e.g. CMAKE_CXX_FLAGS) to emulate GCC inasmuch as it defines those macros to mean the same thing. If not, we have no guarantee that passing those flags to the compiler is correct at all. We warn in that case instead.
I can emulate a big endian machine, no problem. Unfortunately, I don't own any Apple Silicon hardware. I will ask my team if we can take our Apple Silicon CI machine down for a few hours over the weekend to test this.
Testing
Docker
Setup
Prerequisites:
$ sudo apt install qemu binfmt-support qemu-user-static
$ docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
$ docker run --rm -t s390x/ubuntu uname -a
Linux 08c44ccc6c71 5.15.0-46-generic #49~20.04.1-Ubuntu SMP Thu Aug 4 19:15:44 UTC 2022 s390x s390x s390x GNU/Linux
Dockerfile:
FROM ubuntu:focal
WORKDIR /ws
ARG DEBIAN_FRONTEND=noninteractive
ENV TZ=UTC
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
RUN apt-get update
RUN apt-get install -y git g++ cmake ninja-build python3
RUN git clone --recursive https://github.com/alexreinking/wabt.git
ARG WABT_REF=bugfix/1688
RUN git -C wabt checkout $WABT_REF
RUN cmake -G Ninja -S wabt -B build -DCMAKE_BUILD_TYPE=Release && \
cmake --build build
s390x
$ docker build --platform linux/s390x -t wabt-s390x . | tee build-s390x.log
$ docker run --rm -t wabt-s390x ls
build wabt
$ docker run --rm -t wabt-s390x grep WABT_BIG_ENDIAN build/config.h
#define WABT_BIG_ENDIAN 1
So at this point, we can see that big endian detection is working correctly. :tada:
Unfortunately, running the unit tests reveals a failure in the ROT13 test.
$ docker run --rm -t wabt-s390x cmake --build build --target run-unittests
...
[ RUN ] InterpTest.Rot13
/ws/wabt/src/test-interp.cc:546: Failure
Expected equality of these values:
"Uryyb, JroNffrzoyl!"
string_data
Which is: "Hello, WebAssembly!"
[ FAILED ] InterpTest.Rot13 (9 ms)
...
[----------] Global test environment tear-down
[==========] 138 tests from 19 test suites ran. (17943 ms total)
[ PASSED ] 137 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] InterpTest.Rot13
1 FAILED TEST
FAILED: CMakeFiles/run-unittests
cd /ws/wabt && /ws/build/wabt-unittests
ninja: build stopped: subcommand failed.
I re-ran the above steps using commit 7a1d82605b96f07a6a576b563b39acb536db4069, which is the commit prior to my first PR being merged, and observed the same failure. So whatever is broken here is not due to the build system changes. Here's the command I used to build with that commit:
$ docker build --platform linux/s390x --build-arg WABT_REF=7a1d826 -t wabt-s390x-prev . | tee build-s390x-prev.log
i386
I had to edit the Dockerfile's first line to read FROM i386/ubuntu:focal
$ docker build --platform linux/i386 -t wabt-i386 . | tee build-i386.log
Running tests reveals failures in the run-tests target related to floating point.
Once again, however, these errors are independent of this PR:
$ docker build --platform linux/i386 -t wabt-i386-prev --build-arg WABT_REF=7a1d826 .
...
$ docker run --rm -it wabt-i386-prev | tee test-i386-prev.log
Once again, however, these errors are independent of this PR:
$ docker build --platform linux/i386 -t wabt-i386-prev --build-arg WABT_REF=7a1d826 .
...
$ docker run --rm -it wabt-i386-prev | tee test-i386-prev.log
@keithw - is there a known-good commit for s390x? I have gone all the way back to 3c4bad02b95e8ef6b4f773647adcbe467cc316f3 and haven't found one that passes IntepTest.Rot13. All the other tests in that same binary are fine.
Same thing for i386. I can't find a good commit off which to bisect the floating point errors.
Unfortunately I don't know the answer for s390x or i686 with sse2. Some of the others probably know the history much better (@binji @sbc100 @kripken).
I think for now, my view would be that if we can document the pre-existing testsuite failure on s390x and i686/sse2, and that these commits don't make any additional tests fail, I think we can defer fixing those architectures for another day. We should make tracking issues for them.
What I do think we need (before we can push a commit that auto-closes the issues related to failure on macOS/arm64) is a positive report that it works there. (I wonder if this is also something we can test in emulation?)
@keithw - that's perfectly reasonable. I'm changing the status of this PR to "Draft" for now, but I'll come back to these issues in a bit. I have some more pressing PRs I'd like to propose.
@sbc100 - good to merge? Would be nice to sneak this in ahead of 1.0.30
If we're going to auto-close the ARM Mac bugs, I would be a lot happier if we can get at least one person to test this and report that this fixes the bug before merging. I hope it can't be that hard; those ARM Macs are incredibly popular. (If you don't have anybody on hand with a Mac, I can find somebody around here.)
@keithw - I just SSH'd into our ARM Mac buildbot and I see the following output on this branch:
$ cmake -G Ninja -S wabt -B build -DCMAKE_BUILD_TYPE=Release
-- The C compiler identification is AppleClang 13.1.6.13160021
-- The CXX compiler identification is AppleClang 13.1.6.13160021
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Git: /usr/bin/git (found version "2.32.0 (Apple Git-132)")
git: fatal: No names found, cannot describe anything.
** Did you forget to run `git fetch --tags`?
-- Looking for alloca.h
-- Looking for alloca.h - found
-- Looking for unistd.h
-- Looking for unistd.h - found
-- Looking for snprintf
-- Looking for snprintf - found
-- Looking for strcasecmp
-- Looking for strcasecmp - found
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of ssize_t
-- Check size of ssize_t - done
-- Check size of size_t
-- Check size of size_t - done
-- Looking for __i386__
-- Looking for __i386__ - not found
-- Looking for __SSE2_MATH__
-- Looking for __SSE2_MATH__ - not found
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Found PythonInterp: /opt/homebrew/bin/python3 (found suitable version "3.9.13", minimum required is "3.5")
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/halidenightly/temp/build
$ cmake --build build/
[143/188] Building CXX object CMakeFiles/spectest-interp.dir/src/tools/spectest-interp.cc.o
/Users/halidenightly/temp/wabt/src/tools/spectest-interp.cc:192:7: warning: unused variable 'lane_count' [-Wunused-variable]
int lane_count = LaneCountFromType(ev.lane_type);
^
/Users/halidenightly/temp/wabt/src/tools/spectest-interp.cc:239:7: warning: unused variable 'lane_count' [-Wunused-variable]
int lane_count = LaneCountFromType(lane_type);
^
2 warnings generated.
[188/188] cd /Users/halidenightly/temp/build && /Applications/CMake.app/Cont...py /Users/halidenightly/temp/build/wasm2c /Users/halidenightly/temp
Ignoring the git-describe warning here (because I'm working from a fork with no tags), everything looks good. In particular, the two issues are gone.
Also worth noting that there are a bunch of issues on macOS arm of the following form:
- test/wasm2c/spec/utf8-import-field.txt
expected error code 0, got 1.
STDERR MISMATCH:
--- expected
+++ actual
@@ -0,0 +1,13 @@
+error: overriding currently unsupported rounding mode on this target [-Werror,-Wunsupported-floating-point-opt]
+1 error generated.
+Traceback (most recent call last):
+ File "/Users/halidenightly/temp/wabt/test/run-spec-wasm2c.py", line 543, in <module>
+ sys.exit(main(sys.argv[1:]))
+ File "/Users/halidenightly/temp/wabt/test/run-spec-wasm2c.py", line 512, in main
+ o_filenames.append(Compile(cc, c_filename, out_dir, includes))
+ File "/Users/halidenightly/temp/wabt/test/run-spec-wasm2c.py", line 400, in Compile
+ cc.RunWithArgsForStdout(*args)
+ File "/Users/halidenightly/temp/wabt/test/utils.py", line 88, in RunWithArgsForStdout
+ raise error
+utils.Error: Error running "cc -I/Users/halidenightly/temp/wabt/wasm2c -c out/test/wasm2c/spec/utf8-import-field/utf8-import-field-dummy.c -o out/test/wasm2c/spec/utf8-import-field/utf8-import-field-dummy.o -O2 -Wall -Werror -Wno-unused -Wno-ignored-optimization-argument -Wno-tautological-constant-out-of-range-compare -Wno-infinite-recursion -fno-optimize-sibling-calls -frounding-math -fsignaling-nans -std=c99 -D_DEFAULT_SOURCE" (1):
+None
STDOUT MISMATCH:
--- expected
+++ actual
@@ -1 +0,0 @@
-0/0 tests passed.
But this has nothing to do with this PR. I think it just doesn't like -frounding-math.
I think it just doesn't like
-frounding-math.
Indeed that is so. Removing this flag from test/run-spec-wasm2c.py causes all but 24 of the 794 tests inside test/wasm2c/spec/float_exprs.txt to pass.
- test/wasm2c/spec/float_exprs.txt
expected error code 0, got 1.
STDERR MISMATCH:
--- expected
+++ actual
@@ -0,0 +1,33 @@
+Traceback (most recent call last):
+ File "/Users/halidenightly/temp/wabt/test/run-spec-wasm2c.py", line 544, in <module>
+ sys.exit(main(sys.argv[1:]))
+ File "/Users/halidenightly/temp/wabt/test/run-spec-wasm2c.py", line 538, in main
+ utils.Executable(main_exe, forward_stdout=True).RunWithArgs()
+ File "/Users/halidenightly/temp/wabt/test/utils.py", line 96, in RunWithArgs
+ raise error
+utils.Error: Error running "out/test/wasm2c/spec/float_exprs/float_exprs" (1):
+None
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:492: assertion failed: in Z_float_exprs_4_wasmZ_f32Z2Eno_fold_sub_zero(&Z_float_exprs_4_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0x7fa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:495: assertion failed: in Z_float_exprs_4_wasmZ_f64Z2Eno_fold_sub_zero(&Z_float_exprs_4_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:532: assertion failed: in Z_float_exprs_6_wasmZ_f32Z2Eno_fold_mul_one(&Z_float_exprs_6_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0x7fa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:535: assertion failed: in Z_float_exprs_6_wasmZ_f64Z2Eno_fold_mul_one(&Z_float_exprs_6_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:572: assertion failed: in Z_float_exprs_8_wasmZ_f32Z2Eno_fold_div_one(&Z_float_exprs_8_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0x7fa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:575: assertion failed: in Z_float_exprs_8_wasmZ_f64Z2Eno_fold_div_one(&Z_float_exprs_8_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:583: assertion failed: in Z_float_exprs_9_wasmZ_f32Z2Eno_fold_div_neg1(&Z_float_exprs_9_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0xffa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:586: assertion failed: in Z_float_exprs_9_wasmZ_f64Z2Eno_fold_div_neg1(&Z_float_exprs_9_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:594: assertion failed: in Z_float_exprs_10_wasmZ_f32Z2Eno_fold_neg0_sub(&Z_float_exprs_10_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0xffa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:597: assertion failed: in Z_float_exprs_10_wasmZ_f64Z2Eno_fold_neg0_sub(&Z_float_exprs_10_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:605: assertion failed: in Z_float_exprs_11_wasmZ_f32Z2Eno_fold_neg1_mul(&Z_float_exprs_11_wasm_instance, make_nan_f32(0x200000)): expected result to be a arithmetic nan, got 0xffa00000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:608: assertion failed: in Z_float_exprs_11_wasmZ_f64Z2Eno_fold_neg1_mul(&Z_float_exprs_11_wasm_instance, make_nan_f64(0x4000000000000)): expected result to be a arithmetic nan, got 0x0000000000000000.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3124: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_sub_zero(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3127: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_neg0_sub(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3130: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_mul_one(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3133: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_neg1_mul(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3136: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_div_one(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3139: assertion failed: in Z_float_exprs_87_wasmZ_f32Z2Eno_fold_div_neg1(&Z_float_exprs_87_wasm_instance, 2141192192u): expected 2143289344, got 2139095040.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3142: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_sub_zero(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3145: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_neg0_sub(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3148: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_mul_one(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3151: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_neg1_mul(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3154: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_div_one(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
+out/test/wasm2c/spec/float_exprs/float_exprs-main.c:3157: assertion failed: in Z_float_exprs_87_wasmZ_f64Z2Eno_fold_div_neg1(&Z_float_exprs_87_wasm_instance, 9219994337134247936ull): expected 9221120237041090560, got 9218868437227405312.
STDOUT MISMATCH:
--- expected
+++ actual
@@ -1 +1 @@
-794/794 tests passed.
+770/794 tests passed.
Ok, good enough for me -- thanks for testing. Support for -frounding-math in LLVM on arm64 seems to be semi-tracked at https://github.com/llvm/llvm-project/issues/54975