From ec8e1458503447c9917de99e92c2c1135edee4c6 Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Sat, 26 Apr 2014 21:50:57 +0000 Subject: [PATCH] Add a new test that shows a surprising side effect of the medium resolution (4 callers) used to compare errors. To look at the strange side effect, do: ./vg-in-place -v --suppressions=memcheck/tests/suppfreecollision.supp memcheck/tests/suppfree activatenondangerouserror You obtain at the end: ... --19240-- used_suppression: 2 suppressnondangerouserror memcheck/tests/suppfreecollision.supp:2 ... showing that the suppression aiming at suppressing a nondangerous error has in fact suppressed more than expected. This is because m_errormgr.c compares the exe_context in medium resolution/4 calls (or low resolution/2 calls once 100 errors have been collected). The error machinery first encounters the non dangerous error. This error is suppressed, because all callers match the suppression entry. In particular, we have in the stacktrace the function ok_to_suppress_double_free_from_this_fun Then the error machinery encounters the second error. The stacktrace of the 2nd error has the same first 4 callers than the non dangerous error. So the 2nd error is considered equal to the first one and is (unexpectedly in my opinion) suppressed. This looks a bug (or at least something very surprising). (the doc mentions the fact that errors are 'commoned up' on 4 callers, but I am not sure the above side effect was understood). There are several ways this can be improved, some are more easier than other * have --error-resolution=low/med/high similar to the memcheck --leak-resolution=low/med/high (which default value would we take for this new clo ?) * have a lot more intelligent error comparison: when comparing an error with a suppressed error, one must check that the callers used for suppression are equal. This looks difficult to implement and probably a significant slow down in the error machinery, which will impact applications producing many suppressed errors (e.g. helgrind + some pthread lib errors). This also implies more memory (e.g. one byte per caller in the error, to indicate which caller(s) were used to suppress. Still wondering what to do with * and ... ? * have a somewhat more intelligent error comparison: Instead of comparing only the callers used for suppression, we compare the range first..last caller used (so including some callers in the range that were not used to suppressed if e.g. a ... matching was put in the supp entry). Probably still a slowdown (less than previous solution ?) and less memory than the previous solution. But also not completely clear how to compute the range. * always re-evaluate the suppression : this will very probably be a significant slow down. * do nothing, as nobody complained about this behaviour up to now :) * ??? any other idea git-svn-id: svn://svn.valgrind.org/valgrind/trunk@13914 --- memcheck/tests/Makefile.am | 1 + memcheck/tests/suppfree.c | 10 +++++++++- memcheck/tests/suppfree.stderr.exp | 4 ++-- memcheck/tests/suppfreecollision.stderr.exp | 0 memcheck/tests/suppfreecollision.supp | 11 +++++++++++ memcheck/tests/suppfreecollision.vgtest | 13 +++++++++++++ 6 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 memcheck/tests/suppfreecollision.stderr.exp create mode 100644 memcheck/tests/suppfreecollision.supp create mode 100644 memcheck/tests/suppfreecollision.vgtest diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am index 6ce9a8875..09ce552ad 100644 --- a/memcheck/tests/Makefile.am +++ b/memcheck/tests/Makefile.am @@ -230,6 +230,7 @@ EXTRA_DIST = \ supp2.stderr.exp supp2.vgtest \ supp.supp \ suppfree.stderr.exp suppfree.supp suppfree.vgtest \ + suppfreecollision.stderr.exp suppfreecollision.supp suppfreecollision.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/suppfree.c b/memcheck/tests/suppfree.c index 8298f021a..1173f1bb5 100644 --- a/memcheck/tests/suppfree.c +++ b/memcheck/tests/suppfree.c @@ -22,9 +22,17 @@ void aaa (char* x) bbb(x); } -int main ( void ) +void ok_to_suppress_double_free_from_this_fun(char* y) +{ + aaa(y); +} + +int main ( int argc, char*argv[] ) { char* x = malloc(10); + char* y = malloc(10); + if (argc > 1) + ok_to_suppress_double_free_from_this_fun(y); aaa(x); return 0; } diff --git a/memcheck/tests/suppfree.stderr.exp b/memcheck/tests/suppfree.stderr.exp index e29d17bfd..62f5cccc7 100644 --- a/memcheck/tests/suppfree.stderr.exp +++ b/memcheck/tests/suppfree.stderr.exp @@ -4,12 +4,12 @@ Invalid free() / delete / delete[] / realloc() by 0x........: ccc (suppfree.c:12) by 0x........: bbb (suppfree.c:17) by 0x........: aaa (suppfree.c:22) - by 0x........: main (suppfree.c:28) + by 0x........: main (suppfree.c:36) Address 0x........ is 0 bytes inside a block of size 10 free'd at 0x........: free (vg_replace_malloc.c:...) by 0x........: ddd (suppfree.c:6) by 0x........: ccc (suppfree.c:12) by 0x........: bbb (suppfree.c:17) by 0x........: aaa (suppfree.c:22) - by 0x........: main (suppfree.c:28) + by 0x........: main (suppfree.c:36) diff --git a/memcheck/tests/suppfreecollision.stderr.exp b/memcheck/tests/suppfreecollision.stderr.exp new file mode 100644 index 000000000..e69de29bb diff --git a/memcheck/tests/suppfreecollision.supp b/memcheck/tests/suppfreecollision.supp new file mode 100644 index 000000000..414e8ae02 --- /dev/null +++ b/memcheck/tests/suppfreecollision.supp @@ -0,0 +1,11 @@ +{ + suppressnondangerouserror + Memcheck:Free + fun:free + fun:ddd + fun:ccc + fun:bbb + fun:aaa + fun:ok_to_suppress_double_free_from_this_fun + fun:main +} diff --git a/memcheck/tests/suppfreecollision.vgtest b/memcheck/tests/suppfreecollision.vgtest new file mode 100644 index 000000000..cda50aa7e --- /dev/null +++ b/memcheck/tests/suppfreecollision.vgtest @@ -0,0 +1,13 @@ +# this test the case of two errors, one considered not dangerous and +# suppressed, the other considered dangerous, and the user does +# not want to supress it. +# The suppression entry only match the non dangerous error. +# However, when a medium resolution is used to compare 2 errors, +# only the last 4 calls are used to determine that two errors are similar +# So, the nondangerous suppressed error "absorbs and hides" the dangerous +# error. +# This side effect of a medium (or low) resolution for error matching +# is I guess unexpected by most users. +prog: suppfree +args: activatenondangerouserror +vgopts: --suppressions=suppfreecollision.supp -q