Purify allows "..." in suppression stack traces meaning an arbitrary number of stack frames. That is extremely useful when writing suppressions that have to work across different library versions, compilers and compilation options where the compilers unfolding decisions are interfering. For example, I have... { Cond/deflate_cairo_debian Memcheck:Cond fun:* fun:deflate fun:compress2 fun:compress fun:* fun:* fun:* fun:* fun:* fun:* fun:cairo_surface_finish } { Cond/deflate_cairo_opensuse Memcheck:Cond fun:* fun:* fun:deflate fun:compress2 fun:compress fun:* fun:* fun:* fun:* fun:* fun:* fun:cairo_surface_finish } { Cond/deflate3_cairo_opensuse Memcheck:Cond fun:* fun:* fun:deflate fun:compress2 fun:compress_dup fun:* fun:* fun:* fun:* fun:* fun:cairo_surface_finish } and more of the same. I want that to become { deflate_cairo Memcheck:Cond fun:... fun:deflate fun:... fun:cairo_surface_finish } or even { deflate_cairo Memcheck:Cond fun:...; deflate; ...; cairo_surface_finish }
I also like this idea, and made a small patch, which causes "fun:..." match an arbitrary number of callers. Please check if it's worth applying. [It is against the 3.3.1 release tarball, as I cannot check out the svn head now.]
Created attachment 27165 [details] extends suppression matching to allow for arbitrary number of callers
This looks like quite a clean patch, which is good. Did you test it carefully, eg for "..." as the first entry, "..." as the last entry, "..." as the only entry, two adjacent entries with "...", no entries at all (is that possible?), etc? I am unhappy about the use of recursion in supp_wildmatch_callers. There could be up to VG_DEEPEST_BACKTRACE levels of recursion (correct?) and each one requires at least ERRTXT_LEN bytes of stack space, which adds up to quite a lot, when you consider that Valgrind itself runs on a very small stack (64KB) and we have had problems with stack overflows in the past. Can you either avoid the recursion, or remove caller_name[] from the frame?
Created attachment 28230 [details] revised patch (In reply to comment #3) > This looks like quite a clean patch, which is good. Did you test > it carefully, eg for "..." as the first entry, "..." as the last > entry, "..." as the only entry, two adjacent entries with "...", > no entries at all (is that possible?), etc? I admit I didn't back then, but now I have, and fixed a few issues. Attaching a revised patch and a set of tests I've run. > I am unhappy about the use of recursion in > supp_wildmatch_callers. There could be up to > VG_DEEPEST_BACKTRACE levels of recursion (correct?) and each one > requires at least ERRTXT_LEN bytes of stack space, which adds up > to quite a lot, when you consider that Valgrind itself runs on > a very small stack (64KB) and we have had problems with stack > overflows in the past. Can you either avoid the recursion, or > remove caller_name[] from the frame? I've changed them to be 'static's. I think this works, as those variables are not passed to subsequent calls, and for this reason it doesn't matter if they get overwritten. Thanks for the feedback!
Created attachment 28231 [details] test cases the test cases
Thanks for the updated patch and test cases. I am part way though merging it into the trunk. One question, though. In the new code, the part that deals with wildcards, you have if (!VG_(get_fnname_Z_demangle_only)(trace[j], tmp, ERRTXT_LEN)) return False; whereas the existing code (in "case FunName" and "case ObjName") we have if (!VG_(get_fnname_Z_demangle_only)(a, caller_name, ERRTXT_LEN)) VG_(strcpy)(caller_name, "???"); Why is that? If the name of a function cannot be found, why should the new code return immediately with False, instead of continuing with "???" as the function name, like in the other 2 cases? ------- I will also add a check which rejects patterns of the form "fun:..." and no other frames, since that would match absolutely any call stack, and I don't think that's a good idea.
(In reply to comment #6) > One question, though. In the new code, the part that deals with > wildcards, you have > > if (!VG_(get_fnname_Z_demangle_only)(trace[j], tmp, ERRTXT_LEN)) > return False; > > whereas the existing code (in "case FunName" and "case ObjName") > we have > > if (!VG_(get_fnname_Z_demangle_only)(a, caller_name, ERRTXT_LEN)) > VG_(strcpy)(caller_name, "???"); > > Why is that? If the name of a function cannot be found, why should > the new code return immediately with False, instead of continuing > with "???" as the function name, like in the other 2 cases? Your point seems valid. Then that 'if' can be 'and'-ed with the following one (string_match). And then, I presume the code will step over functions for which no name is available. > I will also add a check which rejects patterns of the form "fun:..." > and no other frames, since that would match absolutely any call > stack, and I don't think that's a good idea. Yes, that can cause surprise if unintentional.
Committed, with some modifications and extra stuff (r8725). Akos, thanks v. much for the patch. Note, the final syntax is slightly different from what you implemented: it is simply "..." and not "fun:...".