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

[Alex Brachet via Phabricator via libc-commits]

abrachet marked 4 inline comments as done.
abrachet added inline comments.


================
Comment at: libc/spec/posix.td:1-5
+def SigSetType : NamedType<"sigset_t">;
+def SigSetPtrType : PtrType<SigSetType>;
+def ConstSigSetPtrType : ConstType<SigSetPtrType>;
+def RestrictSigSetType : RestrictedPtrType<SigSetType>;
+def ConstRestrictSigSetType : ConstType<RestrictSigSetType>;
----------------
sivachandra wrote:
> abrachet wrote:
> > Perhaps not where these should go?
> If `sigset_t` is also defined and used by the C standard, then put it in `spec/spec.td`
It is not. It is in posix.


================
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;
----------------
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?


================
Comment at: libc/src/signal/linux/sigaddset.cpp:20
+  if (!set || (unsigned)signum > (8 * sizeof(sigset_t))) {
+    llvmlibc_errno = EINVAL;
+    return -1;
----------------
MaskRay wrote:
> (I think I mentioned we don't necessarily invent an `llvmlibc_errno` but the ship has sailed..)
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.


================
Comment at: libc/src/signal/linux/signal.h:23
 // to different architectures.
 struct Sigset {
   sigset_t nativeSigset;
----------------
MaskRay wrote:
> I believe `struct Sigset` is an overengineering. 
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?


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

https://reviews.llvm.org/D75026