llvm dev - Apr 2016 - [TSAN] LLVM statistics and pass initialization trigger race detection

Hello,

I trying TSAN on Darwin on LLVM itself (sanitizing multi-threaded ThinLTO link).
However I see two main issues on my debug build: 

1) Statistics: the pre/post increment is not safe, it seems to be acknowledge in
the code itself:

    // FIXME: This function and all those that follow carefully use an
    // atomic operation to update the value safely in the presence of
    // concurrent accesses, but not to read the return value, so the
    // return value is not thread safe.

Can we tell TSAN to ignore the statistics somehow? Alternatively, is there a fix
someone can suggest?

2) Pass initialization. This macro does not please TSAN (even though it seems
written with TSAN in mind):

#define CALL_ONCE_INITIALIZATION(function) \
  static volatile sys::cas_flag initialized = 0; \
  sys::cas_flag old_val = sys::CompareAndSwap(&initialized, 1, 0); \
  if (old_val == 0) { \
    function(Registry); \
    sys::MemoryFence(); \
    TsanIgnoreWritesBegin(); \
    TsanHappensBefore(&initialized); \
    initialized = 2; \
    TsanIgnoreWritesEnd(); \
  } else { \
    sys::cas_flag tmp = initialized; \
    sys::MemoryFence(); \
    while (tmp != 2) { \
      tmp = initialized; \
      sys::MemoryFence(); \
    } \
  } \
  TsanHappensAfter(&initialized);

The failure looks like:

=================WARNING: ThreadSanitizer: data race (pid=17192)
  Atomic write of size 4 at 0x0001113619e8 by thread T13:
    #0 __tsan_atomic32_compare_exchange_val <null>:568340296
(libclang_rt.tsan_osx_dynamic.dylib+0x00000003e7aa)
    #1 llvm::sys::CompareAndSwap(unsigned int volatile*, unsigned int, unsigned
int) Atomic.cpp:52 (libLTO.dylib+0x000000511914)
    #2 llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82 (libLTO.dylib+0x000000ab2b55)
    #3 (anonymous namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:50
(libLTO.dylib+0x000000ab2e8e)
    #4 (anonymous namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:49
(libLTO.dylib+0x000000ab2d19)
    #5 llvm::createFunctionInliningPass() InlineSimple.cpp:85
(libLTO.dylib+0x000000ab2ce3)
...

  Previous read of size 4 at 0x0001113619e8 by thread T14:
    #0 llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82 (libLTO.dylib+0x000000ab2b65)
    #1 (anonymous namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:50
(libLTO.dylib+0x000000ab2e8e)
    #2 (anonymous namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:49
(libLTO.dylib+0x000000ab2d19)
    #3 llvm::createFunctionInliningPass() InlineSimple.cpp:85
(libLTO.dylib+0x000000ab2ce3)
...
  Location is global
'llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)::initialized'
at 0x0001113619e8 (libLTO.dylib+0x0000016389e8)


Thanks!

Mehdi


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160415/b34ec2a8/attachment.html>

+dvyukov



On Fri, Apr 15, 2016 at 10:24 PM, Mehdi Amini via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hello,
>
> I trying TSAN on Darwin on LLVM itself (sanitizing multi-threaded ThinLTO
> link).
> However I see two main issues on my debug build:
>
> 1) Statistics: the pre/post increment is not safe, it seems to be
> acknowledge in the code itself:
>
>     // FIXME: This function and all those that follow carefully use an
>     // atomic operation to update the value safely in the presence of
>     // concurrent accesses, but not to read the return value, so the
>     // return value is not thread safe.
>
> Can we tell TSAN to ignore the statistics somehow? Alternatively, is there
> a fix someone can suggest?
>
It's better to add proper synchronization, most likely an atomic read
with memory_order_relaxed is enough.

>
> 2) Pass initialization. This macro does not please TSAN (even though it
> seems written with TSAN in mind):
>
> #define CALL_ONCE_INITIALIZATION(function) \
>   static volatile sys::cas_flag initialized = 0; \
>   sys::cas_flag old_val = sys::CompareAndSwap(&initialized, 1, 0); \
>   if (old_val == 0) { \
>     function(Registry); \
>     sys::MemoryFence(); \
>     TsanIgnoreWritesBegin(); \
>     TsanHappensBefore(&initialized); \
>     initialized = 2; \
>     TsanIgnoreWritesEnd(); \
>   } else { \
>     sys::cas_flag tmp = initialized; \
>     sys::MemoryFence(); \
>     while (tmp != 2) { \
>       tmp = initialized; \
>       sys::MemoryFence(); \
>     } \
>   } \
>   TsanHappensAfter(&initialized);
>

This code might have been added to please the old valgrind-based tsan
(which did not understand atomics).
For the current tsan we should just implement proper synchronization and
remove the annotations.
Why can't we use std::call_once here?

Looks like you are serious about running llvm under tsan. Very cool!
Does it make sense to add a tsan build to our sanitizer bot
<http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast> (which
already has asan, ubsan, lsan, and msan)?
What tests and what build mode would you suggest for the bot?

--kcc

>
> The failure looks like:
>
> =================> WARNING: ThreadSanitizer: data race (pid=17192)
>   Atomic write of size 4 at 0x0001113619e8 by thread T13:
>     #0 __tsan_atomic32_compare_exchange_val
> <null>:568340296 (libclang_rt.tsan_osx_dynamic.dylib+0x00000003e7aa)
>     #1 llvm::sys::CompareAndSwap(unsigned int volatile*, unsigned
> int, unsigned int) Atomic.cpp:52 (libLTO.dylib+0x000000511914)
>     #2
> llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82
> (libLTO.dylib+0x000000ab2b55)
>     #3 (anonymous
> namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:50
> (libLTO.dylib+0x000000ab2e8e)
>     #4 (anonymous
> namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:49
> (libLTO.dylib+0x000000ab2d19)
>     #5 llvm::createFunctionInliningPass()
> InlineSimple.cpp:85 (libLTO.dylib+0x000000ab2ce3)
> ...
>
>   Previous read of size 4 at 0x0001113619e8 by thread T14:
>     #0
> llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82
> (libLTO.dylib+0x000000ab2b65)
>     #1 (anonymous
> namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:50
> (libLTO.dylib+0x000000ab2e8e)
>     #2 (anonymous
> namespace)::SimpleInliner::SimpleInliner() InlineSimple.cpp:49
> (libLTO.dylib+0x000000ab2d19)
>     #3 llvm::createFunctionInliningPass()
> InlineSimple.cpp:85 (libLTO.dylib+0x000000ab2ce3)
> ...
>   Location is
> global
'llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)::initialized'
at
> 0x0001113619e8 (libLTO.dylib+0x0000016389e8)
>
>
> Thanks!
>
> Mehdi
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/98502b18/attachment-0001.html>

Hi,

Thanks for the answers.
> On Apr 18, 2016, at 10:38 AM, Kostya Serebryany <kcc at google.com>
wrote:
> 
> +dvyukov
> 
> 
> 
> On Fri, Apr 15, 2016 at 10:24 PM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> Hello,
> 
> I trying TSAN on Darwin on LLVM itself (sanitizing multi-threaded ThinLTO
link).
> However I see two main issues on my debug build: 
> 
> 1) Statistics: the pre/post increment is not safe, it seems to be
acknowledge in the code itself:
> 
>     // FIXME: This function and all those that follow carefully use an
>     // atomic operation to update the value safely in the presence of
>     // concurrent accesses, but not to read the return value, so the
>     // return value is not thread safe.
> 
> Can we tell TSAN to ignore the statistics somehow? Alternatively, is there
a fix someone can suggest?
> 
> It's better to add proper synchronization, most likely an atomic read
with memory_order_relaxed is enough.
Ok, will look into this!
>  
> 
> 2) Pass initialization. This macro does not please TSAN (even though it
seems written with TSAN in mind):
> 
> #define CALL_ONCE_INITIALIZATION(function) \
>   static volatile sys::cas_flag initialized = 0; \
>   sys::cas_flag old_val = sys::CompareAndSwap(&initialized, 1, 0); \
>   if (old_val == 0) { \
>     function(Registry); \
>     sys::MemoryFence(); \
>     TsanIgnoreWritesBegin(); \
>     TsanHappensBefore(&initialized); \
>     initialized = 2; \
>     TsanIgnoreWritesEnd(); \
>   } else { \
>     sys::cas_flag tmp = initialized; \
>     sys::MemoryFence(); \
>     while (tmp != 2) { \
>       tmp = initialized; \
>       sys::MemoryFence(); \
>     } \
>   } \
>   TsanHappensAfter(&initialized);
> 
> 
> This code might have been added to please the old valgrind-based tsan
(which did not understand atomics).
> For the current tsan we should just implement proper synchronization and
remove the annotations.
> Why can't we use std::call_once here? 
Maybe some pre c++11 thing? 
I'll try to upgrade std::call_once.

> 
> Looks like you are serious about running llvm under tsan. Very cool! 
> Does it make sense to add a tsan build to our sanitizer bot
<http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast> (which
already has asan, ubsan, lsan, and msan)?
> What tests and what build mode would you suggest for the bot? 
I plan on adding a TSan bot on Green Dragon in the near future.
My idea is to perform a ThinLTO bootstrap of clang (i.e. stage 1 being a
Release+Assert+TSAN and stage2 Release-ThinLTO). This involves running the
optimizer and the codegen in multiple threads.


-- 
Mehdi


> 
> --kcc 
>  
> 
> The failure looks like:
> 
> =================> WARNING: ThreadSanitizer: data race (pid=17192)
>   Atomic write of size 4 at 0x0001113619e8 by thread T13:
>     #0 __tsan_atomic32_compare_exchange_val <null>:568340296
(libclang_rt.tsan_osx_dynamic.dylib+0x00000003e7aa)
>     #1 llvm::sys::CompareAndSwap(unsigned int volatile*, unsigned int,
unsigned int) Atomic.cpp:52 (libLTO.dylib+0x000000511914)
>     #2 llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82 (libLTO.dylib+0x000000ab2b55)
>     #3 (anonymous namespace)::SimpleInliner::SimpleInliner()
InlineSimple.cpp:50 (libLTO.dylib+0x000000ab2e8e)
>     #4 (anonymous namespace)::SimpleInliner::SimpleInliner()
InlineSimple.cpp:49 (libLTO.dylib+0x000000ab2d19)
>     #5 llvm::createFunctionInliningPass() InlineSimple.cpp:85
(libLTO.dylib+0x000000ab2ce3)
> ...
> 
>   Previous read of size 4 at 0x0001113619e8 by thread T14:
>     #0 llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)
InlineSimple.cpp:82 (libLTO.dylib+0x000000ab2b65)
>     #1 (anonymous namespace)::SimpleInliner::SimpleInliner()
InlineSimple.cpp:50 (libLTO.dylib+0x000000ab2e8e)
>     #2 (anonymous namespace)::SimpleInliner::SimpleInliner()
InlineSimple.cpp:49 (libLTO.dylib+0x000000ab2d19)
>     #3 llvm::createFunctionInliningPass() InlineSimple.cpp:85
(libLTO.dylib+0x000000ab2ce3)
> ...
>   Location is global
'llvm::initializeSimpleInlinerPass(llvm::PassRegistry&)::initialized'
at 0x0001113619e8 (libLTO.dylib+0x0000016389e8)
> 
> 
> Thanks!
> 
> Mehdi
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/abc82446/attachment.html>