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

[Ben Craig via cfe-commits]

bcraig 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");
----------------
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.

================
Comment at: src/cxa_thread_atexit.cpp:99
@@ +98,3 @@
+
+  if (__cxa_thread_atexit_impl) {
+    return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
----------------
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.

================
Comment at: test/thread_local_destruction_order.pass.cpp:1
@@ +1,2 @@
+//===--------------------- cxa_thread_atexit_test.cpp ---------------------===//
+//
----------------
Nit: file name is wrong here.

================
Comment at: test/thread_local_destruction_order.pass.cpp:48
@@ +47,3 @@
+  thread_local OrderChecker fn_thread_local{0};
+}
+
----------------
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.

================
Comment at: test/thread_local_destruction_order.pass.cpp:54
@@ +53,3 @@
+  std::thread{thread_fn}.join();
+
+  thread_local OrderChecker fn_thread_local{2};
----------------
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.


http://reviews.llvm.org/D21803