Subject: More quality quality

From: Daniel van Vugt <[email-masked]>
Date: Tue, 13 May 2014 11:52:38 +0800

All,

I'd like (in the somewhat distant future) for us to be able to run Mir's 
tests under helgrind [1] so that we can automatically detect races in 
continuous integration.

It's not going to be a short journey, as there's a fair amount of errors 
to fix yet. They're mostly benign but the noise easily hides real bugs. 
So in the mean time, if you write any new code that crosses multiple 
threads, please try to run your individual test cases under helgrind and 
check no new races are being introduced.

[1] valgrind --tool=helgrind

- Daniel

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Tue, 13 May 2014 06:05:05 +0010

On Tue, May 13, 2014 at 1:52 PM, Daniel van Vugt 
<[email-masked]> wrote:
> All,
> 
> I'd like (in the somewhat distant future) for us to be able to run 
> Mir's tests under helgrind [1] so that we can automatically detect 
> races in continuous integration.
> 
> It's not going to be a short journey, as there's a fair amount of 
> errors to fix yet. They're mostly benign but the noise easily hides 
> real bugs. So in the mean time, if you write any new code that 
> crosses multiple threads, please try to run your individual test 
> cases under helgrind and check no new races are being introduced.

If we want to use helgrind seriously we should probably fix some of its 
more glaring shortcomings. In particular, it doesn't understand 
std::atomic and doesn't appear to understand std::condition_variable, 
which means it generates a _lot_ of false positives for code that's 
transparently correct.

---

From: Daniel van Vugt <[email-masked]>
Date: Tue, 13 May 2014 14:21:42 +0800

The std::atomic issue is minor and well understood (helgrind only 
understands critical sections implemented using pthread primitives). We 
can either suppress std::atomic using valgrind rules or replace it (less 
efficiently). That said, plenty of people using std::atomic are actually 
writing racy code, which should be more correctly implemented with a 
{condition_variable,mutex,bool} triplet.

As for std::condition_variable, that should be implemented as a 
pthread_cond_t that helgrind fully understands... no? What false 
positives have you seen from std::condition_variable?

- Daniel

On 13/05/14 13:55, Christopher James Halse Rogers wrote:
>> real bugs. So in the mean time, if you write any new code that crosses
>> multiple threads, please try to run your individual test cases under
>> helgrind and check no new races are being introduced.
>

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Tue, 13 May 2014 06:44:34 +0010

On Tue, May 13, 2014 at 4:21 PM, Daniel van Vugt 
<[email-masked]> wrote:
> understands critical sections implemented using pthread primitives). 
> We can either suppress std::atomic using valgrind rules or replace it 
> (less efficiently). That said, plenty of people using std::atomic are 
> actually writing racy code, which should be more correctly 
> implemented with a {condition_variable,mutex,bool} triplet.
> 

It warns on broadcast with no blocked threads, which is not 
unreasonable but is also not actually an error.

It also gets confused when things interact with shared_ptr<>; if you 
have a std::condition_variable in something in a shared_ptr<> it will 
detect a race between deletion and waiting.

I've run it on my code occasionally, and each time it has generated 
pages of results, all of which have been false-positives.

It'd be great if we could trust it, but I don't think we can.

---

From: Daniel van Vugt <[email-masked]>
Date: Tue, 13 May 2014 14:49:47 +0800

I trust helgrind after using it for years... it always won the argument 
despite not being able to easily answer "what's wrong with my code?". 
The eraser algorithm is extremely reliable and provably correct. 
Although we don't always understand the errors it reports, that's a 
human problem :)

If there are some "safe races" floating around, I think it's worth the 
pain to be forced to assess and deal with them individually.

On 13/05/14 14:34, Christopher James Halse Rogers wrote:
>> actually writing racy code, which should be more correctly implemented
>> with a {condition_variable,mutex,bool} triplet.
>>
> It warns on broadcast with no blocked threads, which is not unreasonable
> but is also not actually an error.
>