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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
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:
> 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?
 I wouldn't really consider it as a bad thing if we take many more patches/commits to get it all sorted out correctly.

We want to take the approach of not adding something until it is required. So, I am totally fine with adding `_NSIG` if, and only if, required.


================
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
----------------
abrachet wrote:
> abrachet wrote:
> > 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.
> Disregard that, now that I think about it, that doesn't matter because the class name would be SignalTest_Raise. I'll remove this and in sigaddset_test.cpp in the next round.
You can then remove `SignalTest.h` and put its content inside of the `sigprocmask` test?


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

https://reviews.llvm.org/D75026