[libc][signal] fix declaration of sighandler_t

[nickdesaulniers]

cross compiling our unit tests (built hermetically) with llvm-libc fails with:

llvm-project/libc/test/UnitTest/FPExceptMatcher.cpp:40:3: error: unknown type name 'sighandler_t'; did you mean '__sighandler_t'?
   40 |   sighandler_t oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);
      |   ^~~~~~~~~~~~
      |   __sighandler_t
llvm-project/build_aarch64/libc/include/llvm-libc-types/struct_sigaction.h:28:16: note: '__sighandler_t' declared here
   28 | typedef void (*__sighandler_t)(int);
      |                ^

it looks like our generated signal.h defines sighandler_t with two leading underscores (for some reason, 215c9fa seems related). Whatever the problem was should be revisited.

[llvmbot]

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

cross compiling our unit tests (built hermetically) with llvm-libc fails with: ``` llvm-project/libc/test/UnitTest/FPExceptMatcher.cpp:40:3: error: unknown type name 'sighandler_t'; did you mean '__sighandler_t'? 40 | sighandler_t oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler); | ^~~~~~~~~~~~ | __sighandler_t llvm-project/build_aarch64/libc/include/llvm-libc-types/struct_sigaction.h:28:16: note: '__sighandler_t' declared here 28 | typedef void (*__sighandler_t)(int); | ^ ``` it looks like our generated signal.h defines `sighandler_t` with two leading underscores (for some reason, 215c9fa seems related). Whatever the problem was should be revisited.

[nickdesaulniers]

hmm...reading 215c9fa, I think the larger issue is "when targeting linux, do we generally reach for kernel headers or not?" My two recent PRs aren't even consistent in that regard.

#125572 DOES reach for <linux/wait.h>, while #125118 DOES NOT reach for <linux/poll.h>.

My initial thoughts are that it sure would be nice when targeting linux not to have to re-duplicate what's already available in kernel headers. That said, that's probably a naive take that's missing some context as to why that's not advisable or always feasible. I do think cleaning up kernel headers is something I would be comfortable doing though, if need be. Hmm...

[nickdesaulniers]

Ah, we're simply missing a typedef in our generated header. But the organization of __sighandler_t is a bit of a mess; it seems declared in BOTH libc/include/llvm-libc-types/struct_sigaction.h (BAD) AND libc/include/llvm-libc-types/__sighandler_t.h (GOOD). It's then used in BOTH libc/include/llvm-libc-macros/linux/signal-macros.h AND libc/include/llvm-libc-macros/gpu/signal-macros.h without any explicit inclusion of that type (BAD).

I think if we add the typedef to our generated header, we can also drop the using statement in hdr/types/sighandler_t.h.

[nickdesaulniers]

t's then used in BOTH libc/include/llvm-libc-macros/linux/signal-macros.h AND libc/include/llvm-libc-macros/gpu/signal-macros.h without any explicit inclusion of that type (BAD).

So trying to fix this runs into an ordering issue where in libc/include/CMakeLists.txt

11 add_subdirectory(llvm-libc-macros)
12 add_subdirectory(llvm-libc-types)

so we can have types depend on macros, but not the other way around (adding a type dependency to a macro in cmake will error that the dependency target does not (yet) exist).

And if we simply reverse the order, we get a failure between libc.include.llvm-libc-macros.sys_select_macros and libc.include.llvm-libc-types.fd_set. So it's possible IRL to have types depend on macros, and macros depend on types; but for now llvm-libc only supports types that depend on macros, and does not (yet) support macros that depend on types.

Probably will be an issue that come up again in the future; will work around it for now.

[nickdesaulniers]

Reading bionic sources, there's at least three related typedefs here...

https://android.googlesource.com/platform/bionic/+/main/libc/include/bits/signal_types.h#72
https://android.googlesource.com/platform/bionic/+/main/libc/kernel/uapi/asm-generic/signal-defs.h#46

include/bits/signal_types.h
48:typedef __sighandler_t sig_t; /* BSD compatibility. */
49:typedef __sighandler_t sighandler_t; /* glibc compatibility. */
kernel/uapi/asm-generic/signal-defs.h
46:typedef __signalfn_t  * __sighandler_t;

The C standard doesn't mention sighandler_t at all, and neither does POSIX.

So I think I should make sighandler_t a linux only thing in our headers, and change our implementation not to refer to that type at all. While the function pointer syntax is ugly, it's not impossible to work with.

cc @enh-google

[enh-google]

Reading bionic sources, there's at least three related typedefs here...

https://android.googlesource.com/platform/bionic/+/main/libc/include/bits/signal_types.h#72 https://android.googlesource.com/platform/bionic/+/main/libc/kernel/uapi/asm-generic/signal-defs.h#46

include/bits/signal_types.h
48:typedef __sighandler_t sig_t; /* BSD compatibility. /
49:typedef __sighandler_t sighandler_t; / glibc compatibility. */
kernel/uapi/asm-generic/signal-defs.h
46:typedef __signalfn_t * __sighandler_t;
The C standard doesn't mention sighandler_t at all, and neither does POSIX.

yeah, and -- as you can see -- BSD has sig_t instead of sighandler_t.

bionic, as usual, has both for source compatibility with both.

musl's interesting here where they have the type but they don't use it for the argument/return type of signal(). (which is actually similar to what BSD does with their type.)

the double-underscore type is just the kernel's name for the same type, as is typical for them.

So I think I should make sighandler_t a linux only thing in our headers, and change our implementation not to refer to that type at all. While the function pointer syntax is ugly, it's not impossible to work with.

cc @enh-google

musl's gone that way already (s/linux only/_GNU_SOURCE/), so it's probably not too hard to follow.

i suspect the argument for sighandler_t is that it makes the signal() prototype easier to read for callers. that certainly seems strong enough reason to me for bionic to stick with it ... you'd be surprised how many c programmers can't read c declaration syntax.

(it's also what the man page has, so there's that too...)