[bigendian] Fix crash during PThread initialization by slavek-kucera · Pull Request #23700 · emscripten-core/emscripten

slavek-kucera · 2025-02-19T08:53:05Z

Notes:

There are other places using Atomic.something that likely have the same issue.
I tried keeping the nativeByteOrder32 function in the libpthread.js and use the dependency mechanism, but it was always optimized into x => x.
I tried to consider the MEMORY64 option, but it seems to not work at all in node (23 + appropriate experimental flag).
Simple hello world output was built using the instructions from the issue and tested on s390x z/os machine.

sbc100 · 2025-02-19T15:42:54Z

+    var h16 = new Int16Array(1);
+    var h8 = new Int8Array(h16.buffer);
+    h16[0] = 42;
+    return h8[0] === 42 && h8[1] === 0; // little endian ordering


Surely we must already have helpers that perform this kind of thing in emscripten? Since all the non-atomic read/writes already work?

Other accesses use the DataView methods.

Similar test is performed in the runtime_debug.js, but it is only included with ASSERTIONS option.

kripken · 2025-02-19T21:28:32Z

The general mechanism we use for this is

emscripten/tools/acorn-optimizer.mjs

Lines 1103 to 1105 in bb7fd25

    
           // Replaces each HEAP access with function call that uses DataView to enforce 
        
           // LE byte order for HEAP buffer 
        
           function littleEndianHeap(ast) {

Perhaps the best thing is to update that pass so it handles Atomic operations?

slavek-kucera · 2025-02-19T21:47:30Z

@kripken

The runtime test will need to be performed somewhere.
The generic transformation might get quite complicated for e.g. waitAsync - wouldn't it be better at that point to introduce wrappers around these functions?

kripken · 2025-02-20T00:53:16Z

The runtime test will need to be performed somewhere.

I agree, yes, that is necessary. I am just saying that doing it in the processing pass has advantages. Specifically it will find all atomic operations, guaranteeing that we don't forget any (as you wrote above, "There are other places using Atomic.something that likely have the same issue").

The generic transformation might get quite complicated for e.g. waitAsync - wouldn't it be better at that point to introduce wrappers around these functions?

Oh, definitely, yes, we want to use wrapper functions. The processing pass should just add calls to those wrappers. That is what it does today: it replaces e.g. HEAP16[x] = a with LE_HEAP_STORE_I16(x * 2, a). All the real work happens in the called function. For more examples, see

test/js_optimizer/LittleEndianHeap.js
test/js_optimizer/LittleEndianHeap-output.js

And LE_HEAP_STORE etc. is not defined in the pass, but in the JS, see

src/lib/liblittle_endian_heap.js

I am suggesting that Atomics be implemented in a similar way.

slavek-kucera · 2025-02-20T16:40:54Z

@kripken @sbc100 Is this more in line with the direction you had in mind?

sbc100 · 2025-02-20T17:38:46Z

+   HEAPU32.unsigned = (x => x >>> 0);
+#if WASM_BIGINT
+   HEAPU64.unsigned = (x => x >= 0 ? x : BigInt(2**64) + x);
+#endif


Ideally we could avoid adding this here and keep all the code chagnes to liblittle_endian_heap.js.

Ideally we could also avoid adding extra methods to these heaps, but maybe thats not easy.

I didn't really find a better way to keep the extra methods out while not introducing per-type functions (and associated transformations).

Can you just use the uppercase version here? i.e. why do we need the new lower case versions?

I wonder if wen also move these lines to liblittle_endian_heap.js?

Perhaps they could be part of the __postset too?

re the new variables - if the uppercased variables are used they get replaced by e.g. the growable heap wrappers.

One option would be to rename updateMemoryViews to _updateMemoryViews. Assign _updateMemoryViews to updateMemoryViews and use to __postset to override the new updateMemoryViews variable.

If you prefer this option, I can look into that on Monday.

I tried this approach and it does not look any cleaner. The maybeExportHeap needs to be accessed via globalThis and there is still no good way to access to the HEAPxx variables without the acorn optimizer interfering with the code.
Additionally, the effective updateMemoryViews changes during initialization.

Regarding the HEAPXX variable substitution, we do have a list of function for which this is not performed. I don't know if that will be useful in this case but the list is here:

emscripten/tools/acorn-optimizer.mjs

Lines 1484 to 1491 in cb8d502

if (

node.id.type === 'Identifier' &&

(node.id.name.startsWith('SAFE_HEAP') ||

node.id.name === 'setValue_safe' ||

node.id.name === 'getValue_safe')

) {

// do not recurse into this js impl function, which we use during

// startup before the wasm is ready

I moved the heap update functions, I am unable to move the part that depends on maybeExportHeap.
I cannot handle the whole situation in the postset, as the updateMemoryViews is called before postset code runs.

sbc100 · 2025-02-20T17:40:05Z

+      '$LE_ATOMICS_SUB',
+      '$LE_ATOMICS_WAIT',
+      '$LE_ATOMICS_WAITASYNC',
+      '$LE_ATOMICS_XOR',


While I agree it would be good to add all of these, I also thing we might want to only add the ones that we actually use (and also test).

I thought that the whole point of this approach was to actually implement comprehensive support like was done to the non-atomic memory accesses done from JS code.

Yes, I suppose you right, seems reasonable to leave them all in then.

But lets skip 64-bit atomics and just fail in that case, to keep things simple. I don't think anyone should be doing that.

I've tested it on the following program:

#include <atomic> #include <print> #include <emscripten.h> int main(){ std::atomic<unsigned long long> x = 0x0123456789ABCDEF; std::println("x: {:X}", x.load()); EM_ASM({ Atomics.compareExchange(HEAP64, ($0)/8, BigInt("0x0123456789ABCDEF"), BigInt("0xFEDCBA9876543210")); }, &x); std::println("x: {:X}", x.load()); }

built with

em++ -o main.js -s USE_PTHREADS=1 -s PROXY_TO_PTHREAD=1 -s PTHREAD_POOL_SIZE=8 -s SUPPORT_BIG_ENDIAN=1 -pthread -s EXIT_RUNTIME=1 -s ALLOW_MEMORY_GROWTH=1 -std=c++23 main.cpp

And I do get the expected result on both LE and BE:

x: 123456789ABCDEF x: FEDCBA9876543210

sbc100 · 2025-02-20T17:53:19Z

+      else
+        return res - BigInt(2**64);
+    }
+    ),


Do we need to support more than 4 byte access? Maybe lets just skip then completely? If anyone tries to do an atomic 64-bit access they would see a crash accessing nativeByteOrder OOB.

See the post above... I think that the 8B atomics will be needed eventually for the MEMORY64.

I dont think we want to be adding complexity to support APIs that we don't use in emscripten.

That was just an example, perhaps more realistic would be just accessing 8B atomic or perhaps 2x4B pointer+counter pair value.

Sure, but until we get a request that somebody wants to to actually do that from JS then I don't think we should support it. Better to simply error out for now maybe?

Perhaps I can explain my motivation a little better: Emscripten has a lot of different settings and configurations in which it can be run. One of our biggest issues is controlling the combinatorial complexity of all these settings. We are always looks for way to avoid new options, or limit their scope. I am also always looking for settings can can be removed completely or downscoped.

This often means I will, by default, argue against new settings of the expansion of existing ones. Of course I often agree that certain settings are useful and will agree, especially if we can understand or limit their scope.

The BIG_ENDIAN support we have in emscripten has, IIUC, very few users, and almost zero testing. If posssible, I think it would be great to delete it one day, but I'm also fine with fixing bugs in it, like this one, as along is it continues to exist.

Ideally we could add some real testing on a big endian machine in order to prevent/detect regression, but that doesn't exist today.

Going back to this PR in particular, I think its great to get this bug fixed. However, I would like to limit to scope of the fix to features that emscripten actually uses. Because we don't have any usage of 64-bit atomics in JS I think we it would be reasonable to simple crash/assert if 64-bit JS atomics are used. Otherwise we are shipping code that we neither use or test.

I removed the 8B support, even though I think it is a mistake.

As for testing in CI, there is an option on e.g. Ubuntu:

apt-get install -y qemu-user libc6-s390x-cross libstdc++6-s390x-cross wget https://nodejs.org/dist/v22.14.0/node-v22.14.0-linux-s390x.tar.xz tar xJf node-v22.14.0-linux-s390x.tar.xz qemu-s390x -L /usr/s390x-linux-gnu/ node-v22.14.0-linux-s390x/bin/node program.js

Sorry I just saw this comment. This looks like a good idea. Should we do this as part of this PR, or open a bug to do this as a followup?

I suggest a follow up. It is not strictly connected to this PR.

sbc100

Nice, this looks much cleaner now.

Do you think we can find some way to test this in CI? Or do we just have to rely on folks like you to manually test it once in while? Can we perhaps run a few tests in quemu or some such?

slavek-kucera · 2025-02-25T17:05:31Z

It could be tested in CI via QEMU

apt-get install -y qemu-user libc6-s390x-cross libstdc++6-s390x-cross
wget https://nodejs.org/dist/v22.14.0/node-v22.14.0-linux-s390x.tar.xz
tar xJf node-v22.14.0-linux-s390x.tar.xz
qemu-s390x -L /usr/s390x-linux-gnu/ node-v22.14.0-linux-s390x/bin/node program.js

sbc100

lgtm, either with some testing, or a bug to add some testing as a followup.

Can you update the PR description before we land?

@kripken can you take a final look at this?

kripken

Looks good to me, thanks!

kripken · 2025-02-25T18:16:08Z

About testing, it does seem like QEMU or such is the only option. I would definitely not block on that, but it would be nice to have eventually, though only if it isn't very slow on CI.

sbc100 · 2025-02-25T18:21:48Z

About testing, it does seem like QEMU or such is the only option. I would definitely not block on that, but it would be nice to have eventually, though only if it isn't very slow on CI.

True but we should just run a handful of tests in this most (maybe just one even).

slavek-kucera force-pushed the fix_bigendian_pthread_init branch from 2dae9cd to 13d97b3 Compare February 19, 2025 08:55

sbc100 reviewed Feb 19, 2025

View reviewed changes

slavek-kucera force-pushed the fix_bigendian_pthread_init branch 2 times, most recently from 76f6d46 to 6d93671 Compare February 20, 2025 12:08

sbc100 reviewed Feb 20, 2025

View reviewed changes

slavek-kucera force-pushed the fix_bigendian_pthread_init branch 2 times, most recently from 3a3138f to 319b9e5 Compare February 24, 2025 08:19

fix: Crash during PThread initialization on big-endian machine

331431c

slavek-kucera force-pushed the fix_bigendian_pthread_init branch from 319b9e5 to 331431c Compare February 25, 2025 08:32

sbc100 reviewed Feb 25, 2025

View reviewed changes

sbc100 approved these changes Feb 25, 2025

View reviewed changes

kripken approved these changes Feb 25, 2025

View reviewed changes

sbc100 changed the title ~~fix: Crash during PThread initialization on big-endian machine~~ [bigendian] Fix crash during PThread initialization Feb 25, 2025

kripken and others added 2 commits February 25, 2025 10:28

Merge branch 'main' into fix_bigendian_pthread_init

1d77a71

Merge branch 'main' into fix_bigendian_pthread_init

97adaf4

sbc100 merged commit 10257ae into emscripten-core:main Feb 25, 2025

slavek-kucera deleted the fix_bigendian_pthread_init branch February 26, 2025 08:25

dschuff mentioned this pull request Mar 14, 2025

Temporarily disable some tests #23933

Merged

dschuff mentioned this pull request Mar 29, 2025

Revert "Temporarily disable some tests (#23933) #24011

Merged

slavek-kucera mentioned this pull request Aug 25, 2025

fix: Big endian support does not work since 4.0.9 #25042

Merged

	if (
	node.id.type === 'Identifier' &&
	(node.id.name.startsWith('SAFE_HEAP') \|\|
	node.id.name === 'setValue_safe' \|\|
	node.id.name === 'getValue_safe')
	) {
	// do not recurse into this js impl function, which we use during
	// startup before the wasm is ready

Uh oh!

Conversation

slavek-kucera commented Feb 19, 2025 • edited by sbc100 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Feb 19, 2025

Uh oh!

slavek-kucera commented Feb 19, 2025

Uh oh!

kripken commented Feb 20, 2025

Uh oh!

slavek-kucera commented Feb 20, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

slavek-kucera commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

kripken commented Feb 25, 2025

Uh oh!

slavek-kucera commented Feb 19, 2025 •

edited by sbc100

Loading

slavek-kucera commented Feb 25, 2025 •

edited

Loading