Implement default signal handlers #20257

RReverser · 2023-09-14T21:19:27Z

This implements best-effort attempt to implement default signal handlers as per https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html:

Terminate action is mapped to _Exit aka wasi_proc_exit - variant of exit that doesn't run atexit handlers. Exit code is set to 128 + sig like it is on other platforms.
Abort action (terminate with core dump) is mapped to __builtin_trap aka Wasm unreachable instruction.
Process pause/continue actions are ignored as it's unclear what that would do in Wasm (maybe use Asyncify in the future?).

Fixes #20254.

RReverser · 2023-09-14T21:19:53Z

@sbc100 Are there any tests from POSIX test suite we could reuse here or do I need to implement my own?

sbc100 · 2023-09-14T21:30:51Z

@sbc100 Are there any tests from POSIX test suite we could reuse here or do I need to implement my own?

Except for the pthreads tests we have been writing out own. (we also write our own pthreads tests in addition to the posttest suite ones).

sbc100 · 2023-09-14T21:31:33Z

I think you can/should just extend the existing test/test_signals.c

sbc100

Nice! lgtm with test

RReverser · 2023-09-14T21:34:34Z

I think you can/should just extend the existing test/test_signals.c

I don't think that will work since it's being executed as a core test and the program is expected to finish. Whereas here we want kinda the opposite - to test that it exits abnormally.

RReverser · 2023-09-14T21:35:47Z

OTOH... it does verify output, so maybe.

sbc100 · 2023-09-14T21:37:05Z

OTOH... it does verify output, so maybe.

You can also death tests using assert_returncode=NON_ZERO if you want.

RReverser · 2023-09-14T21:39:30Z

Yeah it just gets a bit more complicated if I want to test several signals (one for abort, one for terminate behaviour)... I could do that via argument parsing but I think I'd better add a separate test instead.

RReverser · 2023-09-14T22:23:51Z

Heh looks like it caught a test that assumed SIGUSR1 will do nothing. I'll change it to SIGCHLD which is ignored per spec.

RReverser · 2023-09-14T22:51:42Z

Just realised that this is not fully correct approach. It should instead implement WASI's proc_raise so that it's compatible both with this emulated approach and with standalone engines.

I think I'll just wrap it into #ifndef EMSCRIPTEN_STANDALONE_WASM for now right in the C code + add alternative branch.

RReverser · 2023-09-14T23:12:58Z

Hm #ifndef EMSCRIPTEN_STANDALONE_WASM code seems to get compiled even when using -s PURE_WASI or -s STANDALONE_WASM... Do I need another ifdef?

RReverser · 2023-09-14T23:14:30Z

Oh I see, looks like libc is not built separately for standalone Wasm as it expects that WASI will be implemented only in JS? I guess I can move this implementation into JS, but seems unfortunate...

sbc100 · 2023-09-14T23:56:23Z

Why don't you ignore the standalone wasm stuff for now and just land this without that support. We can try to move to an external proc_raise as a followup maybe?

RReverser · 2023-09-15T00:10:10Z

I guess I could do that. It's just that when I noticed that CI error I decided to test my tests in standalone Wasm mode as well and it was a rabbit hole.

That said, I'm almost done - but now noticed that Wasmtime doesn't support proc_raise yet so I'll have to disable it as a runner anyway 😅

sbc100 · 2023-09-15T00:22:37Z

I guess I could do that. It's just that when I noticed that CI error I decided to test my tests in standalone Wasm mode as well and it was a rabbit hole.

That said, I'm almost done - but now noticed that Wasmtime doesn't support proc_raise yet so I'll have to disable it as a runner anyway 😅

We use an old/pinned version of wasmtime in CI .. it might already be fixed.

That being said, I'd prefer to land the smaller/simpler version of this change first, is possible.

RReverser · 2023-09-15T00:27:03Z

We use an old/pinned version of wasmtime in CI .. it might already be fixed.

No, I already checked Github.

That being said, I'd prefer to land the smaller/simpler version of this change first, is possible.

I could revert the JS change I guess... It just seemed better to do this the right way from the start rather than have to rewrite later.

The only complicated addition is that I also added all the signal codes to generated struct info. I needed this for the --js-library anyway and was planning to send as a PR, but now that it's needed by proc_raise implementation too, might as well kill two birds with one stone.

RReverser · 2023-09-15T00:29:53Z

Ah interesting... apparently they want to remove proc_raise from WASI? Weird but okay. I guess in that case it's better to revert to C implementation after all... bytecodealliance/wasmtime#420

Or should we keep it? What do you think?

RReverser · 2023-09-15T00:31:40Z

One benefit of the new implementation is that it uses abort with strsignal and can print a nice message. But otherwise I don't care either way.

sbc100

Fair enough. lgtm!

I wonder if there is an open bug for wasmtime? Seems odd they wouldn't support it.

sbc100 · 2023-09-15T00:36:27Z

Run test_gen_struct_info --rebase to regenerate the struct info files.

RReverser · 2023-09-15T00:38:14Z

I wonder if there is an open bug for wasmtime? Seems odd they wouldn't support it.

See the link above - apparently they didn't implement it because it didn't make sense and they want to remove it from WASI at some point in the future instead. But I don't get it - for this scenario (invoking default handler) it seems to make perfect sense IMO.

Run test_gen_struct_info --rebase to regenerate the struct info files.

Oh, weird that it worked locally but fails on CI. Will try.

RReverser · 2023-09-15T00:39:54Z

I don't see test_gen_struct_info. Did you mean tools/gen_struct_info? If so, I did run it.

sbc100 · 2023-09-15T00:40:08Z

I wonder if there is an open bug for wasmtime? Seems odd they wouldn't support it.

See the link above - apparently they didn't implement it because it didn't make sense and they want to remove it from WASI at some point in the future instead. But I don't get it - for this scenario (invoking default handler) it seems to make perfect sense IMO.

Right, but you might as well implement it in wasm though, right? I don't see any advantage to having the host do it.. is there one?

Run test_gen_struct_info --rebase to regenerate the struct info files.

Oh, weird that it worked locally but fails on CI. Will try.

sbc100 · 2023-09-15T00:40:44Z

I don't see test_gen_struct_info. Did you mean tools/gen_struct_info? If so, I did run it.

Sorry ./test/runner other.test_gen_struct_info --rebase

RReverser · 2023-09-15T00:43:00Z

Right, but you might as well implement it in wasm though, right? I don't see any advantage to having the host do it.. is there one?

In this particular case I wanted to do it in the host because Wasmtime prohibits from exiting with exit code >127. And signals are supposed to exit with 128+sig like I did in my implementation, so my test failed when executed in Wasmtime.

I thought, okay, then I'll use proper WASI method for that, I assume proc_raise will set the correct exit code while also making my implementation simpler for standalone case... but nope, because proc_raise is not implemented altogether.

Turned out to be waste of time, one way or another if we want to support Wasmtime, we'll have to change exit code just for them.

RReverser · 2023-09-15T00:43:49Z

But yeah, to your general question, if Wasmtime didn't prohibit such exit codes, I think implementing in Wasm would be fully analogous.

RReverser · 2023-09-15T00:53:30Z

I reverted to non-JS implementation to keep things simple; I'll submit the cDefs thing as a separate PR.

sbc100 · 2023-09-15T00:58:13Z

Awesome! Thanks

test/test_core.py

This implements best-effort attempt to implement default signal handlers as per https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html: - Terminate action is mapped to `_Exit` aka `wasi_proc_exit` - variant of exit that doesn't run `atexit` handlers. - Abort action (terminate with core dump) is mapped to `__builtin_trap` aka Wasm `unreachable` instruction. - Process pause/continue actions are ignored as it's unclear what that would do in Wasm (maybe use Asyncify in the future?). Fixes #20254.

SIGTRAP is not available on Windows.

Deleted during rebase?

RReverser · 2023-09-15T15:39:49Z

Sigh, anotyer failing test. This was supposed to be a trivial PR 😂

sbc100 · 2023-09-15T15:43:03Z

The test_pthread_proxying_refcount is a known flake

test/other/test_standalone_syscalls.cpp

RReverser · 2023-09-15T17:28:25Z

More unrelated tests failing. @sbc100 can you please force-merge it for me?

sbc100 · 2023-09-15T17:30:13Z

Yup.. I have fixed for the current failures: #20263

kripken · 2023-09-18T16:36:08Z

test_sigaction_default_sigabrt is now failing consistently on the chromium roll,

Somehow this rolled in, but I can't figure that part out... this is the roll with this PR:

https://chromium-review.googlesource.com/c/emscripten-releases/+/4868242?tab=checks

But that test was not run..? Very strange. It only starts to be run right after it, and immediately fails.

If you don't have a quick idea for a fix here, can we perhaps disable that one test while we investigate?

sbc100 · 2023-09-18T16:45:17Z

Fix in flight here: #20272

kripken · 2023-09-18T17:21:06Z

Thanks for the quick fix!

Any idea how this passed the initial chromium roll, btw? That's really mysterious... it's like chromium CI ran the wrong checkout for the tests. cc @dschuff

sbc100 · 2023-09-19T00:06:12Z

Its very odd that the try job succeeded yes. Try job only runs 907 tests but the normal runner runs 909 tests in core0. Very odd

sbc100 · 2023-09-19T00:09:14Z

The trybot does seem to sync the correct emscripten rev:

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8769867725819477137/+/u/bot_update/stdout

_____ emscripten-releases\emscripten at aab180a72739c4865d3e9b4c4403991f45b5f572

aab180a is Implement default signal handlers (#20257)

RReverser requested a review from sbc100 September 14, 2023 21:19

sbc100 approved these changes Sep 14, 2023

View reviewed changes

sbc100 reviewed Sep 15, 2023

View reviewed changes

RReverser force-pushed the default-signal-handelrs branch from 558785d to 8fba77e Compare September 15, 2023 00:45

sbc100 enabled auto-merge (squash) September 15, 2023 00:49

sbc100 reviewed Sep 15, 2023

View reviewed changes

test/test_core.py Outdated Show resolved Hide resolved

sbc100 reviewed Sep 15, 2023

View reviewed changes

test/test_core.py Outdated Show resolved Hide resolved

RReverser added 6 commits September 15, 2023 12:22

Add default signal handler tests

3d23d08

Change WASI test to use SIGCHLD

140497a

Switch to SIGABRT

ddea2cc

SIGTRAP is not available on Windows.

Lint fix

1581bf3

Simplify & rename test

89166d1

RReverser force-pushed the default-signal-handelrs branch from b6201e7 to 89166d1 Compare September 15, 2023 11:22

Re-add test_sigaction_default.out

b6dddad

Deleted during rebase?

sbc100 reviewed Sep 15, 2023

View reviewed changes

test/other/test_standalone_syscalls.cpp Outdated Show resolved Hide resolved

sbc100 approved these changes Sep 15, 2023

View reviewed changes

Fix typo in test_standalone_syscalls.cpp

dfca3a2

sbc100 disabled auto-merge September 15, 2023 17:29

sbc100 merged commit aab180a into main Sep 15, 2023

sbc100 deleted the default-signal-handelrs branch September 15, 2023 17:30

sbc100 mentioned this pull request Sep 18, 2023

Fix test_sigaction_default on windows. NFC #20272

Merged

RReverser mentioned this pull request Oct 16, 2023

Termios addon to allow for more typical terminal behavior xtermjs/xterm.js#3394

Open

vadimkantorov mentioned this pull request Nov 27, 2024

Add WebAssembly as supported platform for Perl build Perl/PPCs#61

Open

Implement default signal handlers #20257

Implement default signal handlers #20257

Uh oh!

Conversation

RReverser commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

sbc100 commented Sep 14, 2023

Uh oh!

sbc100 commented Sep 14, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

sbc100 commented Sep 14, 2023

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

RReverser commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

RReverser commented Sep 14, 2023

Uh oh!

sbc100 commented Sep 14, 2023

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

Uh oh!

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

Uh oh!

RReverser commented Sep 15, 2023

Uh oh!

sbc100 commented Sep 15, 2023

Uh oh!

kripken commented Sep 18, 2023

RReverser commented Sep 14, 2023 •

edited

Loading

RReverser commented Sep 14, 2023 •

edited

Loading

RReverser commented Sep 15, 2023 •

edited

Loading

RReverser commented Sep 15, 2023 •

edited

Loading

kripken commented Sep 18, 2023 •

edited

Loading