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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
