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

[Fangrui Song via Phabricator via libc-commits]

MaskRay added inline comments.


================
Comment at: libc/src/stdlib/abort.cpp:34
+  // interrupted by any async signals. The user handler will be run once we
+  // unblock if it was blocked previously.
+  sigset_t set;
----------------
sivachandra wrote:
> MaskRay wrote:
> > abrachet wrote:
> > > MaskRay wrote:
> > > > Another thread may concurrently install a signal handler for SIGABRT or unblock SIGABRT. There needs to be a lock.
> > > Yes this was discussed, we need a recursive lock here but also in sigaction, we don't have those yet. I don't personally think the benefits are worth that complexity anyway though.
> > This suggests that the patches should be applied in a better way. If a mutex is implemented before improving abort, then we don't need to care about this.
> > 
> > I think a non-recursive mutex suffices here.
> Perfect is the enemy of the good: https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good
> FWIW, this albeit incomplete implementation allowed us to make progress with other pieces. Not to claim other pieces are complete or perfect. As I said in another post, the quest for perfection hinders progress, and more importantly, hinders our own learning.
I am not sure this is the excuse to check in the imperfect code at this point. Actually the simply abort() is sufficient for 99.9% use cases. Nobody will sigaction SIGABRT to abort itself. When the other infrastructure is not ready, it is premature to over-optimize abort. You can disregard it but that will eventually clutter up the history of llvm-libc.

A messy history will just hinder learning for posterity, which may be a lot of people.


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

https://reviews.llvm.org/D76676