Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8637025:
Add tsan suppression for gamepad unittest code (Closed)
|
Created:
9 years, 1 month ago by scottmg Modified:
9 years, 1 month ago CC:
chromium-reviews, Timur Iskhodzhanov, Alexander Potapenko, pam+watch_chromium.org, stuartmorgan+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd tsan suppression for gamepad unittest code
BUG=79050
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111262
Patch Set 1 #
Total comments: 1
Patch Set 2 : add bug number #Patch Set 3 : fix spelling of 'bug', sigh #
Total comments: 2
Patch Set 4 : don't elide memcpy #Messages
Total messages: 12 (0 generated)
http://codereview.chromium.org/8637025/diff/1/tools/valgrind/tsan/suppression... File tools/valgrind/tsan/suppressions.txt (right): http://codereview.chromium.org/8637025/diff/1/tools/valgrind/tsan/suppression... tools/valgrind/tsan/suppressions.txt:396: Test to detect read contention & benign race There should be a bug logged to track this, then "bug_123456" added to this suppression.
Looks like you already have a bug, so "bug_79050".
http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... File tools/valgrind/tsan/suppressions.txt (right): http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... tools/valgrind/tsan/suppressions.txt:397: ThreadSanitizer:Race Why did you choose to elide the lines?: fun:Replace_memcpy fun:memcpy
On 2011/11/22 21:35:09, dhollowa wrote: > http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... > File tools/valgrind/tsan/suppressions.txt (right): > > http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... > tools/valgrind/tsan/suppressions.txt:397: ThreadSanitizer:Race > Why did you choose to elide the lines?: > > fun:Replace_memcpy > fun:memcpy No good reason I guess? They sort of seemed like implementation details, but I don't really know the policy for what to keep and what to hide.
On 2011/11/22 21:47:13, scottmg wrote: > On 2011/11/22 21:35:09, dhollowa wrote: > > > http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... > > File tools/valgrind/tsan/suppressions.txt (right): > > > > > http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... > > tools/valgrind/tsan/suppressions.txt:397: ThreadSanitizer:Race > > Why did you choose to elide the lines?: > > > > fun:Replace_memcpy > > fun:memcpy > > No good reason I guess? They sort of seemed like implementation details, but I > don't really know the policy for what to keep and what to hide. I typically only elide when there are multiple suppressions that vary only slightly but point to the same underlying fault. And then I put the full set of details/suppressions in the bug. In this case, since there's only the one unique occurrence it is better to leave in the details in the suppression to aid the person that comes along later to fix the issue.
LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/8637025/6004
In the future it is better to land suppressions directly. It greens the tree faster. The commit bot doesn't provide any additional security when it comes to suppressions.
OK, thanks. I'm not a committer, but once I am, I will do that (or get someone else to in the meantime). On Tue, Nov 22, 2011 at 3:07 PM, <dhollowa@chromium.org> wrote: > > In the future it is better to land suppressions directly. It greens the > tree > faster. The commit bot doesn't provide any additional security when it > comes to > suppressions. > > http://codereview.chromium.**org/8637025/<http://codereview.chromium.org/8637... >
Change committed as 111262
http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... File tools/valgrind/tsan/suppressions.txt (right): http://codereview.chromium.org/8637025/diff/6002/tools/valgrind/tsan/suppress... tools/valgrind/tsan/suppressions.txt:397: ThreadSanitizer:Race Actually, the eliding would help: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28t... WARNING: Possible data race during read of size 8 at 0x45D8008: {{{ T83 (Gamepad polling thread) (L{}): #0 ::MockDataFetcher::GetGamepadData content/browser/gamepad/gamepad_provider_unittest.cc:22 #1 gamepad::Provider::DoPoll content/browser/gamepad/gamepad_provider.cc:123 #2 base::internal::RunnableAdapter::Run base/bind_internal.h:140 #3 base::internal::InvokeHelper::MakeItSo base/bind_internal.h:771 #4 base::internal::Invoker::Run base/bind_internal.h:1010 #5 base::Callback::Run base/callback.h:268 #6 MessageLoop::RunTask base/message_loop.cc:500 Concurrent write(s) happened at (OR AFTER) these points: T0 (L{}): #0 ::MockDataFetcher::SetData content/browser/gamepad/gamepad_provider_unittest.cc:26 #1 ::GamepadProviderTest_FLAKY_PollingAccess_Test::TestBody content/browser/gamepad/gamepad_provider_unittest.cc:82 #2 testing::internal::HandleSehExceptionsInMethodIfSupported testing/gtest/src/gtest.cc:2090 Location 0x45D8008 is 8 bytes inside a block starting at 0x45D8000 of size 1656 allocated by T0 from heap: #0 operator new /tmp/valgrind-src/tsan/tsan/ts_valgrind_intercepts.c:426 #1 ::GamepadProviderTest::CreateProvider content/browser/gamepad/gamepad_provider_unittest.cc:44 #2 ::GamepadProviderTest_FLAKY_PollingAccess_Test::TestBody content/browser/gamepad/gamepad_provider_unittest.cc:70 #3 testing::internal::HandleSehExceptionsInMethodIfSupported testing/gtest/src/gtest.cc:2090 Race verifier data: 0x453486,0x4530BA }}} Rationale for dropping lines like memcpy for TSan: a) the code lacks synchronization. hence b) writes below GetGamepadData may race with something else. memcpy is a form of write, right? On 2011/11/22 21:35:09, dhollowa wrote: > Why did you choose to elide the lines?: > > fun:Replace_memcpy > fun:memcpy |
