[libc-commits] [PATCH] D74528: [libc] Add Initial Support for Signals

[Alex Brachet via Phabricator via libc-commits]

abrachet marked 4 inline comments as done.
abrachet added inline comments.


================
Comment at: libc/config/linux/api.td:144
+    // We use <linux/signal.h> so these should be defined.
+    MacroDefineIfNot<"NSIG", "64">,
+
----------------
sivachandra wrote:
> abrachet wrote:
> > sivachandra wrote:
> > > abrachet wrote:
> > > > MaskRay wrote:
> > > > > MaskRay wrote:
> > > > > > What is it 64?
> > > > > 65 may be a reasonable choice for most architectures.
> > > > > 
> > > > > MIPS may require 129. A MIPS signal mask contains 128 bits.
> > > > > 
> > > > > The Linux uapi may be stuck with `#define SIGRTMAX _NSIG` but a libc should not replicate that (mistake). A libc should have `SIGRTMAX-1 == _NSIG`, though `SIGRTMAX` is not necessarily a constant.
> > > > > 
> > > > Good catch.
> > > > 
> > > > My /usr/include/x86_64-linux-gnu/asm/signal.h has:
> > > > ```
> > > > #define NSIG		32
> > > > typedef unsigned long sigset_t;
> > > > // ... further down
> > > > #define SIGRTMIN	32
> > > > #define SIGRTMAX	_NSIG
> > > > ```
> > > > This is where I am currently getting `sigset_t` and NSIG. Should we not rely on this header then and define these our self? I do want to use this include to give me `sigset_t`, `struct sigaction`, because I think this lessens the burden on us. I would want to hear your thoughts on how difficult (or easy) it is to maintain portability between architectures without using the Linux headers.
> > > > 
> > > > > 65 may be a reasonable choice for most architectures.
> > > > >
> > > > > MIPS may require 129. A MIPS signal mask contains 128 bits.
> > > > We already have x86_64 specific directory, for header gen config here, it shouldn't be a big problem to make NSIG platform dependent, I'm happy to do it in this patch, if we step away from using <linux/signal.h>
> > > Though I accepted, I was not sure about employing this pattern here. I am guilty of setting an example with this pattern and I think I did not get it correct entirely.
> > > 
> > > We should strive to use platform headers if available and avoid copying them into the libc tree. Considering that we are keeping the platform specific definitions open anyway (as in, one can add them to the signal.h.in file in this case), we are not really blocking any platform which wants such definitions from a libc.
> > > 
> > > Which means, we should not need to list these macros at all in this file (and also the `sigset_t` type).
> > > 
> > I've removed those macros, except for `SIG_BLOCK` and friends because those are not defined in the linux headers. Are we happy with it like this?
> On my machine, I do get `SIG_BLOCK` and friends from the linux headers. They are defined in `asm-generic/signal-defs.h`, which is included by `asm/signal.h`, which is included by `linux/signal.h`.
Indeed, good catch.


================
Comment at: libc/src/signal/linux/signal.h:22
+// All bits set meaning all signals.
+const static sigset_t all_set = -1;
+
----------------
sivachandra wrote:
> abrachet wrote:
> > sivachandra wrote:
> > > I should have pointed this out earlier: this is x86/x86_64 specific. I am OK for now as it would cause a compilation error for other architectures where `sigset_t` is not of an integer type. We can fix it then unless you want to fix it now.
> > I think this wrapper type is a good solution for when we need to deal with such `sigset_t`'s. 
>  As long as it does not need a big surgery to extend, I am OK. I do not have any strong opinion here. Essentially because I have not worked on any other arch.
I feel the same way. I think that I should be able to keep any big changes inside `Sigset` in the future.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74528/new/

https://reviews.llvm.org/D74528