Conversation
a298ab1 to
6ab435a
Compare
d20530c to
e582eb4
Compare
c4bdb57 to
cfd9520
Compare
Although we don't have any support for external or async signal a lot of this functionality works fine within a single process (e.g. using raise the syncronously run signal handlers in your own process). I'm adding this so that as a followup we can implement `pthread_kill` to send signal between threads. See: #14872
|
|
||
| return 0; | ||
| } | ||
| // Avoid a direct call to the handler, and instead call via JS so we can |
There was a problem hiding this comment.
should we add a test with a mismatched signature below, or is posixtestsuite sufficient?
There was a problem hiding this comment.
I think we can call posixtestsuite enough.
kripken
left a comment
There was a problem hiding this comment.
I see, thanks. As you wrote them yourself, I read the code more than I would have had it been musl. Looks good, thought I am not an expert on signal corner cases, but I guess we don't really want to handle the corner cases anyhow.
| - Added some support for signal handling libc functions (raise, kill, | ||
| sigaction, sigpending, etc). We still don't have a way to deliver signals from | ||
| the outside but these at least now work for sending signals to the current | ||
| thread (JS context) (#14883). |
There was a problem hiding this comment.
This changelog note should probably be moved to 2.0.28 (since 2.0.27 was already released).
There was a problem hiding this comment.
Oops, I guess we never mark 2.0.27 as released: #14898
…ipten-core#14883) Although we don't have any support for external or async signal a lot of this functionality works fine within a single process (e.g. using raise the syncronously run signal handlers in your own process). I'm adding this so that as a followup we can implement `pthread_kill` to send signal between threads. See: emscripten-core#14872
| #define SIG_ERR ((void (*)(int))-1) | ||
| #define SIG_DFL ((void (*)(int)) 0) | ||
| #define SIG_IGN ((void (*)(int)) 1) | ||
| #define SIG_IGN ((void (*)(int))-2) /* XXX EMSCRIPTEN: use -2 since 1 is a valid function address */ |
There was a problem hiding this comment.
This change seems to be a regression for Python. Some code somewhere must assume that SIG_IGN is 1.
There was a problem hiding this comment.
Reverting this could be tricky because, as the comment says, 1 is a valid function point in Wasm.. so comparing and incoming function pointer with SIG_IGN could produce false positives.
Can you find the fix the code that contains that assumption? ..
Could it be that not all object files were re-built?
There was a problem hiding this comment.
Yeah the rational for the change is clear. I am trying to hunt down what is going on. The error reproduces in our CI so it's definitely from a clean build. But we could patch it back in Pyodide if need be.
@tiran Have you run into this problem? Is there a Python patch we can backport?
There was a problem hiding this comment.
As a last resort we could shift the range of valid functions pointers from a range that starts at 1 to range that starts at something higher, such as 16, but it seems like quite in intrusive change (and not 100% free at runtime)... so I would much rather find an fix offending code.
There was a problem hiding this comment.
It looks to me like the Python signalmodule code has a bug that is fixed in v3.11: in v3.10 in compares the memory addresses of the Python objects, and it's returning False because they are two different -2's. I suspect the Python interpreter interns 1 in some special way which makes the wrong code work by chance when the value in question is 1.
There was a problem hiding this comment.
There was a problem hiding this comment.
Okay I will apply the patch in @tiran's comment there.
There was a problem hiding this comment.
Yeah, I ran into the problem, too. PR python/cpython#31759 fixed the issue in 3.11 / main. I did not backport the fix to 3.10.
There was a problem hiding this comment.
Backports cleanly though. Thanks @tiran it's interesting rediscovering your work like this.
Although we don't have any support for external or async signal a lot of
this functionality works fine within a single process (e.g. using raise
the syncronously run signal handlers in your own process).
I'm adding this so that as a followup we can implement
pthread_killto send signal between threads. See: #14872