Tsan: Ignore DiskCacheBackendTest::BackendTrimInvalidEntry2

Tsan: Ignore DiskCacheBackendTest::BackendTrimInvalidEntry2

BUG=101853
TEST=none

[Timur Iskhodzhanov, 2011-10-27 20:28:29]

I'd rather put ANNOTATE_ignore_begin/end in the racey function.
+kcc for the review in your timezone

[Timur Iskhodzhanov, 2011-10-27 20:30:44]

actually +kcc, blame Android for that

[rvargas (doing something else), 2011-10-27 21:19:29]

On 2011/10/27 20:28:29, Timur Iskhodzhanov wrote:
> I'd rather put ANNOTATE_ignore_begin/end in the racey function.
> +kcc for the review in your timezone

I was looking for that in the docs but didn't find it.

However, now that I found this it sounds better this way 'cause it ignores just
this test, not all calls to the underlying function (as far as I understand
func_r, don't know for sure what it does).

In any case, gcl is acting on me so I have to create another rietveld issue.

Thanks.

[kcc2, 2011-10-28 02:00:58]

http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt
File tools/valgrind/tsan/ignores.txt (right):

http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt...
tools/valgrind/tsan/ignores.txt:163: # Ignore intentional races from this
particular test.
1. Why do we prefer ignore over a suppression? Ignores are usually trickier,
especially fun_r which actually has known problems in the implementation. I'd
leave fun_r for the internal stuff. 
2. All ignores/suppressions that hide real races should have a bug reference
that explains the race and why we are suppressing it (instead of fixing).

[rvargas (doing something else), 2011-10-28 02:21:50]

On 2011/10/28 02:00:58, kcc2 wrote:
> http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt
> File tools/valgrind/tsan/ignores.txt (right):
> 
>
http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt...
> tools/valgrind/tsan/ignores.txt:163: # Ignore intentional races from this
> particular test.
> 1. Why do we prefer ignore over a suppression? Ignores are usually trickier,
> especially fun_r which actually has known problems in the implementation. I'd
> leave fun_r for the internal stuff.

Because it is way more noise?. I mean, instead of a single line that tells
directly what I want ("do not run tsan on this test"), we have to deal with n
different stacks (4 on the latest incarnation) that  may change in the future,
adding maintenance burden for everyone (the change that triggered this was not
directly related with this test).

> 2. All ignores/suppressions that hide real races should have a bug reference
> that explains the race and why we are suppressing it (instead of fixing).

Sure... the comment looked self-explanatory to me, but I'll add the bug number.

[rvargas (doing something else), 2011-10-28 02:26:32]

On 2011/10/27 20:28:29, Timur Iskhodzhanov wrote:
> I'd rather put ANNOTATE_ignore_begin/end in the racey function.

does that mean that 

Foo_Test(Foo, test) {
  ANNOTATE_ignore_begin
...
  ANNOTATE_ignore_end
}

works?. For some reason I thought it would not, but if it does it indeed looks
like the best solution.

> +kcc for the review in your timezone

[kcc2, 2011-10-28 02:27:42]

On 2011/10/28 02:21:50, rvargas wrote:
> On 2011/10/28 02:00:58, kcc2 wrote:
> >
http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt
> > File tools/valgrind/tsan/ignores.txt (right):
> > 
> >
>
http://codereview.chromium.org/8400033/diff/1/tools/valgrind/tsan/ignores.txt...
> > tools/valgrind/tsan/ignores.txt:163: # Ignore intentional races from this
> > particular test.
> > 1. Why do we prefer ignore over a suppression? Ignores are usually trickier,
> > especially fun_r which actually has known problems in the implementation.
I'd
> > leave fun_r for the internal stuff.
> 
> Because it is way more noise?. I mean, instead of a single line that tells
> directly what I want ("do not run tsan on this test"), we have to deal with n
> different stacks (4 on the latest incarnation) that  may change in the future,
> adding maintenance burden for everyone (the change that triggered this was not
> directly related with this test).
> 
> > 2. All ignores/suppressions that hide real races should have a bug reference
> > that explains the race and why we are suppressing it (instead of fixing).
> 
> Sure... the comment looked self-explanatory to me, but I'll add the bug
number.

fun_r will ignore the accesses in the provided function and all functions called
from it. We usually use it for the code we do not control (e.g. third_party
libraries) as a performance optimization for hot spots and for tricky and scary
code that is hard to annotate. 

For a test which contains an intentional race (and which is not present in the
production code), I'd suggest to add ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN/END
to the main test function. 
This will be functionally equivalent to fun_r, but will be easier to maintain.

[Timur Iskhodzhanov, 2011-10-28 12:20:54]

kcc
> Why do we prefer ignore over a suppression?
The real answer: because TSan doesn't test "previous access" stacks
against suppressions.
The suppression was there AFAIU, but it wasn't enough due to accesses
happening in both A-B and B-A order.

rvargas
> instead of a single line that tells directly what I want ("do not run tsan on
this test"),
> we have to deal with n different stacks
You can add that one line+comment to
tools/valgrind/gtest_exclude/net_unittests.gtest-tsan.txt