[PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation

[Tavian Barnes via cfe-commits]

tavianator added inline comments.

================
Comment at: src/cxa_thread_atexit.cpp:47
@@ +46,3 @@
+    // called during the loop.
+    if (pthread_setspecific(dtors, ptr) != 0) {
+      abort_message("pthread_setspecific() failed during thread_local destruction");
----------------
bcraig wrote:
> The loop doesn't read pthread_getspecific anymore.  I get the need for the setspecific call here for your previous design, but I don't think it's needed now.
__cxa_thread_atexit() calls pthread_getspecific(), so it's still needed.  Otherwise it would create a new list instead of adding to the current one, and the ordering would be wrong.

================
Comment at: src/cxa_thread_atexit.cpp:99
@@ +98,3 @@
+
+  if (__cxa_thread_atexit_impl) {
+    return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
----------------
bcraig wrote:
> I'm going to have to agree with @majnemer.  I think that the config check for LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL should stay in place.  If cxa_thread_atexit_impl exists, then all of the fallback code can disappear at preprocessing time.
> 
> We do lose out on the minor benefit of avoiding some libc++ recompiles, but we also avoid code bloat.
> 
> For what it's worth, I'm willing to keep the weak symbol check in place if __cxa_thread_atexit_impl isn't present, I just don't want to pay for the fallback when I know I'm not going to use it.
Makes sense, I'll do that.

================
Comment at: test/thread_local_destruction_order.pass.cpp:48
@@ +47,3 @@
+  thread_local OrderChecker fn_thread_local{0};
+}
+
----------------
bcraig wrote:
> Can we have a CreatesThreadLocalInDestructor in the thread_fn as well?  That way we can test both the main function and a pthread.  If I understand your code and comments correctly, those go through different code paths.
Yep, meant to do that actually!

================
Comment at: test/thread_local_destruction_order.pass.cpp:54
@@ +53,3 @@
+  std::thread{thread_fn}.join();
+
+  thread_local OrderChecker fn_thread_local{2};
----------------
bcraig wrote:
> In the places where you can, validate that dtors actually are getting called.  This may be your only place where you can do that.
> 
> So something like 'assert(seq == 1)' here.
Sounds good.  What I wanted to do was print some output in the destructors, and check for a certain expected output.  But I couldn't figure out how to do that with lit.


http://reviews.llvm.org/D21803