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 all commits
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
9 changes: 0 additions & 9 deletions libc/hdr/types/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,15 +250,6 @@ add_proxy_header_library(
libc.include.locale
)

add_proxy_header_library(
sighandler_t
HDRS
sighandler_t.h
FULL_BUILD_DEPENDS
libc.include.llvm-libc-types.__sighandler_t
libc.include.signal
)

add_proxy_header_library(
stack_t
HDRS
Expand Down
24 changes: 0 additions & 24 deletions libc/hdr/types/sighandler_t.h
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions libc/include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,13 +275,14 @@ add_header_macro(
signal.h
DEPENDS
.llvm-libc-macros.signal_macros
.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
.llvm-libc-types.struct_sigaction
.llvm-libc-types.union_sigval
.llvm-libc-types.siginfo_t
.llvm-libc-types.stack_t
.llvm-libc-types.pid_t
)

add_header_macro(
Expand Down
6 changes: 3 additions & 3 deletions libc/include/llvm-libc-macros/gpu/signal-macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
#define SIGSEGV 11
#define SIGTERM 15

#define SIG_DFL ((__sighandler_t)(0))
#define SIG_IGN ((__sighandler_t)(1))
#define SIG_ERR ((__sighandler_t)(-1))
#define SIG_DFL ((void (*)(int))(0))
#define SIG_IGN ((void (*)(int))(1))
#define SIG_ERR ((void (*)(int))(-1))

// Max signal number
#define NSIG 64
Expand Down
6 changes: 3 additions & 3 deletions libc/include/llvm-libc-macros/linux/signal-macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@
#error "Signal stack sizes not defined for your platform."
#endif

#define SIG_DFL ((__sighandler_t)0)
#define SIG_IGN ((__sighandler_t)1)
#define SIG_ERR ((__sighandler_t)-1)
#define SIG_DFL ((void (*)(int))0)
#define SIG_IGN ((void (*)(int))1)
#define SIG_ERR ((void (*)(int))(-1))

// SIGCHLD si_codes
#define CLD_EXITED 1 // child has exited
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
13 changes: 8 additions & 5 deletions .../include/llvm-libc-types/__sighandler_t.h → libc/include/llvm-libc-types/sighandler_t.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
//===-- Definition of struct __sighandler_t -------------------------------===//
//===-- 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
#ifndef LLVM_LIBC_TYPES_SIGHANDLER_T_H
#define LLVM_LIBC_TYPES_SIGHANDLER_T_H

typedef void (*__sighandler_t)(int);
#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
#endif // LLVM_LIBC_TYPES_SIGHANDLER_T_H
2 changes: 0 additions & 2 deletions libc/include/llvm-libc-types/struct_sigaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,4 @@ struct sigaction {
#endif
};

typedef void (*__sighandler_t)(int);

#endif // LLVM_LIBC_TYPES_STRUCT_SIGACTION_H
16 changes: 11 additions & 5 deletions libc/include/signal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ header_template: signal.h.def
macros: []
types:
- type_name: pid_t
- type_name: stack_t
- type_name: sig_atomic_t
- type_name: sighandler_t
- type_name: siginfo_t
- type_name: struct_sigaction
- type_name: sigset_t
- type_name: stack_t
- type_name: struct_sigaction
- type_name: union_sigval
- type_name: sig_atomic_t
enums: []
objects: []
functions:
Expand Down Expand Up @@ -69,10 +70,15 @@ functions:
- name: signal
standards:
- stdc
return_type: __sighandler_t
# May the Geneva Convention have mercy on my soul... Why this insanity?
# Well: signal returns a function pointer to a function with no return
# value and which accepts an int. The parameter list appears on the far
# right of the declaration. i.e.
# void (*signal(int, void (*)(int)))(int);
return_type: void (*
arguments:
- type: int
- type: __sighandler_t
- type: void (*)(int)))(int
- name: sigprocmask
standards:
- POSIX
Expand Down
1 change: 0 additions & 1 deletion libc/src/signal/linux/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ add_entrypoint_object(
DEPENDS
.sigaction
libc.hdr.signal_macros
libc.hdr.types.sighandler_t
)

add_entrypoint_object(
Expand Down
7 changes: 5 additions & 2 deletions libc/src/signal/linux/signal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@

#include "src/signal/signal.h"
#include "hdr/signal_macros.h"
#include "hdr/types/sighandler_t.h"
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/signal/sigaction.h"

namespace LIBC_NAMESPACE_DECL {

LLVM_LIBC_FUNCTION(sighandler_t, signal, (int signum, sighandler_t handler)) {
// Our LLVM_LIBC_FUNCTION macro doesn't handle function pointer return types.
using signal_handler = void (*)(int);
Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 4, 2025

Choose a reason for hiding this comment

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

perhaps decltype would be better here?

Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 4, 2025

Choose a reason for hiding this comment

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

hmm...no nevermind. We'd only be able to do decltype(signal), which would not be the type of the parameter or return value.

Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 4, 2025

Choose a reason for hiding this comment

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

Perhaps auto would work...

Copy link
Member Author

@nickdesaulniers nickdesaulniers Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ah, no, I get an error related to overload resolution. Ok will leave for now.
error: functions that differ only in their return type cannot be overloaded


LLVM_LIBC_FUNCTION(signal_handler, signal,
(int signum, signal_handler handler)) {
struct sigaction action, old;
action.sa_handler = handler;
action.sa_flags = SA_RESTART;
Expand Down
3 changes: 1 addition & 2 deletions libc/src/signal/signal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@
#ifndef LLVM_LIBC_SRC_SIGNAL_SIGNAL_H
#define LLVM_LIBC_SRC_SIGNAL_SIGNAL_H

#include "hdr/types/sighandler_t.h"
#include "src/__support/macros/config.h"

namespace LIBC_NAMESPACE_DECL {

sighandler_t signal(int signum, sighandler_t handler);
void (*signal(int signum, void (*handler)(int)))(int);

} // namespace LIBC_NAMESPACE_DECL

Expand Down
2 changes: 1 addition & 1 deletion libc/test/UnitTest/FPExceptMatcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ static void sigfpeHandler(int sig) {
}

FPExceptMatcher::FPExceptMatcher(FunctionCaller *func) {
sighandler_t oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);
auto *oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);

caughtExcept = false;
fenv_t oldEnv;
Expand Down
1 change: 0 additions & 1 deletion libc/test/src/signal/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ add_libc_unittest(
SRCS
signal_test.cpp
DEPENDS
libc.hdr.types.sighandler_t
libc.src.errno.errno
libc.src.signal.raise
libc.src.signal.signal
Expand Down
4 changes: 1 addition & 3 deletions libc/test/src/signal/signal_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,12 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"

#include "hdr/types/sighandler_t.h"

using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;

TEST(LlvmLibcSignal, Invalid) {
LIBC_NAMESPACE::libc_errno = 0;
sighandler_t valid = +[](int) {};
auto *valid = +[](int) {};
EXPECT_THAT((void *)LIBC_NAMESPACE::signal(0, valid),
Fails(EINVAL, (void *)SIG_ERR));
EXPECT_THAT((void *)LIBC_NAMESPACE::signal(65, valid),
Expand Down