Bug 317381

Qt has this method:
(x86 implementation shown here)
template<int size> template <typename T> inline
T QBasicAtomicOps<size>::fetchAndStoreRelaxed(T &_q_value, T newValue) Q_DECL_NOTHROW
{
    asm volatile("xchg %0,%1"
                 : "=r" (newValue), "+m" (_q_value)
                 : "0" (newValue)
                 : "memory");
    return newValue;
}

Two threads calling this method do not race, given that it's an atomic operation.

If another thread is doing a basic "store" by simply writing to the memory, then we have a race (as far as helgrind is concerned).

==11541== Possible data race during read of size 8 at 0x8DC1EE0 by thread #2
==11541==    at 0x5109544: QMutexData* QBasicAtomicOps<8>::fetchAndStoreRelaxed<QMutexData*>(QMutexData*&, QMutexData*) (qatomic_x86.h:275)
==11541== 
==11541== This conflicts with a previous write of size 8 by thread #1
==11541==    at 0x5109464: QBasicAtomicPointer<QMutexData>::storeRelease(QMutexData*) (qgenericatomic.h:116)

However, I already have a suppression for "storeRelease", since helgrind cannot detect this as being safe (due to it being implemented differently on non-x86 platforms).

{
   QBasicAtomicPointer_store
   Helgrind:Race
   # catch store and storeRelease
   fun:_ZN19QBasicAtomicPointer*store*
}

Shouldn't this also apply to code that races against that store? I'd rather not have to suppress safe atomic operations, only the "apparently unsafe" operations.



Reproducible: Always

Steps to Reproduce:
1. Qt 5 from git (stable branch), in debug mode
2. QBasicAtomicPointer_store suppression in a supp file seen by hg
3. alias helgrind="QT_NO_GLIB=1 valgrind --tool=helgrind --track-lockorders=no"
4. cd qtbase/tests/auto/corelib/thread/qthread ; helgrind ./tst_qthread
Actual Results:  
When helgrind detects a race between opB and a previous opA, it only checks suppressions for opB, not for opA.

Expected Results:  
I would expect a suppression to apply to either of the racy operations. If opA is OK, then opB can't race with it.

The order of opA and opB doesn't really mean much anyway - on a subsequent run, they could be in reverse order. So it doesn't really make sense to check only the second operation for suppressions.