Fix architecture checks #1969
Fix architecture checks #1969
Conversation
alexreinking
force-pushed
the
bugfix/1688
branch
3 times, most recently
from
a1bd606
to
1701ca0
Compare
There was a problem hiding this comment.
Certainly looks reasonable (and it's great to be able to cut out the dummy compilation step), but we don't have any automated tests on a big-endian platform or i686 (which supposedly work today and this touches) or Apple Silicon or aarch64 Linux (which I guess don't work today and this is supposed to fix), so it feels a bit like flying blind. Are you able to test on any of these to give some more confidence?
|
I did test on x86 cross compilation just using |
|
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 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. |
TestingDockerSetupPrerequisites: Dockerfile: s390xSo at this point, we can see that big endian detection is working correctly. 🎉 Unfortunately, running the unit tests reveals a failure in the ROT13 test. I re-ran the above steps using commit 7a1d826, 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: i386I had to edit the Dockerfile's first line to read Running tests reveals failures in the Once again, however, these errors are independent of this PR: Once again, however, these errors are independent of this PR: |
|
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. |
alexreinking
force-pushed
the
bugfix/1688
branch
from
1701ca0
to
ace9bc2
Compare
alexreinking
force-pushed
the
bugfix/1688
branch
2 times, most recently
from
969b2d7
to
f59ab11
Compare
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.
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 WebAssembly#1709 Fixes WebAssembly#1688
alexreinking
force-pushed
the
bugfix/1688
branch
from
f59ab11
to
a92e054
Compare
|
@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: 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: But this has nothing to do with this PR. I think it just doesn't like |
Indeed that is so. Removing this flag from |
|
Ok, good enough for me -- thanks for testing. Support for |
* 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. * 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 WebAssembly#1709 Fixes WebAssembly#1688
This PR consists of two tightly related changes. It depends on #1968
1. Fix x87 math detection
TARGET_ARCHwas 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, weadd 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_ENDIANdefine. As of 3.20, theCMAKE_<LANG>_BYTE_ORDERvariable should be preferred. Leaving this as a note for the future.