[libcxx] r280588 - Fix PR30202 - notify_all_at_thread_exit seg faults if run from a raw pthread context.
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 3 01:07:40 PDT 2016
Author: ericwf
Date: Sat Sep 3 03:07:40 2016
New Revision: 280588
URL: http://llvm.org/viewvc/llvm-project?rev=280588&view=rev
Log:
Fix PR30202 - notify_all_at_thread_exit seg faults if run from a raw pthread context.
Summary:
This patch allows threads not created using `std::thread` to use `std::notify_all_at_thread_exit` by ensuring the TL state has been initialized within `std::notify_all_at_thread_exit`.
Additionally this patch "fixes" a potential oddity in `__thread_local_pointer::reset(pointer)`, which would previously delete the old thread local data. However there should *never* be old thread local data because pthread *should* null it out on thread exit. Unfortunately it's possible that pthread failed to do this according to the spec:
>
> Upon key creation, the value NULL shall be associated with the new key in all active threads. Upon thread creation, the value NULL shall be associated with all defined keys in the new thread.
>
> An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.
>
> If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop.
However if pthread fails to delete the value it is probably incorrect for us to do it. Destroying the value performs all of the "at thread exit" actions registered with it but we are way past "at thread exit".
Reviewers: mclow.lists, bcraig, EricWF
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D24159
Added:
libcxx/trunk/test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.pass.cpp
Modified:
libcxx/trunk/include/thread
libcxx/trunk/src/condition_variable.cpp
Modified: libcxx/trunk/include/thread
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/thread?rev=280588&r1=280587&r2=280588&view=diff
==============================================================================
--- libcxx/trunk/include/thread (original)
+++ libcxx/trunk/include/thread Sat Sep 3 03:07:40 2016
@@ -99,6 +99,7 @@ void sleep_for(const chrono::duration<Re
#include <tuple>
#endif
#include <__threading_support>
+#include <__debug>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
@@ -159,8 +160,7 @@ public:
pointer operator*() const {return *get();}
_LIBCPP_INLINE_VISIBILITY
pointer operator->() const {return get();}
- pointer release();
- void reset(pointer __p = nullptr);
+ void set_pointer(pointer __p);
};
template <class _Tp>
@@ -191,21 +191,12 @@ __thread_specific_ptr<_Tp>::~__thread_sp
}
template <class _Tp>
-typename __thread_specific_ptr<_Tp>::pointer
-__thread_specific_ptr<_Tp>::release()
-{
- pointer __p = get();
- __libcpp_tl_set(__key_, nullptr);
- return __p;
-}
-
-template <class _Tp>
void
-__thread_specific_ptr<_Tp>::reset(pointer __p)
+__thread_specific_ptr<_Tp>::set_pointer(pointer __p)
{
- pointer __p_old = get();
+ _LIBCPP_ASSERT(get() == nullptr,
+ "Attempting to overwrite thread local data");
__libcpp_tl_set(__key_, __p);
- delete __p_old;
}
class _LIBCPP_TYPE_VIS thread;
@@ -351,7 +342,7 @@ void* __thread_proxy(void* __vp)
{
// _Fp = std::tuple< unique_ptr<__thread_struct>, Functor, Args...>
std::unique_ptr<_Fp> __p(static_cast<_Fp*>(__vp));
- __thread_local_data().reset(_VSTD::get<0>(*__p).release());
+ __thread_local_data().set_pointer(_VSTD::get<0>(*__p).release());
typedef typename __make_tuple_indices<tuple_size<_Fp>::value, 2>::type _Index;
__thread_execute(*__p, _Index());
return nullptr;
@@ -392,7 +383,7 @@ template <class _Fp>
void* __thread_proxy_cxx03(void* __vp)
{
std::unique_ptr<_Fp> __p(static_cast<_Fp*>(__vp));
- __thread_local_data().reset(__p->__tsp_.release());
+ __thread_local_data().set_pointer(__p->__tsp_.release());
(__p->__fn_)();
return nullptr;
}
Modified: libcxx/trunk/src/condition_variable.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/condition_variable.cpp?rev=280588&r1=280587&r2=280588&view=diff
==============================================================================
--- libcxx/trunk/src/condition_variable.cpp (original)
+++ libcxx/trunk/src/condition_variable.cpp Sat Sep 3 03:07:40 2016
@@ -79,6 +79,12 @@ condition_variable::__do_timed_wait(uniq
void
notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk)
{
+ auto& tl_ptr = __thread_local_data();
+ // If this thread was not created using std::thread then it will not have
+ // previously allocated.
+ if (tl_ptr.get() == nullptr) {
+ tl_ptr.set_pointer(new __thread_struct);
+ }
__thread_local_data()->notify_all_at_thread_exit(&cond, lk.release());
}
Added: libcxx/trunk/test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.pass.cpp
URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.pass.cpp?rev=280588&view=auto
==============================================================================
--- libcxx/trunk/test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.pass.cpp (added)
+++ libcxx/trunk/test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.pass.cpp Sat Sep 3 03:07:40 2016
@@ -0,0 +1,75 @@
+//===----------------------------------------------------------------------===//
+//
+// 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: libcpp-has-no-threads
+
+// notify_all_at_thread_exit(...) requires move semantics to transfer the
+// unique_lock.
+// UNSUPPORTED: c++98, c++03
+
+// <condition_variable>
+
+// void
+// notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
+
+// Test that this function works with threads that were not created by
+// std::thread. See http://llvm.org/PR30202.
+
+
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+#include <chrono>
+#include <cassert>
+#include <pthread.h>
+
+std::condition_variable cv;
+std::mutex mut;
+bool exited = false;
+
+typedef std::chrono::milliseconds ms;
+typedef std::chrono::high_resolution_clock Clock;
+
+void* func(void*)
+{
+ std::unique_lock<std::mutex> lk(mut);
+ std::notify_all_at_thread_exit(cv, std::move(lk));
+ std::this_thread::sleep_for(ms(300));
+ exited = true;
+ return nullptr;
+}
+
+int main()
+{
+ {
+ std::unique_lock<std::mutex> lk(mut);
+ pthread_t id;
+ int res = pthread_create(&id, 0, &func, nullptr);
+ assert(res == 0);
+ Clock::time_point t0 = Clock::now();
+ assert(exited == false);
+ cv.wait(lk);
+ Clock::time_point t1 = Clock::now();
+ assert(exited);
+ assert(t1-t0 > ms(250));
+ pthread_join(id, 0);
+ }
+ exited = false;
+ {
+ std::unique_lock<std::mutex> lk(mut);
+ std::thread t(&func, nullptr);
+ Clock::time_point t0 = Clock::now();
+ assert(exited == false);
+ cv.wait(lk);
+ Clock::time_point t1 = Clock::now();
+ assert(exited);
+ assert(t1-t0 > ms(250));
+ t.join();
+ }
+}
More information about the cfe-commits
mailing list