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

[Alex Brachet via Phabricator via libc-commits]

abrachet marked an inline comment as done.
abrachet 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:
> > 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.
> You seem to argue for perfection and imperfection at the same time (because this patch is trying to reduce the existing imperfection). It is probably getting into subjective opinions at this point so I am going to let @alexbrachet decide if they want to submit it as is, or if they want to wait for a recursive mutex implementation. I am OK with either approach as I don't think "clutter up history" is an issue.
I could add a lock here and make `sigaction` and `sigprocmask` acquire it. But I really wonder if it is worth the added complexity to handle this case. Also, what if the user handler `longjmp`s? Then the next call to abort will be a deadlock too. I don't think there is 0 tradeoff here, it isn't obvious as you make it seem that we must handle this case. The standard says very little about what is required here. @MaskRay if you feel strongly then I will add it, its a trivial change on my end.


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

https://reviews.llvm.org/D76676