This patch adds sigprocmask, sigemptyset and sigaddset
Details
Diff Detail
Event Timeline
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. |
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. |
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? | |
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? |
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....) |
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). |
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? |
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. 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. |
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. |
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? |
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.