Fix PR30202 - notify_all_at_thread_exit seg faults if run from a raw pthread context.

[EricWF]

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".

[bcraig]

src/condition_variable.cpp

86

The standard synopsis of notify_all_at_thread_exit in the standard doesn't have a "Throws:" clause. I think that means that this is non-conformant.

There is some phrasing in 30.2.2 [thread.req.exception]:

Failure to allocate storage shall be reported as described in 17.6.5.12 [res.on.exception.handling].

But I don't see wording in that section that gives permission to throw from functions without Throws clauses.

From a user perspective, I generally expect my synchronization primitives to be no-throw. I don't know if it is really possible to provide that guarantee with notify_all_at_thread_exit though, especially if we want it to work outside of a std::thread.

[EricWF]

src/condition_variable.cpp

86

[res.on.exception.handling]/p4

Destructor operations defined in the C ++ standard library shall not throw exceptions. Every destructor
in the C ++ standard library shall behave as if it had a non-throwing exception specification. Any other
functions defined in the C ++ standard library that do not have an exception-specification may throw
implementation-defined exceptions unless otherwise specified. [188] An implementation may strengthen this
implicit exception-specification by adding an explicit one.

And the relevant note:

188. In particular, they can report a failure to allocate storage by throwing an exception of type bad_alloc, or a class derived

from bad_alloc (18.6.3.1). Library implementations should report errors by throwing exceptions of or derived from the standard
exception classes (18.6.3.1, 18.8, 19.2).

So throwing std::bad_alloc is allowed, as it is almost everywhere in the STL. Also the function already allocates memory to store the new entry.

[bcraig]

LGTM

src/condition_variable.cpp

86

Yet again, I find that reading comprehension with regards to the standard isn't my strong suit :). I got to "Destructor operations" in p4 and stopped reading.

I will still wish for a noexcept version of this function, but it's a pretty impractical wish. The implementation you have here is fine.

include/thread

 #include <memory>
 #include <system_error>
 #include <chrono>
 #include <__mutex_base>
 #ifndef _LIBCPP_HAS_NO_VARIADICS
 #include <tuple>
 #endif
 #include <__threading_support>
+#include <__debug>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 #define __STDCPP_THREADS__ __cplusplus
 
 #ifdef _LIBCPP_HAS_NO_THREADS
public:
     ~__thread_specific_ptr();
 
     _LIBCPP_INLINE_VISIBILITY
     pointer get() const {return static_cast<_Tp*>(__libcpp_tl_get(__key_));}
     _LIBCPP_INLINE_VISIBILITY
     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>
 void
 __thread_specific_ptr<_Tp>::__at_thread_exit(void* __p)
 {
     delete static_cast<pointer>(__p);
 }

 {
     // __thread_specific_ptr is only created with a static storage duration
     // so this destructor is only invoked during program termination. Invoking
     // pthread_key_delete(__key_) may prevent other threads from deleting their
     // thread local data. For this reason we leak the key.
 }
 
 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;
 class _LIBCPP_TYPE_VIS __thread_id;
 
 namespace this_thread
 {
 
__thread_execute(tuple<_TSp, _Fp, _Args...>& __t, __tuple_indices<_Indices...>)
     __invoke(_VSTD::move(_VSTD::get<1>(__t)), _VSTD::move(_VSTD::get<_Indices>(__t))...);
 }
 
 template <class _Fp>
 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;
 }
 
 template <class _Fp, class ..._Args,
           class
          >
struct __thread_invoke_pair {
     unique_ptr<__thread_struct> __tsp_;
     _Fp __fn_;
 };
 
 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;
 }
 
 template <class _Fp>
 thread::thread(_Fp __f)
 {
 

src/condition_variable.cpp

condition_variable::__do_timed_wait(unique_lock<mutex>& lk,
     int ec = __libcpp_condvar_timedwait(&__cv_, lk.mutex()->native_handle(), &ts);
     if (ec != 0 && ec != ETIMEDOUT)
         __throw_system_error(ec, "condition_variable timed_wait failed");
 }
 
 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());
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // !_LIBCPP_HAS_NO_THREADS

test/std/thread/thread.condition/PR30202_notify_from_pthread_created_thread.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: 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();
+    }
+}