New ThreadSanitizer runtime (v3)

New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

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)

[Martin Liška]

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)

[Martin Liška]

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)

[Dmitry Vyukov]

On Mon, 22 Nov 2021 at 19:31, Martin Liška <mliska@suse.cz> 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)

[Dmitry Vyukov]

On Mon, 22 Nov 2021 at 19:38, Martin Liška <mliska@suse.cz> 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)

[Martin Liška]

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)

[Jakub Jelinek]

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)

[Marco Elver]

On Mon, 22 Nov 2021 at 20:08, Martin Liška <mliska@suse.cz> 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?

Re: New ThreadSanitizer runtime (v3)

[Florian Weimer]

* Dmitry Vyukov via Gcc:

> 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.

Have there been changes to the glibc integration?  Does it still
hard-code the size of (private) struct pthread?

Thanks,
Florian

Re: New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

On Tue, 23 Nov 2021 at 14:49, Florian Weimer <fweimer@redhat.com> wrote:
>
> * Dmitry Vyukov via Gcc:
>
> > 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.
>
> Have there been changes to the glibc integration?

Hi Florian,

No changes to direct glibc integration.
Or what kind of integration do you mean? Tsan did not have any direct
integration and worked with unmodified glibc.

> Does it still hard-code the size of (private) struct pthread?

I am not aware of any such hard-code.

Re: New ThreadSanitizer runtime (v3)

[Florian Weimer]

* Dmitry Vyukov:

> Or what kind of integration do you mean? Tsan did not have any direct
> integration and worked with unmodified glibc.

I thought there is a false-positive data race report if an initial-exec
or local-exec TLS variable is reused (whose memory is not managed by
malloc).

Thanks,
Florian

Re: New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

On Tue, 23 Nov 2021 at 14:59, Florian Weimer <fweimer@redhat.com> wrote:
>
> * Dmitry Vyukov:
>
> > Or what kind of integration do you mean? Tsan did not have any direct
> > integration and worked with unmodified glibc.
>
> I thought there is a false-positive data race report if an initial-exec
> or local-exec TLS variable is reused (whose memory is not managed by
> malloc).

I don't remember any such open issues.

Re: New ThreadSanitizer runtime (v3)

[Florian Weimer]

* Dmitry Vyukov:

> On Tue, 23 Nov 2021 at 14:59, Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Dmitry Vyukov:
>>
>> > Or what kind of integration do you mean? Tsan did not have any direct
>> > integration and worked with unmodified glibc.
>>
>> I thought there is a false-positive data race report if an initial-exec
>> or local-exec TLS variable is reused (whose memory is not managed by
>> malloc).
>
> I don't remember any such open issues.

This glibc patch submission mentions tsan:

  [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]
  <https://sourceware.org/pipermail/libc-alpha/2021-September/131388.html>

It's currently stalled (adding an interface for something that may stop
working in the future is problematic).

Thanks,
Florian

Re: New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

On Tue, 23 Nov 2021 at 17:16, Florian Weimer <fweimer@redhat.com> wrote:
>
> * Dmitry Vyukov:
>
> > On Tue, 23 Nov 2021 at 14:59, Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Dmitry Vyukov:
> >>
> >> > Or what kind of integration do you mean? Tsan did not have any direct
> >> > integration and worked with unmodified glibc.
> >>
> >> I thought there is a false-positive data race report if an initial-exec
> >> or local-exec TLS variable is reused (whose memory is not managed by
> >> malloc).
> >
> > I don't remember any such open issues.
>
> This glibc patch submission mentions tsan:
>
>   [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]
>   <https://sourceware.org/pipermail/libc-alpha/2021-September/131388.html>
>
> It's currently stalled (adding an interface for something that may stop
> working in the future is problematic).

I see. Nothing has changed with respect to
__libc_get_static_tls_bounds with my patches.

Re: New ThreadSanitizer runtime (v3)

[Martin Liška]

On 11/22/21 20:01, Dmitry Vyukov wrote:
> I've already reverted the change. So I will include a fix into the next version.
> Thanks for notifying.

Hello.

Am I correct that the patch set is installed again? Any near future plans for another
revert of the patch? Do you think it's the right time to merge it to GCC?

Thanks,
Martin

Re: New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

On Mon, 29 Nov 2021 at 19:16, Martin Liška <mliska@suse.cz> wrote:
>
> On 11/22/21 20:01, Dmitry Vyukov wrote:
> > I've already reverted the change. So I will include a fix into the next version.
> > Thanks for notifying.
>
> Hello.
>
> Am I correct that the patch set is installed again? Any near future plans for another
> revert of the patch? Do you think it's the right time to merge it to GCC?

It has been re-landed at least twice since then :)
Well, I can't promise that it won't be reverted again. I obviously do
not want that. Hard to say. I need to send some follow up clean up
patches and I plan to wait till the end of the week (to avoid messy
multi commit reverts).
So if there are no deadlines to miss, I would suggest waiting until
the beginning of the next week.

Re: New ThreadSanitizer runtime (v3)

[Martin Liška]

On 11/30/21 05:17, Dmitry Vyukov wrote:
> On Mon, 29 Nov 2021 at 19:16, Martin Liška <mliska@suse.cz> wrote:
>>
>> On 11/22/21 20:01, Dmitry Vyukov wrote:
>>> I've already reverted the change. So I will include a fix into the next version.
>>> Thanks for notifying.
>>
>> Hello.
>>
>> Am I correct that the patch set is installed again? Any near future plans for another
>> revert of the patch? Do you think it's the right time to merge it to GCC?
> 
> It has been re-landed at least twice since then :)
> Well, I can't promise that it won't be reverted again. I obviously do
> not want that. Hard to say. I need to send some follow up clean up
> patches and I plan to wait till the end of the week (to avoid messy
> multi commit reverts).
> So if there are no deadlines to miss, I would suggest waiting until
> the beginning of the next week.

Hello.

May I please ask about the status of TSANv3? Note we'll flip to stage4 stage in about
3 weeks and so I'm curious if we want to do one more merge from upstream?

@Jakub: What do you think about it?

Cheers,
Martin

Re: New ThreadSanitizer runtime (v3)

[Dmitry Vyukov]

On Thu, 23 Dec 2021 at 13:10, Martin Liška <mliska@suse.cz> wrote:
> >> On 11/22/21 20:01, Dmitry Vyukov wrote:
> >>> I've already reverted the change. So I will include a fix into the next version.
> >>> Thanks for notifying.
> >>
> >> Hello.
> >>
> >> Am I correct that the patch set is installed again? Any near future plans for another
> >> revert of the patch? Do you think it's the right time to merge it to GCC?
> >
> > It has been re-landed at least twice since then :)
> > Well, I can't promise that it won't be reverted again. I obviously do
> > not want that. Hard to say. I need to send some follow up clean up
> > patches and I plan to wait till the end of the week (to avoid messy
> > multi commit reverts).
> > So if there are no deadlines to miss, I would suggest waiting until
> > the beginning of the next week.
>
> Hello.
>
> May I please ask about the status of TSANv3? Note we'll flip to stage4 stage in about
> 3 weeks and so I'm curious if we want to do one more merge from upstream?
>
> @Jakub: What do you think about it?

Hi Martin,

I re-laned it last time 10 days ago:

b332134921b4 Mon, 13 Dec 2021 12:48:34 +0100 tsan: new runtime (v3)

at this point it wasn't yet reverted...
Few more issues were fixed and it now survived testing in Chromium.
I hope it won't be reverted anymore... but can't promise (it's not me
who revert it :))
I would say if we wait a few more days maybe, it's reasonably safe to
assume it sticks.