This is an archive of the discontinued LLVM Phabricator instance.

sanitizer_common: add simpler ThreadRegistry ctor
ClosedPublic

Authored by dvyukov on Jul 9 2021, 9:48 AM.

Details

Summary

Currently ThreadRegistry is overcomplicated because of tsan,
it needs tid quarantine and reuse counters. Other sanitizers
don't need that. It also seems that no other sanitizer now
needs max number of threads. Asan used to need 2^24 limit,
but it does not seem to be needed now. Other sanitizers blindly
copy-pasted that without reasons. Lsan also uses quarantine,
but I don't see why that may be potentially needed.

Add a ThreadRegistry ctor that does not require any sizes
and use it in all sanitizers except for tsan.
In preparation for new tsan runtime, which won't need
any of these parameters as well.

Diff Detail

Event Timeline

dvyukov requested review of this revision.Jul 9 2021, 9:48 AM
dvyukov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 9:48 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Jul 9 2021, 10:17 AM
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
103

UINT32_MAX

115–116

It's not just simplify, it allocates 4B instead of 4M threads?

vitalybuka added inline comments.Jul 9 2021, 10:21 AM
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

MmapNoReserveOrDie?

vitalybuka added inline comments.Jul 9 2021, 10:23 AM
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

but we have i386 support in asan

dvyukov updated this revision to Diff 357571.Jul 9 2021, 10:53 AM
dvyukov marked 4 inline comments as done.

fixed comments

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

You are right.
Switched to InternalMmapVectorNoCtor to fix this.

sanitizer_common/tests/sanitizer_thread_registry_test.cpp:138:18: error: no matching constructor for initialization of '__sanitizer::ThreadRegistry'

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

you don't need to do threads_.Initialize(1024);, it will initialize on the first push

Everything should work without this
however if I remove the line I get

SanitizerCommon-Unit :: ./Sanitizer-i386-Test/SanitizerCommon.ThreadRegistryTest
SanitizerCommon-Unit :: ./Sanitizer-i386-Test/SanitizerCommon.ThreadRegistryThreadedTest
SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.ThreadRegistryTest
SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.ThreadRegistryThreadedTest

which is very strange

141

do we need max_threads_ now?
I guess threads_.push_back will just crash when re-map fails.

146

I am not sure if this is going to work
push_back may reallocate entire vector, so it will invalidate all pointers to ThreadContext

At least PlatformTSDDtor does that

dvyukov added inline comments.Jul 9 2021, 11:47 AM
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

The vector does not have a ctor. It will work if it's zero initialized, but it's not the case at least in tests (as you discovered :)). Initialize is effectively a replacement for ctor.

141

TSan still uses a lower max_threads_.

146

It only invalidates pointers to pointers to ThreadContext (not the ThreadContext objects themselves). These pointers to pointers don't leak from ThreadRegistry I think.

vitalybuka accepted this revision.Jul 9 2021, 12:05 PM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

Oh, I assumed it's InternalMmapVector
so why not just use InternalMmapVector<ThreadContextBase *> threads_; ?

146

Oh, it's pointers, then LGTM

This revision is now accepted and ready to land.Jul 9 2021, 12:05 PM
dvyukov updated this revision to Diff 357786.Jul 11 2021, 3:34 AM

Use InternalMmapVector instead of InternalMmapVectorNoCtor.

This revision was landed with ongoing or failed builds.Jul 11 2021, 3:36 AM
This revision was automatically updated to reflect the committed changes.
dvyukov added inline comments.Jul 11 2021, 5:38 AM
compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
115–116

Good idea. Done.
We don't need reserve as well. I probably assumed that InternalMmapVector does not round up the buffer to page size.

This broke android:
https://lab.llvm.org/buildbot#builders/77/builds/7734

+ FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/Linux/global-overflow-lld.cpp
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/Linux/global-overflow-lld.cpp:15:12: error: CHECK: expected string not found in input
// CHECK: {{READ of size 1 at}}

^

<stdin>:1:1: note: scanning from here
AddressSanitizer: CHECK failed: sanitizer_common.h:491 "((i)) < ((size_))" (0x0, 0x0) (tid=26836)
^
<stdin>:1:19: note: possible intended match here
AddressSanitizer: CHECK failed: sanitizer_common.h:491 "((i)) < ((size_))" (0x0, 0x0) (tid=26836)

^

Input file: <stdin>
Check file: /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/Linux/global-overflow-lld.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<

1: AddressSanitizer: CHECK failed: sanitizer_common.h:491 "((i)) < ((size_))" (0x0, 0x0) (tid=26836)

check:15'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:15'1 ? possible intended match

2:  <empty stack>

check:15'0 ~~~~~~~~~~~~~~~

It seems that android fails to properly initialize asan runtime (don't create main thread).
I see that AsanInitInternal always calls CreateMainThread. So I am not sure how that is possible.
I don't have android to reproduce and fix this.

vitalybuka reopened this revision.Jul 12 2021, 12:05 PM
This revision is now accepted and ready to land.Jul 12 2021, 12:05 PM

I can take a look, but software android emulator works with zorg/buildbot/builders/sanitizers/buildbot_android.sh

I can take a look, but software android emulator works with zorg/buildbot/builders/sanitizers/buildbot_android.sh

I've run this script, but it started to a full fresh build from the current llvm HEAD I guess. This change is already reverted, so it won't reproduce the problem.

I am not sure how I should use it to debug the problem.
Is there a way to point it to a custom checkout? And run a single test? Or at least 'ninja check-asan`?

It also all failed with "unavailable device":

+ local _arch=i686
+ [[ ! -f tested_arch_i686 ]]
+ echo @@@BUILD_STEP unavailable device android/i686@@@
@@@BUILD_STEP unavailable device android/i686@@@
+ echo @@@STEP_EXCEPTION@@@
@@@STEP_EXCEPTION@@@
+ for _arg in "$@"
+ local _arch=arm
+ [[ ! -f tested_arch_arm ]]
+ echo @@@BUILD_STEP unavailable device android/arm@@@
@@@BUILD_STEP unavailable device android/arm@@@
+ echo @@@STEP_EXCEPTION@@@
@@@STEP_EXCEPTION@@@
+ for _arg in "$@"
+ local _arch=aarch64
+ [[ ! -f tested_arch_aarch64 ]]
+ echo @@@BUILD_STEP unavailable device android/aarch64@@@
@@@BUILD_STEP unavailable device android/aarch64@@@
+ echo @@@STEP_EXCEPTION@@@
@@@STEP_EXCEPTION@@@

I wonder if it worked only because GetThreadLocked has DCHECK instead of CHECK:

// Should be guarded by ThreadRegistryLock.
ThreadContextBase *GetThreadLocked(u32 tid) {
  DCHECK_LT(tid, n_contexts_);
  return threads_[tid];
}

Do we test DEBUG configuration for Android?

I am not sure how I should use it to debug the problem.
Is there a way to point it to a custom checkout? And run a single test? Or at least 'ninja check-asan`?

something like
BUILDBOT_MONO_REPO_PATH=~/src/llvm.git/llvm-project ~/src/zorg/zorg/buildbot/builders/sanitizers/buildbot_android.sh
from: https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild

// Should be guarded by ThreadRegistryLock.
ThreadContextBase *GetThreadLocked(u32 tid) {
  DCHECK_LT(tid, n_contexts_);
  return threads_[tid];
}

Do we test DEBUG configuration for Android?

No we don't.
Unfortunately I noticed this comment just now, and spent some time debugging. That's the reason.

Fix android test

This revision was landed with ongoing or failed builds.Jul 13 2021, 10:52 PM
This revision was automatically updated to reflect the committed changes.