[libc-commits] [PATCH] D76676: [libc] Unblock SIGABRT before raising in abort

[Paula Toth via Phabricator via libc-commits]

PaulkaToast added inline comments.


================
Comment at: libc/src/stdlib/abort.cpp:23
+
 void LLVM_LIBC_ENTRYPOINT(abort)() {
+  sigset_t set;
----------------
sivachandra wrote:
> abrachet wrote:
> > The previous comment mentioned locks, it's a little more complicated than I originally imagined. We would also need locks in `sigaction` to ensure if one thread is aborting no others can change `SIGABRT`'s action. Even with the recent mutex patch they still cannot be used because as previously explained in the comment we need recursive mutexes. I could add a comment as a TODO here (and in sigaction) but I wonder how much we care about this behavior, so I have waited before adding such a comment.
> I agree. May be you can add a comment saying we will not worry about MT-safe behavior for now? And also explain how it should all work when we have the recursive mutex available?
I agree that the recursive case is much more likely to occur. I don't feel there is a need to complicate with the multi-thread cases. The only difference I see it would exit with a `SIGKILL` instead of a `SIGABRT`, right?


================
Comment at: libc/src/stdlib/abort.cpp:27
+  __llvm_libc::sigaddset(&set, SIGABRT);
+  __llvm_libc::sigprocmask(SIG_UNBLOCK, &set, nullptr);
+
----------------
sivachandra wrote:
> Is an unblock required before the first raise? If the application blocked it, the first `raise` would just return, no? May be this is related to my other comment below.
The standard doesn't specify if we should unblock, so it might make our code simpler to remove this line. Then maybe the recursive situation is handled without the need for the `if (!called)` because the raise function blocks all incoming signals?


================
Comment at: libc/src/stdlib/abort.cpp:38
+
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
   __llvm_libc::raise(SIGABRT);
----------------
sivachandra wrote:
> Why not use `sigaction`?
I think it would be nice to have a comment here explaining the reason why we install the default sigabrt handler after raising sigabrt above.

Maybe even quote the relevant part of the standard?
> The `abort` function causes abnormal program termination to occur, unless the signal `SIGABRT` is being caught and the signal handler does not return

ie this is handling the case where the `SIGABRT` is caught but the handler does return, so we must terminate the program.


================
Comment at: libc/src/stdlib/abort.cpp:39
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
   __llvm_libc::raise(SIGABRT);
+
----------------
Aside from the multi-thread case, I think there is a single thread re-entry issue here. A signal can be raised in between the execution of this line and the previous line (e.g. from an external program or alarm) that installs the default handler. A possible solution might be to block all signals for this segment?  


================
Comment at: libc/src/stdlib/abort.cpp:41
+
   __llvm_libc::raise(SIGKILL);
   __llvm_libc::_Exit(127);
----------------
I think add a comment to explain this is our last ditch effort to kill the program if all other attempts have failed.


================
Comment at: libc/test/src/stdlib/abort_test.cpp:54
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
 }
----------------
I'd like to see a test-case for the single thread re-entry issue, however I'm not entirely sure if that can be reliably tested. Mind looking into it? If its not possible that's fine. (:


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

https://reviews.llvm.org/D76676