[PATCH] D39619: Correct atexit(3) support in TSan/NetBSD

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

I guess a late comment, but now I am thinking that we probably should just call Acquire/Release on some global var and remove



================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:413
+  // Pop AtExitCtx from the top of the stack of callback functions
+  uptr element = interceptor_ctx()->AtExitStack.Size() - 1;
+  AtExitCtx *ctx = interceptor_ctx()->AtExitStack[element];
----------------
krytarowski wrote:
> dvyukov wrote:
> > This also needs the mutex for 2 reasons:
> > 1. It can race with setup_at_exit_wrapper in other threads.
> > 2. You push onto the stack _after_ calling __cxa_atexit, so if another thread calls exit in between, at_exit_wrapper can see an empty stack.
> atexit(3) are global, not per-thread.
> 
> I'm not convinced that races for 1 can impact execution of `at_exit_wrapper()`.
> 
> I agree that someone might try to keep registering new atexit(3) callbacks inside atexit(3) callback functions. I don't know whether this behavior is even defined or real-world.  I think that function-scoped mutex can cause deadlock with callback registration mutex, so I will protect only the access to `AtExitStack`.
> I'm not convinced that races for 1 can impact execution of at_exit_wrapper().

Why? One thread executes callbacks, another registers new callbacks. AtExitStack is corrupted as the result. Everything explodes.

> I agree that someone might try to keep registering new atexit(3) callbacks inside atexit(3) callback functions. 

That's less of an issue. The problem are other threads that know nothing about process existing yet and continue registering callbacks.


Repository:
  rL LLVM

https://reviews.llvm.org/D39619