Re: [Valgrind-users] helgrind and atomic operations

[David F. <fa...@kd...>]

On Sunday 26 August 2012 12:53:41 Marc Mutz wrote:
> On Sunday August 26 2012, David Faure wrote:
> > On Sunday 26 August 2012 11:28:06 Julian Seward wrote:
> > > On Sunday, August 26, 2012, David Faure wrote:
> > > > Thiago expects that helgrind can't autodetect this case and that
> > > > helgrind- macros markup is needed in Qt, I'm fine with adding that if
> > > > you guys agree -- after you show me how by modifying the attached
> > > > example :-)
> > > 
> > > IIUC then, QBasicAtomicInt::{loadAcquire,storeRelease} just does normal
> > > loads and stores of an int.  Right?
> > 
> > Yep (on x86). This whole API exists so that other architectures can get
> > another implementation.
> > 
> > > What atomicity properties are you expecting onDemandNumber() to have?
> > > Viz, how is onDemandNumber supposed to behave when you have multiple
> > > threads doing it at the same time on the same location?
> > 
> > static int onDemandNumber() {
> > 
> >     static QBasicAtomicInt sharedInt = { 0 };
> >     if (!sharedInt.loadAcquire()) {
> >     
> >         // some calculation goes here
> >         sharedInt.storeRelease(42);
> >     
> >     }
> >     return sharedInt.loadAcquire();
> > 
> > }
> 
> If sharedInt is a std::atomic<int>, then the above is not a C++11 data race,

No, it's a bare int, just like Qt5's QBasicAtomicInt does by default on x86.
You missed the initial email with the testcase, here it is 
(testcase_atomic_ops_helgrind.cpp).

> so Helgrind shouldn't warn.

Helgrind warns, hence my mail here.

> If 'some calculation goes here' doesn't write
> to memory global memory, you could even drop the memory ordering. Atomics
> don't participate in data races, but they might also not establish the
> happens-before that you need elsewhere.

This is obviously a reduced testcase ;)
The calculation, in the example I took, is to register the metatype, which is 
global stuff, but which is somewhat safe if done twice (it will return the same 
number). Inside QMetaType::registerNormalizedType there's a mutex. The whole 
point, though, as I see it, is to avoid locking in the very common case where 
the metatype has been registered already.

> > I think the point is that the first call to loadAcquire() should either
> > return 0 or 42, but never some intermediate value due to a write in
> > progress.
> 
> Correct. What's more: for any thread, any read of an atomic variable must
> return a value previously read, or a value written later by another thread.
> IOW: once a thread sees 42, it can't magically see 0 the next time.
> According to Hans Boehm, this may happen on IA-64 because the architecture
> allows to reorder reads from the same memory location.

Which is why the atomic stuff expands to different code on IA-64, so no problem.

> If it writes to a shared memory location, the code contains a C++11 data
> race, and you need more complex synchronisation

Yes, which is there, but that's not the point. From what you say, the code is 
fine, so helgrind shouldn't warn about sharedInt itself. However, Julian says 
the code is not fine, the compiler could generate code that doesn't work 
correctly. I don't know who to trust at this point, I'm caught in the middle 
:-).

If this is safe, helgrind should be fixed. If this is not safe, Qt should be 
fixed. But it sounds to me like different people have different interpretations 
on what is safe, in such code :-)

I just realized one thing though: I was wrong when I said "it is racy because 
the value could be 0 or 42", that is not a race. Writing the same code with a 
mutex around the int leads to exactly this behavior (0 or 42), which is fine. 
See testcase_int_mutex_helgrind.cpp. No helgrind warning, and I think everyone 
agrees that this code is ok. So if ints are read and written atomically, then 
the helgrind warning in the initial testcase is wrong? (ok I guess it's more 
complicated, if the compiler can reorder stuff in the first testcase and not the 
second....).

-- 
David Faure, fa...@kd..., http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5