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

profile-handler: prefer documented sigev_notify_thread_id in sigevent #1250

Closed
wants to merge 2 commits into from

Conversation

sgn
Copy link
Contributor

@sgn sgn commented Feb 12, 2021

sigevent(7) is documented to have sigev_notify_thread_id as its member.
In glibc system, it's a macro expanded to the legacy _sigev_un._tid,
_sigev_un._tid is obviously an internal implementation detail as
signaled by its underscore prefix.

On Linux that use musl libc, sigev_notify_thread_id is also a macro, but
it's expanded to __sev_fields.sigev_notify_thread_id

Let's use the documented member if exists with a fallback to the
undocumented member instead.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@sgn
Copy link
Contributor Author

sgn commented Feb 12, 2021

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@alk
Copy link
Contributor

alk commented Feb 13, 2021

Thanks for patch. glibc actually has sigev_notify_thread_id macro, but in linux/signal.h include. Notably musl doesn't seem to have this header. So perhaps we detect presence of linux/signal.h include it and use sigev_notify_thread_id for glibc too.

@alk
Copy link
Contributor

alk commented Feb 13, 2021

Also BSD does support this macro too, using your existing include. So very nice. https://www.freebsd.org/cgi/man.cgi?query=sigevent&sektion=3&apropos=0

@sgn
Copy link
Contributor Author

sgn commented Feb 13, 2021 via email

sigevent(7) is documented to have sigev_notify_thread_id as its member.
In glibc system, it's a macro expanded to the legacy _sigev_un._tid,
_sigev_un._tid is obviously an internal implementation detail as
signaled by its underscore prefix. And this macro was hidden inside
linux/signal.h in older version of glibc.

On Linux that use musl libc, sigev_notify_thread_id is also a macro, but
it's expanded to __sev_fields.sigev_notify_thread_id

Let's use the documented member by include linux/signal.h if exists.
@alk
Copy link
Contributor

alk commented Feb 14, 2021

Hm, turns out linux/signal.h is basically unusable. It defines things that are conflicting with libc types (seeing this in both glibc and musl too). I've updated kernel bug at https://bugzilla.kernel.org/show_bug.cgi?id=200081

I think I'll take your change with minimal fix on top to unbreak it for glibc systems (since configure rightfully detects that linux/signal .h is broken and we end up with compile error trying to use that macro that is not defined anywhere). Your first change was most right one given that kernel headers bug.

@alk
Copy link
Contributor

alk commented Feb 14, 2021

Filed glibc bug as well: https://sourceware.org/bugzilla/show_bug.cgi?id=27417

alk pushed a commit that referenced this pull request Feb 14, 2021
sigevent(7) is documented to have sigev_notify_thread_id as its member.
In glibc system, it's a macro expanded to the legacy _sigev_un._tid,
_sigev_un._tid is obviously an internal implementation detail as
signaled by its underscore prefix. And this macro was hidden inside
linux/signal.h in older version of glibc.

On Linux that use musl libc, sigev_notify_thread_id is also a macro, but
it's expanded to __sev_fields.sigev_notify_thread_id

[alkondratenko@gmail.com: amputated broken linux/signal.h dependency]
[alkondratenko@gmail.com: see #1250]
Signed-off-by: Aliaksey Kandratsenka <alkondratenko@gmail.com>
@alk
Copy link
Contributor

alk commented Feb 14, 2021

Merged. Thanks a lot for the patch! Indeed musl compilation was borked without it. As noted above, I've amended your change. Making clear notice about this in commit message. Hope this is okay (I left authorship on you, but hopefully blames on bug(s) in this change I accepted on me).

@alk alk closed this Feb 14, 2021
@sgn
Copy link
Contributor Author

sgn commented Feb 15, 2021

Thanks.

AdamGoode pushed a commit to AdamGoode/gperftools that referenced this pull request Oct 11, 2022
sigevent(7) is documented to have sigev_notify_thread_id as its member.
In glibc system, it's a macro expanded to the legacy _sigev_un._tid,
_sigev_un._tid is obviously an internal implementation detail as
signaled by its underscore prefix. And this macro was hidden inside
linux/signal.h in older version of glibc.

On Linux that use musl libc, sigev_notify_thread_id is also a macro, but
it's expanded to __sev_fields.sigev_notify_thread_id

[alkondratenko@gmail.com: amputated broken linux/signal.h dependency]
[alkondratenko@gmail.com: see gperftools/gperftools#1250]
Signed-off-by: Aliaksey Kandratsenka <alkondratenko@gmail.com>
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 this pull request may close these issues.

None yet

3 participants