From f181d9b03a7bb910ea429ab47767c3654ef676b2 Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Tue, 24 Jun 2014 20:48:25 +0000 Subject: [PATCH] Fix a regression in supp matching with obj: entries Suppression matching logic was changed to understand inlined function calls. A regression was introduced while doing this. This regression could cause false positive supp matches or false negative supp matches, when obj: lines are used. This patch fixes the regression, and adds 2 tests (one that was failing with false positive, one that was failing with false negative). The fix is relatively small (3 places where there was an "off or excess by one"). However, a lot more tracing was added in the supp matching logic, as this logic is quite complex (for performance reasons mostly). We might need more tests to properly cover supp matching logic. So, giving -d -d -d -d produces a trace showing how a stacktrace was expanded by the input completer and which suppression (if any) it matched. Below is an example of trace. It shows a begin/end marker. The end marker indicates if a supp matched. Then it shows the stack trace, and the state of the lazy "input completer" used for the matching. In the below, the trace shows that there are 3 IPs in the stacktrace (n_ips 3) : Two are not shown (below main), and one IP corresponds to main calling 4 inlined functions (so we have only one IP for 5 entries in the stacktrace). The state of the input completer shows that 2 IPs were expanded, resulting in 6 expanded fun: or obj: lines. The offset shows that ips0 corresponds to the entries [0,4] in ip2fo->funoffset or ip2fo->objoffset. This tracing should make it more clear what was used to match a stacktrace with the suppression entries. --10314-- errormgr matching begin --10314-- errormgr matching end suppression main_a_b_c_d ./memcheck/tests/inlinfosupp.supp:2 matched: ==10314== at 0x8048667: fun_d (inlinfo.c:7) ==10314== by 0x8048667: fun_c (inlinfo.c:15) ==10314== by 0x8048667: fun_b (inlinfo.c:21) ==10314== by 0x8048667: fun_a (inlinfo.c:27) ==10314== by 0x8048667: main (inlinfo.c:66) n_ips 3 n_ips_expanded 2 resulting in n_expanded 6 ips 0 0x088048667 offset [0,4] fun:fun_d obj:/home/philippe/valgrind/objcompl/memcheck/tests/inlinfo fun:fun_c obj:/home/philippe/valgrind/objcompl/memcheck/tests/inlinfo fun:fun_b obj:/home/philippe/valgrind/objcompl/memcheck/tests/inlinfo fun:fun_a obj:/home/philippe/valgrind/objcompl/memcheck/tests/inlinfo fun:main obj:/home/philippe/valgrind/objcompl/memcheck/tests/inlinfo ips 1 0x0822abb5 offset [5,5] fun:(below main) obj: Complete tracing (including individual pattern matching) can be activated by recompiling m_errormgr.c after changing #define DEBUG_ERRORMGR 0 to #define DEBUG_ERRORMGR 1 This detailed tracing will be shown between the begin/end marker. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@14095 --- coregrind/m_errormgr.c | 83 ++++++++++++++++++++++---- memcheck/tests/Makefile.am | 2 + memcheck/tests/supponlyobj.stderr.exp | 6 ++ memcheck/tests/supponlyobj.supp | 16 +++++ memcheck/tests/supponlyobj.vgtest | 4 ++ memcheck/tests/suppvarinfo5.stderr.exp | 1 + memcheck/tests/suppvarinfo5.supp | 29 +++++++++ memcheck/tests/suppvarinfo5.vgtest | 3 + 8 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 memcheck/tests/supponlyobj.stderr.exp create mode 100755 memcheck/tests/supponlyobj.supp create mode 100644 memcheck/tests/supponlyobj.vgtest create mode 100644 memcheck/tests/suppvarinfo5.stderr.exp create mode 100644 memcheck/tests/suppvarinfo5.supp create mode 100644 memcheck/tests/suppvarinfo5.vgtest diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c index 978524bba..e7de4eb2e 100644 --- a/coregrind/m_errormgr.c +++ b/coregrind/m_errormgr.c @@ -34,6 +34,7 @@ #include "pub_core_threadstate.h" // For VG_N_THREADS #include "pub_core_debugger.h" #include "pub_core_debuginfo.h" +#include "pub_core_debuglog.h" #include "pub_core_errormgr.h" #include "pub_core_execontext.h" #include "pub_core_gdbserver.h" @@ -50,6 +51,8 @@ #include "pub_core_translate.h" // for VG_(translate)() #include "pub_core_xarray.h" // VG_(xaprintf) et al +#define DEBUG_ERRORMGR 0 // set to 1 for heavyweight tracing + /*------------------------------------------------------------*/ /*--- Globals ---*/ /*------------------------------------------------------------*/ @@ -1541,10 +1544,47 @@ typedef } IPtoFunOrObjCompleter; -// free the memory in ip2fo. -static void clearIPtoFunOrObjCompleter - (IPtoFunOrObjCompleter* ip2fo) +static void pp_ip2fo (IPtoFunOrObjCompleter* ip2fo) { + Int i, j; + Int o; + + VG_(printf)("n_ips %lu n_ips_expanded %lu resulting in n_expanded %lu\n", + ip2fo->n_ips, ip2fo->n_ips_expanded, ip2fo->n_expanded); + for (i = 0; i < ip2fo->n_ips_expanded; i++) { + o = 0; + for (j = 0; j < i; j++) + o += ip2fo->n_offsets_per_ip[j]; + VG_(printf)("ips %d 0x08%lx offset [%d,%d] ", + i, ip2fo->ips[i], + o, o+ip2fo->n_offsets_per_ip[i]-1); + for (j = 0; j < ip2fo->n_offsets_per_ip[i]; j++) { + VG_(printf)("%sfun:%s obj:%s\n", + j == 0 ? "" : " ", + ip2fo->fun_offsets[o+j] == -1 ? + "" : &ip2fo->names[ip2fo->fun_offsets[o+j]], + ip2fo->obj_offsets[o+j] == -1 ? + "" : &ip2fo->names[ip2fo->obj_offsets[o+j]]); + } + } +} + +/* free the memory in ip2fo. + At debuglog 4, su (or NULL) will be used to show the matching (or non matching) + with ip2fo. */ +static void clearIPtoFunOrObjCompleter ( Supp *su, IPtoFunOrObjCompleter* ip2fo) +{ + if (DEBUG_ERRORMGR || VG_(debugLog_getLevel)() >= 4) { + if (su) + VG_(dmsg)("errormgr matching end suppression %s %s:%d matched:\n", + su->sname, + VG_(clo_suppressions)[su->clo_suppressions_i], + su->sname_lineno); + else + VG_(dmsg)("errormgr matching end no suppression matched:\n"); + VG_(pp_StackTrace) (ip2fo->ips, ip2fo->n_ips); + pp_ip2fo(ip2fo); + } if (ip2fo->n_offsets_per_ip) VG_(free)(ip2fo->n_offsets_per_ip); if (ip2fo->fun_offsets) VG_(free)(ip2fo->fun_offsets); if (ip2fo->obj_offsets) VG_(free)(ip2fo->obj_offsets); @@ -1589,6 +1629,9 @@ static HChar* foComplete(IPtoFunOrObjCompleter* ip2fo, if ((*offsets)[ixInput] == -1) { HChar* caller_name = grow_names(ip2fo); (*offsets)[ixInput] = ip2fo->names_free; + if (DEBUG_ERRORMGR) VG_(printf)("marking %s ixInput %d offset %d\n", + needFun ? "fun" : "obj", + ixInput, ip2fo->names_free); if (needFun) { // With inline info, fn names must have been completed already. vg_assert (!VG_(clo_read_inline_info)); @@ -1613,7 +1656,7 @@ static HChar* foComplete(IPtoFunOrObjCompleter* ip2fo, /* First get the pos in ips corresponding to ixInput */ for (pos_ips = 0; pos_ips < ip2fo->n_expanded; pos_ips++) { last_expand_pos_ips += ip2fo->n_offsets_per_ip[pos_ips]; - if (ixInput <= last_expand_pos_ips) + if (ixInput < last_expand_pos_ips) break; } /* pos_ips is the position in ips corresponding to ixInput. @@ -1624,12 +1667,16 @@ static HChar* foComplete(IPtoFunOrObjCompleter* ip2fo, VG_(strcpy)(caller_name, "???"); // Have all inlined calls pointing at this object name - for (i = last_expand_pos_ips - ip2fo->n_offsets_per_ip[pos_ips] - 1; - i <= last_expand_pos_ips; - i++) + for (i = last_expand_pos_ips - ip2fo->n_offsets_per_ip[pos_ips] + 1; + i < last_expand_pos_ips; + i++) { ip2fo->obj_offsets[i] = ip2fo->names_free; + if (DEBUG_ERRORMGR) + VG_(printf) (" set obj_offset %lu to %d\n", i, ip2fo->names_free); + } } ip2fo->names_free += VG_(strlen)(caller_name) + 1; + if (DEBUG_ERRORMGR) pp_ip2fo(ip2fo); } return ip2fo->names + (*offsets)[ixInput]; @@ -1729,6 +1776,7 @@ static Bool supp_pattEQinp ( const void* supplocV, const void* addrV, const SuppLoc* supploc = supplocV; /* PATTERN */ IPtoFunOrObjCompleter* ip2fo = inputCompleter; HChar* funobj_name; // Fun or Obj name. + Bool ret; expandInput(ip2fo, ixInput); vg_assert(ixInput < ip2fo->n_expanded); @@ -1757,9 +1805,15 @@ static Bool supp_pattEQinp ( const void* supplocV, const void* addrV, supploc->name. Hence (and leading to a re-entrant call of VG_(generic_match) if there is a wildcard character): */ if (supploc->name_is_simple_str) - return VG_(strcmp) (supploc->name, funobj_name) == 0; + ret = VG_(strcmp) (supploc->name, funobj_name) == 0; else - return VG_(string_match)(supploc->name, funobj_name); + ret = VG_(string_match)(supploc->name, funobj_name); + if (DEBUG_ERRORMGR) + VG_(printf) ("supp_pattEQinp %s patt %s ixUnput %lu value:%s match:%s\n", + supploc->ty == FunName ? "fun" : "obj", + supploc->name, ixInput, funobj_name, + ret ? "yes" : "no"); + return ret; } ///////////////////////////////////////////////////// @@ -1774,6 +1828,11 @@ static Bool supp_matches_callers(IPtoFunOrObjCompleter* ip2fo, Supp* su) UWord n_supps = su->n_callers; UWord szbPatt = sizeof(SuppLoc); Bool matchAll = False; /* we just want to match a prefix */ + if (DEBUG_ERRORMGR) + VG_(dmsg)(" errormgr Checking match with %s %s:%d\n", + su->sname, + VG_(clo_suppressions)[su->clo_suppressions_i], + su->sname_lineno); return VG_(generic_match)( matchAll, @@ -1851,6 +1910,8 @@ static Supp* is_suppressible_error ( Error* err ) ip2fo.names_free = 0; /* See if the error context matches any suppression. */ + if (DEBUG_ERRORMGR || VG_(debugLog_getLevel)() >= 4) + VG_(dmsg)("errormgr matching begin\n"); su_prev = NULL; for (su = suppressions; su != NULL; su = su->next) { em_supplist_cmps++; @@ -1867,12 +1928,12 @@ static Supp* is_suppressible_error ( Error* err ) su->next = suppressions; suppressions = su; } - clearIPtoFunOrObjCompleter(&ip2fo); + clearIPtoFunOrObjCompleter(su, &ip2fo); return su; } su_prev = su; } - clearIPtoFunOrObjCompleter(&ip2fo); + clearIPtoFunOrObjCompleter(NULL, &ip2fo); return NULL; /* no matches */ } diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am index 4e52ee0e9..0230c9b48 100644 --- a/memcheck/tests/Makefile.am +++ b/memcheck/tests/Makefile.am @@ -235,6 +235,8 @@ EXTRA_DIST = \ supp.supp \ suppfree.stderr.exp suppfree.supp suppfree.vgtest \ suppfreecollision.stderr.exp suppfreecollision.supp suppfreecollision.vgtest \ + supponlyobj.stderr.exp supponlyobj.supp supponlyobj.vgtest \ + suppvarinfo5.stderr.exp suppvarinfo5.supp suppvarinfo5.vgtest \ test-plo-no.vgtest test-plo-no.stdout.exp \ test-plo-no.stderr.exp-le64 test-plo-no.stderr.exp-le32 \ test-plo-yes.vgtest test-plo-yes.stdout.exp \ diff --git a/memcheck/tests/supponlyobj.stderr.exp b/memcheck/tests/supponlyobj.stderr.exp new file mode 100644 index 000000000..c0951cd3b --- /dev/null +++ b/memcheck/tests/supponlyobj.stderr.exp @@ -0,0 +1,6 @@ +Conditional jump or move depends on uninitialised value(s) + at 0x........: main (inlinfo.c:7) + +Conditional jump or move depends on uninitialised value(s) + at 0x........: main (inlinfo.c:7) + diff --git a/memcheck/tests/supponlyobj.supp b/memcheck/tests/supponlyobj.supp new file mode 100755 index 000000000..c31bad673 --- /dev/null +++ b/memcheck/tests/supponlyobj.supp @@ -0,0 +1,16 @@ +{ + obj5 + Memcheck:Cond + obj:*inlinfo + obj:*inlinfo + obj:*inlinfo + obj:*inlinfo + obj:*inlinfo +} +{ + obj2 + Memcheck:Cond + obj:*inlinfo + obj:*inlinfo +} + diff --git a/memcheck/tests/supponlyobj.vgtest b/memcheck/tests/supponlyobj.vgtest new file mode 100644 index 000000000..bead116ce --- /dev/null +++ b/memcheck/tests/supponlyobj.vgtest @@ -0,0 +1,4 @@ +# test suppressions with only obj: markers +prog: inlinfo +vgopts: -q --read-inline-info=no --suppressions=supponlyobj.supp +stderr_filter_args: inlinfo.c diff --git a/memcheck/tests/suppvarinfo5.stderr.exp b/memcheck/tests/suppvarinfo5.stderr.exp new file mode 100644 index 000000000..b7089fd1c --- /dev/null +++ b/memcheck/tests/suppvarinfo5.stderr.exp @@ -0,0 +1 @@ +answer is 0 diff --git a/memcheck/tests/suppvarinfo5.supp b/memcheck/tests/suppvarinfo5.supp new file mode 100644 index 000000000..3ccbd52ac --- /dev/null +++ b/memcheck/tests/suppvarinfo5.supp @@ -0,0 +1,29 @@ +{ + obj4 + Memcheck:User + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5 +} + +{ + obj5 + Memcheck:User + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5 +} + +{ + obj6 + Memcheck:User + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:* + obj:*varinfo5so.so + obj:*varinfo5so.so + obj:*varinfo5 +} diff --git a/memcheck/tests/suppvarinfo5.vgtest b/memcheck/tests/suppvarinfo5.vgtest new file mode 100644 index 000000000..6c1e58e5a --- /dev/null +++ b/memcheck/tests/suppvarinfo5.vgtest @@ -0,0 +1,3 @@ +prog: varinfo5 +vgopts: -q --suppressions=suppvarinfo5.supp +