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

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked 2 inline comments as done.
sivachandra added inline comments.


================
Comment at: libc/include/signal.h.def:14-17
+#define __need_size_t
+#include <stddef.h>
 
 %%include_file(${platform_signal})
----------------
sivachandra wrote:
> abrachet wrote:
> > 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.
> 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>
> ```
I think this is OK.


================
Comment at: libc/src/signal/linux/signal.h:29
+
+  constexpr void delset(int signal) {
+    nativeSigset &= ~(1 << (signal - 1));
----------------
Seems to me like `delset` is not used anywhere. If so, remove it?


================
Comment at: libc/test/src/signal/CMakeLists.txt:23-24
     signal_h
+    sigprocmask
+    __errno_location
+    signal_test
----------------
sivachandra wrote:
> abrachet wrote:
> > @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`.
> It shouldn't be required if CMake is setting up the deps correctly. Let me do some digging and get back to you.
I see the problem. The recursive deps aren't getting set up correctly. I will fix that separately. This is OK for now.


================
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
----------------
Is this required?


================
Comment at: libc/test/src/signal/sigprocmask_test.cpp:20
+
+TEST_F(SignalTest, SigprocmaskInvalid) {
+  sigset_t valid;
----------------
Can you add comments describing what is being tested. For example, explain the numbers like 17 and -4.


================
Comment at: libc/test/src/signal/sigprocmask_test.cpp:41
+
+TEST_F(SignalTest, BlockUnblock) {
+  sigset_t sigset;
----------------
Likewise, an you add a comment explaining the intention of this test?


================
Comment at: libc/test/src/signal/sigprocmask_test.cpp:48
+        __llvm_libc::raise(SIGUSR1);
+        __llvm_libc::raise(SIGKILL);
+      },
----------------
Why is raising `SIGKILL` relevant here?


================
Comment at: libc/utils/UnitTest/Test.h:1
 //===------------------ Base class for libc unittests -----------*- C++ -*-===//
 //
----------------
Can revert the changes to this file I think?


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

https://reviews.llvm.org/D75026