[libc] Add sigprocmask

[abrachet]

Summary

This patch adds sigprocmask, sigemptyset and sigaddset

[abrachet]

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

1–5

Perhaps not where these should go?

libc/test/src/signal/CMakeLists.txt

23–24

@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]

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

1–5

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

23–24

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

[sivachandra]

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

[MaskRay]

libc/src/signal/linux/sigaddset.cpp

19

_NSIG is expected to be compared.

20

(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]

libc/spec/posix.td

1–5

It is not. It is in posix.

libc/src/signal/linux/sigaddset.cpp

19

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

20

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]

libc/src/signal/linux/sigaddset.cpp

19

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]

libc/src/signal/linux/sigaddset.cpp

19

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]

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

23–24

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

15

Is this required?

libc/test/src/signal/sigprocmask_test.cpp

20

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

41

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

48

Why is raising SIGKILL relevant here?

libc/utils/UnitTest/Test.h

1

Can revert the changes to this file I think?

[abrachet]

• Added test for sigaddset
• Added comments throughout tests
• Removed Sigset::delset

[abrachet]

libc/src/signal/linux/sigaddset.cpp

19

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

15

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

48

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]

Add entry points in lib/CMakeLists.txt

[abrachet]

libc/test/src/signal/raise_test.cpp

15

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]

libc/src/signal/linux/sigaddset.cpp

19

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

15

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

[abrachet]

Submitting nits before pushing

libc/config/linux/api.td

def SysMManAPI : PublicAPI<"sys/mman.h"> {
 
   let Functions = [
     "mmap",
     "munmap",
   ];
 }
 
 def SignalAPI : PublicAPI<"signal.h"> {
-  let TypeDeclarations = [
-    SizeT, // This is needed by <linux/signal.h>.
-  ];
-
   let Functions = [
     "raise",
+    "sigprocmask",
+    "sigemptyset",
+    "sigaddset",
   ];
 }

libc/include/signal.h.def

 //===---------------- C standard library header signal.h ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SIGNAL_H
 #define LLVM_LIBC_SIGNAL_H
 
 #include <__llvm-libc-common.h>
 
-%%public_api()
+#define __need_size_t
+#include <stddef.h>
 
 %%include_file(${platform_signal})
 
+%%public_api()
+
 #endif // LLVM_LIBC_SIGNAL_H

libc/spec/posix.td

+def SigSetType : NamedType<"sigset_t">;
+def SigSetPtrType : PtrType<SigSetType>;
+def ConstSigSetPtrType : ConstType<SigSetPtrType>;
+def RestrictSigSetType : RestrictedPtrType<SigSetType>;
+def ConstRestrictSigSetType : ConstType<RestrictSigSetType>;
+
 def POSIX : StandardSpec<"POSIX"> {
   NamedType OffTType = NamedType<"off_t">;
 
   HeaderSpec Errno = HeaderSpec<
       "errno.h",
       [
         Macro<"E2BIG">,
         Macro<"EACCES">,
HeaderSpec SysMMan = HeaderSpec<
         FunctionSpec<
             "munmap",
             RetValSpec<IntType>,
             [ArgSpec<VoidPtr>, ArgSpec<SizeTType>]
         >,
       ]
   >;
 
+  HeaderSpec Signal = HeaderSpec<
+      "signal.h",
+      [], // Macros
+      [], // Types
+      [
+        FunctionSpec<
+          "sigprocmask",
+          RetValSpec<IntType>,
+          [ArgSpec<IntType>, ArgSpec<ConstRestrictSigSetType>, ArgSpec<RestrictSigSetType>]
+        >,
+        FunctionSpec<
+          "sigemptyset",
+          RetValSpec<IntType>,
+          [ArgSpec<SigSetPtrType>]
+        >,
+        FunctionSpec<
+          "sigaddset",
+          RetValSpec<IntType>,
+          [ArgSpec<SigSetPtrType>]
+        >,
+      ]
+  >;
+
   let Headers = [
     Errno,
     SysMMan,
+    Signal,
   ];
 }

libc/src/signal/linux/CMakeLists.txt

 
 add_entrypoint_object(
   raise
   SRCS
     raise.cpp
   HDRS
     signal.h
     ../raise.h
   DEPENDS
     sys_syscall_h
     linux_syscall_h
+    signal_h
+)
+
+add_entrypoint_object(
+  sigprocmask
+  SRCS
+    sigprocmask.cpp
+  HDRS
+    signal.h
+    ../sigprocmask.h
+  DEPENDS
+    sys_syscall_h
+    linux_syscall_h
+    __errno_location
+    signal_h
+)
+
+add_entrypoint_object(
+  sigemptyset
+  SRCS
+    sigemptyset.cpp
+  HDRS
+    signal.h
+    ../sigemptyset.h
+  DEPENDS
+    __errno_location
+    signal_h
+)
+
+add_entrypoint_object(
+  sigaddset
+  SRCS
+    sigaddset.cpp
+  HDRS
+    signal.h
+    ../sigaddset.h
+  DEPENDS
     __errno_location
     signal_h
 )

libc/src/signal/linux/sigaddset.cpp

+//===----------------- Linux implementation of sigaddset ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/signal/sigaddset.h"
+#include "include/errno.h" // For E* macros.
+#include "src/errno/llvmlibc_errno.h"
+#include "src/signal/linux/signal.h"
+
+#include "src/__support/common.h"
+
+namespace __llvm_libc {
+
+int LLVM_LIBC_ENTRYPOINT(sigaddset)(sigset_t *set, int signum) {
+  if (!set || (unsigned)signum > (8 * sizeof(sigset_t))) {
+    llvmlibc_errno = EINVAL;
+    return -1;
+  }
+  auto *sigset = reinterpret_cast<__llvm_libc::Sigset *>(set);
+  sigset->addset(signum);
+  return 0;
+}
+
+} // namespace __llvm_libc

libc/src/signal/linux/sigemptyset.cpp

+//===--------------- Linux implementation of sigemptyset ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/signal/sigemptyset.h"
+#include "include/errno.h" // For E* macros.
+#include "src/errno/llvmlibc_errno.h"
+#include "src/signal/linux/signal.h"
+
+#include "src/__support/common.h"
+
+namespace __llvm_libc {
+
+int LLVM_LIBC_ENTRYPOINT(sigemptyset)(sigset_t *set) {
+  if (!set) {
+    llvmlibc_errno = EINVAL;
+    return -1;
+  }
+  *set = __llvm_libc::Sigset::emptySet();
+  return 0;
+}
+
+} // namespace __llvm_libc

libc/src/signal/linux/signal.h

 #include "include/signal.h"
 
 static_assert(sizeof(sigset_t) * 8 >= NSIG, "sigset_t cannot hold all signals");
 
 namespace __llvm_libc {
 
 // Using this internally defined type will make it easier in the future to port
 // to different architectures.
 struct Sigset {
   sigset_t nativeSigset;
 
   constexpr static Sigset fullset() { return {-1UL}; }
+  constexpr static Sigset emptySet() { return {0}; }
+
+  constexpr void delset(int signal) {
+    nativeSigset &= ~(1 << (signal - 1));
+  }
+
+  constexpr void addset(int signal) {
+    nativeSigset |= (1 << (signal - 1));
+  }
 
   operator sigset_t() const { return nativeSigset; }
 };
 
 constexpr static Sigset all = Sigset::fullset();
 
 static inline int block_all_signals(Sigset &set) {
   sigset_t nativeSigset = all;
   sigset_t oldSet = set;
-  return __llvm_libc::syscall(SYS_rt_sigprocmask, SIG_BLOCK, &nativeSigset,
+  int ret = __llvm_libc::syscall(SYS_rt_sigprocmask, SIG_BLOCK, &nativeSigset,
                               &oldSet, sizeof(sigset_t));
+  set = {oldSet};
+  return ret;
 }
 
 static inline int restore_signals(const Sigset &set) {
   sigset_t nativeSigset = set;
   return __llvm_libc::syscall(SYS_rt_sigprocmask, SIG_SETMASK,
                               &nativeSigset, nullptr,
                               sizeof(sigset_t));
 }
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_SIGNAL_LINUX_SIGNAL_H

libc/src/signal/linux/sigprocmask.cpp

+//===--------------- Linux implementation of sigprocmask ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/signal/sigprocmask.h"
+#include "src/errno/llvmlibc_errno.h"
+#include "src/signal/linux/signal.h"
+
+#include "src/__support/common.h"
+
+namespace __llvm_libc {
+
+int LLVM_LIBC_ENTRYPOINT(sigprocmask)(int how, const sigset_t *__restrict set,
+                                      sigset_t *__restrict oldset) {
+  int ret = __llvm_libc::syscall(SYS_rt_sigprocmask, how, set, oldset,
+                                 sizeof(sigset_t));
+  if (!ret)
+    return 0;
+
+  llvmlibc_errno = -ret;
+  return -1;
+}
+
+} // namespace __llvm_libc

libc/src/signal/sigaddset.h

+//===------------- Implementation header for sigaddset ---------*- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_SIGNAL_SIGEADDSET_H
+#define LLVM_LIBC_SRC_SIGNAL_SIGEADDSET_H
+
+#include "include/signal.h"
+
+namespace __llvm_libc {
+
+int sigaddset(sigset_t *set, int signum);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_SIGNAL_SIGEADDSET_H

libc/src/signal/sigemptyset.h

+//===------------ Implementation header for sigemptyset --------*- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_SIGNAL_SIGEMPTYSET_H
+#define LLVM_LIBC_SRC_SIGNAL_SIGEMPTYSET_H
+
+#include "include/signal.h"
+
+namespace __llvm_libc {
+
+int sigemptyset(sigset_t *set);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_SIGNAL_SIGEMPTYSET_H

libc/src/signal/sigprocmask.h

+//===------------ Implementation header for sigprocmask --------*- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_SIGNAL_SIGPROCMASK_H
+#define LLVM_LIBC_SRC_SIGNAL_SIGPROCMASK_H
+
+#include "include/signal.h"
+
+namespace __llvm_libc {
+
+int sigprocmask(int how, const sigset_t *__restrict set,
+                sigset_t *__restrict oldset);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_SIGNAL_SIGPROCMASK_H

libc/test/src/signal/CMakeLists.txt

 add_libc_testsuite(libc_signal_unittests)
 
+
+add_header_library(
+  signal_test
+  HDRS
+    SignalTest.h
+  DEPENDS
+    sigprocmask
+    __errno_location
+)
+
+
 add_libc_unittest(
   raise_test
   SUITE
     libc_signal_unittests
   SRCS
     raise_test.cpp
   DEPENDS
     raise
     signal_h
+    sigprocmask
+    __errno_location
+    signal_test
+)
+
+add_libc_unittest(
+  sigprocmask_test
+  SUITE
+    libc_signal_unittests
+  SRCS
+    sigprocmask_test.cpp
+  DEPENDS
+    raise
+    sigprocmask
+    sigaddset
+    sigemptyset
+    signal_h
+    __errno_location
+    signal_test
 )

libc/test/src/signal/SignalTest.h

+//===------------------ Test fixture for signal tests ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TEST_SRC_SIGNAL_SIGNALTEST_H
+#define LLVM_LIBC_TEST_SRC_SIGNAL_SIGNALTEST_H
+
+#include "include/signal.h"
+#include "src/signal/sigprocmask.h"
+#include "utils/UnitTest/Test.h"
+
+class SignalTest : public __llvm_libc::testing::Test {
+  sigset_t oldSet;
+
+public:
+  void SetUp() override { __llvm_libc::sigprocmask(0, nullptr, &oldSet); }
+
+  void TearDown() override {
+    __llvm_libc::sigprocmask(SIG_SETMASK, &oldSet, nullptr);
+  }
+};
+
+#endif // LLVM_LIBC_TEST_SRC_SIGNAL_SIGNALTEST_H

libc/test/src/signal/raise_test.cpp

 //===----------------------- Unittests for raise --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "include/signal.h"
 #include "src/signal/raise.h"
+
+#include "SignalTest.h"
 #include "utils/UnitTest/Test.h"
 
-TEST(SignalTest, Raise) {
+TEST_F(SignalTest, Raise) {
   // SIGCONT is ingored unless stopped, so we can use it to check the return
   // value of raise without needing to block.
   EXPECT_EQ(__llvm_libc::raise(SIGCONT), 0);
 
   // SIGKILL is chosen because other fatal signals could be caught by sanitizers
   // for example and incorrectly report test failure.
   EXPECT_DEATH([] { __llvm_libc::raise(SIGKILL); }, SIGKILL);
 }

libc/test/src/signal/sigprocmask_test.cpp

+//===--------------------- Unittests for sigprocmask ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "include/errno.h"
+#include "include/signal.h"
+#include "src/errno/llvmlibc_errno.h"
+#include "src/signal/raise.h"
+#include "src/signal/sigaddset.h"
+#include "src/signal/sigemptyset.h"
+#include "src/signal/sigprocmask.h"
+
+#include "SignalTest.h"
+#include "utils/UnitTest/Test.h"
+
+TEST_F(SignalTest, SigprocmaskInvalid) {
+  sigset_t valid;
+  llvmlibc_errno = 0;
+  EXPECT_EQ(__llvm_libc::sigprocmask(17, &valid, nullptr), -1);
+  EXPECT_EQ(llvmlibc_errno, EINVAL);
+
+  llvmlibc_errno = 0;
+  EXPECT_EQ(__llvm_libc::sigprocmask(-4, &valid, nullptr), -1);
+  EXPECT_EQ(llvmlibc_errno, EINVAL);
+
+  sigset_t *invalid = reinterpret_cast<sigset_t *>(-1);
+
+  llvmlibc_errno = 0;
+  EXPECT_EQ(__llvm_libc::sigprocmask(SIG_SETMASK, invalid, nullptr), -1);
+  EXPECT_EQ(llvmlibc_errno, EFAULT);
+
+  llvmlibc_errno = 0;
+  EXPECT_EQ(__llvm_libc::sigprocmask(-4, nullptr, invalid), -1);
+  EXPECT_EQ(llvmlibc_errno, EFAULT);
+}
+
+TEST_F(SignalTest, BlockUnblock) {
+  sigset_t sigset;
+  EXPECT_EQ(__llvm_libc::sigemptyset(&sigset), 0);
+  EXPECT_EQ(__llvm_libc::sigprocmask(SIG_SETMASK, &sigset, nullptr), 0);
+  EXPECT_DEATH(
+      [] {
+        __llvm_libc::raise(SIGUSR1);
+        __llvm_libc::raise(SIGKILL);
+      },
+      SIGUSR1);
+  EXPECT_EQ(__llvm_libc::sigaddset(&sigset, SIGUSR1), 0);
+  EXPECT_EQ(__llvm_libc::sigprocmask(SIG_SETMASK, &sigset, nullptr), 0);
+  EXPECT_EXITS([] { __llvm_libc::raise(SIGUSR1); }, 0);
+}

libc/utils/UnitTest/Test.h

 //===------------------ Base class for libc unittests -----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#ifndef LLVM_LIBC_UTILS_UNITTEST_H
+#define LLVM_LIBC_UTILS_UNITTEST_H
+
 // This file can only include headers from utils/CPP/ or utils/testutils. No
 // other headers should be included.
 
 #include "testutils/ExecuteFunction.h"
 #include "utils/CPP/TypeTraits.h"
 
 namespace __llvm_libc {
 namespace testing {

 #define EXPECT_DEATH(FUNC, SIG)                                                \
   __llvm_libc::testing::Test::testProcessKilled(                               \
       Ctx, __llvm_libc::testing::Test::createCallable(FUNC), SIG, #FUNC, #SIG, \
       __FILE__, __LINE__)
 
 #define ASSERT_DEATH(FUNC, EXIT)                                               \
   if (!EXPECT_DEATH(FUNC, EXIT))                                               \
   return
+
+#endif // LLVM_LIBC_UTILS_UNITTEST_H