This is an archive of the discontinued LLVM Phabricator instance.

tsan: introduce New/Alloc/Free helpers
ClosedPublic

Authored by dvyukov on Jul 29 2021, 9:31 AM.

Details

Summary

We frequenty allocate sizeof(T) memory and call T ctor on that memory
(C++ new keyword effectively). Currently it's quite verbose and
usually takes 2 lines of code.
Add New<T>() helper that does it much more concisely.

Rename internal_free to Free that also sets the pointer to nullptr.
Shorter and safer.

Rename internal_alloc to Alloc, just shorter.

Diff Detail

Event Timeline

dvyukov requested review of this revision.Jul 29 2021, 9:31 AM
dvyukov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 9:31 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka accepted this revision.Jul 29 2021, 5:08 PM
vitalybuka added inline comments.
compiler-rt/lib/tsan/rtl/tsan_mman.h
71

if (p == nullptr) is needed here or caller will have to check
and it will make it more similar to delete operator.

compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
309

Can you make clang-tidy happy?

This revision is now accepted and ready to land.Jul 29 2021, 5:08 PM
melver accepted this revision.Jul 30 2021, 1:20 AM
melver added inline comments.
compiler-rt/lib/tsan/rtl/tsan_mman.h
55–70

You could make New also forward arguments:

template <typename T, typename... U>
T *New(U&&... u) {
  return new (Alloc(sizeof(T))) T(std::forward<U>(u)...));
}

Then in other locations where you're still doing placement new + alloc you could also just use New<Foo>(args,..)

dvyukov updated this revision to Diff 363011.Jul 30 2021, 2:50 AM
dvyukov marked 2 inline comments as done.

addressed comments

This revision was landed with ongoing or failed builds.Jul 30 2021, 2:52 AM
This revision was automatically updated to reflect the committed changes.
melver added inline comments.Jul 30 2021, 3:40 AM
compiler-rt/lib/tsan/rtl/tsan_mman.h
55–70
dvyukov added inline comments.Jul 30 2021, 3:41 AM
compiler-rt/lib/tsan/rtl/tsan_mman.h
55–70

It would be nice, but I am not sure we can use C++ library in general, and newer features with older host compilers (like the ones we have on some buildbots). We have own declaration of placement new, I dunno if it's needed but FWIW.
So if we try this, I would prefer to not mix with this change.

hctim added a comment.Jul 30 2021, 8:30 AM
FAIL: ThreadSanitizer-x86_64 :: benign_race.cpp (148 of 419)
******************** TEST 'ThreadSanitizer-x86_64 :: benign_race.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';      /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/./bin/clang  -fsanitize=thread -Wall  -m64   -gline-tables-only -I/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/../ -O1 /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/benign_race.cpp -o /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/tools/clang/runtime/compiler-rt-bins/test/tsan/X86_64Config/Output/benign_race.cpp.tmp &&  /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/tools/clang/runtime/compiler-rt-bins/test/tsan/X86_64Config/Output/benign_race.cpp.tmp 2>&1 | FileCheck /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/benign_race.cpp
--
Exit Code: 66
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  ThreadSanitizer-x86_64 :: benign_race.cpp
FAIL: ThreadSanitizer-x86_64 :: benign_race.cpp (148 of 419)
******************** TEST 'ThreadSanitizer-x86_64 :: benign_race.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';      /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/./bin/clang  -fsanitize=thread -Wall  -m64   -gline-tables-only -I/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/../ -O1 /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/benign_race.cpp -o /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/tools/clang/runtime/compiler-rt-bins/test/tsan/X86_64Config/Output/benign_race.cpp.tmp &&  /b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/tools/clang/runtime/compiler-rt-bins/test/tsan/X86_64Config/Output/benign_race.cpp.tmp 2>&1 | FileCheck /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/test/tsan/benign_race.cpp
--
Exit Code: 66
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  ThreadSanitizer-x86_64 :: benign_race.cpp

I am puzzled why this single bot failed on this single test... The change indeed touches tsan_interface_ann.cpp which is involved in benign race handling, but I don't see how this change is not a no-op...

This comment was removed by vitalybuka.

I've tried with SANITIZER_DEBUG=0/1 with clang/gcc, still can't reproduce.
Pity our bots still don't print test output... Is it data race or CHECK failure?...

I've stared at this change more, but don't see anything that could have caused the failure. I am out of ideas now.
We could try:

  • race = New<ExpectRace>(); + race = Alloc(sizeof(ExpectRace));

but I don't see why it would help.

I've stared at this change more, but don't see anything that could have caused the failure. I am out of ideas now.
We could try:

  • race = New<ExpectRace>(); + race = Alloc(sizeof(ExpectRace));

but I don't see why it would help.

I can repro with the steps at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild --

BUILDBOT_MONO_REPO_PATH=$(cd ../llvm-project && pwd) BUILDBOT_CLOBBER= BUILDBOT_REVISION= ./llvm-zorg/zorg/buildbot/builders/sanitizers/buildbot_standard.sh

Running that test only without piping into FileCheck I see:

ThreadSanitizer: CHECK failed: sanitizer_mutex.cpp:198 "((locked[i].recursion)) == ((0))" (0x1, 0x0) (tid=3329281)
    #0 __tsan::CheckUnwind() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp:383:25 (benign_race.cpp.tmp+0x4e97b1)
    #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_termination.cpp:86:5 (benign_race.cpp.tmp+0x43c673)
    #2 __sanitizer::InternalDeadlockDetector::CheckNoLocks() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_mutex.cpp:198:48 (benign_race.cpp.tmp+0x43433f)
    #3 __sanitizer::CheckedMutex::CheckNoLocksImpl() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_mutex.cpp:222:59 (benign_race.cpp.tmp+0x433c0b)
    #4 __sanitizer::CheckedMutex::CheckNoLocks() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_mutex.h:137:5 (benign_race.cpp.tmp+0x4c5268)
    #5 __tsan::ScopedInterceptor::~ScopedInterceptor() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:268:5 (benign_race.cpp.tmp+0x460712)
    #6 memset /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:799:3 (benign_race.cpp.tmp+0x46d1c1)
    #7 __tsan::ExpectRace* __tsan::New<__tsan::ExpectRace>() /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_mman.h:56:33 (benign_race.cpp.tmp+0x4cae04)
    #8 __tsan::AddExpectRace(__tsan::ExpectRace*, char*, int, unsigned long, unsigned long, char*) /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp:92:10 (benign_race.cpp.tmp+0x4c9913)
    #9 BenignRaceImpl(char*, int, unsigned long, unsigned long, char*) /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp:355:3 (benign_race.cpp.tmp+0x4c9b00)
    #10 AnnotateBenignRaceSized /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp:364:3 (benign_race.cpp.tmp+0x4c9aa0)
    #11 main /usr/local/google/home/elver/third_party/llvm/llvm-project/compiler-rt/test/tsan/benign_race.cpp:16:3 (benign_race.cpp.tmp+0x51be4c)
    #12 __libc_start_main csu/../csu/libc-start.c:308:16 (libc.so.6+0x26d09)
    #13 _start <null> (benign_race.cpp.tmp+0x4212c9)

Ok, now it's obvious what happens. Thanks, Marco.
Something happened with out test for memset/memcpy in runtime. It should have been detected it with a clear explanation.
And I still don't understand why there is memset... the default ctor should not store anything into the object...
But anyway I mailed https://reviews.llvm.org/D107212 to revert back to Alloc.