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

Support sigaction with SIG_DFL and SA_SIGINFO. #55673

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

tmds
Copy link
Member

@tmds tmds commented Jul 14, 2021

@stephentoub
Copy link
Member

@janvorli, could you review this one as well? Thanks!

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the nit.

// macOS can return sigaction with SIG_DFL and SA_SIGINFO.
// SA_SIGINFO means we should use sa_sigaction, but here we want to check sa_handler.
// So we ignore SA_SIGINFO when sa_sigaction and sa_handler are at the same address.
return (&action->sa_handler == (void*)&action->sa_sigaction || !IsSaSigInfo(action)) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit - can you please add ( ) to explicitly express the precedence in all the composite conditions? It is a preferred style in this codebase. Like this:

return ((&action->sa_handler == (void*)&action->sa_sigaction) || !IsSaSigInfo(action)) &&
(action->sa_handler == SIG_DFL));

@danmoseley
Copy link
Member

Linux failure is a SIGABRT in the Http tests on Linux. I am not sure how to resolve the frames:

0:000> kp
 # Child-SP          RetAddr               Call Site
00 00007f4b`40cdf430 00007f4b`44cd294d     libc_2_31!gsignal+0x145
01 00007f4b`40cdf550 00007f4b`2eedf479 (T) libc_2_31!abort+0x1df
02 00007f4b`40cdf680 00007f4b`2eecd109     libmsquic!MsQuicOpenVersion+0x607c4
03 00007f4b`40cdf690 00007f4b`2eea1a60     libmsquic!MsQuicOpenVersion+0x4e459
04 00007f4b`40cdf6c0 00007f4b`2ee9a3c5     libmsquic!MsQuicOpenVersion+0x22db0
05 00007f4b`40cdf6d0 00007f4a`cb499e12     libmsquic!MsQuicOpenVersion+0x1b715
06 00007f4b`40cdf7f0 00000000`00000000     0x00007f4a`cb499e12

Seems unlikely it is related.

@danmoseley
Copy link
Member

Because this is breaking Xamarin iOS, and tomorrow is Preview 7 sync, I will merge this and @tmds can perhaps fix the code formatting separately. I hope that is OK.

I am assuming the Http crash is not related.

@danmoseley
Copy link
Member

Oh, I didn't see that one of the arm64 configs timed out. I'll rerun that.

@ghost
Copy link

ghost commented Jul 15, 2021

Hello @danmoseley!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@steveisok steveisok self-requested a review July 15, 2021 02:08
@steveisok
Copy link
Member

Github and azdo are out of sync. Looks like the build has finished successfully.

@steveisok steveisok merged commit 30257e6 into dotnet:main Jul 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Launching dotnet using mono on macOS will hang if dotnet launches processes
7 participants