Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pthread + simd + fexceptions generates SIMD opcodes incompatible with Node.js in emsdk #18084

Open
RReverser opened this issue Oct 21, 2022 · 7 comments

Comments

@RReverser
Copy link
Collaborator

RReverser commented Oct 21, 2022

Please include the following in your bug report:

Version of emscripten/emsdk:
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.24 (68a9f99)
clang version 16.0.0 (https://github.com/llvm/llvm-project 277c382760bf9575cfa2eac73d5ad1db91466d3f)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /home/rreverser/emsdk/upstream/bin

Failing command line in full:

I've noticed this due to CMake detecting size of size_t as... 7:

Input

check_type_size("size_t" SIZE_T)

Output

//CHECK_TYPE_SIZE: sizeof(size_t)
SIZE_T:INTERNAL=7

That, in turn, caused all sorts of weird problems.

This is the C file CMake generated for the check:

#include <sys/types.h>
#include <stdint.h>
#include <stddef.h>


#undef KEY
#if defined(__i386)
# define KEY '_','_','i','3','8','6'
#elif defined(__x86_64)
# define KEY '_','_','x','8','6','_','6','4'
#elif defined(__ppc__)
# define KEY '_','_','p','p','c','_','_'
#elif defined(__ppc64__)
# define KEY '_','_','p','p','c','6','4','_','_'
#endif

#define SIZE (sizeof(size_t))
char info_size[] =  {'I', 'N', 'F', 'O', ':', 's','i','z','e','[',
  ('0' + ((SIZE / 10000)%10)),
  ('0' + ((SIZE / 1000)%10)),
  ('0' + ((SIZE / 100)%10)),
  ('0' + ((SIZE / 10)%10)),
  ('0' +  (SIZE    % 10)),
  ']',
#ifdef KEY
  ' ','k','e','y','[', KEY, ']',
#endif
  '\0'};

#ifdef __CLASSIC_C__
int main(argc, argv) int argc; char *argv[];
#else
int main(int argc, char *argv[])
#endif
{
  int require = 0;
  require += info_size[argc];
  (void)argv;
  return SIZE;
}

When building it with the following reduced set of flags, and then running with the Node.js v14.18.2 included in latest emsdk, this is what I'm getting:

$ emcc -O3 -pthread -fexceptions -msimd128 SIZE_T.c
$ node --experimental-wasm-threads --experimental-wasm-bulk-memory --experimental-wasm-simd a.out.js
failed to asynchronously prepare wasm: CompileError: WebAssembly.instantiate(): Compiling function #50:"n" failed: invalid simd opcode @+16117
Aborted(CompileError: WebAssembly.instantiate(): Compiling function #50:"n" failed: invalid simd opcode @+16117)
[...long output...]
$ echo $?
7 # this is where CMake takes what it assumes to be `sizeof(size_t)`

There are couple of interesting things going on:

  1. Removing any of those 3 emcc flags fixes the issue, so there's some weird combo issue going on. (replacing -fexceptions with -fwasm-exceptions works too, but not something I can do here)
  2. Running with newer Node.js I have on my system works fine and returns 4 as expected, so this might be due to old Node.js having old SIMD opcodes. In that case, I'd argue it's time to upgrade Node.js on the emsdk side.
  3. Apparently the upstream CMake performs size check differently, in a way where exit code should never silently propagate as a valid result. It's worth looking into improving error handling here too.
RReverser added a commit to RReverser/wasm-vips that referenced this issue Oct 21, 2022
-fexceptions causes some issues: emscripten-core/emscripten#18084

After trial and error it seems that moving them to apply only to C++ (via CXXFLAGS) works best.
@kleisauke
Copy link
Collaborator

kleisauke commented Oct 22, 2022

The Wasm SIMD opcodes were finalized in V8 9.1.54, which corresponds to Node.js 16.4.0. So, indeed, emsdk needs to update its Node version (emscripten-core/emsdk#1064).

After PR #18070, Emscripten's test suite only adds those --experimental-wasm-* flags for older Node versions, so that's no longer a blocker. I would also argue for setting NODEJS_CATCH_REJECTION to 0 by default, when the bundled Node version is updated (to avoid issues like #17228).

FWIW, I noticed the commit linked above, another workaround would be to do something similar to this in the Dockerfile:

wget https://nodejs.org/dist/v19.0.0/node-v19.0.0-linux-x64.tar.xz
tar xf node-v19.0.0-linux-x64.tar.xz
echo "export EM_JS_ENGINES=$HOME/node-v19.0.0-linux-x64/bin/node" >> $BASH_ENV
echo "export EM_NODE_JS=$HOME/node-v19.0.0-linux-x64/bin/node" >> $BASH_ENV
echo "export PATH=\"$HOME/node-v19.0.0-linux-x64/bin:\$PATH\"" >> $BASH_ENV

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 22, 2022

FWIW, I noticed the commit linked above, another workaround would be to do something similar to this in the Dockerfile:

Might as well install Node from NodeSources apt :) Either way, it's only used for CMake checks so which workaround to use doesn't matter too much, but the issues above still need to be fixed on Emscripten side for other users.

kleisauke added a commit to kleisauke/wasm-vips that referenced this issue Oct 26, 2022
RReverser added a commit to RReverser/wasm-vips that referenced this issue Oct 28, 2022
-fexceptions causes some issues: emscripten-core/emscripten#18084

After trial and error it seems that moving them to apply only to C++ (via CXXFLAGS) works best.
@dschuff
Copy link
Member

dschuff commented Nov 3, 2022

It would be nice to update node in emsdk. See emscripten-core/emsdk#877 and emscripten-core/emsdk#947 for some context on why we haven't recently. I wonder if those reasons still apply now.

@RReverser
Copy link
Collaborator Author

Looking at the first link, @juj said:

Currently it looks like we need Windows 7 support until April 2022. After that I expect we won't need it anymore, since our LTS versions will rotate, and the next LTS won't support Windows 7 anymore.

Sounds like it should be safe to upgrade by now?

I still wonder why pthread and exception flags matter for even this simple C program that uses neither of those features, but that's just curiosity and doesn't matter much since the Node.js fixes it anyway.

What about part (3) of my issue, about not using exit code for Cmake check, should I extract that into a separate issue? Having sizeof SIZE_T detected as 7 was preeetty weird to debug 😅

@sbc100
Copy link
Collaborator

sbc100 commented Nov 4, 2022

Yes I think the exit code issue might be worth digger deeper into. I think the separate issue would make sense.

I imagine that what we want to do is make this kind of failure look like a crash to the host OS, rather than an exit(7)?

RReverser added a commit to RReverser/wasm-vips that referenced this issue Nov 4, 2022
-fexceptions causes some issues: emscripten-core/emscripten#18084

After trial and error it seems that moving them to apply only to C++ (via CXXFLAGS) works best.
@abram
Copy link

abram commented Nov 15, 2022

What about part (3) of my issue, about not using exit code for Cmake check, should I extract that into a separate issue? Having sizeof SIZE_T detected as 7 was preeetty weird to debug 😅

+1 to this. I've run into this issue twice recently and it was painful (see #17811). I don't see an issue for this yet, are you still planning to file one @RReverser?

@RReverser
Copy link
Collaborator Author

@abram Sorry, no, didn't get around to it. Created one now: #18278

sbc100 added a commit that referenced this issue Dec 2, 2022
Update the CheckTypeSize files to v3.20.0 (mostly arbitrary version).

Local patch to CheckTypeSize is not much smaller.  Instead of executing
the program and using the return value (doesn't work for return values
greater than 255), we simply inject `-oformat=wasm` onto the link
command, and then we can use `strings` on the binary just like the
upstream check.  This is basically a single line patch against upstream
now.

Fixes: #18278 #18238 #17268 #18084 #17811
sbc100 added a commit that referenced this issue Dec 2, 2022
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but
this is what we use in CI).

Local patch to CheckTypeSize is not much smaller.  Instead of executing
the program and using the return value (doesn't work for return values
greater than 255), we simply inject `-oformat=wasm` onto the link
command, and then we can use `strings` on the binary just like the
upstream check.  This is basically a single line patch against upstream
now.

Fixes: #18278 #18238 #17268 #18084 #17811
sbc100 added a commit that referenced this issue Dec 5, 2022
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but
this is what we use in CI).

Local patch to CheckTypeSize is not much smaller.  Instead of executing
the program and using the return value (doesn't work for return values
greater than 255), we simply inject `-oformat=wasm` onto the link
command, and then we can use `strings` on the binary just like the
upstream check.  This is basically a single line patch against upstream
now.

Fixes: #18278 #18238 #17268 #18084 #17811
sbc100 added a commit that referenced this issue Dec 5, 2022
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but
this is what we use in CI).

Local patch to CheckTypeSize is not much smaller.  Instead of executing
the program and using the return value (doesn't work for return values
greater than 255), we simply inject `-oformat=wasm` onto the link
command, and then we can use `strings` on the binary just like the
upstream check.  This is basically a single line patch against upstream
now.

This change also avoids a second issue which is that node failures (non-zero
return codes) are indistinguishable from non-zero return codes from user
code.

Fixes: #18278 #18238 #17268 #18084 #17811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants