[libc-commits] [PATCH] D75026: [libc] Add sigprocmask

[Alex Brachet via Phabricator via libc-commits]

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


================
Comment at: libc/src/signal/linux/sigaddset.cpp:19
+int LLVM_LIBC_ENTRYPOINT(sigaddset)(sigset_t *set, int signum) {
+  if (!set || (unsigned)signum > (8 * sizeof(sigset_t))) {
+    llvmlibc_errno = EINVAL;
----------------
abrachet wrote:
> MaskRay wrote:
> > abrachet wrote:
> > > MaskRay wrote:
> > > > `_NSIG` is expected to be compared.
> > > `_NSIG`'s value has still not been decided, so I didn't want to use it. Should I just go ahead and define it as 65?
> > This is a good example of layering issue. `_NSIG` is not decided yet (for no good reasons. I think we don't care about MIPS, at least for a forseeable future) => so we don't use it in this patch => this patch gets approved => it is committed.
> > 
> > Someday we add `_NSIG` => the patch needs rework.
> > 
> > (Well, doing things this way can cause a large number of commits. If someone judges the contribution by just counting the number of commits....)
> `NSIG` was just not decided previously. It's not about number of commits, I doubt I would even be the one adding support for MIPS anyway. We just didn't come to a conclusion on last patch. I'm happy to decide it now. If you're happy with 65 (or should I just #define NSIG _NSIG [[ https://github.com/torvalds/linux/blob/master/arch/x86/include/asm/signal.h#L11 | which is 64 ]]?)  then I will go ahead and add that and use that instead of `8 * sizeof(sigset_t)`.
> The same for `struct Sigset` I will just use `sigset_t` directly if you think that is better.
I have decided to keep it as such. POSIX says this function may fail if signum is not a valid signal not that it musts. Many libc's take advantage of this leniency. 
>From Apple's libc for example:
```lang=cpp
int f() {
  sigset_t set;
  sigaddset(&set, NSIG + 1); // == 0
}
```
```lang=cpp
#undef sigaddset
int f() {
  sigset_t set;
  sigaddset(&set, NSIG + 1); // != 0, errno == EINVAL
}
```
This doesn't mean that I will not be adding `NSIG` in the future but rather that it isn't necessary for this function. I am still unsure if it should be defined as _NSIG or constant 65, I prefer _NSIG because 65 doesn't make sense to me there are not 65 signals in Linux there are 64. NSIG is defined on my machine as 32 (no realtime signals on Darwin), not 33. I don't understand why other libc's have chosen 65, would you mind explaining?


================
Comment at: libc/test/src/signal/raise_test.cpp:15
 
-TEST(SignalTest, Raise) {
+TEST_F(SignalTest, Raise) {
   // SIGCONT is ingored unless stopped, so we can use it to check the return
----------------
sivachandra wrote:
> Is this required?
Not really, but I think it makes sense to keep it. It works fine right now with no problems. But if this file was linked with the other tests which use the test fixture there would be a problem because these classes and their symbols are different. If we are confident that will never happen then I am fine removing that here and in sigaddset_test.cpp too.


================
Comment at: libc/test/src/signal/sigprocmask_test.cpp:48
+        __llvm_libc::raise(SIGUSR1);
+        __llvm_libc::raise(SIGKILL);
+      },
----------------
sivachandra wrote:
> Why is raising `SIGKILL` relevant here?
I was thinking to make sure it dies to `SIGUSR1`, but you're right the test will still fail if it exits normally. Removed.


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

https://reviews.llvm.org/D75026