-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/6.0] The signal enum in the native library should match the managed code. #58682
Conversation
PosixSignalSIGWINCH = -6, | ||
PosixSignalSIGCONT = -7, | ||
PosixSignalSIGCONT = -6, | ||
PosixSignalSIGWINCH = -7, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, these values map to
runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignal.cs
Lines 28 to 33 in 721a802
[UnsupportedOSPlatform("windows")] | |
SIGCONT = -6, | |
/// <summary>Window resized</summary> | |
[UnsupportedOSPlatform("windows")] | |
SIGWINCH = -7, |
cc: @danmoseley |
@stephentoub @tmds should we pull #58711 into this also? |
Either way is fine. I wouldn't block fixing the bug on getting that test pulled in as well. |
would you mind cherry picking it in? it will only hold us up a couple hours. |
Approved otherwise -- localized fix for self evidently wrong code. |
* PosixSignalRegistrationTests.Unix: validate signal pal mapping * Use InvariantCulture for pid ToString * Invoke kill through shell. * Remove InvariantCulture
Done |
@mdh1418 could you please port into 6.0 the fix for @steveisok I'm not sure what to make of the test jobs that just terminated on iOSSimulator and tvOSSimulator. how can we make these diagnosable? I see no dump or relevant info in any of the logs -- is there any more info we can gather than:
I'm going to merge this though as I can't see how it could have failures specific to just these. |
@danmoseley We have a few commits that @mdh1418 is working to backport that will clean these up. And to answer your question, there are about 7-8 suites that crash immediately for unknown reasons and seemingly only on CI. We will be skipping them for now until we're able to get a better understanding as to why. |
Sounds good thanks @steveisok |
#58841 Backport PR is open :) ! |
Backport of #58658 to release/6.0
/cc @stephentoub @JamesWTruher
Customer Impact
Trying to register for SIGWINCH (window resize) will actually register for SIGCONT (making a suspend process continue), and vice versa.
Testing
CI (a new test was added separately/subsequently to validate our mappings).
Risk
Minimal. It's obvious from code inspection that the code was wrong and is now correct.