[PATCH] D50549: [libcxx] [test] Repair thread unsafety in thread tests

Billy Robert O'Neal III via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 13 10:33:38 PDT 2018


BillyONeal added inline comments.


================
Comment at: test/std/thread/thread.threads/thread.thread.class/thread.thread.member/detach.pass.cpp:73
         assert(!t0.joinable());
         while (!done) {}
         assert(G::op_run);
----------------
dvyukov wrote:
> BillyONeal wrote:
> > BillyONeal wrote:
> > > dvyukov wrote:
> > > > BillyONeal wrote:
> > > > > BillyONeal wrote:
> > > > > > dvyukov wrote:
> > > > > > > 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?
> > > > > > 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)
> > > > > > 
> > > > > (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...)
> > > > 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."?
> > > Because the destructor does `--n_alive;`
> > >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.
> 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.
>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 :)


https://reviews.llvm.org/D50549





More information about the cfe-commits mailing list