Skip to content

[libc][signal] clean up usage of sighandler_t #125745

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 7, 2025
Merged

[libc][signal] clean up usage of sighandler_t #125745

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
62e8a9c
[libc][signal] clean up sighandler_t declarations
nickdesaulniers Feb 4, 2025
c84b8f2
clean up our usage of sighandler_t
nickdesaulniers Feb 4, 2025
669bb96
only include __sighandler_t.h on linux
nickdesaulniers Feb 4, 2025
19e518a
update signal.h hdrgen
nickdesaulniers Feb 4, 2025
d486708
reformat
nickdesaulniers Feb 4, 2025
5864bf7
add dependency so include/llvm-libc-types/__sighandler_t.h gets copied
nickdesaulniers Feb 4, 2025
3730708
remove any typedef for __sighandler_t, rename files
nickdesaulniers Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libc/include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,9 @@ add_header_macro(
signal.h
DEPENDS
.llvm-libc-macros.signal_macros
.llvm-libc-types.__sighandler_t
.llvm-libc-types.pid_t
.llvm-libc-types.sig_atomic_t
.llvm-libc-types.sighandler_t
.llvm-libc-types.siginfo_t
.llvm-libc-types.sigset_t
.llvm-libc-types.stack_t
Expand Down
2 changes: 1 addition & 1 deletion libc/include/llvm-libc-types/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ add_header(__pthread_start_t HDR __pthread_start_t.h)
add_header(__pthread_tss_dtor_t HDR __pthread_tss_dtor_t.h)
add_header(__qsortcompare_t HDR __qsortcompare_t.h)
add_header(__qsortrcompare_t HDR __qsortrcompare_t.h)
add_header(__sighandler_t HDR __sighandler_t.h)
add_header(__thread_type HDR __thread_type.h)
add_header(blkcnt_t HDR blkcnt_t.h)
add_header(blksize_t HDR blksize_t.h)
Expand Down Expand Up @@ -65,6 +64,7 @@ if(LIBC_TYPES_TIME_T_IS_32_BIT)
else()
add_header(time_t HDR time_t_64.h DEST_HDR time_t.h)
endif()
add_header(sighandler_t HDR sighandler_t.h)
add_header(stack_t HDR stack_t.h DEPENDS .size_t)
add_header(suseconds_t HDR suseconds_t.h)
add_header(struct_flock HDR struct_flock.h DEPENDS .off_t .pid_t)
Expand Down
19 changes: 0 additions & 19 deletions libc/include/llvm-libc-types/__sighandler_t.h
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions libc/include/llvm-libc-types/sighandler_t.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//===-- Definition of sighandler_t ----------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_LIBC_TYPES_SIGHANDLER_T_H
#define LLVM_LIBC_TYPES_SIGHANDLER_T_H

#ifdef __linux__
Copy link
Contributor

@enh-google enh-google Feb 5, 2025

Choose a reason for hiding this comment

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

sure you don't want a _GNU_SOURCE or whatever your internal equivalent is? (both glibc and bionic have __USE_GNU as the post-<features.h>-inclusion internal equivalent.)

Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 5, 2025

Choose a reason for hiding this comment

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

No, we don't currently hold folk's code hostage based on them defining _GNU_SOURCE or not, and I don't plan to add it.

Copy link
Contributor

@enh-google enh-google Feb 6, 2025

Choose a reason for hiding this comment

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

i flirted with that myself for bionic, but it's a bit more complicated in practice --- "what when the GNU variant conflicts with the POSIX one?", say, or "what when people are trying to build code that's incompatible with GNU extensions [because they use some of the names themselves, say]"...

(the weird one for Android is _BSD_SOURCE where by historical accident that's always on. but _GNU_SOURCE is something developers are used to turning on/off either via -std=gnu23 vs -std=c23 or -D/#define.)

Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 6, 2025

Choose a reason for hiding this comment

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

I was thinking more about this last night, and qsort_r came to mind.

So I guess I should change my stance slightly from "never" to "perhaps when there is a conflict."

Is there a conflict here? Well, sig_t and sighandler_t don't conflict with each other, and the issue isn't as severe as qsort_r. But Bionic provides both without requiring users to pledge allegiance; if bionic doesn't llvm-libc should not either (IMO).

I learned it from you, Dad! /runs away crying

But I will keep that in mind that _GNU_SOURCE and _BSD_SOURCE do exist more for than just pledges of allegiance and holding user code hostage, and that we may need to deploy those in llvm-libc in the future. I still don't think we need to here (yet; famous last words).

but _GNU_SOURCE is something developers are used to turning on/off either via -std=gnu23 vs -std=c23

Wait...what?!

$ clang -std=gnu++23 --target=x86_64-linux-gnu -dM -E -x c++ /dev/null | grep GNU_SOURCE
#define _GNU_SOURCE 1
$ clang -std=gnu23 --target=x86_64-linux-gnu -dM -E -x c /dev/null | grep GNU_SOURCE
$ g++ -dM -E -x c++ /dev/null | grep GNU_SOURCE
#define _GNU_SOURCE 1
$ gcc -dM -E -x c /dev/null | grep GNU_SOURCE
$

uh...so perhaps not a bug in clang as the first two statements made me think...but WTF!?

Copy link
Contributor

@enh-google enh-google Feb 6, 2025

Choose a reason for hiding this comment

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

I was thinking more about this last night, and qsort_r came to mind.

So I guess I should change my stance slightly from "never" to "perhaps when there is a conflict."

Is there a conflict here? Well, sig_t and sighandler_t don't conflict with each other, and the issue isn't as severe as qsort_r. But Bionic provides both without requiring users to pledge allegiance; if bionic doesn't llvm-libc should not either (IMO).

I learned it from you, Dad! /runs away crying

But I will keep that in mind that _GNU_SOURCE and _BSD_SOURCE do exist more for than just pledges of allegiance and holding user code hostage, and that we may need to deploy those in llvm-libc in the future. I still don't think we need to here (yet; famous last words).

yeah, as you can probably tell, i do think "provide all the things until/unless proven problematic" is a developer-friendly option.

the only real counterargument i've heard is "it's bad for source portability in the other direction". but i've always cared more that people's stuff "just works" when ported to the libc i have control over rather than what might happen when ported away (in part because that's probably less common anyway; any library that's first developed on Android seems likely to be Android-specific in some way, or you'd probably make it cross-platform from the beginning).

but _GNU_SOURCE is something developers are used to turning on/off either via -std=gnu23 vs -std=c23

Wait...what?!

$ clang -std=gnu++23 --target=x86_64-linux-gnu -dM -E -x c++ /dev/null | grep GNU_SOURCE
#define _GNU_SOURCE 1
$ clang -std=gnu23 --target=x86_64-linux-gnu -dM -E -x c /dev/null | grep GNU_SOURCE
$ g++ -dM -E -x c++ /dev/null | grep GNU_SOURCE
#define _GNU_SOURCE 1
$ gcc -dM -E -x c /dev/null | grep GNU_SOURCE
$

uh...so perhaps not a bug in clang as the first two statements made me think...but WTF!?

oh, yeah, that's it ... i never do this myself, so i can never remember which combinations imply _GNU_SOURCE, just that there are dragons in that area.

madness indeed. (especially because gnu vs c does let you opt in/out of language extensions. if you were going to do anything here, consistency would have been nice. i assume there was a historical accident somewhere -- like bionic's always-on _BSD_SOURCE and they just couldn't fix it by the time they realized.)

// For compatibility with glibc.
typedef void (*sighandler_t)(int);
#endif

#endif // LLVM_LIBC_TYPES_SIGHANDLER_T_H
4 changes: 0 additions & 4 deletions libc/include/signal.h.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@

#include "llvm-libc-macros/signal-macros.h"

#ifdef __linux__
#include "llvm-libc-types/__sighandler_t.h"
#endif

%%public_api()

#endif // LLVM_LIBC_SIGNAL_H
1 change: 1 addition & 0 deletions libc/include/signal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ macros: []
types:
- type_name: pid_t
- type_name: sig_atomic_t
- type_name: sighandler_t
- type_name: siginfo_t
- type_name: sigset_t
- type_name: stack_t
Expand Down