[Bug sanitizer/59215] New: tsan: warning in shared_ptr_base.h

[Bug sanitizer/59215] New: tsan: warning in shared_ptr_base.h

[oleg at smolsky dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

            Bug ID: 59215
           Summary: tsan: warning in shared_ptr_base.h
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: oleg at smolsky dot net
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org

I just got what appears to be a false positive in GCC's own implementation of
shared_ptr ref counts:

Read of size 4 by thread T21:
  #0 _M_add_ref_lock ...gcc/include/c++/4.8.x-google/bits/shared_ptr_base.h:236

Previous atomic write of size 4 by main thread:
  #0 __tsan_atomic32_fetch_add ??:0 (libtsan.so.0+0x00000000d3e5)
  #1 __exchange_and_add_dispatch
...gcc/include/c++/4.8.x-google/ext/atomicity.h:49
  ....

The atomic write is obvious - it's an "up ref". The read, however, should have
just worked as the variable is (well, is supposed to be) atomic.

I don't get it... is the tool missing the atomic manipulation? Or is the
library missing the correct annotation? I'm leaning towards the latter:

in ...gcc/include/c++/4.8.x-google/x86_64-unknown-linux/bits/atomic_word.h

       typedef int _Atomic_word;

Should this be std::atomic<int> ?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #1 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
This reminds me http://llvm.org/bugs/show_bug.cgi?id=17066
Do you have this problem with clang's tsan?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[oleg at smolsky dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #2 from Oleg Smolsky <oleg at smolsky dot net> ---
Unfortunately, I cannot repro with Clang (we use gcc48 with sysroot, and 
I failed to get Clang to latch onto that STL. It only discovers the 
system's STL)

I can try to come up with a minimal test case... Yet, I cannot imagine 
that the following would ever work with TSan:
             typedef int _Atomic_word;

I imagine, you'd need to "color" reading sites or change the type to 
std::atomic.

Is this right?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #3 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
> I can try to come up with a minimal test case... Yet, I cannot imagine 
> that the following would ever work with TSan:
>              typedef int _Atomic_word;

It does not matter how _Atomic_word is defined.
What matters is how the code reads/write these atomics. 
If that's via __sync_* or __atomic_* intrinsics, tsan will understand that. 
But only if you re-compile stdlibc++ with tsan, 
otherwise tsan will still not see the atomic instructions. 
See the llvm bug mentioned above. 

> 
> I imagine, you'd need to "color" reading sites or change the type to 
> std::atomic.
> 
> Is this right?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Oleg Smolsky from comment #0)
> in ...gcc/include/c++/4.8.x-google/x86_64-unknown-linux/bits/atomic_word.h
> 
>        typedef int _Atomic_word;
> 
> Should this be std::atomic<int> ?

No.


(In reply to Kostya Serebryany from comment #3)
> If that's via __sync_* or __atomic_* intrinsics, tsan will understand that. 

All ref-count updates are via __atomic built-ins.

> But only if you re-compile stdlibc++ with tsan, 

It's libstdc++ not stdlibc++, I don't know why everyone gets that wrong :-)

Anyway, this is already documented:
http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug.html#debug.races

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #5 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
> > But only if you re-compile stdlibc++ with tsan, 
> 
> It's libstdc++ not stdlibc++, I don't know why everyone gets that wrong :-)

Sorry (I usually get it right) :) 

> 
> Anyway, this is already documented:
> http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug.html#debug.races

I know that's confusing, but this has nothing to do with ThreadSanitizer v2,
which is being discussed in this bug. 
Those annotations are for ThreadSanitizer v1 which used binary translation
and did not understand atomics natively. (also for helgrind, etc).

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I realise that, but the general point is still valid: for race detectors to
understand the atomic updates in the library they library needs to be compiled
with the race detector enabled.  We can update the docs to cover tsan as well,
but the point is till that you need to recompile the library to prevent false
positives from race detectors.

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> tsan as well, but the point is till ...

s/till/still/

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #8 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> I realise that, but the general point is still valid: for race detectors to
> understand the atomic updates in the library they library needs to be
> compiled with the race detector enabled.  We can update the docs to cover
> tsan as well, but the point is till that you need to recompile the library
> to prevent false positives from race detectors.

Yes, correct. Thanks! 
We tried to suppress all known false reports like this 
so that one can *try* to use tsan w/o recompiling libstdc++.
Probably we may need to suppress more, so a small test will help. 
But the preferred way is still recompilation of libstdc++.

Since we ourselves use tsan with a recompiled library, we do not observe
any false positives like this.

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[oleg at smolsky dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #9 from Oleg Smolsky <oleg at smolsky dot net> ---
So, let me see if I understand. The case in question is _M_add_ref_lock() :

  template<>
    inline void
    _Sp_counted_base<_S_atomic>::
    _M_add_ref_lock()
    {
      // Perform lock-free add-if-not-zero operation.
      _Atomic_word __count = _M_use_count;                  <==== the read
      do
        {
          if (__count == 0)
            __throw_bad_weak_ptr();
          // Replace the current counter value with the old value + 1, as
          // long as it's not changed meanwhile.
        }
      while (!__atomic_compare_exchange_n(&_M_use_count, &__count, __count + 1,
                                          true, __ATOMIC_ACQ_REL,
                                          __ATOMIC_RELAXED));
    }


The read is flagged by TSan because it is non-atomic. So, it seems that no
amount of recompilation is going to work here... unless the recompiled actually
redefines _Atomic_word to an atomic.

Or am I missing something here?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
No, you're right, that's a different issue.  I think we've just been relying on
loads of (correctly-aligned) _Atomic_word being atomic, although that's not
going to keep tsan happy!  There's no barrier on the read, but I think the
worst that will happen is we won't see the correct value and the CAS loop will
go round again. We won't see __count==0 spuriously, because once that count
reaches zero it never gets incremented again.

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #11 from Kostya Serebryany <kcc at gcc dot gnu.org> ---

>       _Atomic_word __count = _M_use_count;                  <==== the read

Interesting. 
We haven't seen these (we don't use this implementation of shared_ptr). 
I think the code is simply wrong -- it should be using __atomic_load_n
instead of plain loads. 

You can also suppress it for now 
(https://code.google.com/p/thread-sanitizer/wiki/Suppressions)

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[oleg at smolsky dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #12 from Oleg Smolsky <oleg at smolsky dot net> ---
Hey Kostya, should I try suppressing the report using the function name? Would
it work in optimized builds that have inlining?

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #13 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Oleg Smolsky from comment #12)
> Hey Kostya, should I try suppressing the report using the function name?
> Would it work in optimized builds that have inlining?

Excellent question! 
If you build with -g (at least with clang; -gline-tables-only is ok too)
our symbolizer (llvm-symbolizer) is able to recognize the inlined frame
and then the suppression should work even if the function got inlined.

But this is sort or fragile -- I still don't believe it works :) 
And it won't work w/o debug info or if debug info is broken for any reason. 

So the best solution is still to fix the libstdc++ code. :(

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #15 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Joost VandeVondele from comment #14)
> (In reply to Jonathan Wakely from comment #10)
> > No, you're right, that's a different issue.  I think we've just been relying
> > on loads of (correctly-aligned) _Atomic_word being atomic, although that's
> > not going to keep tsan happy!  There's no barrier on the read, but I think
> > the worst that will happen is we won't see the correct value and the CAS
> > loop will go round again. We won't see __count==0 spuriously, because once
> > that count reaches zero it never gets incremented again.
> 
> Interestingly, a very similar comment was made yesterday in the context of
> libgomp: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194#c4
> 
> If for performance reasons a plain load would nevertheless be preferred over
> an atomic one, 

Why would that be? relaxed atomic loads end up generating the same instructions 
as plain loads. 

> I wonder if these threading libraries could e.g. use
> conditional compilation such that, when compiled with -fsanitize=thread, an
> atomic load is used nevertheless. Does -fsanitize=thread define a
> __SANITIZE_THREAD_IN_USE or similar ?

In clang, tsan uses __has_feature(thread_sanitizer).

In gcc asan defines __SANITIZE_ADDRESS__, but I don't think we have 
__SANITIZE_THREAD__ and it would be very pity if we have to use it in libstdc++

[Bug sanitizer/59215] tsan: warning in shared_ptr_base.h

[dvyukov at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

Dmitry Vyukov <dvyukov at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dvyukov at google dot com

--- Comment #16 from Dmitry Vyukov <dvyukov at google dot com> ---
(In reply to Jonathan Wakely from comment #10)
> No, you're right, that's a different issue.  I think we've just been relying
> on loads of (correctly-aligned) _Atomic_word being atomic, although that's
> not going to keep tsan happy!  There's no barrier on the read, but I think
> the worst that will happen is we won't see the correct value and the CAS
> loop will go round again. We won't see __count==0 spuriously, because once
> that count reaches zero it never gets incremented again.

Data races do not work this way. It's undefined behavior:
http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

The code must use relaxed atomic load. If the code becomes slower due to that
then:
(1) the current generated code is incorrect
or (2) there is a performance bug in gcc handling of atomic operations

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2013-11-21
          Component|sanitizer                   |libstdc++
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Target Milestone|---                         |4.8.3
     Ever confirmed|0                           |1

--- Comment #17 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This should fix it:

--- a/libstdc++-v3/include/bits/shared_ptr_base.h
+++ b/libstdc++-v3/include/bits/shared_ptr_base.h
@@ -233,7 +233,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _M_add_ref_lock()
     {
       // Perform lock-free add-if-not-zero operation.
-      _Atomic_word __count = _M_use_count;
+      _Atomic_word __count = _M_get_use_count();
       do
        {
          if (__count == 0)

But I can't test it yet because libtsan is giving me undefined references to
sigsetjmp.

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #18 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jonathan Wakely from comment #17)

> But I can't test it yet because libtsan is giving me undefined references to
> sigsetjmp.

workaround in PR59188

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #19 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oleg, I can't reproduce this tsan warning with the cases Iv'e tried, what is
the stack trace you see?

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[oleg at smolsky dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #20 from Oleg Smolsky <oleg at smolsky dot net> ---
Hey Jonathan, here is the output:

         Read of size 4 at 0x7d0400005008 by main thread (mutexes: write 
M2344, write M132):
           #0 _M_add_ref_lock 
..../gcc48/include/c++/4.8.x-google/bits/shared_ptr_base.h:236 
(exe+0x000000d0ae09)
           #1 my code (a)

         Previous atomic write of size 4 at 0x7d0400005008 by thread T21 
(mutexes: write M4016):
           #0 __tsan_atomic32_fetch_add ??:0 (libtsan.so.0+0x00000000d3e5)
           #1 __exchange_and_add_dispatch 
/mnt/project/granite/toolchains/elixir/rvbd-gcc48/include/c++/4.8.x-google/ext/atomicity.h:49 
(exe+0x0000003d7738)
           #2 handle_event(....) 
.../gcc48/include/c++/4.8.x-google/bits/shared_ptr_base.h:546 (
              this is my code (b)

(a) string x = config()->iscsi().dc_initiator().name();

this thing very quickly copies a shared_ptr instance, dereferences it, 
gets to a sub-object, gets a field and then destructs.

(b) is a shared_ptr copy too, but it is obscured by heavy inlining

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Author: redi
Date: Mon Jan 27 17:56:40 2014
New Revision: 207147

URL: http://gcc.gnu.org/viewcvs?rev=207147&root=gcc&view=rev
Log:
    PR libstdc++/59215
    * include/bits/shared_ptr_base.h
    (_Sp_counted_base<_S_atomic>::_M_add_ref_lock()): Use relaxed atomic
    load.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/shared_ptr_base.h

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

--- Comment #22 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk so far.

[Bug libstdc++/59215] tsan: warning in shared_ptr_base.h

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59215

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #24 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 4.8.3 now