[libc++] Fix PR22606 - Leak pthread_key with static storage duration to ensure all of thread-local destructors are called.

[EricWF]

Summary

See https://llvm.org/bugs/show_bug.cgi?id=22606 for more discussion.

Most of the changes in this patch are file reorganization to help ensure assumptions about how __thread_specific_pointer is used hold. The assumptions are:

• __thread_specific_ptr<Tp> is only created with a __thread_struct pointer.
• __thread_specific_ptr<Tp> can only be constructed inside the __thread_local_data() function.

I'll remove the comments before committing. They are there for clarity during review.

[mclow.lists]

(just to make sure that I understand), I see two changes here :

1. Some code movement (line 310-ish to line 116-ish). No real functionality change there.
2. Make the constructor for __thread_specific_ptr private, and add a friend decl for __thread_local_data.

Is that correct?

[EricWF]

In D8802#151897, @mclow.lists wrote:

(just to make sure that I understand), I see two changes here :

1. Some code movement (line 310-ish to line 116-ish). No real functionality change there.
2. Make the constructor for __thread_specific_ptr private, and add a friend decl for __thread_local_data.

Is that correct?

That is correct, but for some reason this patch itself is incorrect. there should be a third change:

• ~__thread_specific_ptr() does not call pthread_key_delete(__key_)

[EricWF]

Actually leak the key...

[earthdok]

lgtm

I'd still leave a short comment though.

[mclow.lists]

Other than my inline comment, LGTM.

[EricWF]

Since a rI'm sitting on this for a bit until I determine a couple of things.

1. Could libc++ use std::atexit to call the destructor function before destroying the pthread key?
2. What does the standard say about race conditions between thread-exit and program-exit? Do all created threads have to be terminated before std::exit() begins?
3. How do the pthread_key thread-exit destructors fit into the steps C++ takes at a thread exit?

[EricWF]

OK. I have some new thoughts. The __thread_struct destructor serves three purposes when it is invoked at thread exit.

1. Notifying registered condition variables.
2. Making registered shared state ready.
3. Deleting the tuple of arguments used to start the thread.

The problem this patch is trying to solve is the case where std::exit(0) has been called. Once program termination has begun the only code that should be running are destructors and functions registered with at_exit(...). This means that the code waiting on the CV or shared state must be doing so in a destructor. Furthermore it is likely that the registered CV's have already been destroyed.

For these reasons I don't think it is safe for libc++ to run steps #1 or #2 when program termination has begun. So I'm worried about going out of our way to support this.

I'll write up some more documentation on this and hopefully reach a conclusion.

[EricWF]

In D8802#187303, @EricWF wrote:

OK. I have some new thoughts. The __thread_struct destructor serves three purposes when it is invoked at thread exit.

1. Notifying registered condition variables.
2. Making registered shared state ready.
3. Deleting the tuple of arguments used to start the thread.

I was incorrect in this comment. #3 is unrelated to this problem. They are always freed. Only #1 and #2 are any concern.

The problem is this:

Libc++ uses a single pthread key for thread local storage. This TLS is used by <condition_variable> and <future> to notify/ready consumers on thread exit. The notification is done in the pthread key's destructor which is executed during pthread_exit(...).
When the main thread begins program termination and a detached child thread, t0, has yet to terminate there is a race condition between t0 executing the TLS destructor and the main thread destroying the static pthread key. Once the main thread destroys the pthread key no more TLS destructors are run.

There are two solutions to this race condition:

1. Leak the pthread key. If we never destroy the during program termination then all TLS destructors on detached threads will run.
2. Reference count the key and only destroy it once all TLS have been run. This change would require a MAJOR ABI break that we could not ship for some time.

Because #2 requires an ABI break I believe that #1 is the better option. Because we only leak one key, and this key is leaked during program termination, it is very unlikely (read: almost impossible) for a user to notice this change.

However, I question if is wise to attempt to run the TLS destructors at all once program termination has begun. The shared state referenced by the destructors must have static storage duration and this static storage may or may not have been destroyed already. It seems very unlikely that a well-formed program will attempt to use the results of std::notify_at_thread_exit(...) or std::promise::set_value_at_thread_exit(...) during program termination. This is likely also undefined behavior as noted in [basic.start.term]C++17 3.6.3 p4:

If there is a use of a standard library object or function not permitted within signal handlers (18.10) that
does not happen before (1.10) completion of destruction of objects with static storage duration and execution
of std::atexit registered functions (18.5), the program has undefined behavior. [ Note: If there is a use
of an object with static storage duration that does not happen before the object’s destruction, the program
has undefined behavior. Terminating every thread before a call to std::exit or the exit from main is
sufficient, but not necessary, to satisfy these requirements. These requirements permit thread managers as
static-storage-duration objects. — end note ]

If we choose not to run the TLS destructor's once program termination has begun we should still make an effort to free all memory allocated by the internal TLS structures to placate ASAN.

In summation I believe we should take this patch as a partial fix to the above problem and continue to investigate if we should disable thread exit notifications once program termination has begun.

[EricWF]

@mclow.lists: ping. It would be nice to fix this and prevent the ASAN failures.

[mclow.lists]

LGTM. I hate the leaking, but I think it's the best we can do at this time.

include/thread

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+template <class _Tp> class __thread_specific_ptr;
+class _LIBCPP_TYPE_VIS __thread_struct;
+class _LIBCPP_HIDDEN __thread_struct_imp;
+class __assoc_sub_state;
+
+_LIBCPP_FUNC_VIS __thread_specific_ptr<__thread_struct>& __thread_local_data();
+
+class _LIBCPP_TYPE_VIS __thread_struct
+{
+    __thread_struct_imp* __p_;
+
+    __thread_struct(const __thread_struct&);
+    __thread_struct& operator=(const __thread_struct&);
+public:
+    __thread_struct();
+    ~__thread_struct();
+
+    void notify_all_at_thread_exit(condition_variable*, mutex*);
+    void __make_ready_at_thread_exit(__assoc_sub_state*);
+};
+
 template <class _Tp>
 class __thread_specific_ptr
 {
     pthread_key_t __key_;
 
+     // Only __thread_local_data() may construct a __thread_specific_ptr
+     // and only with _Tp == __thread_struct.
+    static_assert(is_same<_Tp, __thread_struct>::value, "");
+    __thread_specific_ptr();
+    friend _LIBCPP_FUNC_VIS __thread_specific_ptr<__thread_struct>& __thread_local_data();
+
     __thread_specific_ptr(const __thread_specific_ptr&);
     __thread_specific_ptr& operator=(const __thread_specific_ptr&);
 

 public:
     typedef _Tp* pointer;
 
-    __thread_specific_ptr();
     ~__thread_specific_ptr();
 
     _LIBCPP_INLINE_VISIBILITY

 template <class _Tp>
 __thread_specific_ptr<_Tp>::~__thread_specific_ptr()
 {
-    pthread_key_delete(__key_);
+    // __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>

     static unsigned hardware_concurrency() _NOEXCEPT;
 };
 
-class __assoc_sub_state;
-
-class _LIBCPP_HIDDEN __thread_struct_imp;
-
-class _LIBCPP_TYPE_VIS __thread_struct
-{
-    __thread_struct_imp* __p_;
-
-    __thread_struct(const __thread_struct&);
-    __thread_struct& operator=(const __thread_struct&);
-public:
-    __thread_struct();
-    ~__thread_struct();
-
-    void notify_all_at_thread_exit(condition_variable*, mutex*);
-    void __make_ready_at_thread_exit(__assoc_sub_state*);
-};
-
-_LIBCPP_FUNC_VIS __thread_specific_ptr<__thread_struct>& __thread_local_data();
-
 #ifndef _LIBCPP_HAS_NO_VARIADICS
 
 template <class _Fp, class ..._Args, size_t ..._Indices>