Auto merge of #2503 - RalfJung:tls-dtor-order, r=RalfJung

notes on TLS dtor order

Fixes #2486

Author: bors

src/shims/tls.rs

@@ -165,8 +165,8 @@ impl<'tcx> TlsData<'tcx> {
     /// 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.
+    /// **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
@@ -188,6 +188,13 @@ impl<'tcx> TlsData<'tcx> {
             Some(key) => Excluded(key),
             None => Unbounded,
         };
+        // We interpret the documentaion above (taken from POSIX) as saying that we need to iterate
+        // over all keys and run each destructor at least once before running any destructor a 2nd
+        // time. That's why we have `key` to indicate how far we got in the current iteration. If we
+        // return `None`, `schedule_next_pthread_tls_dtor` will re-try with `ket` set to `None` to
+        // start the next round.
+        // TODO: In the future, we might consider randomizing destructor order, but we still have to
+        // uphold this requirement.
         for (&key, TlsEntry { data, dtor }) in thread_local.range_mut((start, Unbounded)) {
             match data.entry(thread_id) {
                 BTreeEntry::Occupied(entry) => {

tests/pass/concurrency/tls_lib_drop.rs

@@ -1,5 +1,3 @@
-//@ignore-target-windows: TLS destructor order is different on Windows.
-
 use std::cell::RefCell;
 use std::thread;
 
@@ -24,14 +22,18 @@ thread_local! {
 /// Check that destructors of the library thread locals are executed immediately
 /// after a thread terminates.
 fn check_destructors() {
+    // We use the same value for both of them, since destructor order differs between Miri on Linux
+    // (which uses `register_dtor_fallback`, in the end using a single pthread_key to manage a
+    // thread-local linked list of dtors to call), real Linux rustc (which uses
+    // `__cxa_thread_atexit_impl`), and Miri on Windows.
     thread::spawn(|| {
         A.with(|f| {
             assert_eq!(*f.value.borrow(), 0);
-            *f.value.borrow_mut() = 5;
+            *f.value.borrow_mut() = 8;
         });
         A_CONST.with(|f| {
             assert_eq!(*f.value.borrow(), 10);
-            *f.value.borrow_mut() = 15;
+            *f.value.borrow_mut() = 8;
         });
     })
     .join()

tests/pass/concurrency/tls_lib_drop.stdout

@@ -1,5 +1,5 @@
-Dropping: 5 (should be before 'Continue main 1').
-Dropping: 15 (should be before 'Continue main 1').
+Dropping: 8 (should be before 'Continue main 1').
+Dropping: 8 (should be before 'Continue main 1').
 Continue main 1.
 Joining: 7 (should be before 'Continue main 2').
 Continue main 2.

tests/pass/concurrency/tls_lib_drop_windows.rs

@@ -1,4 +1,9 @@
 //@ignore-target-windows: No libc on Windows
+//! Test that pthread_key destructors are run in the right order.
+//! Note that these are *not* used by actual `thread_local!` on Linux! Those use
+//! `thread_local_dtor::register_dtor` from the stdlib instead. In Miri this hits the fallback path
+//! in `register_dtor_fallback`, which uses a *single* pthread_key to manage a thread-local list of
+//! dtors to call.
 
 use std::mem;
 use std::ptr;
@@ -44,6 +49,8 @@ unsafe extern "C" fn dtor(ptr: *mut u64) {
     // If the record is wrong, the cannary will never get cleared, leading to a leak -> test fails.
     // If the record is incomplete (i.e., more dtor calls happen), the check at the beginning of this function will fail -> test fails.
     // The correct sequence is: First key 0, then key 1, then key 0.
+    // Note that this relies on dtor order, which is not specified by POSIX, but seems to be
+    // consistent between Miri and Linux currently (as of Aug 2022).
     if RECORD == 0_1_0 {
         drop(Box::from_raw(CANNARY));
         CANNARY = ptr::null_mut();