New ThreadSanitizer runtime (v3)
Hi gcc developers, I wanted to give heads up regarding a significant re-design of the ThreadSanitizer runtime: https://reviews.llvm.org/D112603 Currently it's submitted: https://github.com/llvm/llvm-project/commit/1784fe0532a69ead17793bced060a9bf9d232027 but can well be rolled back if too many buildbots fail, but should be submitted again soon anyway. It was extensively tested and lots of bugs were fixed, but it's still possible it will cause some issues just because of the size of the change and OS/arch sensitivity. For a wide range of real programs it provides 20%-4x speedup on x86_64 and 20-40% memory consumption reduction. One issue that will come up with gcc is the use of the new disable_sanitizer_instrumentation attribute in tests: https://clang.llvm.org/docs/AttributeReference.html#disable-sanitizer-instrumentation e.g.: https://github.com/llvm/llvm-project/blob/1784fe0532a69ead17793bced060a9bf9d232027/compiler-rt/test/tsan/java_symbolization.cpp#L5 ThreadSanitizer is now more picky about recursing from runtime callbacks back into runtime. You may either disable these tests, or move callbacks into non-instrumented files (though, will require forking tests), or implement the attribute. Some uses of the disable_sanitizer_instrumentation attribute were also discussed in the Linux kernel context. KMSAN will use it and kernel noinstr functions could use it, though currently noinstr functions are post-processed with kernel's objtool, which nops any sanitizer callbacks. The objtool approach will continue to work, so it's not that the attribute is mandated.
Re: New ThreadSanitizer runtime (v3)
On 11/22/21 16:22, Dmitry Vyukov wrote:
Hi gcc developers,
Hello.
I wanted to give heads up regarding a significant re-design of the
Thanks for it.
ThreadSanitizer runtime:
https://reviews.llvm.org/D112603
Currently it's submitted:
https://github.com/llvm/llvm-project/commit/1784fe0532a69ead17793bced060a9bf9d232027
but can well be rolled back if too many buildbots fail, but should be
submitted again soon anyway.
It was extensively tested and lots of bugs were fixed, but it's still
possible it will cause some issues just because of the size of the
change and OS/arch sensitivity.
For a wide range of real programs it provides 20%-4x speedup on x86_64
and 20-40% memory consumption reduction.
That are all good news!
One issue that will come up with gcc is the use of the new
disable_sanitizer_instrumentation attribute in tests:
https://clang.llvm.org/docs/AttributeReference.html#disable-sanitizer-instrumentation
e.g.:
https://github.com/llvm/llvm-project/blob/1784fe0532a69ead17793bced060a9bf9d232027/compiler-rt/test/tsan/java_symbolization.cpp#L5
Well, apparently the tsan tests (similarly to other Sanitizer) are not
synchronized and we
only have a small subset of test.
Right now I'm working on the libsanitizer's merge from master and tsan.exp
tests work fine.
ThreadSanitizer is now more picky about recursing from runtime
callbacks back into runtime.
You may either disable these tests, or move callbacks into
non-instrumented files (though, will require forking tests), or
implement the attribute.
Some uses of the disable_sanitizer_instrumentation attribute were also
discussed in the Linux kernel context. KMSAN will use it and kernel
noinstr functions could use it, though currently noinstr functions are
post-processed with kernel's objtool, which nops any sanitizer
callbacks. The objtool approach will continue to work, so it's not
that the attribute is mandated.
Right now, we as GCC have no_sanitize ("sanitize_option") that can be used (or
no_sanitize_* attributes).
Can you please explain why did you invent the new flag?
Cheers,
Martin
Re: New ThreadSanitizer runtime (v3)
On 11/22/21 16:22, Dmitry Vyukov wrote: I wanted to give heads up regarding a significant re-design of the ThreadSanitizer runtime: https://reviews.llvm.org/D112603 Currently it's submitted: https://github.com/llvm/llvm-project/commit/1784fe0532a69ead17793bced060a9bf9d232027 And I noticed the following new warnings: libsanitizer/tsan/tsan_shadow.h:93:32: warning: enumerated and non-enumerated type in conditional expression [-Wextra] libsanitizer/tsan/tsan_shadow.h:94:44: warning: enumerated and non-enumerated type in conditional expression [-Wextra] *typ = (part_.is_read_ ? kAccessRead : kAccessWrite) | (part_.is_atomic_ ? kAccessAtomic : 0) | (part_.access_ == kFreeAccess ? kAccessFree : 0); I think 0 should be replaced with kAccessWrite, am I right? Should I create a pull request for it? Cheers, Martin
Re: New ThreadSanitizer runtime (v3)
On Mon, 22 Nov 2021 at 19:31, Martin Liška wrote:
>
> On 11/22/21 16:22, Dmitry Vyukov wrote:
> > Hi gcc developers,
>
> Hello.
>
> >
> > I wanted to give heads up regarding a significant re-design of the
>
> Thanks for it.
>
> > ThreadSanitizer runtime:
> > https://reviews.llvm.org/D112603
> > Currently it's submitted:
> > https://github.com/llvm/llvm-project/commit/1784fe0532a69ead17793bced060a9bf9d232027
> > but can well be rolled back if too many buildbots fail, but should be
> > submitted again soon anyway.
> >
> > It was extensively tested and lots of bugs were fixed, but it's still
> > possible it will cause some issues just because of the size of the
> > change and OS/arch sensitivity.
> >
> > For a wide range of real programs it provides 20%-4x speedup on x86_64
> > and 20-40% memory consumption reduction.
>
> That are all good news!
>
> >
> > One issue that will come up with gcc is the use of the new
> > disable_sanitizer_instrumentation attribute in tests:
> > https://clang.llvm.org/docs/AttributeReference.html#disable-sanitizer-instrumentation
> > e.g.:
> > https://github.com/llvm/llvm-project/blob/1784fe0532a69ead17793bced060a9bf9d232027/compiler-rt/test/tsan/java_symbolization.cpp#L5
>
> Well, apparently the tsan tests (similarly to other Sanitizer) are not
> synchronized and we
> only have a small subset of test.
>
> Right now I'm working on the libsanitizer's merge from master and tsan.exp
> tests work fine.
Good. But I already reverted the change (some issues on Mac). Plan to
resubmit soon.
> > ThreadSanitizer is now more picky about recursing from runtime
> > callbacks back into runtime.
> > You may either disable these tests, or move callbacks into
> > non-instrumented files (though, will require forking tests), or
> > implement the attribute.
> > Some uses of the disable_sanitizer_instrumentation attribute were also
> > discussed in the Linux kernel context. KMSAN will use it and kernel
> > noinstr functions could use it, though currently noinstr functions are
> > post-processed with kernel's objtool, which nops any sanitizer
> > callbacks. The objtool approach will continue to work, so it's not
> > that the attribute is mandated.
> >
>
> Right now, we as GCC have no_sanitize ("sanitize_option") that can be used
> (or no_sanitize_* attributes).
>
> Can you please explain why did you invent the new flag?
Not sure about gcc, but in clang the old no_sanitize_thread attribute
disabled only part of instrumentation (only memory accesses, but not
atomics and function entry/exit). The new attribute disables all
instrumentation.
Re: New ThreadSanitizer runtime (v3)
On Mon, 22 Nov 2021 at 19:38, Martin Liška wrote: > > On 11/22/21 16:22, Dmitry Vyukov wrote: > > I wanted to give heads up regarding a significant re-design of the > > ThreadSanitizer runtime: > > https://reviews.llvm.org/D112603 > > Currently it's submitted: > > https://github.com/llvm/llvm-project/commit/1784fe0532a69ead17793bced060a9bf9d232027 > > And I noticed the following new warnings: > > libsanitizer/tsan/tsan_shadow.h:93:32: warning: enumerated and non-enumerated > type in conditional expression [-Wextra] > libsanitizer/tsan/tsan_shadow.h:94:44: warning: enumerated and non-enumerated > type in conditional expression [-Wextra] > >*typ = (part_.is_read_ ? kAccessRead : kAccessWrite) | > (part_.is_atomic_ ? kAccessAtomic : 0) | > (part_.access_ == kFreeAccess ? kAccessFree : 0); > > I think 0 should be replaced with kAccessWrite, am I right? Should I create a > pull request for it? I've already reverted the change. So I will include a fix into the next version. Thanks for notifying.
Re: New ThreadSanitizer runtime (v3)
On 11/22/21 20:00, Dmitry Vyukov wrote:
Not sure about gcc, but in clang the old no_sanitize_thread attribute
disabled only part of instrumentation (only memory accesses, but not
atomics and function entry/exit). The new attribute disables all
instrumentation.
And what about no_sanitize("thread"):
https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
How is that different from the new attribute?
Please document how that differs (if you really need the new attribute).
Thanks,
Martin
Re: New ThreadSanitizer runtime (v3)
On Mon, Nov 22, 2021 at 08:00:38PM +0100, Dmitry Vyukov wrote:
> Not sure about gcc, but in clang the old no_sanitize_thread attribute
> disabled only part of instrumentation (only memory accesses, but not
> atomics and function entry/exit). The new attribute disables all
> instrumentation.
In gcc no_sanitize("thread") as well as no_sanitize_thread attributes
affect everything (function entry/exit, atomics and memory accesses).
Jakub
Re: New ThreadSanitizer runtime (v3)
On Mon, 22 Nov 2021 at 20:08, Martin Liška wrote:
>
> On 11/22/21 20:00, Dmitry Vyukov wrote:
> > Not sure about gcc, but in clang the old no_sanitize_thread attribute
> > disabled only part of instrumentation (only memory accesses, but not
> > atomics and function entry/exit). The new attribute disables all
> > instrumentation.
>
> And what about no_sanitize("thread"):
> https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
>
> How is that different from the new attribute?
> Please document how that differs (if you really need the new attribute).
The new attribute is documented here:
https://clang.llvm.org/docs/AttributeReference.html#disable-sanitizer-instrumentation
Specifically "This is not the same as
__attribute__((no_sanitize(...))), which depending on the tool may
still insert instrumentation to prevent false positive reports."
(Some discussion that led to this name is also here:
https://reviews.llvm.org/D108029)
And it seems it's mainly relevant for MSan and TSan, although the
documentation also suggests that it disables all other sanitizers
(which would make it slightly redundant for those other sanitizers vs.
listing all no_sanitize*).
In Clang, for TSan no_sanitize("thread") still inserts
__func_entry/exit and also instrumentation for atomic operations to
avoid false positives; MSan still unpoisons shadow memory in
no_sanitize("memory"), but does not perform checks.
The problem is that certain code just does not want any
instrumentation at all, and we needed an attribute to express that
(e.g. the TSan tests, or some very special Linux-kernel code). We
first discussed having an extension of no_sanitize* attribute, but
some folks didn't like that exactly because it could be confusing and
lead to users using the wrong attribute.
And since this attribute is only supposed to be used very sparingly,
usually in very low level code, to make it clear it's not a substitute
for no_sanitize* it was given this rather distinctive name
"disable_sanitizer_instrumentation".
Now, given you said that GCC also removes __func_entry/exit and atomic
operations from no_sanitize("thread"), this new attribute then seems
redundant for GCC. But then the question is, should GCC's
no_sanitize("thread") be adjusted to avoid false positives?
