Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 71934.
tavianator marked an inline comment as done.
tavianator added a comment.
s/indended/intended/
https://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,56 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+template
+class CreatesThreadLocalInDestructor {
+public:
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{ID};
+ }
+};
+
+OrderChecker global{7};
+
+void thread_fn() {
+ static OrderChecker fn_static{5};
+ thread_local CreatesThreadLocalInDestructor<2> creates_tl2;
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor<0> creates_tl0;
+}
+
+int main() {
+ static OrderChecker fn_static{6};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 3);
+
+ thread_local OrderChecker fn_thread_local{4};
+ thread_local CreatesThreadLocalInDestructor<3> creates_tl;
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,133 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol is used to detect this function's presence in the C library
+ // at runtime, even if libc++ is built against an older libc
+ __attribute__((__weak__))
+#endif
+ int __cxa_thread_atexit_impl(Dtor, void*, void*)
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks for the patch.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+
+ void run_dtors(void*) {
+while (auto head = dtors) {
Na I thought I was solving a bug that didn't exist. You give me too much credit.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator marked 5 inline comments as done.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
EricWF wrote:
> tavianator wrote:
> > EricWF wrote:
> > > tavianator wrote:
> > > > EricWF wrote:
> > > > > There is a bug here. If `head->next == nullptr` and if
> > > > > `head->dtor(head->obj))` creates a TL variable in the destructor then
> > > > > that destructor will not be invoked.
> > > > >
> > > > > Here's an updated test case which catches the bug:
> > > > > https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
> > > > I can't reproduce that failure here, your exact test case passes (even
> > > > with `#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test
> > > > commented out).
> > > >
> > > > Tracing the implementation logic, it seems correct. If `head->next ==
> > > > nullptr` then this line does `dtors = nullptr`. Then if
> > > > `head->dtor(head->obj)` registers a new `thread_local`,
> > > > `__cxa_thread_atexit()` does `head = malloc(...); ... dtors = head;`.
> > > > Then the next iteration of the loop `while (auto head = dtors) {` picks
> > > > up that new node.
> > > >
> > > > Have I missed something?
> > > I can't reproduce this morning either, I must have been doing something
> > > funny. I'll look at this with a fresh head tomorrow. If I can't find
> > > anything this will be good to go. Thanks for working on this.
> > No problem! I can integrate your updated test case anyway if you want.
> Yeah I would like to see the upgraded test case applied. At least that way
> we're testing the case in question.
>
> So I agree with your above analysis of what happens, and that all destructors
> are correctly called during the first iteration of pthread key destruction.
> My one issues is that we still register a new non-null key which forces
> pthread to run the destructor for the key again. I would like to see this
> fixed.
Yep, done! I guess that was the point of `thread_alive` from your original
patch-to-my-patch, sorry for stripping it out.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 71508.
tavianator added a comment.
Herald added subscribers: mgorny, beanz.
Integrated @EricWF's expanded test case, and avoid an unneeded
pthread_setspecific() call if the last thread_local's destructor initializes a
new thread_local.
https://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,56 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+template
+class CreatesThreadLocalInDestructor {
+public:
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{ID};
+ }
+};
+
+OrderChecker global{7};
+
+void thread_fn() {
+ static OrderChecker fn_static{5};
+ thread_local CreatesThreadLocalInDestructor<2> creates_tl2;
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor<0> creates_tl0;
+}
+
+int main() {
+ static OrderChecker fn_static{6};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 3);
+
+ thread_local OrderChecker fn_thread_local{4};
+ thread_local CreatesThreadLocalInDestructor<3> creates_tl;
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,133 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol is used to detect this function's presence in the C library
+ // at runtim
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added a comment.
LGTM after addressing the inline comments.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
tavianator wrote:
> EricWF wrote:
> > tavianator wrote:
> > > EricWF wrote:
> > > > There is a bug here. If `head->next == nullptr` and if
> > > > `head->dtor(head->obj))` creates a TL variable in the destructor then
> > > > that destructor will not be invoked.
> > > >
> > > > Here's an updated test case which catches the bug:
> > > > https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
> > > I can't reproduce that failure here, your exact test case passes (even
> > > with `#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test
> > > commented out).
> > >
> > > Tracing the implementation logic, it seems correct. If `head->next ==
> > > nullptr` then this line does `dtors = nullptr`. Then if
> > > `head->dtor(head->obj)` registers a new `thread_local`,
> > > `__cxa_thread_atexit()` does `head = malloc(...); ... dtors = head;`.
> > > Then the next iteration of the loop `while (auto head = dtors) {` picks
> > > up that new node.
> > >
> > > Have I missed something?
> > I can't reproduce this morning either, I must have been doing something
> > funny. I'll look at this with a fresh head tomorrow. If I can't find
> > anything this will be good to go. Thanks for working on this.
> No problem! I can integrate your updated test case anyway if you want.
Yeah I would like to see the upgraded test case applied. At least that way
we're testing the case in question.
So I agree with your above analysis of what happens, and that all destructors
are correctly called during the first iteration of pthread key destruction. My
one issues is that we still register a new non-null key which forces pthread to
run the destructor for the key again. I would like to see this fixed.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added inline comments.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
EricWF wrote:
> tavianator wrote:
> > EricWF wrote:
> > > There is a bug here. If `head->next == nullptr` and if
> > > `head->dtor(head->obj))` creates a TL variable in the destructor then
> > > that destructor will not be invoked.
> > >
> > > Here's an updated test case which catches the bug:
> > > https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
> > I can't reproduce that failure here, your exact test case passes (even with
> > `#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test commented
> > out).
> >
> > Tracing the implementation logic, it seems correct. If `head->next ==
> > nullptr` then this line does `dtors = nullptr`. Then if
> > `head->dtor(head->obj)` registers a new `thread_local`,
> > `__cxa_thread_atexit()` does `head = malloc(...); ... dtors = head;`. Then
> > the next iteration of the loop `while (auto head = dtors) {` picks up that
> > new node.
> >
> > Have I missed something?
> I can't reproduce this morning either, I must have been doing something
> funny. I'll look at this with a fresh head tomorrow. If I can't find anything
> this will be good to go. Thanks for working on this.
No problem! I can integrate your updated test case anyway if you want.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added inline comments.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
tavianator wrote:
> EricWF wrote:
> > There is a bug here. If `head->next == nullptr` and if
> > `head->dtor(head->obj))` creates a TL variable in the destructor then that
> > destructor will not be invoked.
> >
> > Here's an updated test case which catches the bug:
> > https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
> I can't reproduce that failure here, your exact test case passes (even with
> `#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test commented
> out).
>
> Tracing the implementation logic, it seems correct. If `head->next ==
> nullptr` then this line does `dtors = nullptr`. Then if
> `head->dtor(head->obj)` registers a new `thread_local`,
> `__cxa_thread_atexit()` does `head = malloc(...); ... dtors = head;`. Then
> the next iteration of the loop `while (auto head = dtors) {` picks up that
> new node.
>
> Have I missed something?
I can't reproduce this morning either, I must have been doing something funny.
I'll look at this with a fresh head tomorrow. If I can't find anything this
will be good to go. Thanks for working on this.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added inline comments.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
EricWF wrote:
> There is a bug here. If `head->next == nullptr` and if
> `head->dtor(head->obj))` creates a TL variable in the destructor then that
> destructor will not be invoked.
>
> Here's an updated test case which catches the bug:
> https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
I can't reproduce that failure here, your exact test case passes (even with
`#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test commented out).
Tracing the implementation logic, it seems correct. If `head->next == nullptr`
then this line does `dtors = nullptr`. Then if `head->dtor(head->obj)`
registers a new `thread_local`, `__cxa_thread_atexit()` does `head =
malloc(...); ... dtors = head;`. Then the next iteration of the loop `while
(auto head = dtors) {` picks up that new node.
Have I missed something?
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added a comment.
LGTM modulo bug fix.
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
There is a bug here. If `head->next == nullptr` and if `head->dtor(head->obj))`
creates a TL variable in the destructor then that destructor will not be
invoked.
Here's an updated test case which catches the bug:
https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 70406.
tavianator added a comment.
Uses a __thread variable to hold the destructor list, as @EricWF suggested.
https://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+class CreatesThreadLocalInDestructor {
+public:
+ CreatesThreadLocalInDestructor(int n) : n_{n} { }
+
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{n_};
+ }
+
+private:
+ int n_;
+};
+
+OrderChecker global{6};
+
+void thread_fn() {
+ static OrderChecker fn_static{4};
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor creates_tl{0};
+}
+
+int main() {
+ static OrderChecker fn_static{5};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 2);
+
+ thread_local OrderChecker fn_thread_local{3};
+ thread_local CreatesThreadLocalInDestructor creates_tl{2};
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,129 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol is used to detect this function's presence in the C library
+ // at runtime, even if libc++ is built against an older libc
+ __attribute__((__weak__))
+#endif
+ int __cxa_thread_atexit_impl(Dtor,
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added inline comments.
Comment at: src/cxa_thread_atexit.cpp:42
@@ +41,3 @@
+ // - thread_local destructors on other threads run on the first iteration
+ // through the pthread_key destructors. std::notify_all_at_thread_exit()
+ // and similar functions must be careful to wait until the second iteration
tavianator wrote:
> EricWF wrote:
> > Can you clarify what you mean by "other threads"?
> >
> > How is libc++ supposed to detect and handle this problem?
> I meant "non-main threads" ("other" is in relation to the bullet point
> above), but I can clarify this, sure.
>
> libc++ could be patched to do something like this:
>
> ```
> pthread_key_t key1, key2;
>
> void destructor1(void* ptr) {
> pthread_setspecific(key2, ptr);
> }
>
> void destructor2(void* ptr) {
> // Runs in the second iteration through pthread_key destructors,
> // therefore after thread_local destructors
> }
>
> pthread_key_create(&key1, destructor1);
> pthread_key_create(&key2, destructor2);
>
> pthread_setspecific(key1, ptr);
> ```
>
> (Or it could use a counter/flag and a single pthread_key.)
>
> libstdc++ has the same bug when __cxa_thread_atexit_impl() isn't available,
> so I'm not sure that change would really be necessary. If it is, I can write
> up the libc++ patch.
I don't think we need to patch this in libc++. Especially because it would be
incorrect in the vast majority of cases.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment.
In https://reviews.llvm.org/D21803#532309, @EricWF wrote:
> `__thread`
>
> What do you think of this idea?
Makes sense to me, I'll integrate it into the next revision.
Comment at: src/cxa_thread_atexit.cpp:42
@@ +41,3 @@
+ // - thread_local destructors on other threads run on the first iteration
+ // through the pthread_key destructors. std::notify_all_at_thread_exit()
+ // and similar functions must be careful to wait until the second iteration
EricWF wrote:
> Can you clarify what you mean by "other threads"?
>
> How is libc++ supposed to detect and handle this problem?
I meant "non-main threads" ("other" is in relation to the bullet point above),
but I can clarify this, sure.
libc++ could be patched to do something like this:
```
pthread_key_t key1, key2;
void destructor1(void* ptr) {
pthread_setspecific(key2, ptr);
}
void destructor2(void* ptr) {
// Runs in the second iteration through pthread_key destructors,
// therefore after thread_local destructors
}
pthread_key_create(&key1, destructor1);
pthread_key_create(&key2, destructor2);
pthread_setspecific(key1, ptr);
```
(Or it could use a counter/flag and a single pthread_key.)
libstdc++ has the same bug when __cxa_thread_atexit_impl() isn't available, so
I'm not sure that change would really be necessary. If it is, I can write up
the libc++ patch.
https://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
jroelofs added a comment. In https://reviews.llvm.org/D21803#532382, @EricWF wrote: > In https://reviews.llvm.org/D21803#532347, @jroelofs wrote: > > > In https://reviews.llvm.org/D21803#532309, @EricWF wrote: > > > > > `__thread` > > > > > > What do you think of this idea? > > > > > > You'll have to guard it against all the platforms that don't support TLS. > > Darwin 10.6 is one of them. > > > Which is fine because we shouldn't supply a definition of > `__cxa_thread_atexit` on those platforms. Ah, ok. Sounds good. https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added a comment. In https://reviews.llvm.org/D21803#532347, @jroelofs wrote: > In https://reviews.llvm.org/D21803#532309, @EricWF wrote: > > > `__thread` > > > > What do you think of this idea? > > > You'll have to guard it against all the platforms that don't support TLS. > Darwin 10.6 is one of them. Which is fine because we shouldn't supply a definition of `__cxa_thread_atexit` on those platforms. We might have to guard against it to keep libc++abi building, but if the platform doesn't support TLS then https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
jroelofs added a comment. In https://reviews.llvm.org/D21803#532309, @EricWF wrote: > `__thread` > > What do you think of this idea? You'll have to guard it against all the platforms that don't support TLS. Darwin 10.6 is one of them. https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added a comment. We can perform far fewer calls to `pthread_getspecific`/`pthread_setspecific` if we represent the list head using a global `__thread DtorList* list_head = nullptr`. This also allows us to avoid the hack of setting/unsetting the key during `run_dtors()` which I really do not like. Here is a patch that applies such changes: https://gist.github.com/EricWF/a071376b1216aabdd1695eec2175c374 What do you think of this idea? Comment at: src/cxa_thread_atexit.cpp:42 @@ +41,3 @@ + // - thread_local destructors on other threads run on the first iteration + // through the pthread_key destructors. std::notify_all_at_thread_exit() + // and similar functions must be careful to wait until the second iteration Can you clarify what you mean by "other threads"? How is libc++ supposed to detect and handle this problem? https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
EricWF added a comment. I'll look at this within the hour. https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. The "@" will do a ping through phabricator, but a direct email is probably going to be your best bet at this point. https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment. In https://reviews.llvm.org/D21803#530681, @bcraig wrote: > In https://reviews.llvm.org/D21803#530678, @tavianator wrote: > > > Ping? > > > Well, I still think it's fine. Maybe a direct message to @mclow.lists or > @EricWF? Is there a way to do that through Phabricator? Or did you mean to email them directly? (Not sure what their emails are but I can probably figure it out from the list history.) https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. In https://reviews.llvm.org/D21803#530678, @tavianator wrote: > Ping? Well, I still think it's fine. Maybe a direct message to @mclow.lists or @EricWF? https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment. Ping? https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment. Anything else I need to do for this patch? https://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 63806.
tavianator added a comment.
Make sure the tail of the list is null.
http://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+class CreatesThreadLocalInDestructor {
+public:
+ CreatesThreadLocalInDestructor(int n) : n_{n} { }
+
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{n_};
+ }
+
+private:
+ int n_;
+};
+
+OrderChecker global{6};
+
+void thread_fn() {
+ static OrderChecker fn_static{4};
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor creates_tl{0};
+}
+
+int main() {
+ static OrderChecker fn_static{5};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 2);
+
+ thread_local OrderChecker fn_thread_local{3};
+ thread_local CreatesThreadLocalInDestructor creates_tl{2};
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,151 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol is used to detect this function's presence in the C library
+ // at runtime, even if libc++ is built against an older libc
+ __attribute__((__weak__))
+#endif
+ int __cxa_thread_atexit_impl(Dtor, void*, void*);
+
+#ifndef HAVE___CXA_
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 63801.
tavianator added a comment.
Update comments to mention that late-initialized thread_locals invoke undefined
behavior.
http://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+class CreatesThreadLocalInDestructor {
+public:
+ CreatesThreadLocalInDestructor(int n) : n_{n} { }
+
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{n_};
+ }
+
+private:
+ int n_;
+};
+
+OrderChecker global{6};
+
+void thread_fn() {
+ static OrderChecker fn_static{4};
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor creates_tl{0};
+}
+
+int main() {
+ static OrderChecker fn_static{5};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 2);
+
+ thread_local OrderChecker fn_thread_local{3};
+ thread_local CreatesThreadLocalInDestructor creates_tl{2};
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,150 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol is used to detect this function's presence in the C library
+ // at runtime, even if libc++ is built against an older libc
+ __attribute__((__weak__))
+#endif
+ int __cxa_thread_atex
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. LGTM (with a comment nit), but you'll need to get approval from @ericwf or @mclow.lists. I would like some of the information from your stack overflow post to make it's way to the comments. In particular, I think I would like to see it documented that we have made a choice for some undefined behavior. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator marked 9 inline comments as done. tavianator added a comment. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 63232.
tavianator marked 6 inline comments as done.
tavianator added a comment.
- Bring back HAVE___CXA_THREAD_ATEXIT_IMPL, and avoid the weak symbol/fallback
implementation in that case
- Fix a leak in an error path
- Add a CreatesThreadLocalInDestructor to a non-main thread in the destructor
ordering test
http://reviews.llvm.org/D21803
Files:
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===-- thread_local_destruction_order.pass.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker {
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+class CreatesThreadLocalInDestructor {
+public:
+ CreatesThreadLocalInDestructor(int n) : n_{n} { }
+
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{n_};
+ }
+
+private:
+ int n_;
+};
+
+OrderChecker global{6};
+
+void thread_fn() {
+ static OrderChecker fn_static{4};
+ thread_local OrderChecker fn_thread_local{1};
+ thread_local CreatesThreadLocalInDestructor creates_tl{0};
+}
+
+int main() {
+ static OrderChecker fn_static{5};
+
+ std::thread{thread_fn}.join();
+ assert(seq == 2);
+
+ thread_local OrderChecker fn_thread_local{3};
+ thread_local CreatesThreadLocalInDestructor creates_tl{2};
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,138 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
-extern "C" {
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+ using Dtor = void(*)(void*);
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
-void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern "C"
+#ifndef HAVE___CXA_THREAD_ATEXIT_IMPL
+ // A weak symbol
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added inline comments.
Comment at: test/thread_local_destruction_order.pass.cpp:54
@@ +53,3 @@
+ std::thread{thread_fn}.join();
+
+ thread_local OrderChecker fn_thread_local{2};
tavianator wrote:
> 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.
Normally, you would do that by piping the output to the llvm FileCheck utility.
My unconfirmed suspicion is that that approach will run into difficulties
because of libcxx and libcxxabi specific setups.
I think just having the global object's dtor check is good enough for the final
post condition though. I'm not terribly worried about global dtors
malfunctioning.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 62691.
tavianator added a comment.
Added a test case for destructor ordering. Got rid of
pthread_{get,set}specific in a loop.
http://reviews.llvm.org/D21803
Files:
cmake/config-ix.cmake
src/CMakeLists.txt
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
test/thread_local_destruction_order.pass.cpp
Index: test/thread_local_destruction_order.pass.cpp
===
--- test/thread_local_destruction_order.pass.cpp
+++ test/thread_local_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===- cxa_thread_atexit_test.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+#include
+#include
+
+int seq = 0;
+
+class OrderChecker
+{
+public:
+ explicit OrderChecker(int n) : n_{n} { }
+
+ ~OrderChecker() {
+assert(seq++ == n_);
+ }
+
+private:
+ int n_;
+};
+
+class CreatesThreadLocalInDestructor
+{
+public:
+ CreatesThreadLocalInDestructor(int n) : n_{n} { }
+
+ ~CreatesThreadLocalInDestructor() {
+thread_local OrderChecker checker{n_};
+ }
+
+private:
+ int n_;
+};
+
+OrderChecker global{5};
+
+void thread_fn() {
+ static OrderChecker fn_static{3};
+ thread_local OrderChecker fn_thread_local{0};
+}
+
+int main() {
+ static OrderChecker fn_static{4};
+
+ std::thread{thread_fn}.join();
+
+ thread_local OrderChecker fn_thread_local{2};
+ thread_local CreatesThreadLocalInDestructor creates_tl{1};
+
+ return 0;
+}
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,126 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
+namespace {
+ // This implementation is used if the C library does not provide
+ // __cxa_thread_atexit_impl() for us. It has a number of limitations that are
+ // difficult to impossible to address without ..._impl():
+ //
+ // - dso_symbol is ignored. This means that a shared library may be unloaded
+ // (via dlclose()) before its thread_local destructors have run.
+ //
+ // - thread_local destructors for the main thread are run by the destructor of
+ // a static object. This is later than expected; they should run before the
+ // destructors of any objects with static storage duration.
+ //
+ // - thread_local destructors on other thr
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. Also, can you add test cases for a lot of these things? I don't expect test cases for the DSO side of things, but a lot of the tricky atexit cases should be covered. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added inline comments.
Comment at: src/cxa_thread_atexit.cpp:47
@@ +46,3 @@
+ void run_dtors(void* ptr) {
+if (pthread_setspecific(dtors, ptr) != 0) {
+ abort_message("pthread_setspecific() failed during thread_local
destruction");
bcraig wrote:
> Why are we doing this? I can see it being a little useful when debugging /
> developing, so that you get an early warning that something has gone wrong,
> but it seems like this will always be setting a value to the value it already
> has.
pthread_key destructors run after the key is set to null. I re-set it here
since the loop reads the key.
Comment at: src/cxa_thread_atexit.cpp:54
@@ +53,3 @@
+// __cxa_thread_atexit() is called during the loop.
+while (auto elem = static_cast(pthread_getspecific(dtors))) {
+ if (pthread_setspecific(dtors, elem->next) != 0) {
bcraig wrote:
> Maybe this concern is unfounded, but I'm not overly fond of
> pthread_getspecific and setspecific in a loop. I've always been under the
> impression that those functions are rather slow. Could we add a layer of
> indirection so that we don't need to call getspecific and setspecific so
> often? Basically make the pointer that is directly stored in TLS an
> immutable pointer to pointer.
Sure, I can do that. Would reduce the number of setspecific() calls in
__cxa_thread_atexit too.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added inline comments.
Comment at: src/cxa_thread_atexit.cpp:47
@@ +46,3 @@
+ void run_dtors(void* ptr) {
+if (pthread_setspecific(dtors, ptr) != 0) {
+ abort_message("pthread_setspecific() failed during thread_local
destruction");
Why are we doing this? I can see it being a little useful when debugging /
developing, so that you get an early warning that something has gone wrong, but
it seems like this will always be setting a value to the value it already has.
Comment at: src/cxa_thread_atexit.cpp:54
@@ +53,3 @@
+// __cxa_thread_atexit() is called during the loop.
+while (auto elem = static_cast(pthread_getspecific(dtors))) {
+ if (pthread_setspecific(dtors, elem->next) != 0) {
Maybe this concern is unfounded, but I'm not overly fond of pthread_getspecific
and setspecific in a loop. I've always been under the impression that those
functions are rather slow. Could we add a layer of indirection so that we
don't need to call getspecific and setspecific so often? Basically make the
pointer that is directly stored in TLS an immutable pointer to pointer.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 62394.
tavianator added a comment.
Fix copy-pasta that result in an infinite loop.
http://reviews.llvm.org/D21803
Files:
cmake/config-ix.cmake
src/CMakeLists.txt
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,137 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
+namespace {
+ // This implementation is used if the C library does not provide
+ // __cxa_thread_atexit_impl() for us. It has a number of limitations that are
+ // difficult to impossible to address without ..._impl():
+ //
+ // - dso_symbol is ignored. This means that a shared library may be unloaded
+ // (via dlclose()) before its thread_local destructors have run.
+ //
+ // - thread_local destructors for the main thread are run with __cxa_atexit().
+ // This is later than expected; they should run before the destructors of
+ // any objects with static storage duration.
+ //
+ // - thread_local destructors on other threads run on the first iteration
+ // through the pthread_key destructors. std::notify_all_at_thread_exit()
+ // and similar functions must be careful to wait until the second iteration
+ // to provide their indended ordering guarantees.
+
+ typedef void (*Dtor)(void*);
+
+ struct DtorList {
+Dtor dtor;
+void* obj;
+DtorList* next;
+ };
+
+ pthread_key_t dtors;
+ pthread_once_t dtors_once = PTHREAD_ONCE_INIT;
+ bool dtors_ready = false;
+ bool dtors_atexit = false;
+
+ void run_dtors(void* ptr) {
+if (pthread_setspecific(dtors, ptr) != 0) {
+ abort_message("pthread_setspecific() failed during thread_local destruction");
+}
+
+// Rather than iterate over the list directly, use the pthread_key to get
+// the head of the list every time. This gives the correct ordering if
+// __cxa_thread_atexit() is called during the loop.
+while (auto elem = static_cast(pthread_getspecific(dtors))) {
+ if (pthread_setspecific(dtors, elem->next) != 0) {
+abort_message("pthread_setspecific() failed during thread_local destruction");
+ }
+ elem->dtor(elem->obj);
+ std::free(elem);
+}
+ }
+
+ void run_dtors_atexit(void*) {
+// Signify that we need to re-register this function if any new
+// thread_locals are created.
+dtors_atexit = false;
+
+auto ptr = pthread_getspecific(dtors);
+run_dtors(ptr);
+ }
+
+ // This is the DSO handle for libc++abi.so itself
+ extern "C" void* __dso_handle;
+
+ void dtors_
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 62388.
tavianator added a comment.
Added missing __dso_handle declaration.
http://reviews.llvm.org/D21803
Files:
cmake/config-ix.cmake
src/CMakeLists.txt
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,137 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
+namespace {
+ // This implementation is used if the C library does not provide
+ // __cxa_thread_atexit_impl() for us. It has a number of limitations that are
+ // difficult to impossible to address without ..._impl():
+ //
+ // - dso_symbol is ignored. This means that a shared library may be unloaded
+ // (via dlclose()) before its thread_local destructors have run.
+ //
+ // - thread_local destructors for the main thread are run with __cxa_atexit().
+ // This is later than expected; they should run before the destructors of
+ // any objects with static storage duration.
+ //
+ // - thread_local destructors on other threads run on the first iteration
+ // through the pthread_key destructors. std::notify_all_at_thread_exit()
+ // and similar functions must be careful to wait until the second iteration
+ // to provide their indended ordering guarantees.
+
+ typedef void (*Dtor)(void*);
+
+ struct DtorList {
+Dtor dtor;
+void* obj;
+DtorList* next;
+ };
+
+ pthread_key_t dtors;
+ pthread_once_t dtors_once = PTHREAD_ONCE_INIT;
+ bool dtors_ready = false;
+ bool dtors_atexit = false;
+
+ void run_dtors(void* ptr) {
+if (pthread_setspecific(dtors, ptr) != 0) {
+ abort_message("pthread_setspecific() failed during thread_local destruction");
+}
+
+// Rather than iterate over the list directly, use the pthread_key to get
+// the head of the list every time. This gives the correct ordering if
+// __cxa_thread_atexit() is called during the loop.
+while (auto elem = static_cast(pthread_getspecific(dtors))) {
+ if (pthread_setspecific(dtors, ptr) != 0) {
+abort_message("pthread_setspecific() failed during thread_local destruction");
+ }
+ elem->dtor(elem->obj);
+ std::free(elem);
+}
+ }
+
+ void run_dtors_atexit(void*) {
+// Signify that we need to re-register this function if any new
+// thread_locals are created.
+dtors_atexit = false;
+
+auto ptr = pthread_getspecific(dtors);
+run_dtors(ptr);
+ }
+
+ // This is the DSO handle for libc++abi.so itself
+ extern "C" void* __dso_handle;
+
+ void dtors_init() {
+/
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator marked 3 inline comments as done.
tavianator added a comment.
In http://reviews.llvm.org/D21803#470448, @bcraig wrote:
> What that means for this implementation is that I think that
> _cxa_thread_atexit is allowed to be called during run_dtors. If running the
> dtor for a thread local variable 'cat', we encounter a previously unseen
> thread_local 'dog', the compiler will call the ctor, then register the dtor
> with _cxa_thread_atexit. Since it is the most recently constructed thread
> local object, I would expect the 'dog' dtor to be the next dtor to be run.
> You may be able to support this just by moving "elem = elem->next" below the
> dtor invocation.
It wasn't quite that easy (have to re-look at the pthread_key to get newly
added thread_locals), but that's done in the latest patch.
Comment at: src/cxa_thread_atexit.cpp:115
@@ +114,3 @@
+if (!dtors_atexit) {
+ if (__cxa_atexit(run_dtors_atexit, NULL, __dso_handle) != 0) {
+return -1;
See http://stackoverflow.com/q/38130185/502399 for a test case that would
trigger this. This may not be necessary depending on the answer to that
question.
Comment at: src/cxa_thread_atexit.cpp:122
@@ +121,3 @@
+auto head = static_cast(std::malloc(sizeof(DtorList)));
+if (!head) {
+ return -1;
This has changed somewhat in the latest patch, but the gist is similar. If
libc++abi.so is dlclose()d, there had better not be any still-running threads
that expect to execute thread_local destructors (or any other C++ code, for
that matter). In the usual case (libc++abi.so loaded at startup, not by a
later dlopen()), the last run_dtors() call happens as the final thread is
exiting.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 62386.
tavianator added a comment.
Fixed some corner cases regarding destruction order and very-late-initialized
thread_locals. Explicitly documented the known limitations compared to
__cxa_thread_atexit_impl().
http://reviews.llvm.org/D21803
Files:
cmake/config-ix.cmake
src/CMakeLists.txt
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -7,20 +7,134 @@
//
//===--===//
+#include "abort_message.h"
#include "cxxabi.h"
+#include
+#include
namespace __cxxabiv1 {
+namespace {
+ // This implementation is used if the C library does not provide
+ // __cxa_thread_atexit_impl() for us. It has a number of limitations that are
+ // difficult to impossible to address without ..._impl():
+ //
+ // - dso_symbol is ignored. This means that a shared library may be unloaded
+ // (via dlclose()) before its thread_local destructors have run.
+ //
+ // - thread_local destructors for the main thread are run with __cxa_atexit().
+ // This is later than expected; they should run before the destructors of
+ // any objects with static storage duration.
+ //
+ // - thread_local destructors on other threads run on the first iteration
+ // through the pthread_key destructors. std::notify_all_at_thread_exit()
+ // and similar functions must be careful to wait until the second iteration
+ // to provide their indended ordering guarantees.
+
+ typedef void (*Dtor)(void*);
+
+ struct DtorList {
+Dtor dtor;
+void* obj;
+DtorList* next;
+ };
+
+ pthread_key_t dtors;
+ pthread_once_t dtors_once = PTHREAD_ONCE_INIT;
+ bool dtors_ready = false;
+ bool dtors_atexit = false;
+
+ void run_dtors(void* ptr) {
+if (pthread_setspecific(dtors, ptr) != 0) {
+ abort_message("pthread_setspecific() failed during thread_local destruction");
+}
+
+// Rather than iterate over the list directly, use the pthread_key to get
+// the head of the list every time. This gives the correct ordering if
+// __cxa_thread_atexit() is called during the loop.
+while (auto elem = static_cast(pthread_getspecific(dtors))) {
+ if (pthread_setspecific(dtors, ptr) != 0) {
+abort_message("pthread_setspecific() failed during thread_local destruction");
+ }
+ elem->dtor(elem->obj);
+ std::free(elem);
+}
+ }
+
+ void run_dtors_atexit(void*) {
+// Signify that we need to re-register this function if any new
+// thread_locals are created.
+dtors_atexit = false;
+
+auto ptr = pthread_getspecific(dtors);
+r
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
dimitry added inline comments. Comment at: src/cxa_thread_atexit.cpp:46 @@ +45,3 @@ + pthread_key_delete(key_); +} + bcraig wrote: > dimitry wrote: > > run_dtors() is called when/if libc++.so gets unloaded... but only for the > > thread calling dlclose()? > Most of the dtor magic is on the pthread_key_create side. pthreads lets you > register a per-thread destructor. > > This destructor is only run on process termination (I think). I meant the call from ~DtorListHolder() http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. In http://reviews.llvm.org/D21803#470564, @joerg wrote: > On the topic of __cxa_thread_atexit, was it ever specified how it interacts > with things like thread cancellation? I don't think it's officially specified anywhere. C++ threads don't have a cancel method. The POSIX spec doesn't speak about the C++ ABI. The Itanium ABI could talk about this, but hasn't yet. I think this implementation does the right thing with regards to cancellation though. POSIX says that first cancellation cleanup handlers are called, then thread-specific data destructors are called. pthread_cancel is still a really bad idea due to how it (doesn't) interact with RAII, but at least TLS data won't get leaked. Comment at: src/cxa_thread_atexit.cpp:46 @@ +45,3 @@ + pthread_key_delete(key_); +} + dimitry wrote: > run_dtors() is called when/if libc++.so gets unloaded... but only for the > thread calling dlclose()? Most of the dtor magic is on the pthread_key_create side. pthreads lets you register a per-thread destructor. This destructor is only run on process termination (I think). http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
dimitry added a subscriber: dimitry. Comment at: src/cxa_thread_atexit.cpp:46 @@ +45,3 @@ + pthread_key_delete(key_); +} + run_dtors() is called when/if libc++.so gets unloaded... but only for the thread calling dlclose()? http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
joerg added a subscriber: joerg. joerg added a comment. On the topic of __cxa_thread_atexit, was it ever specified how it interacts with things like thread cancellation? http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. > Hmm, maybe? If other global destructors run after ~DtorListHolder(), and > they cause a thread_local to be initialized for the first time, > __cxa_thread_atexit() might be called again. I was thinking that dtors would > get re-initialized in that case but it appears it does not. So yeah, I think > I'll need to leak the pthread_key_t. > > I'm not sure how to avoid leaking the actual thread_local objects that get > created in that situation. There's nothing left to trigger run_dtors() a > second time. I'm not concerned about the loss of memory or pthread_key resources in this leak, as it is a very short-lived leak (the process is going away after all). We do need to have an idea of what happens with the destructor invocations for the other kinds of resources though. I think the C++14 spec says what should happen. > 3.6.3 Termination > > 1. [...] The completions of the destructors for all initialized objects with > thread storage duration within that thread are sequenced before the > initiation of the destructors of any object with static storage duration. If > the completion of the constructor or dynamic initialization of an object with > thread storage duration is sequenced before that of another, the completion > of the destructor of the second is sequenced before the initiation of the > destructor of the first. If the completion of the constructor or dynamic > initialization of an object with static storage duration is sequenced before > that of another, the completion of the destructor of the second is sequenced > before the initiation of the destructor of the first. What that means for this implementation is that I think that __cxa_thread_atexit is allowed to be called during run_dtors. If running the dtor for a thread local variable 'cat', we encounter a previously unseen thread_local 'dog', the compiler will call the ctor, then register the dtor with __cxa_thread_atexit. Since it is the most recently constructed thread local object, I would expect the 'dog' dtor to be the next dtor to be run. You may be able to support this just by moving "elem = elem->next" below the dtor invocation. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment.
In http://reviews.llvm.org/D21803#470086, @bcraig wrote:
> It also intentionally leaks the pthread key. Does the __thread_specific_ptr
> rationale hold for this change as well?
Hmm, maybe? If other global destructors run after ~DtorListHolder(), and they
cause a thread_local to be initialized for the first time,
__cxa_thread_atexit() might be called again. I was thinking that dtors would
get re-initialized in that case but it appears it does not. So yeah, I think
I'll need to leak the pthread_key_t.
I'm not sure how to avoid leaking the actual thread_local objects that get
created in that situation. There's nothing left to trigger run_dtors() a
second time.
Comment at: src/cxa_thread_atexit.cpp:64-90
@@ -18,8 +63,29 @@
+_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void *obj,
void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern int __cxa_thread_atexit_impl(Dtor, void *, void *)
+__attribute__((__weak__));
+
+ if (__cxa_thread_atexit_impl) {
+return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
+ } else {
+static DtorListHolder dtors;
+
+auto head = static_cast(std::malloc(sizeof(DtorList)));
+if (!head) {
+ return -1;
+}
+
+head->dtor = dtor;
+head->obj = obj;
+head->next = dtors.get();
+
+if (!dtors.set(head)) {
+ std::free(head);
+ return -1;
+}
-#endif // HAVE__CXA_THREAD_ATEXIT_IMPL
+return 0;
+ }
+}
} // extern "C"
majnemer wrote:
> I think that this should be an opt-in mechanism, there are platforms that
> presumably never need to pay the cost of the unused code (macOS comes to
> mind).
This file is only built for UNIX AND NOT (APPLE OR CYGWIN). Other platforms
use something other than __cxa_thread_atexit() I assume.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
majnemer added inline comments.
Comment at: src/cxa_thread_atexit.cpp:64-90
@@ -18,8 +63,29 @@
+_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void *obj,
void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern int __cxa_thread_atexit_impl(Dtor, void *, void *)
+__attribute__((__weak__));
+
+ if (__cxa_thread_atexit_impl) {
+return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
+ } else {
+static DtorListHolder dtors;
+
+auto head = static_cast(std::malloc(sizeof(DtorList)));
+if (!head) {
+ return -1;
+}
+
+head->dtor = dtor;
+head->obj = obj;
+head->next = dtors.get();
+
+if (!dtors.set(head)) {
+ std::free(head);
+ return -1;
+}
-#endif // HAVE__CXA_THREAD_ATEXIT_IMPL
+return 0;
+ }
+}
} // extern "C"
I think that this should be an opt-in mechanism, there are platforms that
presumably never need to pay the cost of the unused code (macOS comes to mind).
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
rmaprath added a comment. In http://reviews.llvm.org/D21803#470060, @tavianator wrote: > In http://reviews.llvm.org/D21803#469988, @bcraig wrote: > > > @rmaprath has been doing some work to make the threading runtime library > > swappable. I don't recall if his work extended to libcxxabi or not, but > > I'll page him anyway. > > > <__threading_support>? Seems to be libc++-specific. There's a few other raw > uses of pthreads in libc++abi. Plan to get started with `libc++abi` as soon as I'm done with `libc++`, I've hit a couple of bumps with the latter, resolving them at the moment. I don't see any new `pthread` dependencies introduced in this patch, so it sounds OK to me. Thanks for the ping though! The patch itself looks OK to me. Will need approval from @mclow.lists or @ericwf. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added a comment. In http://reviews.llvm.org/D21803#470060, @tavianator wrote: > In http://reviews.llvm.org/D21803#469988, @bcraig wrote: > > > You should look at __thread_specific_ptr in libcxx's . It does a > > lot of these things in order to satisfy the requirements of > > notify_all_at_thread_exit, set_value_at_thread_exit, and > > make_ready_at_thread_exit. > > > Had a look at it. One thing that stands out is that > notify_all_at_thread_exit() and friends are supposed to be invoked *after* > thread_local destructors. But the order that pthread_key destructors run in > is unspecified. This could be worked around by waiting for the second > iteration through pthread_key destructors before triggering > ~__thread_struct_imp(). It looks like libstdc++ has a similar bug if > ..._impl() isn't available. It also intentionally leaks the pthread key. Does the __thread_specific_ptr rationale hold for this change as well? > > This implementation of __cxa_thread_atexit doesn't interact nicely with > > shared libraries. The following sequence of events causes unloaded code to > > get invoked. > > > > > > - Create thread 42 > > > - Load library foo from thread 42 > > > - Call a function with a thread_local object with a dtor. > > > - Unload library foo. > > > - Join thread 42 > > > > > > glibc does some extra work during __cxa_thread_atexit_impl to bump the > > reference count of the shared library so that the user's "unload" doesn't > > actually unload the library. > > > Yep. This is about as good as libc++abi can do on its own though. Note that > libstdc++ has similar limitations if ..._impl() isn't available. I was going to tell you that this is implementable with dladdr (which I think Android has). Then I looked more at the "prevent unloading" side of things, and it looks like that requires digging into the library structures directly. Ugh. Comment on the limitation in the source, but you don't need to change any code for this item. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added a comment. In http://reviews.llvm.org/D21803#469988, @bcraig wrote: > You should look at __thread_specific_ptr in libcxx's . It does a lot > of these things in order to satisfy the requirements of > notify_all_at_thread_exit, set_value_at_thread_exit, and > make_ready_at_thread_exit. Had a look at it. One thing that stands out is that notify_all_at_thread_exit() and friends are supposed to be invoked *after* thread_local destructors. But the order that pthread_key destructors run in is unspecified. This could be worked around by waiting for the second iteration through pthread_key destructors before triggering ~__thread_struct_imp(). It looks like libstdc++ has a similar bug if ..._impl() isn't available. > @rmaprath has been doing some work to make the threading runtime library > swappable. I don't recall if his work extended to libcxxabi or not, but I'll > page him anyway. <__threading_support>? Seems to be libc++-specific. There's a few other raw uses of pthreads in libc++abi. > This implementation of __cxa_thread_atexit doesn't interact nicely with > shared libraries. The following sequence of events causes unloaded code to > get invoked. > > - Create thread 42 > - Load library foo from thread 42 > - Call a function with a thread_local object with a dtor. > - Unload library foo. > - Join thread 42 > > glibc does some extra work during __cxa_thread_atexit_impl to bump the > reference count of the shared library so that the user's "unload" doesn't > actually unload the library. Yep. This is about as good as libc++abi can do on its own though. Note that libstdc++ has similar limitations if ..._impl() isn't available. http://reviews.llvm.org/D21803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
bcraig added subscribers: rmaprath, bcraig.
bcraig added a comment.
You should look at __thread_specific_ptr in libcxx's . It does a lot
of these things in order to satisfy the requirements of
notify_all_at_thread_exit, set_value_at_thread_exit, and
make_ready_at_thread_exit.
@rmaprath has been doing some work to make the threading runtime library
swappable. I don't recall if his work extended to libcxxabi or not, but I'll
page him anyway.
This implementation of __cxa_thread_atexit doesn't interact nicely with shared
libraries. The following sequence of events causes unloaded code to get
invoked.
- Create thread 42
- Load library foo from thread 42
- Call a function with a thread_local object with a dtor.
- Unload library foo.
- Join thread 42
glibc does some extra work during __cxa_thread_atexit_impl to bump the
reference count of the shared library so that the user's "unload" doesn't
actually unload the library.
Comment at: src/cxa_thread_atexit.cpp:40
@@ +39,3 @@
+
+~DtorListHolder() {
+ run_dtors(get());
I think this is correct, but it needs some comments because it is not obvious
what (or why) this is implemented this way.
More specifically, document the cases where run_dtors is run because of
~DtorListHolder vs. the cases where run_dtors is run because of the callback
registered at pthread_key_create.
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator updated this revision to Diff 62218.
tavianator added a comment.
Added error handling for pthread_key uses
http://reviews.llvm.org/D21803
Files:
cmake/config-ix.cmake
src/CMakeLists.txt
src/cxa_thread_atexit.cpp
test/CMakeLists.txt
test/cxa_thread_atexit_test.pass.cpp
test/libcxxabi/test/config.py
test/lit.site.cfg.in
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -13,7 +13,6 @@
config.enable_32bit = "@LIBCXXABI_BUILD_32_BITS@"
config.target_info = "@LIBCXXABI_TARGET_INFO@"
config.executor = "@LIBCXXABI_EXECUTOR@"
-config.thread_atexit= "@LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL@"
config.libcxxabi_shared = "@LIBCXXABI_ENABLE_SHARED@"
config.enable_shared= "@LIBCXX_ENABLE_SHARED@"
config.enable_exceptions= "@LIBCXXABI_ENABLE_EXCEPTIONS@"
Index: test/libcxxabi/test/config.py
===
--- test/libcxxabi/test/config.py
+++ test/libcxxabi/test/config.py
@@ -37,8 +37,6 @@
super(Configuration, self).configure_features()
if not self.get_lit_bool('enable_exceptions', True):
self.config.available_features.add('libcxxabi-no-exceptions')
-if self.get_lit_bool('thread_atexit', True):
-self.config.available_features.add('thread_atexit')
def configure_compile_flags(self):
self.cxx.compile_flags += ['-DLIBCXXABI_NO_TIMER']
Index: test/cxa_thread_atexit_test.pass.cpp
===
--- test/cxa_thread_atexit_test.pass.cpp
+++ test/cxa_thread_atexit_test.pass.cpp
@@ -8,7 +8,6 @@
//===--===//
// REQUIRES: linux
-// REQUIRES: thread_atexit
#include
#include
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,6 @@
pythonize_bool(LIBCXXABI_ENABLE_THREADS)
pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
-pythonize_bool(LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.")
set(LIBCXXABI_EXECUTOR "None" CACHE STRING
Index: src/cxa_thread_atexit.cpp
===
--- src/cxa_thread_atexit.cpp
+++ src/cxa_thread_atexit.cpp
@@ -8,19 +8,85 @@
//===--===//
#include "cxxabi.h"
+#include
+#include
+#include "abort_message.h"
namespace __cxxabiv1 {
-extern "C" {
+namespace {
+ typedef void (*Dtor)(void *);
+
+ struct DtorList {
+Dtor dtor;
+void* obj;
+DtorList* next;
+ };
+
+ void run_dtors(void* ptr) {
+auto elem = static_cast(ptr);
+while (elem) {
+ auto saved = elem;
+ elem = elem->next;
+ saved->dtor(saved->obj);
+ std::free(saved);
+}
+ }
+
+ class DtorListHolder {
+ public:
+DtorListHolder() {
+ if (pthread_key_create(&key_, run_dtors) != 0) {
+abort_message("cannot create pthread key for __cxa_thread_atexit()");
+ }
+}
+
+~DtorListHolder() {
+ run_dtors(get());
+ pthread_key_delete(key_);
+}
+
+DtorList* get() {
+ return static_cast(pthread_getspecific(key_));
+}
+
+bool set(DtorList* list) {
+ return pthread_setspecific(key_, list) == 0;
+}
+
+ private:
+pthread_key_t key_;
+ };
+} // namespace
-#ifdef HAVE___CXA_THREAD_ATEXIT_IMPL
+extern "C" {
-_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*dtor)(void *), void *obj,
+_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void *obj,
void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern int __cxa_thread_atexit_impl(Dtor, void *, void *)
+__attribute__((__weak__));
+
+ if (__cxa_thread_atexit_impl) {
+return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
+ } else {
+static DtorListHolder dtors;
+
+auto head = static_cast(std::malloc(sizeof(DtorList)));
+if (!head) {
+ return -1;
+}
+
+head->dtor = dtor;
+head->obj = obj;
+head->next = dtors.get();
+
+if (!dtors.set(head)) {
+ std::free(head);
+ return -1;
+}
-#endif // HAVE__CXA_THREAD_ATEXIT_IMPL
+return 0;
+ }
+}
} // extern "C"
} // namespace __cxxabiv1
Index: src/CMakeLists.txt
===
--- src/CMakeLists.txt
+++ src/CMakeLists.txt
@@ -41,10 +41,6 @@
include_directories("${LIBCXXABI_LIBCXX_INCLUDES}")
-if (LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
-
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
tavianator added inline comments.
Comment at: src/cxa_thread_atexit.cpp:36-47
@@ +35,14 @@
+ public:
+DtorListHolder() {
+ pthread_key_create(&key_, run_dtors);
+}
+
+~DtorListHolder() {
+ run_dtors(get());
+ pthread_key_delete(key_);
+}
+
+DtorList* get() {
+ return static_cast(pthread_getspecific(key_));
+}
+
majnemer wrote:
> What happens if `pthread_key_create` fails?
Nothing good! I'll add the necessary error handling.
Comment at: src/cxa_thread_atexit.cpp:61-67
@@ -18,6 +60,9 @@
+_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void *obj,
void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern int __cxa_thread_atexit_impl(Dtor, void *, void *)
+__attribute__((__weak__));
-#endif // HAVE__CXA_THREAD_ATEXIT_IMPL
+ if (__cxa_thread_atexit_impl) {
+return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
+ } else {
+static DtorListHolder dtors;
majnemer wrote:
> Is there a reason to use the weak symbol on non-Android platforms?
Just simplicity. (There are some fringe benefits, like the ability to build
against pre-2.18 glibc but have it detect ..._impl() support when run with
newer libraries. Or magically supporting musl etc. if they add it, without
having to recompile.)
Would you rather keep the build-time checks for non-Android platforms?
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation
majnemer added a subscriber: majnemer.
Comment at: src/cxa_thread_atexit.cpp:36-47
@@ +35,14 @@
+ public:
+DtorListHolder() {
+ pthread_key_create(&key_, run_dtors);
+}
+
+~DtorListHolder() {
+ run_dtors(get());
+ pthread_key_delete(key_);
+}
+
+DtorList* get() {
+ return static_cast(pthread_getspecific(key_));
+}
+
What happens if `pthread_key_create` fails?
Comment at: src/cxa_thread_atexit.cpp:61-67
@@ -18,6 +60,9 @@
+_LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void *obj,
void *dso_symbol) throw() {
- extern int __cxa_thread_atexit_impl(void (*)(void *), void *, void *);
- return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
-}
+ extern int __cxa_thread_atexit_impl(Dtor, void *, void *)
+__attribute__((__weak__));
-#endif // HAVE__CXA_THREAD_ATEXIT_IMPL
+ if (__cxa_thread_atexit_impl) {
+return __cxa_thread_atexit_impl(dtor, obj, dso_symbol);
+ } else {
+static DtorListHolder dtors;
Is there a reason to use the weak symbol on non-Android platforms?
http://reviews.llvm.org/D21803
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
