Page MenuHomePhabricator

sanitizer_common: add thread safety annotations
ClosedPublic

Authored by dvyukov on Jul 9 2021, 10:34 AM.

Details

Summary

Enable clang Thread Safety Analysis for sanitizers:
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

Thread Safety Analysis can detect inconsistent locking,
deadlocks and data races. Without GUARDED_BY annotations
it has limited value. But this does all the heavy lifting
to enable analysis and allows to add GUARDED_BY incrementally.

Diff Detail

Event Timeline

dvyukov created this revision.Jul 9 2021, 10:34 AM
dvyukov requested review of this revision.Jul 9 2021, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 10:34 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
melver accepted this revision.Jul 12 2021, 1:08 AM
melver added inline comments.
compiler-rt/CMakeLists.txt
362

Should this be CMAKE_CXX_COMPILER_ID ?

This revision is now accepted and ready to land.Jul 12 2021, 1:08 AM
dvyukov updated this revision to Diff 357874.Jul 12 2021, 2:46 AM

switch to CMAKE_CXX_COMPILER_ID

dvyukov marked an inline comment as done.Jul 12 2021, 2:46 AM
This revision was landed with ongoing or failed builds.Jul 12 2021, 2:46 AM
This revision was automatically updated to reflect the committed changes.
protze.joachim added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
315

Same annotations should go into sanitizer_allocator_primary32.h for 32-bit systems

see greendragon failure: https://green.lab.llvm.org/green/job/lldb-cmake/33604/consoleFull#-77815080549ba4694-19c4-4d7e-bec5-911270d8a58c

dvyukov added inline comments.Jul 12 2021, 4:55 AM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
315

Thanks. I am looking into this right now.
Also trying to make 64-bit tests tests detect this.

dvyukov added inline comments.Jul 12 2021, 5:00 AM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
315

I've mailed https://reviews.llvm.org/D105808
Currently running all compiler-rt tests, will merge once they pass.

dim added subscribers: bkramer, dim.Aug 3 2021, 2:09 PM

After this commit (tentative) I'm getting these compile errors on FreeBSD, when it's building the googletest parts of the compiler-rt tests:

FAILED: projects/compiler-rt/lib/sanitizer_common/tests/SANITIZER_TEST_OBJECTS.gmock-all.cc.i386.o
cd /home/dim/obj/llvm-llvmorg-14-init-588-g7c921753e0f5-freebsd14-amd64-ninja-clang-rel-1/projects/compiler-rt/lib/sanitizer_common/tests && /home/dim/obj/llvm-llvmorg-14-init-588-g7c921753e0f5-freebsd14-amd64-ninja-clang-rel-1/./bin/clang -Werror=thread-safety -Werror=thread-safety-reference -Werror=thread-safety-beta -g -Wno-covered-switch-default -Wno-suggest-override -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googletest/include -I/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googletest -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/include -I/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock -I/share/dim/src/llvm/llvm-project/compiler-rt/include -I/share/dim/src/llvm/llvm-project/compiler-rt/lib -I/share/dim/src/llvm/llvm-project/compiler-rt/lib/sanitizer_common -fno-rtti -O2 -Werror=sign-compare -Wno-gnu-zero-variadic-macro-arguments -gline-tables-only -m32 -c -o SANITIZER_TEST_OBJECTS.gmock-all.cc.i386.o /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/src/gmock-all.cc
In file included from /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/src/gmock-all.cc:39:
In file included from /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock.h:59:
In file included from /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:53:
In file included from /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:48:
In file included from /share/dim/src/llvm/llvm-project/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-port.h:57:
/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:1636:3: error: mutex 'mutex_' is still held at the end of function [-Werror,-Wthread-safety-analysis]
  }
  ^
/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:1633:32: note: mutex acquired here
    GTEST_CHECK_POSIX_SUCCESS_(pthread_mutex_lock(&mutex_));
                               ^
/share/dim/src/llvm/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:1645:32: error: releasing mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis]
    GTEST_CHECK_POSIX_SUCCESS_(pthread_mutex_unlock(&mutex_));
                               ^
2 errors generated.

Do we need to annotate the googletest stuff now too? Or exclude this from the -Wthread-safety flags somehow?

After this commit (tentative) I'm getting these compile errors on FreeBSD, when it's building the googletest parts of the compiler-rt tests:
Do we need to annotate the googletest stuff now too? Or exclude this from the -Wthread-safety flags somehow?

googletest wraps pthread and it seems FreeBSD pthread.h is the only one annotated with thread-safety annotations:
https://github.com/freebsd/freebsd-src/blob/9bb8a4091c4f63dacda5108f4f994f7f6b35bbf1/include/pthread.h#L240

Long term it would be good to annotation googletest to be compatible with thread-safety annotations (at least headers). But it probably won't happen over-night.
I've checked if FreeBSD pthread annotations can be disabled with some macro or something, but it seems they are enabled unconditionally whenever compiler supports them:
https://github.com/freebsd/freebsd-src/blob/ea3fbe0707f9a02a29875966668b6f15284f335a/sys/sys/cdefs.h#L834-L845

The only thing I can think of is to disable -Wthread-safety on FreeBSD for now, or turn it into a non-fatal error.

nikic added a subscriber: nikic.Aug 9 2021, 9:51 AM

It looks like this also causes a build error on FreeBSD 11 even without tests, due to a false positive warning:

2021-08-08T22:26:43.9999824Z sccache /usr/local/bin/x86_64-unknown-freebsd11-clang++   -I/checkout/src/llvm-project/compiler-rt/lib/asan/.. -ffunction-sections -fdata-sections -fPIC --target=x86_64-unknown-freebsd -static-libstdc++ -Wall -std=c++14 -O3 -DNDEBUG    -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -Werror=thread-safety -Werror=thread-safety-reference -Werror=thread-safety-beta -O3 -gline-tables-only -nostdinc++ -fno-rtti -MD -MT lib/asan/CMakeFiles/RTAsan.x86_64.dir/asan_allocator.cpp.o -MF lib/asan/CMakeFiles/RTAsan.x86_64.dir/asan_allocator.cpp.o.d -o lib/asan/CMakeFiles/RTAsan.x86_64.dir/asan_allocator.cpp.o -c /checkout/src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp
2021-08-08T22:26:44.0004227Z clang: warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument]
2021-08-08T22:26:44.0005611Z In file included from /checkout/src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:31:
2021-08-08T22:26:44.0007444Z /checkout/src/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_quarantine.h:121:7: error: calling function 'Recycle' requires holding mutex 'recycle_mutex_' exclusively [-Werror,-Wthread-safety-analysis]
2021-08-08T22:26:44.0009017Z       Recycle(atomic_load_relaxed(&min_size_), cb);
2021-08-08T22:26:44.0009477Z       ^
2021-08-08T22:26:44.0011389Z /checkout/src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:765:16: note: in instantiation of member function '__sanitizer::Quarantine<__asan::QuarantineCallback, __asan::AsanChunk>::Drain' requested here
2021-08-08T22:26:44.0012845Z     quarantine.Drain(GetQuarantineCache(ms), QuarantineCallback(ac, stack));
2021-08-08T22:26:44.0013531Z                ^
2021-08-08T22:26:44.0014540Z In file included from /checkout/src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:31:
2021-08-08T22:26:44.0016241Z /checkout/src/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_quarantine.h:121:7: error: releasing mutex 'recycle_mutex_' that was not held [-Werror,-Wthread-safety-analysis]
2021-08-08T22:26:44.0017465Z       Recycle(atomic_load_relaxed(&min_size_), cb);
2021-08-08T22:26:44.0017923Z       ^
2021-08-08T22:26:44.0018303Z 2 errors generated.

Apparently the used compiler is clang 6.0.

Can you please drop the -Werror? This is exactly the reason for the general recommendation that -Werror should never be enabled in default configurations: Unless you're actually testing all supported compilers and compiler versions, you're pretty much guaranteed to break the build somewhere due to differences in warnings...

dim added a comment.Aug 9 2021, 10:55 AM

It looks like this also causes a build error on FreeBSD 11 even without tests, due to a false positive warning:

...

Apparently the used compiler is clang 6.0.

Weird, the currently supported FreeBSD 11 version is at 11.4, and it has clang 10.0.1. But I see /usr/local/bin/x86_64-unknown-freebsd11-clang++ in the command line, so it's likely an installed port or hand-built installation?

That said, FreeBSD 11.4 (and the whole 11.x branch) will expire 2021-09-30 so in less than two months. Not sure if it's worth much effort to fix now. :)

Can you please drop the -Werror? This is exactly the reason for the general recommendation that -Werror should never be enabled in default configurations: Unless you're actually testing all supported compilers and compiler versions, you're pretty much guaranteed to break the build somewhere due to differences in warnings...

Yes, -Werror is also a bane of FreeBSD port maintainers. You either have to fiddle with configure scripts to get rid of the -Werror flag or add -Wno-xxx flags, or to monkey-patch the source to silence those warnings...

nikic added a comment.Aug 9 2021, 11:04 AM

@dim Oh right, looks like this build actually uses clang 6 from Ubuntu 18.04 together with a FreeBSD 11.4 sysroot.

Don't think it really matters though, as ultimately LLVM requires clang >= 3.5 (per https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library), so that version should be supported.

dvyukov added a comment.EditedAug 9 2021, 11:11 AM

Yikes.
Turning off -Werror renders it all useless, even worse it will add code with no benefit (nobody is checking warnings, bots don't fail/notify, there are tons of warnings already, no chance of noticing a new one).
So instead of removing -Werror, I would rather remove all of the annotations altogether.
Will it work for you if we enable the thread-safety annotations only if clang_major >= 13? (not sure what's the exact version, but 13 should be safe enough)

nikic added a comment.Aug 9 2021, 1:16 PM

Yikes.
Turning off -Werror renders it all useless, even worse it will add code with no benefit (nobody is checking warnings, bots don't fail/notify, there are tons of warnings already, no chance of noticing a new one).
So instead of removing -Werror, I would rather remove all of the annotations altogether.
Will it work for you if we enable the thread-safety annotations only if clang_major >= 13? (not sure what's the exact version, but 13 should be safe enough)

The usual approach is to not enable -Werror in a default configuration, but explicitly enable it in CI where the used compiler version is known. For compiler-rt there is a COMPILER_RT_ENABLE_WERROR flag for for purpose. Based on a quick search, it seems to be enabled at least in some buildbot configurations, for example it's used here: https://github.com/llvm/llvm-zorg/blob/855fdb4cdc09e306370533c0715897e6ac7f3027/zorg/buildbot/builders/sanitizers/buildbot_cmake.sh#L245 However, if you say that there are already warnings (with clang), then possibly that isn't working properly.

Anyway, limiting by clang version seems like a reasonable workaround for this issue, though that wouldn't fix the error @dim reported. (From our side an upstream fix isn't strictly necessary -- as it's only a build issue, we can patch it.)

I did not know about COMPILER_RT_ENABLE_WERROR enabled on some bots.
And also I was wrong about existing warnings. They present but only with gcc (since I compile with both regularly, I had impression I always see warnings).
I've mailed https://reviews.llvm.org/D107826 is it what you meant?
I've also removed the FreeBSD workaround added by @dim. It won't be needed anymore, right?

dim added a comment.Aug 10 2021, 7:40 AM

I did not know about COMPILER_RT_ENABLE_WERROR enabled on some bots.
And also I was wrong about existing warnings. They present but only with gcc (since I compile with both regularly, I had impression I always see warnings).
I've mailed https://reviews.llvm.org/D107826 is it what you meant?
I've also removed the FreeBSD workaround added by @dim. It won't be needed anymore, right?

If they're warnings now, they won't break the build, indeed. (Would still be nice to patch up gtest at some point, but it's not necessary right away.)

I can keep that part if you prefer (with REPLACE "-W" "-Wno-").

dim added a comment.Aug 10 2021, 8:09 AM

I can keep that part if you prefer (with REPLACE "-W" "-Wno-").

No, leave the warnings in, then there is at least *some* incentive to fix them at some point, even if it's just to fix the annoyance. :)