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...@kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5
#include <pthread.h>
template <typename T> struct QAtomicOps // qgenericatomic.h
{
static inline
T loadAcquire(const T &_q_value)
{
T tmp = *static_cast<const volatile T *>(&_q_value);
return tmp;
}
static inline
void storeRelease(T &_q_value, T newValue)
{
*static_cast<volatile T *>(&_q_value) = newValue;
}
};
class QBasicAtomicInt // qbasicatomic.h
{
public:
typedef QAtomicOps<int> Ops;
int _q_value;
int loadAcquire() const { return Ops::loadAcquire(_q_value); }
void storeRelease(int newValue) { Ops::storeRelease(_q_value, newValue); }
};
// Modelled after qt_metatype_id()
static int onDemandNumber() {
static QBasicAtomicInt sharedInt = { 0 };
if (!sharedInt.loadAcquire()) {
// some calculation goes here
sharedInt.storeRelease(42);
}
return sharedInt.loadAcquire();
}
void * threadStart(void *)
{
onDemandNumber();
onDemandNumber();
onDemandNumber();
onDemandNumber();
return 0;
}
int main( int argc, char** argv ) {
pthread_t thread1;
if ( pthread_create(&thread1, 0, threadStart, 0) )
return 1;
pthread_t thread2;
if ( pthread_create(&thread2, 0, threadStart, 0) )
return 1;
void* v;
pthread_join(thread1, &v);
pthread_join(thread2, &v);
return 0;
}
#include <pthread.h>
#include <QtCore/QMutex>
class MyBasicAtomicInt
{
public:
MyBasicAtomicInt() : _q_value(0) {}
int _q_value;
mutable QMutex mutex;
int loadAcquire() const { QMutexLocker lock(&mutex); return _q_value; }
void storeRelease(int newValue) { QMutexLocker lock(&mutex); _q_value = newValue; }
};
// Modelled after qt_metatype_id()
static int onDemandNumber() {
static MyBasicAtomicInt sharedInt;
if (!sharedInt.loadAcquire()) {
// some calculation goes here
sharedInt.storeRelease(42);
}
return sharedInt.loadAcquire();
}
void * threadStart(void *)
{
onDemandNumber();
onDemandNumber();
onDemandNumber();
onDemandNumber();
return 0;
}
int main( int argc, char** argv ) {
pthread_t thread1;
if ( pthread_create(&thread1, 0, threadStart, 0) )
return 1;
pthread_t thread2;
if ( pthread_create(&thread2, 0, threadStart, 0) )
return 1;
void* v;
pthread_join(thread1, &v);
pthread_join(thread2, &v);
return 0;
}
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Valgrind-users mailing list
Valgrind-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-users
