[libc-commits] [PATCH] D134567: [libc] Re-enable functions from signal.h and re-enable abort.

[Alex Brachet via Phabricator via libc-commits]

abrachet accepted this revision.
abrachet added inline comments.


================
Comment at: libc/include/llvm-libc-macros/signal-macros.h:12
 
-#include <__llvm-libc-common.h>
+#ifdef __unix__
+#include "linux/signal-macros.h"
----------------
Should this be `__linux__`?


================
Comment at: libc/include/llvm-libc-types/siginfo_t.h:2
+//===-- Definition of siginfo_t type
+//---------------------------------------===//
+//
----------------
This got moved to the next line


================
Comment at: libc/include/llvm-libc-types/siginfo_t.h:27
+  union sigval si_value;
+} siginfo_t;
+
----------------
FWIW, this looks like it's architecture dependent. Admittedly I don't remember the story about why we moved away from using Linux headers. It might be nice to leave around something like
```
#if ! (defined(LLVM_LIBC_ARCH_X86_64) || defined(LLVM_LIBC_ARCH_AARCH64))
#error "this siginfo may not be valid for your architecture"
#endif
```
Though I have no strong preference because it looks like only mips and ia64 have other implementations of this type


================
Comment at: libc/include/llvm-libc-types/struct_sigaction.h:22
   int sa_flags;
+#ifdef __unix__
+  // This field is present on linux for most targets.
----------------
Should this be `__linux__`? From looking at various other man pages (and your comment right bellow) this is seemingly only on Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134567