Message ID | 20121201122318.GN2315@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
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
================== 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) #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; }