[tsan] Small bugfix

On Sat, Dec 01, 2012 at 01:53:52PM +0400, Dmitry Vyukov wrote:
> On Sat, Dec 1, 2012 at 4:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > Hi!
> >
> > When I've tried to compile the attached testcase (I was trying to see
> > if tsan could discover the emutls.c data race), I got ICEs because
> > expr_ptr in certain cases wasn't is_gimple_val and thus was invalid to
> > pass it directly to a call as argument, fixed thusly.
> >
> > Unfortunately, trying to compile it dynamically against libtsan.so
> > doesn't work (apparently it is using -fvisibility=hidden, but not
> > saying the public entry points have default visibility),
> 
> Runtime needs to mark all interface functions as visibility("default"), right?

Yeah, == SANITIZER_INTERFACE_ATTRIBUTE .

> > compiling it
> > by hand statically against libtsan.a (we don't have -static-libtsan yet)
> > failed at runtime, complaining the binary isn't a PIE - can't it really
> > support normal executables?
> 
> It's not trivial to do fast shadow memory mapping in this case.
> Initially non pie builds ware not planned at. But I am starting to
> think that I know how to do it.
> I will try to look into it in next weeks.

Perhaps libtsan.a could be for PIEs only, and document that -static-libtsan
has that limitation.  And libtsan.so.0 could add in some offset to allow
even non-PIE binaries.

> > and when compiled/linked as PIE, I got
> > ==================
> > WARNING: ThreadSanitizer: thread leak (pid=31150)
> >   Thread 3 (tid=31153, finished) created at:
> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
> >     #1 main ??:0 (exe+0x00000000505f)
> >
> > ==================
> > ==================
> > WARNING: ThreadSanitizer: thread leak (pid=31150)
> >   Thread 4 (tid=31155, finished) created at:
> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
> >     #1 main ??:0 (exe+0x00000000505f)
> >
> > ==================
> > ==================
> > WARNING: ThreadSanitizer: thread leak (pid=31150)
> >   Thread 5 (tid=31156, finished) created at:
> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
> >     #1 main ??:0 (exe+0x00000000505f)
> >
> > ==================
> > ThreadSanitizer: reported 3 warnings
> >
> > which is probably not what I was expecting to see.
> 
> The thread leak reports are correct, right?

No idea what do you mean by thread leak.  What exactly is leaking?

> The race must be detectable. Can you show the code? The first thing to
> check is that the memory accesses are instrumented. Also if you build
> runtime with -DTSAN_DEBUG_OUTPUT=2 it will print all incoming events;
> if you post the log most likely I will be able to say why the race is
> not detected.

Ah, on closer inspection I found the bug on the GCC side, forgotten
gimple_insert_before instead of gimple_insert_seq_before in one case.

Now it reports the race (3 times):


	Jakub

On Sat, Dec 1, 2012 at 4:23 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > Hi!
>> >
>> > When I've tried to compile the attached testcase (I was trying to see
>> > if tsan could discover the emutls.c data race), I got ICEs because
>> > expr_ptr in certain cases wasn't is_gimple_val and thus was invalid to
>> > pass it directly to a call as argument, fixed thusly.
>> >
>> > Unfortunately, trying to compile it dynamically against libtsan.so
>> > doesn't work (apparently it is using -fvisibility=hidden, but not
>> > saying the public entry points have default visibility),
>>
>> Runtime needs to mark all interface functions as visibility("default"), right?
>
> Yeah, == SANITIZER_INTERFACE_ATTRIBUTE .
>
>> > compiling it
>> > by hand statically against libtsan.a (we don't have -static-libtsan yet)
>> > failed at runtime, complaining the binary isn't a PIE - can't it really
>> > support normal executables?
>>
>> It's not trivial to do fast shadow memory mapping in this case.
>> Initially non pie builds ware not planned at. But I am starting to
>> think that I know how to do it.
>> I will try to look into it in next weeks.
>
> Perhaps libtsan.a could be for PIEs only, and document that -static-libtsan
> has that limitation.  And libtsan.so.0 could add in some offset to allow
> even non-PIE binaries.
>> > and when compiled/linked as PIE, I got
>> > ==================
>> > WARNING: ThreadSanitizer: thread leak (pid=31150)
>> >   Thread 3 (tid=31153, finished) created at:
>> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>> >     #1 main ??:0 (exe+0x00000000505f)
>> >
>> > ==================
>> > ==================
>> > WARNING: ThreadSanitizer: thread leak (pid=31150)
>> >   Thread 4 (tid=31155, finished) created at:
>> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>> >     #1 main ??:0 (exe+0x00000000505f)
>> >
>> > ==================
>> > ==================
>> > WARNING: ThreadSanitizer: thread leak (pid=31150)
>> >   Thread 5 (tid=31156, finished) created at:
>> >     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>> >     #1 main ??:0 (exe+0x00000000505f)
>> >
>> > ==================
>> > ThreadSanitizer: reported 3 warnings
>> >
>> > which is probably not what I was expecting to see.
>>
>> The thread leak reports are correct, right?
>
> No idea what do you mean by thread leak.  What exactly is leaking?


Thread leak is joinable but not joined thread.
I have a pending todo to aggregate them by stack, so in this case it
will "3 threads leaked here". Perhaps I need to report only finished
threads, if a thread runs during exit, perhaps it does not matter.
And there is a flag to disable it at all:
TSAN_OPTIONS="report_thread_leaks=0" ./app


>> The race must be detectable. Can you show the code? The first thing to
>> check is that the memory accesses are instrumented. Also if you build
>> runtime with -DTSAN_DEBUG_OUTPUT=2 it will print all incoming events;
>> if you post the log most likely I will be able to say why the race is
>> not detected.
>
> Ah, on closer inspection I found the bug on the GCC side, forgotten
> gimple_insert_before instead of gimple_insert_seq_before in one case.
>
> Now it reports the race (3 times):

Great!
Do you see 3 similar reports? TSan suppreses further reports on the
same address and with the same stacks.

> ==================
> WARNING: ThreadSanitizer: data race (pid=3613)
>   Write of size 8 at 0x7fc5c8bfae40 by thread 1:
>     #0 foo emutlstest.c:56 (exe+0x0000000053b7)
>     #1 tf emutlstest.c:102 (exe+0x0000000054a9)
>
>   Previous read of size 8 at 0x7fc5c8bfae40 by thread 2:
>     #0 foo emutlstest.c:46 (exe+0x0000000052c7)
>     #1 tf emutlstest.c:102 (exe+0x0000000054a9)
>
>   Thread 1 (tid=3615, running) created at:
>     #0 pthread_create ??:0 (exe+0x00000000e54c)

Is the runtime built with -g/-gmlt?


>     #1 main emutlstest.c:115 (exe+0x00000000505f)
>
>   Thread 2 (tid=3616, running) created at:
>     #0 pthread_create ??:0 (exe+0x00000000e54c)
>     #1 main emutlstest.c:115 (exe+0x00000000505f)
>
> ==================
>
> while when using uintptr_t offset = __atomic_load_n (&x->offset, __ATOMIC_ACQUIRE);
> instead of uintptr_t offset = x->offset; it doesn't report it.
>
> So here is the fixed up patch:
>
> 2012-12-01  Jakub Jelinek  <jakub@redhat.com>
>
>         * tsan.c (instrument_expr): If expr_ptr isn't a gimple val, first
>         store it into a SSA_NAME.
>
> --- gcc/tsan.c.jj       2012-12-01 12:51:40.437808319 +0100
> +++ gcc/tsan.c  2012-12-01 13:11:31.347889889 +0100
> @@ -93,10 +93,11 @@ is_vptr_store (gimple stmt, tree expr, b
>  static bool
>  instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
>  {
> -  tree base, rhs, expr_type, expr_ptr, builtin_decl;
> +  tree base, rhs, expr_ptr, builtin_decl;
>    basic_block bb;
>    HOST_WIDE_INT size;
>    gimple stmt, g;
> +  gimple_seq seq;
>    location_t loc;
>
>    size = int_size_in_bytes (TREE_TYPE (expr));
> @@ -139,21 +140,25 @@ instrument_expr (gimple_stmt_iterator gs
>    rhs = is_vptr_store (stmt, expr, is_write);
>    gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
>    expr_ptr = build_fold_addr_expr (unshare_expr (expr));
> -  if (rhs == NULL)
> +  seq = NULL;
> +  if (!is_gimple_val (expr_ptr))
>      {
> -      expr_type = TREE_TYPE (expr);
> -      while (TREE_CODE (expr_type) == ARRAY_TYPE)
> -       expr_type = TREE_TYPE (expr_type);
> -      size = int_size_in_bytes (expr_type);
> -      g = gimple_build_call (get_memory_access_decl (is_write, size),
> -                            1, expr_ptr);
> +      g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr_ptr), NULL),
> +                              expr_ptr);
> +      expr_ptr = gimple_assign_lhs (g);
> +      gimple_set_location (g, loc);
> +      gimple_seq_add_stmt_without_update (&seq, g);
>      }
> +  if (rhs == NULL)
> +    g = gimple_build_call (get_memory_access_decl (is_write, size),
> +                          1, expr_ptr);
>    else
>      {
>        builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
>        g = gimple_build_call (builtin_decl, 1, expr_ptr);
>      }
>    gimple_set_location (g, loc);
> +  gimple_seq_add_stmt_without_update (&seq, g);
>    /* Instrumentation for assignment of a function result
>       must be inserted after the call.  Instrumentation for
>       reads of function arguments must be inserted before the call.
> @@ -170,13 +175,13 @@ instrument_expr (gimple_stmt_iterator gs
>           bb = gsi_bb (gsi);
>           e = find_fallthru_edge (bb->succs);
>           if (e)
> -           gsi_insert_seq_on_edge_immediate (e, g);
> +           gsi_insert_seq_on_edge_immediate (e, seq);
>         }
>        else
> -       gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +       gsi_insert_seq_after (&gsi, seq, GSI_NEW_STMT);
>      }
>    else
> -    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> +    gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
>
>    return true;
>  }
>
>
>         Jakub

On Sat, Dec 01, 2012 at 04:55:35PM +0400, Dmitry Vyukov wrote:
> > No idea what do you mean by thread leak.  What exactly is leaking?
> 
> 
> Thread leak is joinable but not joined thread.
> I have a pending todo to aggregate them by stack, so in this case it
> will "3 threads leaked here". Perhaps I need to report only finished
> threads, if a thread runs during exit, perhaps it does not matter.
> And there is a flag to disable it at all:
> TSAN_OPTIONS="report_thread_leaks=0" ./app

Actually, seems tsan was right about this, when tweaking the test from 2 to
5 threads I ended up with:
int
main (void)
{
  pthread_t p[2]; // wrong, should have been p[5]
  int i;
  for (i = 0; i < 5; i++)
    if (pthread_create (&p[i], NULL, tf, NULL))
      return 0;
  for (i = 0; i < 5; i++)
    pthread_join (p[i], NULL);
  return 0;
}

and gcc when optimizing kept the first loop to iterate 5 times
(just taking address of past the end of buffer, undefined too),
while the second loop got optimized into just two iterations (cunrolli
opt).

	Jakub