This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add sigprocmask
ClosedPublic

Authored by abrachet on Feb 23 2020, 1:51 PM.

Details

Summary

This patch adds sigprocmask, sigemptyset and sigaddset

Diff Detail

Event Timeline

abrachet created this revision.Feb 23 2020, 1:51 PM
abrachet marked 3 inline comments as done.Feb 23 2020, 1:55 PM
abrachet added inline comments.
libc/include/signal.h.def
14–17

I need to get <linux/signal.h> included first now because the functions take sigset_t *. I couldn't figure out a clean way to do this but I'm pretty sure this isn't it.

libc/spec/posix.td
9–13

Perhaps not where these should go?

libc/test/src/signal/CMakeLists.txt
12–13

@sivachandra, I'm not sure how add_header_library is supposed to work, but I needed to add these to use signal_test even though it already depends on sigprocmask and __errno_location.

sivachandra added inline comments.Feb 24 2020, 9:17 AM
libc/include/signal.h.def
14–17

If linux.h require size_t, the platform file should be constructed this way:

#define __need_size_t
#include <stddef.h>

#include <linux/signal.h>
libc/spec/posix.td
9–13

If sigset_t is also defined and used by the C standard, then put it in spec/spec.td

libc/test/src/signal/CMakeLists.txt
12–13

It shouldn't be required if CMake is setting up the deps correctly. Let me do some digging and get back to you.

I only answered the questions and have not done a full review.

MaskRay added inline comments.Feb 24 2020, 6:25 PM
libc/src/signal/linux/sigaddset.cpp
20

_NSIG is expected to be compared.

21

(I think I mentioned we don't necessarily invent an llvmlibc_errno but the ship has sailed..)

libc/src/signal/linux/signal.h
23

I believe struct Sigset is an overengineering.

abrachet marked 4 inline comments as done.Feb 24 2020, 6:51 PM
abrachet added inline comments.
libc/spec/posix.td
9–13

It is not. It is in posix.

libc/src/signal/linux/sigaddset.cpp
20

_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?

21

What do you mean by invent an llvmlibc_errno?
[[ https://github.com/llvm/llvm-project/blob/master/libc/src/errno/llvmlibc_errno.h#L14 | llvmlibc_errno ]] expands to (*__llvm_libc::__errno_location()), it isn't the thread local reference I stupidly suggested.

libc/src/signal/linux/signal.h
23

I'm inclined to agree but this was chosen so that architectures which use non integral sigset_t will be easier to deal with. Should I remove Sigset then?

MaskRay added inline comments.Feb 26 2020, 4:09 PM
libc/src/signal/linux/sigaddset.cpp
20

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....)

abrachet marked an inline comment as done.Feb 26 2020, 4:23 PM
abrachet added inline comments.
libc/src/signal/linux/sigaddset.cpp
20

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 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.

sivachandra marked 2 inline comments as done.Feb 27 2020, 10:07 AM
sivachandra added inline comments.
libc/include/signal.h.def
14–17

I think this is OK.

libc/src/signal/linux/signal.h
29

Seems to me like delset is not used anywhere. If so, remove it?

libc/test/src/signal/CMakeLists.txt
12–13

I see the problem. The recursive deps aren't getting set up correctly. I will fix that separately. This is OK for now.

libc/test/src/signal/raise_test.cpp
14

Is this required?

libc/test/src/signal/sigprocmask_test.cpp
21

Can you add comments describing what is being tested. For example, explain the numbers like 17 and -4.

42

Likewise, an you add a comment explaining the intention of this test?

49

Why is raising SIGKILL relevant here?

libc/utils/UnitTest/Test.h
1 ↗(On Diff #246125)

Can revert the changes to this file I think?

abrachet updated this revision to Diff 247430.Feb 29 2020, 12:42 AM
  • Added test for sigaddset
  • Added comments throughout tests
  • Removed Sigset::delset
abrachet marked 9 inline comments as done.Feb 29 2020, 12:53 AM
abrachet added inline comments.
libc/src/signal/linux/sigaddset.cpp
20

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:

int f() {
  sigset_t set;
  sigaddset(&set, NSIG + 1); // == 0
}
#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?

libc/test/src/signal/raise_test.cpp
14

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.

libc/test/src/signal/sigprocmask_test.cpp
49

I was thinking to make sure it dies to SIGUSR1, but you're right the test will still fail if it exits normally. Removed.

abrachet updated this revision to Diff 247431.Feb 29 2020, 1:36 AM
abrachet marked an inline comment as done.

Add entry points in lib/CMakeLists.txt

abrachet marked an inline comment as done.Feb 29 2020, 10:43 PM
abrachet added inline comments.
libc/test/src/signal/raise_test.cpp
14

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.

sivachandra accepted this revision.Mar 1 2020, 10:31 PM
sivachandra added inline comments.
libc/src/signal/linux/sigaddset.cpp
20

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.

libc/test/src/signal/raise_test.cpp
14

You can then remove SignalTest.h and put its content inside of the sigprocmask test?

This revision is now accepted and ready to land.Mar 1 2020, 10:31 PM
abrachet updated this revision to Diff 247560.Mar 1 2020, 11:42 PM

Submitting nits before pushing

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 1:06 AM