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

WASI signal numbers out of sync with wasi-libc signal numbers #272

Closed
vapier opened this issue Mar 21, 2022 · 15 comments · Fixed by #278
Closed

WASI signal numbers out of sync with wasi-libc signal numbers #272

vapier opened this issue Mar 21, 2022 · 15 comments · Fixed by #278

Comments

@vapier
Copy link

vapier commented Mar 21, 2022

if we compare wasi-libc/libc-top-half/musl/arch/wasm32/bits/signal.h with wasi/api.h, we see wasi-libc defines SIGSTKFLT as signal 16, but this signal does not exist in the WASI spec at all. this causes all remaining signals to be off-by-one.

for example, the first few around the desync:

signal wasi-libc wasi API
SIGPIPE 13 13
SIGALRM 14 14
SIGTERM 15 15
SIGSTKFLT 16 -
SIGCHLD 17 16
SIGCONT 18 17
SIGSTOP 19 18

this makes working with signals kind of a pita as we have to maintain 2 tables and their mappings between them. from a simplicity pov, wasi-libc should just use the same set of signals & signal numbers as wasi api and call it a day.

@sbc100
Copy link
Member

sbc100 commented Mar 21, 2022

Can I ask what you using those macros for? __wasi_proc_raise (along with all the signal macros) was removed from WASI back in 2019: WebAssembly/WASI#136. Given that, I think it might make more sense to simply remove these macros from signal.h.

@vapier
Copy link
Author

vapier commented Mar 21, 2022

the public WASI API still shows signals:
https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md

and wasi-libc provides a signal.h still in latest git (along with emulated code). i'm not sure what you mean by "remove these macros from signal.h" -- if signal.h doesn't provide signal numbers, then there's no point in providing any signal functions, and at that point, why provide a signal.h at all ?

the project i'm working on relies on SIGWINCH to get keep track of terminal size changes. so when that happens, the next time the app hits a WASI interface (and transitions to the JS side), i call back into the app to run the registered SIGWINCH handler. having the emulated signal APIs has helped so i didn't have to write all the stubs myself.

@sbc100
Copy link
Member

sbc100 commented Mar 21, 2022

Can you use case be supported solely by emulation code in libc? Or does it really the (since removed) __wasi_proc_raise function import?

Assuming it can all be done in emulation at the libc layer perhaps we can just remove the signal related stuff from our local copy of wasi/api.h (or order to remove this conflict)?

@sunfishcode
Copy link
Member

Yes, signals are meant to be removed from WASI, since WebAssembly itself doesn't support signal delivery. So yes, it seems we should use the values in libc-top-half/musl/arch/wasm32/bits/signal.h and delete the values in libc-bottom-half/headers/public/wasi/api.h.

@vapier
Copy link
Author

vapier commented Mar 21, 2022

Can you use case be supported solely by emulation code in libc?

no, because the terminal isn't in WASM. it's a web component. it being resized is an async event. rewriting the code to constantly poll "has this been resized" means a lot of wasted CPU cycles.

Or does it really the (since removed) __wasi_proc_raise function import?

i'm not using that API, so its removal doesn't matter to me ;). i've implemented my own delivery mechanism (app exports a symbol for the JS side to callback into). it's ugly which is why i'm not asking for it to be generalized.

Yes, signals are meant to be removed from WASI, since WebAssembly itself doesn't support signal delivery.

fwiw, i think a signalfd approach would be feasible and wouldn't get into any async/messaging concerns.

@sbc100
Copy link
Member

sbc100 commented Mar 21, 2022

Can you use case be supported solely by emulation code in libc?

no, because the terminal isn't in WASM. it's a web component. it being resized is an async event. rewriting the code to constantly poll "has this been resized" means a lot of wasted CPU cycles.

Sorry, I guess what I was really asking here was: are you dependent on any of the removed/legacy WASI signal stuff in wasi/api.h?

It sounds like you are not, and you have instead rolled you own signal delivery mechanism outside of WASI? Assuming that is true I think we can just remove the signal stuff from wasi/api.h to resolve this issue.

@vapier
Copy link
Author

vapier commented Mar 21, 2022

I'm fine if both places drop signal logic. the stubs are not significant or complicated.

@sbc100
Copy link
Member

sbc100 commented Mar 21, 2022

Actually, updating wasi/api.h is non-trivial since its auto-generated from the preview1 witx file.. so we could need to first update that file. Unless we decide its very important to fix this inconsistency it might be simplest to let it be.

@vapier
Copy link
Author

vapier commented Mar 22, 2022

it makes using WASI difficult when the API/SDK/libc are out sync, and people don't fix bugs under the guise "it'll probably resolve itself eventually ... somehow"

even the latest WASI API still defines signals. removing the raise syscall doesn't change that.

@sunfishcode
Copy link
Member

I'm optimistic we can find a way to remove the signal values from wasi/api.h without too much trouble.

@sbc100
Copy link
Member

sbc100 commented Mar 22, 2022

even the latest WASI API still defines signals. removing the raise syscall doesn't change that.

I'm not sure what you mean, WebAssembly/WASI#136 didn't just remove the raise syscall, it also removed the entire enumeration of signals.

@vapier
Copy link
Author

vapier commented Mar 22, 2022

sorry, my checkout was still stuck on master. not sure when it migrated to main, but there's still a bunch of references to master in there.

at any rate, the snapshot still talks about signals, and it continues to have changes merged that aren't merged into ephemeral (like sock_accept), so it's not clear which API docs one should be reading. which means falling back to wasi/api.h which still has signals in it ...

@sunfishcode
Copy link
Member

WASI is in a transitional state right now. The goal has always been for WASI to build on underlying WebAssembly standards, and avoid having its own competing ways of doing things. A lot of the things we started building for WASI weren't initially covered by other standards, such as the snapshot-style versioning scheme, "new-style commands", weak symbols, the witx tooling, and so on, however in part due to WASI's experience with these things, these are now being subsumed by lower layers, such as interface types, module linking, and wit tooling. That said, some of these things are still evolving, and the tooling is not quite ready for things like wasi-libc, and this puts WASI in a somewhat awkward position where we aren't working on major changes to the old systems because we're expecting them to be replaced, and the new systems aren't here yet.

All that said, I think there are things we can do better within the old systems to make them better for people using them today, and I think cleaning up the signal numbers is one of those things we can do. I'll work on a PR to propose a way to do this.

@vapier
Copy link
Author

vapier commented Mar 22, 2022

oblig https://goomics.net/50/

from a user's pov, it's hard to square things. the commit to remove signal logic landed over two years ago. presumably debate & review started even before that. yet the wasi-sdk release from a few months ago still has it (in the API & libc). i guess changing the API is a "major change" which is why it wasn't included, but it also sees llvm release upgrades which i would call "major". new changes to wasi-libc are still landing too.

i grok that WASI is breaking new ground here, and there are growing pains. the messaging is a bit inconsistent, so i don't really know what to expect. https://wasi.dev points to examples & outdated repos & outdated branches. are bug reports for wasi-libc & wasi-sdk & WASI API not desirable ? they're are in a frozen state until WASI API figures things out ?

wasi-sdk-14 seems to have all the functionality i need at this point, and i doubt WASM will break backwards compat with it. so maybe i should be freezing to that API & SDK release, bugs/idiosyncrasies & all, and check back in on WASI API every year to see when it's usable again ? if there's any functionality missing, i'll keep adding my own APIs in a parallel namespace. i've done this for signals, sockets, termios, and some basic file/fd stuff, so it's not a blocker for me.

@sunfishcode
Copy link
Member

Yes, I'm not trying to say it's amazing; I'm just trying to describe the current situation. If it looks like fallible humans doing their best to build a coherent system while the foundation underneath them is radically changing, it's because that's what it is. The existing code and docs aren't explicitly frozen, but we don't have many people volunteering to help maintain them, so we have limited resources and we're doing the best we can.

sunfishcode added a commit that referenced this issue Mar 23, 2022
The WASI signal constants and proc_raise function were removed in the
latest [ephemeral], which had been scheduled to be in the next snapshot,
however WASI itself is now transitioning away from the snapshot system.

WASI libc will also be transitioning to updated wit specs once they're
ready, however until that time, we can make the simple change of
removing these signal constants to avoid confusion.

Fixes #271.
Fixes #272.

[ephemeral]: https://github.com/WebAssembly/WASI/tree/main/phases/ephemeral/witx
sunfishcode added a commit that referenced this issue Mar 28, 2022
The WASI signal constants and proc_raise function were removed in the
latest [ephemeral], which had been scheduled to be in the next snapshot,
however WASI itself is now transitioning away from the snapshot system.

WASI libc will also be transitioning to updated wit specs once they're
ready, however until that time, we can make the simple change of
removing these signal constants to avoid confusion.

Fixes #271.
Fixes #272.

[ephemeral]: https://github.com/WebAssembly/WASI/tree/main/phases/ephemeral/witx
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

Successfully merging a pull request may close this issue.

3 participants