From: Philippe Waroquiers Date: Tue, 19 Aug 2014 22:46:44 +0000 (+0000) Subject: Add option a new sim-hint no-nptl-pthread-stackcache. X-Git-Tag: svn/VALGRIND_3_10_0~170 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8e605f14c58a740d05f888508e1e4cf0cddf70b8;p=thirdparty%2Fvalgrind.git Add option a new sim-hint no-nptl-pthread-stackcache. Activating this hint using --sim-hints=no-nptl-pthread-stackcache means the glibc nptl stack cache will be disabled. Disabling this stack/tls cache avoids helgrind false positive race conditions errors when using __thread variables. Note: disabling the stack cache is done by a kludge, dependent on internal knowledge of glibc code, and using libpthread debug info. So, this kludge might be broken with newer glibc version. This has been tested on various platforms and various glibc versions 2.11, 2.16 and 2.18 To check if the disabling works, you can do: valgrind --tool=helgrind --sim-hints=no-nptl-pthread-stackcache -d -v ./helgrind/tests/tls_threads |& grep kludge If you see the below 2 lines, then hopefully the stack cache has been disabled. --12624-- deactivate nptl pthread stackcache via kludge: found symbol stack_cache_actsize at addr 0x3AF178 --12624:1:sched pthread stack cache size disabling done via kludge git-svn-id: svn://svn.valgrind.org/valgrind/trunk@14313 --- diff --git a/coregrind/m_clientstate.c b/coregrind/m_clientstate.c index 718d3cd386..b251f1bf87 100644 --- a/coregrind/m_clientstate.c +++ b/coregrind/m_clientstate.c @@ -107,6 +107,11 @@ Addr VG_(client___libc_freeres_wrapper) = 0; VG_(get_StackTrace) in m_stacktrace.c for further info. */ Addr VG_(client__dl_sysinfo_int80) = 0; +/* Address of the (internal) glibc nptl pthread stack cache size, + declared as: + static size_t stack_cache_actsize; + in nptl/allocatestack.c */ +SizeT* VG_(client__stack_cache_actsize__addr) = 0; /*--------------------------------------------------------------------*/ /*--- end ---*/ diff --git a/coregrind/m_main.c b/coregrind/m_main.c index 1a3374c085..fc0c8c994b 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -175,8 +175,8 @@ static void usage_NORETURN ( Bool debug_help ) " --vgdb-prefix= prefix for vgdb FIFOs [%s]\n" " --run-libc-freeres=no|yes free up glibc memory at exit on Linux? [yes]\n" " --sim-hints=hint1,hint2,... activate unusual sim behaviours [none] \n" -" where hint is one of no-inner-prefix lax-ioctls enable-outer\n" -" fuse-compatible none\n" +" where hint is one of lax-ioctls fuse-compatible enable-outer\n" +" no-inner-prefix no-nptl-pthread-stackcache none\n" " --fair-sched=no|yes|try schedule threads fairly on multicore systems [no]\n" " --kernel-variant=variant1,variant2,... handle non-standard kernel" " variants [none]\n" @@ -380,7 +380,8 @@ static void early_process_cmd_line_options ( /*OUT*/Int* need_help, // as early as possible. else if VG_USETX_CLO (str, "--sim-hints", "no-inner-prefix,fuse-compatible," - "lax-ioctls,enable-outer", + "lax-ioctls,enable-outer," + "no-nptl-pthread-stackcache", VG_(clo_sim_hints)) {} } } diff --git a/coregrind/m_redir.c b/coregrind/m_redir.c index 9e6a10c70e..1fb3df2961 100644 --- a/coregrind/m_redir.c +++ b/coregrind/m_redir.c @@ -403,6 +403,10 @@ void VG_(redir_notify_new_DebugInfo)( DebugInfo* newdi ) Bool check_ppcTOCs = False; Bool isText; const HChar* newdi_soname; + Bool dehacktivate_pthread_stack_cache_var_search = False; + const HChar* const pthread_soname = "libpthread.so.0"; + const HChar* const pthread_stack_cache_actsize_varname + = "stack_cache_actsize"; # if defined(VG_PLAT_USES_PPCTOC) check_ppcTOCs = True; @@ -497,6 +501,10 @@ void VG_(redir_notify_new_DebugInfo)( DebugInfo* newdi ) specList = NULL; /* the spec list we're building up */ + dehacktivate_pthread_stack_cache_var_search = + SimHintiS(SimHint_no_nptl_pthread_stackcache, VG_(clo_sim_hints)) + && 0 == VG_(strcmp)(newdi_soname, pthread_soname); + nsyms = VG_(DebugInfo_syms_howmany)( newdi ); for (i = 0; i < nsyms; i++) { VG_(DebugInfo_syms_getidx)( newdi, i, &sym_avmas, @@ -513,8 +521,22 @@ void VG_(redir_notify_new_DebugInfo)( DebugInfo* newdi ) demangled_fnpatt, N_DEMANGLED, &isWrap, &becTag, &becPrio ); /* ignore data symbols */ - if (!isText) + if (!isText) { + /* But search for dehacktivate stack cache var if needed. */ + if (dehacktivate_pthread_stack_cache_var_search + && 0 == VG_(strcmp)(*names, + pthread_stack_cache_actsize_varname)) { + if ( VG_(clo_verbosity) > 1 ) { + VG_(message)( Vg_DebugMsg, + "deactivate nptl pthread stackcache via kludge:" + " found symbol %s at addr %p\n", + *names, (void*) sym_avmas.main); + } + VG_(client__stack_cache_actsize__addr) = (SizeT*) sym_avmas.main; + dehacktivate_pthread_stack_cache_var_search = False; + } continue; + } if (!ok) { /* It's not a full-scale redirect, but perhaps it is a load-notify fn? Let the load-notify department see it. */ @@ -589,6 +611,13 @@ void VG_(redir_notify_new_DebugInfo)( DebugInfo* newdi ) } free_symname_array(names_init, &twoslots[0]); } + if (dehacktivate_pthread_stack_cache_var_search) { + VG_(message)(Vg_DebugMsg, + "WARNING: could not find symbol for var %s in %s\n", + pthread_stack_cache_actsize_varname, pthread_soname); + VG_(message)(Vg_DebugMsg, + "=> pthread stack cache cannot be disabled!\n"); + } if (check_ppcTOCs) { for (i = 0; i < nsyms; i++) { diff --git a/coregrind/m_scheduler/scheduler.c b/coregrind/m_scheduler/scheduler.c index 64d45cb5b7..3a85e524e1 100644 --- a/coregrind/m_scheduler/scheduler.c +++ b/coregrind/m_scheduler/scheduler.c @@ -61,14 +61,15 @@ #include "pub_core_basics.h" #include "pub_core_debuglog.h" #include "pub_core_vki.h" -#include "pub_core_vkiscnums.h" // __NR_sched_yield -#include "pub_core_libcsetjmp.h" // to keep _threadstate.h happy +#include "pub_core_vkiscnums.h" // __NR_sched_yield +#include "pub_core_libcsetjmp.h" // to keep _threadstate.h happy #include "pub_core_threadstate.h" +#include "pub_core_clientstate.h" #include "pub_core_aspacemgr.h" -#include "pub_core_clreq.h" // for VG_USERREQ__* +#include "pub_core_clreq.h" // for VG_USERREQ__* #include "pub_core_dispatch.h" -#include "pub_core_errormgr.h" // For VG_(get_n_errs_found)() -#include "pub_core_gdbserver.h" // for VG_(gdbserver) and VG_(gdbserver_activity) +#include "pub_core_errormgr.h" // For VG_(get_n_errs_found)() +#include "pub_core_gdbserver.h" // for VG_(gdbserver)/VG_(gdbserver_activity) #include "pub_core_libcbase.h" #include "pub_core_libcassert.h" #include "pub_core_libcprint.h" @@ -1579,6 +1580,30 @@ VgSchedReturnCode VG_(scheduler) ( ThreadId tid ) if (VG_(clo_trace_sched)) print_sched_event(tid, "exiting VG_(scheduler)"); + if (SimHintiS(SimHint_no_nptl_pthread_stackcache, VG_(clo_sim_hints))) { + if (VG_(client__stack_cache_actsize__addr)) { + if (*VG_(client__stack_cache_actsize__addr) == 0) { + /* We disable the stack cache just before the first + thread exits. At this moment, we are sure the pthread lib + loading is done/variable was initialised by + pthread lib/... */ + VG_(debugLog)(1,"sched", + "pthread stack cache size disabling done" + " via kludge\n"); + *VG_(client__stack_cache_actsize__addr) = 1000 * 1000 * 1000; + /* Set a value big enough to be above the hardcoded maximum stack + cache size in glibc, small enough to allow a pthread stack size + to be added without risk of overflow. */ + } + } else { + VG_(debugLog)(0,"sched", + "WARNING: pthread stack cache cannot be disabled!\n"); + VG_(clo_sim_hints) &= !SimHint2S(SimHint_no_nptl_pthread_stackcache); + /* Remove SimHint_no_nptl_pthread_stackcache from VG_(clo_sim_hints) + to avoid having a msg for at all thread terminations. */ + } + } + vg_assert(VG_(is_exiting)(tid)); return tst->exitreason; diff --git a/coregrind/pub_core_clientstate.h b/coregrind/pub_core_clientstate.h index fe95b82c51..64ec161917 100644 --- a/coregrind/pub_core_clientstate.h +++ b/coregrind/pub_core_clientstate.h @@ -93,6 +93,24 @@ extern Addr VG_(client___libc_freeres_wrapper); extern Addr VG_(client__dl_sysinfo_int80); +/* glibc nptl pthread systems only, when no-nptl-pthread-stackcache + was given in --sim-hints. + Used for a (kludgy) way to disable the cache of stacks as implemented in + nptl glibc. + Based on internal knowledge of the pthread glibc nptl/allocatestack.c code: + a huge value in stack_cache_actsize (bigger than the constant + stack_cache_maxsize) makes glibc believes the cache is full + and so stacks are always released when a pthread terminates. + Several ugliness in this kludge: + * hardcodes private glibc var name "stack_cache_maxsize" + * based on knowledge of the code of the functions + queue_stack and __free_stacks + * static symbol for "stack_cache_maxsize" must be in + the debug info. + It would be much cleaner to have a documented and supported + way to disable the pthread stack cache. */ +extern SizeT* VG_(client__stack_cache_actsize__addr); + #endif // __PUB_CORE_CLIENTSTATE_H /*--------------------------------------------------------------------*/ diff --git a/coregrind/pub_core_options.h b/coregrind/pub_core_options.h index 1c1d285f1d..9bf2d04f10 100644 --- a/coregrind/pub_core_options.h +++ b/coregrind/pub_core_options.h @@ -223,10 +223,11 @@ extern Int VG_(clo_dump_error); /* Engage miscellaneous weird hacks needed for some progs. */ typedef enum { - SimHint_no_inner_prefix, - SimHint_fuse_compatible, SimHint_lax_ioctls, - SimHint_enable_outer + SimHint_fuse_compatible, + SimHint_enable_outer, + SimHint_no_inner_prefix, + SimHint_no_nptl_pthread_stackcache } SimHint; diff --git a/docs/xml/manual-core.xml b/docs/xml/manual-core.xml index 4d601313b6..949874b2c8 100644 --- a/docs/xml/manual-core.xml +++ b/docs/xml/manual-core.xml @@ -1956,6 +1956,7 @@ need to use them. the simulated behaviour in nonstandard or dangerous ways, possibly to help the simulation of strange features. By default no hints are enabled. Use with caution! Currently known hints are: + Be very lax about ioctl @@ -1965,11 +1966,23 @@ need to use them. large number of strange ioctl commands becomes very tiresome. + + + Enable special + handling for certain system calls that may block in a FUSE + file-system. This may be necessary when running Valgrind + on a multi-threaded program that uses one thread to manage + a FUSE file-system and another thread to access that + file-system. + + + Enable some special magic needed when the program being run is itself Valgrind. + Disable printing a prefix in front of each stdout or @@ -1980,13 +1993,39 @@ need to use them. front of the inner debug logging lines. - Enable special - handling for certain system calls that may block in a FUSE - file-system. This may be necessary when running Valgrind - on a multi-threaded program that uses one thread to manage - a FUSE file-system and another thread to access that - file-system. - + + This hint is only relevant when running Valgrind on Linux. + + The GNU glibc pthread library + (libpthread.so), which is used by + pthread programs, maintains a cache of pthread stacks. + When a pthread terminates, the memory used for the pthread + stack and some thread local storage related data structure + are not always directly released. This memory is kept in + a cache (up to a certain size), and is re-used if a new + thread is started. + + This cache causes the helgrind tool to report some + false positive race condition errors on this cached + memory, as helgrind does not understand the internal glibc + cache synchronisation primitives. So, when using helgrind, + disabling the cache helps to avoid false positive race + conditions, in particular when using thread local storage + variables (e.g. variables using the + __thread qualifier). + + When using the memcheck tool, disabling the cache + ensures the memory used by glibc to handle __thread + variables is directly released when a thread + terminates. + + Note: Valgrind disables the cache using some internal + knowledge of the glibc stack cache implementation and by + examining the debug information of the pthread + library. This technique is thus somewhat fragile and might + not work for all glibc versions. This has been succesfully + tested with various glibc versions (e.g. 2.11, 2.16, 2.18) + on various platforms. diff --git a/helgrind/docs/hg-manual.xml b/helgrind/docs/hg-manual.xml index 21328b4330..bc9f49a226 100644 --- a/helgrind/docs/hg-manual.xml +++ b/helgrind/docs/hg-manual.xml @@ -971,6 +971,16 @@ unlock(mx) unlock(mx) supported. + + If your application is using thread local variables, + helgrind might report false positive race conditions on these + variables, despite being very probably race free. On Linux, you can + use + to avoid such false positive error messages + (see ). + + + Round up all finished threads using pthread_join. Avoid diff --git a/helgrind/tests/Makefile.am b/helgrind/tests/Makefile.am index 79dc9452a5..1a8b1f8242 100644 --- a/helgrind/tests/Makefile.am +++ b/helgrind/tests/Makefile.am @@ -102,7 +102,9 @@ EXTRA_DIST = \ tc23_bogus_condwait.stderr.exp \ tc23_bogus_condwait.stderr.exp-mips32 \ tc24_nonzero_sem.vgtest tc24_nonzero_sem.stdout.exp \ - tc24_nonzero_sem.stderr.exp + tc24_nonzero_sem.stderr.exp \ + tls_threads.vgtest tls_threads.stdout.exp \ + tls_threads.stderr.exp # XXX: tc18_semabuse uses operations that are unsupported on Darwin. It # should be conditionally compiled like tc20_verifywrap is. @@ -144,7 +146,8 @@ check_PROGRAMS = \ tc19_shadowmem \ tc21_pthonce \ tc23_bogus_condwait \ - tc24_nonzero_sem + tc24_nonzero_sem \ + tls_threads # DDD: it seg faults, and then the Valgrind exit path hangs # JRS 29 July 09: it craps out in the stack unwinder, in diff --git a/helgrind/tests/tls_threads.c b/helgrind/tests/tls_threads.c new file mode 100644 index 0000000000..def8567903 --- /dev/null +++ b/helgrind/tests/tls_threads.c @@ -0,0 +1,109 @@ +#include +#include +#include + +#ifdef HAVE_TLS + +static int only_touch_stackvar; + +/* We should have no error on local and global + as these are both thread local variables. */ +static __thread int local; +__thread int global; + +/* We will wrongly share this variable indirectly through a pointer + We should have an error for this. */ +static __thread int badly_shared_local; + +/* ptr_to_badly_shared_local allows to have multiple threads seeing + the same thread local storage. This is however really bad sharing + as this can cause SEGV or whatever, as when the thread disappears, + the badly_shared_local var pointed to can also disappear. + By default, the regtest does not test this really bad sharing. */ +pthread_mutex_t protect_ptr_to_badly_shared_local = PTHREAD_MUTEX_INITIALIZER; +int *ptr_to_badly_shared_local; + +static void local_false_positive(void) +{ + local = local + 1; // no error is expected +} + +static void global_false_positive(void) +{ + global = global + 1; // no error is expected +} + +static void badly_shared_local_error_expected(void) +{ + *ptr_to_badly_shared_local = *ptr_to_badly_shared_local + 1; // an error is expected + // This can cause a SIGSEGV. +} + +static void *level2(void *p) +{ + int stackvar = 0; + + stackvar = stackvar + only_touch_stackvar; + + local_false_positive(); + global_false_positive(); + if (only_touch_stackvar != 0) { + badly_shared_local_error_expected(); + } + return 0; +} + +#define NLEVEL2 10 +static void *level1(void *p) +{ + pthread_t threads[NLEVEL2]; + int curthread = 0; + int i; + + pthread_mutex_lock(&protect_ptr_to_badly_shared_local); + if (ptr_to_badly_shared_local == NULL) + ptr_to_badly_shared_local = &badly_shared_local; + pthread_mutex_unlock(&protect_ptr_to_badly_shared_local); + + for(i = 0; i < NLEVEL2; i++) { + pthread_create(&threads[curthread++], NULL, level2, NULL); + } + + for(i = 0; i < curthread; i++) + pthread_join(threads[i], NULL); + + return 0; +} + +#define NLEVEL1 3 +int main(int argc, char*argv[]) +{ + pthread_t threads[NLEVEL1]; + int curthread = 0; + int i; + + only_touch_stackvar = argc > 1; + + for(i = 0; i < NLEVEL1; i++) { + pthread_create(&threads[curthread++], NULL, level1, NULL); + } + + fprintf(stderr, "starting join in main\n"); + fflush(stderr); + for(i = 0; i < curthread; i++) + pthread_join(threads[i], NULL); + fprintf(stderr, "finished join in main\n"); + fflush(stderr); + return 0; +} +#else +int main() +{ + fprintf(stderr, "starting join in main\n"); + fflush(stderr); + /* do nothing */ + fprintf(stderr, "finished join in main\n"); + fflush(stderr); + return 0; +} +#endif diff --git a/helgrind/tests/tls_threads.stderr.exp b/helgrind/tests/tls_threads.stderr.exp new file mode 100644 index 0000000000..be3b8904e7 --- /dev/null +++ b/helgrind/tests/tls_threads.stderr.exp @@ -0,0 +1,2 @@ +starting join in main +finished join in main diff --git a/helgrind/tests/tls_threads.stdout.exp b/helgrind/tests/tls_threads.stdout.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/helgrind/tests/tls_threads.vgtest b/helgrind/tests/tls_threads.vgtest new file mode 100644 index 0000000000..5fb4587372 --- /dev/null +++ b/helgrind/tests/tls_threads.vgtest @@ -0,0 +1,2 @@ +prog: tls_threads +vgopts: -q --sim-hints=no-nptl-pthread-stackcache diff --git a/none/tests/cmdline1.stdout.exp b/none/tests/cmdline1.stdout.exp index 122d7e550f..02a0fe36d2 100644 --- a/none/tests/cmdline1.stdout.exp +++ b/none/tests/cmdline1.stdout.exp @@ -88,8 +88,8 @@ usage: valgrind [options] prog-and-args --vgdb-prefix= prefix for vgdb FIFOs [/tmp/vgdb-pipe] --run-libc-freeres=no|yes free up glibc memory at exit on Linux? [yes] --sim-hints=hint1,hint2,... activate unusual sim behaviours [none] - where hint is one of no-inner-prefix lax-ioctls enable-outer - fuse-compatible none + where hint is one of lax-ioctls fuse-compatible enable-outer + no-inner-prefix no-nptl-pthread-stackcache none --fair-sched=no|yes|try schedule threads fairly on multicore systems [no] --kernel-variant=variant1,variant2,... handle non-standard kernel variants [none] where variant is one of bproc none diff --git a/none/tests/cmdline2.stdout.exp b/none/tests/cmdline2.stdout.exp index a26abafafb..7f4277c94f 100644 --- a/none/tests/cmdline2.stdout.exp +++ b/none/tests/cmdline2.stdout.exp @@ -88,8 +88,8 @@ usage: valgrind [options] prog-and-args --vgdb-prefix= prefix for vgdb FIFOs [/tmp/vgdb-pipe] --run-libc-freeres=no|yes free up glibc memory at exit on Linux? [yes] --sim-hints=hint1,hint2,... activate unusual sim behaviours [none] - where hint is one of no-inner-prefix lax-ioctls enable-outer - fuse-compatible none + where hint is one of lax-ioctls fuse-compatible enable-outer + no-inner-prefix no-nptl-pthread-stackcache none --fair-sched=no|yes|try schedule threads fairly on multicore systems [no] --kernel-variant=variant1,variant2,... handle non-standard kernel variants [none] where variant is one of bproc none