Bug 328490 - drd reports false positive for concurrent __atomic_base access
Summary: drd reports false positive for concurrent __atomic_base access
Status: RESOLVED NOT A BUG
Alias: None
Product: valgrind
Classification: Developer tools
Component: drd (show other bugs)
Version: 3.9.0
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Bart Van Assche
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-06 15:37 UTC by ewirch
Modified: 2013-12-10 17:37 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description ewirch 2013-12-06 15:37:17 UTC
Valgrind compiled from trunk r13748. Simple test cases uses threads to access a boolean atomic variable. DRD reports a data race for the variable.

Reproducible: Always




Address trace for conflicting address:

==2561== store 0xa2f7ca0 size 1 val 1/0x1 (thread 10 / vc [ 1: 1434, 2: 45, 3: 4, 4: 4, 5: 4, 6: 4, 7: 4, 8: 4, 9: 4, 10: 4 ])
==2561==    at 0x421E74: std::__atomic_base<bool>::store(bool, std::memory_order) (atomic_base.h:474)
==2561==    by 0x421E0F: std::__atomic_base<bool>::operator=(bool) (atomic_base.h:375)
==2561==    by 0x421578: std::atomic_bool::operator=(bool) (atomic:70)
==2561==    by 0x4B9D73: Lock_test::AutoLockDoesLock_Thread1::doAutoLock() (Locktest.cpp:62)
==2561==    by 0x4B9D23: Lock_test::AutoLockDoesLock_Thread1::threadWork() (Locktest.cpp:56)
==2561==    by 0x4BC8AE: Thread::invokeRun(void*) (Thread.cpp:66)
==2561==    by 0x4C30834: vgDrd_thread_wrapper (drd_pthread_intercepts.c:355)
==2561==    by 0x4E4FE99: start_thread (pthread_create.c:308)
==2561==    by 0x5B71CCC: clone (clone.S:112)
==2561== load  0xa2f7ca0 size 1 (thread 12 / vc [ 1: 1434, 2: 49, 3: 4, 4: 4, 5: 4, 6: 4, 7: 4, 8: 4, 9: 4, 10: 2, 12: 3 ])
==2561==    at 0x421E3B: std::__atomic_base<bool>::load(std::memory_order) const (atomic_base.h:496)
==2561==    by 0x421598: std::atomic_bool::operator bool() const (atomic:77)
==2561==    by 0x4B9DFA: Lock_test::LockDoesLock_Thread2::threadWork() (Locktest.cpp:75)
==2561==    by 0x4BC8AE: Thread::invokeRun(void*) (Thread.cpp:66)
==2561==    by 0x4C30834: vgDrd_thread_wrapper (drd_pthread_intercepts.c:355)
==2561==    by 0x4E4FE99: start_thread (pthread_create.c:308)
==2561==    by 0x5B71CCC: clone (clone.S:112)
==2561== Thread 12:
--2561-- warning: evaluate_Dwarf3_Expr: unhandled DW_OP_ 0xf3
--2561-- warning: evaluate_Dwarf3_Expr: unhandled DW_OP_ 0xf3
--2561-- warning: evaluate_Dwarf3_Expr: unhandled DW_OP_ 0xf3
--2561-- warning: evaluate_Dwarf3_Expr: unhandled DW_OP_ 0xf3
==2561== Conflicting load by thread 12 at 0x0a2f7ca0 size 1
==2561==    at 0x421E3B: std::__atomic_base<bool>::load(std::memory_order) const (atomic_base.h:496)
==2561==    by 0x421598: std::atomic_bool::operator bool() const (atomic:77)
==2561==    by 0x4B9DFA: Lock_test::LockDoesLock_Thread2::threadWork() (Locktest.cpp:75)
==2561==    by 0x4BC8AE: Thread::invokeRun(void*) (Thread.cpp:66)
==2561==    by 0x4C30834: vgDrd_thread_wrapper (drd_pthread_intercepts.c:355)
==2561==    by 0x4E4FE99: start_thread (pthread_create.c:308)
==2561==    by 0x5B71CCC: clone (clone.S:112)
==2561== Location 0xa2f7ca0 is 16 bytes inside local var "t1"
==2561== declared at Locktest.cpp:108, in frame #5 of thread 2
==2561== Other segment start (thread 10)
==2561==    at 0x4C3349E: pthread_mutex_lock (drd_pthread_intercepts.c:630)
==2561==    by 0x4BC5A6: Lock::lock() (Lock.cpp:47)
==2561==    by 0x405697: AutoLock::AutoLock(Lock&) (Lock.h:65)
==2561==    by 0x4B9D5E: Lock_test::AutoLockDoesLock_Thread1::doAutoLock() (Locktest.cpp:61)
==2561==    by 0x4B9D23: Lock_test::AutoLockDoesLock_Thread1::threadWork() (Locktest.cpp:56)
==2561==    by 0x4BC8AE: Thread::invokeRun(void*) (Thread.cpp:66)
==2561==    by 0x4C30834: vgDrd_thread_wrapper (drd_pthread_intercepts.c:355)
==2561==    by 0x4E4FE99: start_thread (pthread_create.c:308)
==2561==    by 0x5B71CCC: clone (clone.S:112)
==2561== Other segment end (thread 10)
==2561==    at 0x4B9D75: Lock_test::AutoLockDoesLock_Thread1::doAutoLock() (Locktest.cpp:63)
==2561==    by 0x4B9D23: Lock_test::AutoLockDoesLock_Thread1::threadWork() (Locktest.cpp:56)
==2561==    by 0x4BC8AE: Thread::invokeRun(void*) (Thread.cpp:66)
==2561==    by 0x4C30834: vgDrd_thread_wrapper (drd_pthread_intercepts.c:355)
==2561==    by 0x4E4FE99: start_thread (pthread_create.c:308)
==2561==    by 0x5B71CCC: clone (clone.S:112)
Comment 1 ewirch 2013-12-06 15:38:21 UTC
Forgot to mention: address is onto stack, valgrind executed with --check-stack-var=yes
Comment 2 ewirch 2013-12-06 15:38:50 UTC
Similar bug reported for helgrind: 327881
Comment 3 Bart Van Assche 2013-12-06 16:08:40 UTC
It is not possible for a run-time analysis tool like DRD to recognize sig_atomic_t variables at runtime. My recommendation is either to use a data type that can be recognized by DRD (std::atomic ?) or to use DRD_IGNORE_VAR().
Comment 4 ewirch 2013-12-09 07:10:00 UTC
Thanks for the info Bart. But I am using std::atomic:

==2561==    at 0x421E3B: std::__atomic_base<bool>::load(std::memory_order) const (atomic_base.h:496)
==2561==    by 0x421598: std::atomic_bool::operator bool() const (atomic:77)
Comment 5 Bart Van Assche 2013-12-09 09:02:55 UTC
If you can provide a test program that allows to reproduce the above output I will look further into this issue.
Comment 6 ewirch 2013-12-09 10:02:19 UTC
Sure:

#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <string>
#include <atomic>
using namespace std;

class Thread {
public:

	Thread() {
	}

	virtual ~Thread() {
	}

	void run() {
		int err;
		err = pthread_create(&pThread_, NULL, &Thread::invokeRun, (void*)this);
		if (err != 0) {
			throw string("Thread::run(): failed to create a thread.");
		}
	}

	void join() {
		void *threadRetValue;
		int err = pthread_join(pThread_, &threadRetValue);
		if (err != 0) {
			throw string("Thread::join(): failed to join.");
		}
	}

	virtual void threadWork() = 0;
private:
	static void *invokeRun(void *instance);
	pthread_t pThread_;
};

void *Thread::invokeRun(void *instance) {
	reinterpret_cast<Thread*>(instance)->threadWork();
	return NULL;
}


class LockDoesLock_Thread1: public Thread {
public:
	atomic<bool> lockAcquired_;
	atomic<bool> tryLocked_;
	atomic<bool> unlocked_;
	atomic<bool> tryLockSuccess1_;
	atomic<bool> tryLockSuccess2_;
	pthread_mutex_t &mutex_;

	LockDoesLock_Thread1(pthread_mutex_t &pLock) :
			mutex_(pLock) {
		lockAcquired_ = false;
		tryLocked_ = false;
		unlocked_ = false;
		tryLockSuccess1_ = false;
		tryLockSuccess2_ = false;
	}

	void threadWork() {
		pthread_mutex_lock(&mutex_);
		lockAcquired_ = true;
		while (!tryLocked_) {
		};
		pthread_mutex_unlock(&mutex_);
		unlocked_ = true;
	}
};

class AutoLockDoesLock_Thread1: public LockDoesLock_Thread1 {
public:
	AutoLockDoesLock_Thread1(pthread_mutex_t &pLock) :
			LockDoesLock_Thread1(pLock) {
	}

	void threadWork() {
		doAutoLock();
		unlocked_ = true;
	}

	void doAutoLock() {
		pthread_mutex_lock(&mutex_);
		lockAcquired_ = true;
		while (!tryLocked_) {
		};
		pthread_mutex_unlock(&mutex_);
	}
};

class LockDoesLock_Thread2: public Thread {
public:
	LockDoesLock_Thread2(LockDoesLock_Thread1 &buddy) :
			buddy_(buddy) {
	}

	void threadWork() {
		while (!buddy_.lockAcquired_) {
		}

		buddy_.tryLockSuccess1_ = (pthread_mutex_trylock(&buddy_.mutex_) == 0);
		buddy_.tryLocked_ = true;

		while (!buddy_.unlocked_) {
		}
		buddy_.tryLockSuccess2_ = (pthread_mutex_trylock(&buddy_.mutex_) == 0);

		if (buddy_.tryLockSuccess2_) {
			pthread_mutex_unlock(&buddy_.mutex_);
		}
	}
private:
	LockDoesLock_Thread1 &buddy_;
};

int main(int argc, char** argv) {
	pthread_mutex_t mutex;
	pthread_mutex_init(&mutex, NULL);
	AutoLockDoesLock_Thread1 t1(mutex);
	LockDoesLock_Thread2 t2(t1);
	t1.run();
	t2.run();
	t1.join();
	t2.join();
	if (t1.tryLockSuccess1_ != false) printf("Test fail 1\n");
	if (t1.tryLockSuccess2_ != true) printf("Test fail 2\n");
	return true;
}




Compile with: g++ -std=c++11 atomictest.cpp -o atomictest -lpthread
Test with: valgrind --tool=drd --check-stack-var=yes ./atomictest
Comment 7 Bart Van Assche 2013-12-10 17:37:28 UTC
What I see in libstdc++ is the following:
* atomic<bool> is represented by a single byte.
* atomic<bool>::store() uses __atomic_store_n(), and for bool __atomic_store_n() is a compiler-built-in that uses a regular single-byte store.

Sorry but it's not clear to me how DRD or any other Valgrind tool could discern such store operations from regular store operations. Please use DRD_IGNORE_VAR() or equivalent to suppress race reports on atomic<bool> variables.