ANNOTATE_IGNORE_READS_BEGIN()/END() don't work?

GoogleCodeExporter commented 9 years ago

Following the example of ANNOTATE_UNPROTECTED_READ in dynamic_annotations.h I 
created the following function for std::atomic<T>::load

template<class T>
inline T ANNOTATE_UNPROTECTED_LOAD(const atomic<T>& x, memory_order order)
{
    ANNOTATE_IGNORE_READS_BEGIN();
    T res = x.load(order);
    ANNOTATE_IGNORE_READS_END();
    return res;
}

But I still get the races reported (using the pure mode):

==7981==   Concurrent read(s) happened at (OR AFTER) these points:
==7981==    T1 (L{}):
==7981==     #0  std::__atomic_base::load 
/usr/include/c++/4.7/bits/atomic_base.h:453
==7981==     #1  std::atomic_bool::load /usr/include/c++/4.7/atomic:94
==7981==     #2  ANNOTATE_UNPROTECTED_LOAD 
/home/eloff/projects/bin/../src/threading.hpp:39

Thinking it might only work for the current frame in the callstack, I tried 
this horrible hack:

template<class T>
inline T ANNOTATE_UNPROTECTED_LOAD(const atomic<T>& x, memory_order order)
{
    (void)order;
    ANNOTATE_IGNORE_READS_BEGIN();
    T res = *(volatile T*)&x;
    ANNOTATE_IGNORE_READS_END();
    return res;
}

Which still causes a race to be detected:

==30041==   Concurrent read(s) happened at (OR AFTER) these points:
==30041==    T2 (L{}):
==30041==     #0  ANNOTATE_UNPROTECTED_LOAD 
/home/eloff/projects/bin/../src/threading.hpp:36

Any idea why this is not working for me? I am using x64 Linux (Ubuntu 12.10) 
and GCC-4.7

Original issue reported on code.google.com by dan.el...@gmail.com on 1 Nov 2012 at 4:04

GoogleCodeExporter commented 9 years ago

Please check that ANNOTATE_IGNORE_READS_BEGIN() actually compiles into a 
function call. You may need to define some macro to enable annotations.

Original comment by dvyu...@google.com on 1 Nov 2012 at 5:10

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

You may take a look at the next version of ThreadSanitizer:
http://clang.llvm.org/docs/ThreadSanitizer.html

Original comment by dvyu...@google.com on 1 Nov 2012 at 5:11

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Thanks Dmitry, that was a n00b mistake not noticing I have to define 
DYNAMIC_ANNOTATIONS_ENABLED=1 for it to work. I will try out the new version of 
thread sanitizer now. I use a lot of the C++11 threading library, so I'm not 
sure how well it will work. You guys are doing an amazing job in Google Moscow 
with this ThreadSanitizer project, keep it up!

Original comment by dan.el...@gmail.com on 1 Nov 2012 at 5:49

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I've just sent a patch with support for atomic operations:
https://codereview.appspot.com/6775091

When it's landed, the following program should work fine:

#include <atomic>
#include <thread>
int main() {
  int X = 0;
  std::atomic<int> A;
  std::thread t([&]() {
    X = 42;
    A.store(42, std::memory_order_release);
  });
  while (A.load(std::memory_order_acquire) == 0) {}
  X = 43;
  t.join();
}

However, your libcxx must use atomic buildins in implementation of std::atomic. 
E.g. llvm libcxx does that.
And std threads must be implemented on top of pthread.

Original comment by dvyu...@google.com on 1 Nov 2012 at 6:48

Changed state: Fixed
Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I get a race on X in your code sample, but not on A (which, I imagine, would 
normally have a benign racey-read.) Is that what you get or did I do something 
wrong?

Being an impatient fellow, I applied your patch, uninstalled clang, re-built it 
(removed the build directory and started over to make sure it included your 
change) and compiled your sample. I am using llvm/clang/libcxx trunk.

clang++ -std=c++0x -stdlib=libc++ -fthread-sanitizer -g -O1 main.cpp -fPIE -pie

==================
WARNING: ThreadSanitizer: data race (pid=3348)
  Write of size 4 at 0x7fff7b309944 by thread 1:
    #0 operator() /home/eloff/projects/prototypes/tsanatomics/main.cpp:7 (exe+0x00000000eaa4)
    #1 _ZNSt3__18__invokeIZ4mainE3$_0JEEEDTclclsr3std3__1E7forwardIT_Efp_Espclsr3std3__1E7forwardIT0_Efp0_EEEOS2_DpOS3_ /usr/include/c++/v1/__functional_base:340 (exe+0x00000000ea51)

  Previous write of size 4 at 0x7fff7b309944 by main thread:
    #0 main /home/eloff/projects/prototypes/tsanatomics/main.cpp:11 (exe+0x00000000e90b)

  Thread 1 (tid=3349, running) created at:
    #0 pthread_create ??:0 (exe+0x0000000118ce)
    #1 thread<<lambda at main.cpp:6:17>, , void> /usr/include/c++/v1/thread:352 (exe+0x00000000e9b8)
    #2 main /home/eloff/projects/prototypes/tsanatomics/main.cpp:9 (exe+0x00000000e8e8)

==================
ThreadSanitizer: reported 1 warnings

Original comment by dan.el...@gmail.com on 1 Nov 2012 at 8:57

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Perhaps that's because llvm does not understand atomics in your std lib. Llvm 
understands __sync_xxx, __atomic_xxx builtins and _Atomic() C1x intrinsics.
Can your check the object code with objdump? E.g. atomic<>::load() must be 
compiled as __tsan_atomic_load().
Here is what I see:

void *Thread1(void *x) {
    fa50:       53                      push   %rbx
    fa51:       48 8b 7c 24 08          mov    0x8(%rsp),%rdi
    fa56:       e8 d5 6f 01 00          callq  26a30 <__tsan_func_entry>
  Global = 42;
  A.store(42, std::memory_order_release);
    fa5b:       48 8b 1d 8e e4 22 00    mov    0x22e48e(%rip),%rbx        # 23def0 <_DYNAMIC+0xb40>
    fa62:       48 89 df                mov    %rbx,%rdi
    fa65:       e8 c6 4a 01 00          callq  24530 <__tsan_write4>
    fa6a:       c7 03 2a 00 00 00       movl   $0x2a,(%rbx)
    fa70:       48 8b 3d 19 df 22 00    mov    0x22df19(%rip),%rdi        # 23d990 <_DYNAMIC+0x5e0>
    fa77:       be 2a 00 00 00          mov    $0x2a,%esi
    fa7c:       ba 9c 88 01 00          mov    $0x1889c,%edx
    fa81:       e8 da 8d 00 00          callq  18860 <__tsan_atomic32_store>
  return NULL;
    fa86:       e8 35 70 01 00          callq  26ac0 <__tsan_func_exit>
    fa8b:       31 c0                   xor    %eax,%eax
    fa8d:       5b                      pop    %rbx
    fa8e:       c3                      retq   
    fa8f:       90                      nop
}

Original comment by dvyu...@google.com on 2 Nov 2012 at 6:11

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Btw, if you are eager to use it, then tsan provides own interface to atomics. 
You can see it in compiler-rt/lib/tsan/rtl/tsan_interface_atomic.h. It looks 
along the lines of:

__tsan_atomic8 __tsan_atomic8_exchange(volatile __tsan_atomic8 *a,
    __tsan_atomic8 v, __tsan_memory_order mo);
__tsan_atomic16 __tsan_atomic16_exchange(volatile __tsan_atomic16 *a,
    __tsan_atomic16 v, __tsan_memory_order mo);
__tsan_atomic32 __tsan_atomic32_exchange(volatile __tsan_atomic32 *a,
    __tsan_atomic32 v, __tsan_memory_order mo);
__tsan_atomic64 __tsan_atomic64_exchange(volatile __tsan_atomic64 *a,
    __tsan_atomic64 v, __tsan_memory_order mo);

E.g. it's essentially C1x/C++11 interface. You may copy it to your project and 
then switch between C++11 atomics and tsan atomics based on some macro.

You can see how it is done in e.g. Chromium:
http://src.chromium.org/viewvc/chrome/trunk/src/base/atomicops.h?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/base/atomicops_internals_tsan.h?
view=markup

Thanks!

Original comment by dvyu...@google.com on 2 Nov 2012 at 6:18

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I'm using the libcxx from the llvm project. Disassembling main shows that it 
does indeed use __tsan_atomic32_load. If you think of something else I can try, 
let me know, it'd be really cool to get this working. I'm like a kid at 
Christmas time over here playing with clang and the new ThreadSanitizer.

Dump of assembler code for function main():
   0x000000000000e890 <+0>: push   %r14
   0x000000000000e892 <+2>: push   %rbx
   0x000000000000e893 <+3>: sub    $0x28,%rsp
   0x000000000000e897 <+7>: mov    0x38(%rsp),%rdi
   0x000000000000e89c <+12>:    callq  0x25a10 <__tsan_func_entry>
   0x000000000000e8a1 <+17>:    lea    0x24(%rsp),%rbx
   0x000000000000e8a6 <+22>:    mov    %rbx,%rdi
   0x000000000000e8a9 <+25>:    callq  0x23510 <__tsan_write4>
   0x000000000000e8ae <+30>:    movl   $0x0,0x24(%rsp)
   0x000000000000e8b6 <+38>:    lea    0x8(%rsp),%r14
   0x000000000000e8bb <+43>:    mov    %r14,%rdi
   0x000000000000e8be <+46>:    callq  0x24160 <__tsan_write8>
   0x000000000000e8c3 <+51>:    lea    0x10(%rsp),%rdi
   0x000000000000e8c8 <+56>:    mov    %rbx,0x8(%rsp)
   0x000000000000e8cd <+61>:    callq  0x24160 <__tsan_write8>
   0x000000000000e8d2 <+66>:    lea    0x18(%rsp),%rdi
   0x000000000000e8d7 <+71>:    lea    0x20(%rsp),%rbx
   0x000000000000e8dc <+76>:    mov    %rbx,0x10(%rsp)
   0x000000000000e8e1 <+81>:    mov    %r14,%rsi
   0x000000000000e8e4 <+84>:    callq  0xe950 <std::__1::thread::thread<<lambda at main.cpp:7:17>, , void>(<unknown type in /home/eloff/projects/prototypes/tsanatomics/a.out, CU 0x0, DIE 0x135e>)>
   0x000000000000e8e9 <+89>:    nopl   0x0(%rax)
   0x000000000000e8f0 <+96>:    mov    %rbx,%rdi
   0x000000000000e8f3 <+99>:    mov    $0x18898,%esi
   0x000000000000e8f8 <+104>:   callq  0x17360 <__tsan_atomic32_load>
   0x000000000000e8fd <+109>:   test   %eax,%eax
   0x000000000000e8ff <+111>:   je     0xe8f0 <main()+96>
   0x000000000000e901 <+113>:   lea    0x24(%rsp),%rdi
   0x000000000000e906 <+118>:   callq  0x23510 <__tsan_write4>
   0x000000000000e90b <+123>:   movl   $0x2b,0x24(%rsp)
   0x000000000000e913 <+131>:   lea    0x18(%rsp),%rdi
   0x000000000000e918 <+136>:   callq  0xe5b0 <_ZNSt3__16thread4joinEv@plt>
   0x000000000000e91d <+141>:   lea    0x18(%rsp),%rdi
   0x000000000000e922 <+146>:   callq  0xe560 <_ZNSt3__16threadD1Ev@plt>
   0x000000000000e927 <+151>:   callq  0x25aa0 <__tsan_func_exit>
   0x000000000000e92c <+156>:   xor    %eax,%eax
   0x000000000000e92e <+158>:   add    $0x28,%rsp
   0x000000000000e932 <+162>:   pop    %rbx

Original comment by dan.el...@gmail.com on 2 Nov 2012 at 1:32

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Can you show the code?

Original comment by dvyu...@google.com on 2 Nov 2012 at 1:38

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I'm attaching the binary+source here in case you want to look at it.

Original comment by dan.el...@gmail.com on 2 Nov 2012 at 1:39

Added labels: ****
Removed labels: ****

Attachments:

tsanatomics.tar.gz

GoogleCodeExporter commented 9 years ago

Initialize the atomic to zero.

Original comment by dvyu...@google.com on 2 Nov 2012 at 2:08

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Whoops, didn't catch that. Thanks Dmitry! It works great now, I can't wait to 
try it on my codebase.

Original comment by dan.el...@gmail.com on 2 Nov 2012 at 2:29

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Will be happy to hear any feedback or new bugreports.

Original comment by dvyu...@google.com on 2 Nov 2012 at 2:46

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

I found one issue that's blocking me: 
http://code.google.com/p/thread-sanitizer/issues/detail?id=4

I tried to define the missing functions myself:

__tsan_atomic64 __tsan_atomic64_compare_exchange_val(volatile __tsan_atomic64 
*a,
        __tsan_atomic64 *c, __tsan_atomic64 v, __tsan_memory_order mo)
    {
        return __tsan_atomic64_compare_exchange_strong(a, c, v, mo);
    }

but that causes the following error:
fatal error: error in backend: ThreadSanitizer interface function redefined

Any ideas as to how to proceed?

Thanks,
Dan

Original comment by dan.el...@gmail.com on 9 Nov 2012 at 10:34

Added labels: ****
Removed labels: ****

GoogleCodeExporter commented 9 years ago

Hi,

You need to sync to 
http://www.llvm.org/viewvc/llvm-project?view=rev&revision=167611
You can define the functions manually as you do, but you need to compile them 
w/o -fthread-sanitier (-fsanitize=thread), i.e in a separate file.

Original comment by dvyu...@google.com on 10 Nov 2012 at 10:13

Added labels: ****
Removed labels: ****

staticanalysis / data-race-test

ANNOTATE_IGNORE_READS_BEGIN()/END() don't work? #107