http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40362
--- Comment #12 from Dmitry Vyukov <dvyukov at google dot com> 2012-12-29
12:31:02 UTC ---
(In reply to comment #11)
> (In reply to comment #10)
> > gomp_ptrlock_get() function is not thread-safe. It's not only about
> > atomicity,
> > it's also about memory ordering. Thread that calls gomp_ptrlock_set() can
> > reorder stores to ws and the store to ptrlock->ptr. Alpha processors can
> > even
> > reorder load from ws and load of ptrlock->ptr (they do not respect data
> > dependencies).
>
> Wait a second ptrlock->ptr is really ws->next_ws.ptr which is really
> (ws+offset) so there is no data dependency here as far as I can see.
If that would be the case, then it would be even worser. But it's not the case,
there is certainly a data dependency between load of prtlock->ptr itself and
load of data through prtlock->ptr.
ws = gomp_ptrlock_get (&ws->next_ws);
if (ws == NULL)
...
else
{
thr->ts.work_share = ws;
> As Jakub
> mentioned before ws->next_ws.ptr only from NULL to non-null.
I do not see how it affects anything.
> So as far as I
> can tell it still stands as being thread-safe just not detectable from either
> valgrind or tsan.
tsan does not produce false warnings. data races are undefined behavior.
> This is just like some of the false uninitialized warnings
> which gcc can emit. It is sometimes hard to prove that the code is fine
> except
> with extra knowledge like the NULL to non-NULL.
>
> And about the stores being reordered:
> gomp_ptrlock_set (&thr->ts.last_work_share->next_ws, thr->ts.work_share);
>
> so ptr is always ws.
What do you mean?
> I again looked at the code and I still agree with Jakub's reasoning.
Among other things (e.g. just launching nuclear missiles), the following code:
if (gomp_work_share_start (false))
{
gomp_loop_init (thr->ts.work_share, start, end, incr,
GFS_STATIC, chunk_size);
gomp_work_share_init_done ();
}
can be executed as:
if (gomp_work_share_start (false))
{
thr->ts.last_work_share->next_ws.ptr = thr->ts.work_share;
gomp_loop_init (thr->ts.work_share, start, end, incr,
GFS_STATIC, chunk_size);
gomp_mutex_unlock (&thr->ts.last_work_share->next_ws.lock);
}
Then it will kaboom!
