fix: Big endian support does not work since 4.0.9 #25042
Conversation
slavek-kucera
force-pushed
the
fix_big_endian_after_4_0_9
branch
from
83028e7 to
fbe881c
Compare
|
What hardware do you run on that is Big Endian? Are there maybe single board computers, or specific Big Endian OSes for ARM computers that would fit this target? I'm pondering how this could be kept working in the future. I presume e.g. CircleCI doesn't have BE targets in its catalog. |
|
I am running it on s390x both z/linux and z/os. The testing could be handled as described here. I am just not familiar enough with the project to implement it without a lot of research or trial and error. |
|
Oh that's cool, I wasn't familiar with qemu, interesting to see it can be used to run a single program invocation like that. Gave that a spin at main...juj:emscripten:bigendian_test_suite , and indeed it does work out, and some Emscripten core tests pass at least. |
|
I think we should probably add an "experimental" warning when using the BIGENDIAN setting, since its maintained at only best effort level. There are probably only one or two users our there so I think its fine for it to be incumbent upon those users to keep it alive and kicking. |
|
Would it be ok to pre-install the dependencies into the |
|
I think it would be good to be able to run a smoke test or two at least. Though I don't know what the capabilities of CircleCI would be. I ran the core suite in bigendian mode locally on the contents of this PR. Many tests do pass there out of the box: https://clbri.com/dump/emscripten/toolchain_profiler.results_20250826_1351.html though some are still red. Skipping individual tests in big endian mode is easy, if you would like to work through the failures in the core suite, and skip any tests that might be still failing: 46a2cdf That way you could mark down TODOs/BUGs to get towards a passing big endian suite. |
|
To reproduce a red test in that graph, you can run e.g. |
|
It looks like most of the failing tests are SAFE_HEAP and asan related and then x86 specific. As far as I can tell, the SAFE_HEAP is currently incompatible with both big endian and growable heap support. I'd say the same is true for the asan support (for the same reason). |
|
I think there might be a way to make all these options compatible. The I could look into that after this PR is merged. |
|
I should be able to plop in a run of the core suite with the big endian Node.js on my test lab CI at http://clbri.com:8010/#/builders Posted a PR emscripten-core/emsdk#1590 that will help automate the setup of the big endian Node on the CI, it should be a trivial one-liner setup then. |
Regarding the |
|
I put together a patch that would make all the options compatible slavek-kucera/emscripten@fix_big_endian_after_4_0_9...fix_big_endian_other_options_preview I was able to successfully run with |
Gave that a try, though there seems to be some interaction with Also, posted PR #25068 to land the bigendian0 suite. |
The transforms are replacing the very identifiers other transforms are looking for, so I don't think this is correct. |
Yeah sorry, I realised that shortly after posting my comment, so deleted it but I guess not quickly enough. Your approach from the last comment, combining the transforms, seems like a better one. |
I ran the https://clbri.com/dump/emscripten/toolchain_profiler.results_20250827_1943.html |
|
After #25074 lands, the big endian test suite will be live on That CI is my test lab CI, so not sure if it will be able to live indefinitely, though I'll try to keep it alive into the future. It will be a post-landing runner unfortunately, so will only run the bigendian0 suite on Emscripten's main branch after code has landed to it. But it should show the latest status then. |
We also run a bunch of post-landing test suites on the "Test Suites" running here: https://ci.chromium.org/p/emscripten-releases/g/main/console. We consider that an "FYI bot" which is what chromium calls bots that don't close the tree, and could , therefore, be red to long-ish periods of time. We should probably document here in For configurations such as bigendian I don't really think its reasonable for the core team them green at all times so FYI/best-effort/post-landing is where I think it belongs. |
|
Ok, all the necessary pieces for the bigendian0 suite infra have landed in emsdk and emscripten repository. This was the first live run of that suite against Emscripten |
Support for big endian targets is "best effort" and not fully supported or tested. Also, as far as I'm aware there is only one or two users of this feature so I think it makes sense for it to stay as "best effort" for the foreseeable future. See #25042
|
Looking through the last test report, there is still a few SSE/AVX tests running. |
|
You can go ahead and mark any still failing tests as |
slavek-kucera
force-pushed
the
fix_big_endian_after_4_0_9
branch
from
9108c9b to
71e47ac
Compare
|
It looks like with node24 even the EH tests are passing. As far as I can tell. The only problem is with test_modularize_instance_embind. It is as if the littleEndian transform is not applied at all, I am not sure why at the moment. |
|
I ran the test suites on top of this PR, at http://clbri.com:8010/#/builders/11/builds/321 Looks like this is in a very good shape now, only few reds remaining. |
src/lib/liblittle_endian_heap.js
Outdated
| /** @suppress {checkTypes} */ | ||
| return Atomics.waitAsync(heap, offset, order(value), timeout); | ||
| // this is suppressing incorrect closure JSC_INEXISTENT_PROPERTY warning that cannot be suppress via annotation | ||
| return Atomics.waitAsync && Atomics.waitAsync(heap, offset, order(value), timeout); |
There was a problem hiding this comment.
Try adding Atomics.waitAsync to this list:
emscripten/src/closure-externs/closure-externs.js
Lines 63 to 69 in 452e7db
There was a problem hiding this comment.
It looks like any way you define it, there is a test case that breaks.
There was a problem hiding this comment.
Interesting.. can you list some test cases that croak with the different approaches?
There was a problem hiding this comment.
I think each of the following failed differently due to the closure compiler warning
- test_modularize_closure_pre
- test_closure_full_js_library_pthread
- test_audio_worklet_minimal_runtime_pthreads_and_closure
There was a problem hiding this comment.
Oh I see. Try with the following:
diff --git a/src/closure-externs/closure-externs.js b/src/closure-externs/closure-externs.js
index 1681cd2b4..b5bfc5857 100644
--- a/src/closure-externs/closure-externs.js
+++ b/src/closure-externs/closure-externs.js
@@ -64,6 +64,11 @@ var Atomics = {};
Atomics.compareExchange = function() {};
Atomics.exchange = function() {};
Atomics.wait = function() {};
+/**
+ * @param {number=} maxWaitMilliseconds
+ * @suppress {duplicate, checkTypes}
+ */
+Atomics.waitAsync = function(i32a, index, value, maxWaitMilliseconds) {};
Atomics.notify = function() {};
Atomics.load = function() {};
Atomics.store = function() {};I think that should help Closure avoid tripping itself up in different build modes.
slavek-kucera
force-pushed
the
fix_big_endian_after_4_0_9
branch
from
9c35bdf to
68f51b9
Compare
This PR fixes issues with big endian support introduced in the following PRs:
littleEndianHeaptransform implications,The
littleEndianHeaptransform code is simplified and updated to detect the code introduced bygrowableHeappass.A new test was added to verify the combined transformation generates working code.