Disk Cache: Supress tsan warnings for BackendTrimInvalidEntry2

Disk Cache: Supress tsan warnings for BackendTrimInvalidEntry2

BUG=101853
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107844

[rvargas (doing something else), 2011-10-28 03:03:06]

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest.cc
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
net/disk_cache/backend_unittest.cc:787:
ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
I'm assuming that it is fine if the test asserts and doesn't reach the _end()
macro. Or do I have to make sure that doesn't happen?

[kcc2, 2011-10-28 03:15:32]

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest.cc
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
net/disk_cache/backend_unittest.cc:787:
ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
If the test asserts and then exits -- fine. 
But if you call BEGIN, don't call END and the test proceeds, a bad thing will
happen -- race detection will be disable for all further tests. 
I'd suggest to move the annotations to the place where you call
BackendTrimInvalidEntry2

[Timur Iskhodzhanov, 2011-10-28 12:14:02]

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest.cc
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
net/disk_cache/backend_unittest.cc:787:
ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
Assert indeed interrupts the test.

On 2011/10/28 03:15:32, kcc2 wrote:
> If the test asserts and then exits -- fine. 
> But if you call BEGIN, don't call END and the test proceeds, a bad thing will
> happen -- race detection will be disable for all further tests. 
> I'd suggest to move the annotations to the place where you call
> BackendTrimInvalidEntry2

http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
net/disk_cache/backend_unittest.cc:832: EXPECT_GE(30, cache_->GetEntryCount());
ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
int cache_entry_count = cache_->GetEntryCount();
ANNOTATE_IGNORE_READS_AND_WRITES_END();
EXPECT_GE(30, cache_entry_count);

[Timur Iskhodzhanov, 2011-10-28 12:21:52]

Or if you just don't want to run the test you may use
tools/valgrind/gtest_exclude/net_unittests.gtest-tsan.txt
(see http://codereview.chromium.org/8400033/ )
but I'd prefer finer granularity suggested above.

On 2011/10/28 12:14:02, Timur Iskhodzhanov wrote:
>
http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest.cc
> File net/disk_cache/backend_unittest.cc (right):
> 
>
http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
> net/disk_cache/backend_unittest.cc:787:
> ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
> Assert indeed interrupts the test.
> 
> On 2011/10/28 03:15:32, kcc2 wrote:
> > If the test asserts and then exits -- fine. 
> > But if you call BEGIN, don't call END and the test proceeds, a bad thing
will
> > happen -- race detection will be disable for all further tests. 
> > I'd suggest to move the annotations to the place where you call
> > BackendTrimInvalidEntry2
> 
>
http://codereview.chromium.org/8343048/diff/1/net/disk_cache/backend_unittest...
> net/disk_cache/backend_unittest.cc:832: EXPECT_GE(30,
cache_->GetEntryCount());
> ANNOTATE_IGNORE_READS_AND_WRITES_BEGIN();
> int cache_entry_count = cache_->GetEntryCount();
> ANNOTATE_IGNORE_READS_AND_WRITES_END();
> EXPECT_GE(30, cache_entry_count);

[rvargas (doing something else), 2011-10-28 19:27:40]

Thanks a lot.

I uploaded a new version.

[kcc2, 2011-10-28 19:33:26]

LGTM
But please add a comment describing why you do that.