[libcxx] [test] Repair thread unsafety in thread tests

[BillyONeal]

Summary

• Repair thread-unsafe modifications of n_alive in F.pass.cpp

In this example, the ctor of G runs in the main thread in the expression G(), and also in the copy ctor of G() in the DECAY_COPY inside std::thread. The main thread destroys the G() instance at the semicolon, and the started thread destroys the G() after it returns. Thus there is a race between the threads on the n_alive variable.

The fix is to join with the background thread before attempting to destroy the G in the main thread.

[EricWF]

We should add some TSAN folks to this review, since I think these are also TSAN false negatives; perhaps correctly, perhaps not.

[BillyONeal]

In D50549#1194852, @EricWF wrote:

We should add some TSAN folks to this review, since I think these are also TSAN false negatives; perhaps correctly, perhaps not.

Do you know who that is / can you add them?

[EricWF]

I was hoping my comment would summon them magically. I'll figure it out shortly.

[jfb]

CC some sanitizer folks.

[dvyukov]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

I don't immediately see how the race on n_alive/op_run happens. It seems that the modifications in the thread happen before this line, and modifications in main happen after this line. How can both of them modify the variables at the same time?

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

The destruction of g here races with the destruction of the DECAY_COPY'd copy of G used as the parameter of operator(). That is, line 69 creates a copy of g, passes that to the started thread, the started thread calls gCopy(). gCopy() doesn't return until the done flag is set, but the destruction of the object on which op() is being called is not so synchronized. Most of the other thread tests avoid this problem by joining with the thread; joining waits for the destruction of the DECAY_COPY'd parameters, but this does not.

(This is one of the reasons detach() should basically never be called anywhere)

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

(That is to say, there's nothing to prevent both threads from executing G::!G() on the two different copies of g... making op_run atomic is probably avoidable but I'm being paranoid given that there was already thread unsafety here...)

[dvyukov]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

What is gCopy? I don't see anything named gCopy in this file...

Do we care about completion of destruction? Why? We wait for done to be set, and other variables are already updated at that point. Why does it matter that "the destruction of the object on which op() is being called is not so synchronized."?

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

Because the destructor does --n_alive;

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

What is gCopy? I don't see anything named gCopy in this file...

The copy is made in the constructor of std::thread. std::thread makes a copy of all the input parameters, gives the copy to the started thread, and then std::invoke is called there.

Why does it matter that "the destruction of the object on which op() is being called is not so synchronized."?

Because the two dtors race on --n_alive; when n_alive is not atomic.

[dvyukov]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

But the first dtor runs before "while (!done) {}" line and the second dtor runs after "while (!done) {}" line, no?
Or there is third object involved? But then I don't see how joining the thread would help either.

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

But the first dtor runs before "while (!done) {}" line

No, both dtors are run after the while (!done) {} line. The first dtor runs on line 76 (when the local variable g is destroyed), and the second dtor runs after operator() returns in the constructed thread. The constructed thread is morally doing:

void threadproc(G * g) {
    g->operator(); // setting done happens in here
    delete g; // dtor of second copy runs here
}

I don't see how joining the thread would help either.

Joining with the thread would wait for the second dtor -- the one after op() returns -- to complete. Of course joining with the thread isn't doable here given that the point is to test thread::detach :)

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

75

Actually I just realized that this assert is bogus too; there's no synchronization to ensure the copied g is destroyed here.

[dvyukov]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

No, both dtors are run after the while (!done) {} line.

But how do we get past while (!done) line before the desctructor in the thread has finished? The destructor sets done. So after while (!done) line the destructor is effectively finished. What am I missing?

[BillyONeal]

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

73

The destructor sets done.

Hmmm I thought done was getting set on 56 but that's done_ (with a trailing underscore). :sigh:

[BillyONeal]

Remove changes to detach tests.

[BillyONeal]

Committed r344820

test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp

test_throwing_new_during_thread_creation();
         std::thread t(f);
         t.join();
         assert(f_run == true);
     }
 
     {
         assert(G::n_alive == 0);
         assert(!G::op_run);
-        std::thread t((G()));
+        {
+            G g;
+            std::thread t(g);
             t.join();
+        }
         assert(G::n_alive == 0);
         assert(G::op_run);
     }
     G::op_run = false;
 #ifndef TEST_HAS_NO_EXCEPTIONS
     {
         try
         {
#ifndef TEST_HAS_NO_EXCEPTIONS
             assert(!G::op_run);
         }
     }
 #endif
 #if TEST_STD_VER >= 11
     {
         assert(G::n_alive == 0);
         assert(!G::op_run);
-        std::thread t(G(), 5, 5.5);
+        {
+            G g;
+            std::thread t(g, 5, 5.5);
             t.join();
+        }
         assert(G::n_alive == 0);
         assert(G::op_run);
     }
     {
         std::thread t = std::thread(MoveOnly(), MoveOnly());
         t.join();
     }
 #endif
 }

test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp

 
 std::atomic_bool done(false);
 
 class G
 {
     int alive_;
     bool done_;
 public:
-    static int n_alive;
-    static bool op_run;
+    static std::atomic_int n_alive;
+    static std::atomic_bool op_run;
 
     G() : alive_(1), done_(false)
     {
         ++n_alive;
     }
 
     G(const G& g) : alive_(g.alive_), done_(false)
     {
public:
     {
         assert(alive_ == 1);
         assert(n_alive >= 1);
         op_run = true;
         done_ = true;
     }
 };
 
-int G::n_alive = 0;
-bool G::op_run = false;
+std::atomic_int G::n_alive = 0;
+std::atomic_bool G::op_run = false;
 
 void foo() {}
 
 int main()
 {
     {
         G g;
         std::thread t0(g);
         assert(t0.joinable());
         t0.detach();
         assert(!t0.joinable());
         while (!done) {}
         assert(G::op_run);
         assert(G::n_alive == 1);
     }
     assert(G::n_alive == 0);
 #ifndef TEST_HAS_NO_EXCEPTIONS
     {
         std::thread t0(foo);
         assert(t0.joinable());
         t0.detach();
         assert(!t0.joinable());
         try {
             t0.detach();
         } catch (std::system_error const&) {
         }
     }
 #endif
 }