From: Mark Wielaard Date: Fri, 19 Apr 2024 20:46:11 +0000 (+0200) Subject: core errors suppression support X-Git-Tag: VALGRIND_3_23_0~26 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d68d584cead390c339b9575c5c9679e8ce50c37f;p=thirdparty%2Fvalgrind.git core errors suppression support Add two new functions core_get_extra_suppression_info and core_get_error_name as alternatives for tool suppression callbacks. These functions are used in gen_suppression. Instead of a tool name, a core error component name is "CoreError". Two new suppression kinds FdBadCloseSupp and FdNotClosedSupp were added. Corresponding to the FdBadClose and FdNotClosed error kinds. core_error_matches_suppression matches these suppression kinds with error kinds. core_get_extra_suppression_info and core_print_extra_suppression_use are noops for core errors. is_suppressible_error, supp_matches_error, load_one_suppressions_file and show_used_suppressions have been adjusted to work with core error kinds. A new function VG_(found_or_suppressed_errs) helps to not output an empty error summary if only core errors are requested, but no errors were detected. VG_(clo_track_fds) has been moved from pub_core_options.h to pub_tool_options.h. And VG_(needs_core_errors) now takes a Bool that can be set to false in the tool post_clo_init handler. This is used in the none tool to request core errors, but disable all reporting if no option has been given that enables such errors. This make sure only the none/tests/fdleak_ipv4.stderr.exp needs adjustment. For all other tests the output is exactly as before. https://bugs.kde.org/show_bug.cgi?id=485778 --- diff --git a/NEWS b/NEWS index 954dae3fe..250602fa5 100644 --- a/NEWS +++ b/NEWS @@ -98,6 +98,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 485148 vfmadd213ss instruction is instrumented incorrectly (the remaining part of the register is cleared instead of kept unmodified) 485487 glibc built with -march=x86-64-v3 does not work due to ld.so strcmp +485778 Crash with --track-fds=all and --gen-suppressions=all n-i-bz Add redirect for memccpy To see details of a given bug, visit diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c index d3004c316..4984d30ab 100644 --- a/coregrind/m_errormgr.c +++ b/coregrind/m_errormgr.c @@ -101,6 +101,9 @@ static void core_before_pp_Error (const Error*); static void core_pp_Error (const Error*); static UInt core_update_extra (const Error*); +static const HChar *core_get_error_name(const Error*); +static SizeT core_get_extra_suppression_info(const Error*,HChar*,Int); + static ThreadId last_tid_printed = 1; /* Stats: number of searches of the error list initiated. */ @@ -185,6 +188,11 @@ UInt VG_(get_n_errs_shown)( void ) return n_errs_shown; } +Bool VG_(found_or_suppressed_errs)( void ) +{ + return errors != NULL; +} + /*------------------------------------------------------------*/ /*--- Suppression type ---*/ /*------------------------------------------------------------*/ @@ -197,6 +205,8 @@ typedef // could detect them. This example is left commented-out as an // example should new core errors ever be added. ThreadSupp = -1, /* Matches ThreadErr */ + FdBadCloseSupp = -2, + FdNotClosedSupp = -3 } CoreSuppKind; @@ -365,6 +375,7 @@ static void printSuppForIp_nonXML(UInt n, DiEpoch ep, Addr ip, void* textV) static void gen_suppression(const Error* err) { const HChar* name; + const HChar* component; ExeContext* ec; XArray* /* HChar */ text; @@ -373,7 +384,13 @@ static void gen_suppression(const Error* err) vg_assert(err); ec = VG_(get_error_where)(err); - vg_assert(ec); // XXX + if (ec == NULL) { + /* This can happen with core errors for --track-fds=all + with "leaked" inherited file descriptors, which aren't + created in the current program. */ + VG_(umsg)("(No origin, error cannot be suppressed)\n"); + return; + } if (err->ekind >= 0) { name = VG_TDICT_CALL(tool_get_error_name, err); @@ -382,6 +399,12 @@ static void gen_suppression(const Error* err) VG_(details).name); return; } + } else { + name = core_get_error_name(err); + if (NULL == name) { + VG_(umsg)("(core error cannot be suppressed)\n"); + return; + } } /* In XML mode, we also need to print the plain text version of the @@ -392,9 +415,13 @@ static void gen_suppression(const Error* err) VG_(free), sizeof(HChar) ); /* Ok. Generate the plain text version into TEXT. */ + if (err->ekind >= 0) + component = VG_(details).name; + else + component = "CoreError"; VG_(xaprintf)(text, "{\n"); VG_(xaprintf)(text, " <%s>\n", dummy_name); - VG_(xaprintf)(text, " %s:%s\n", VG_(details).name, name); + VG_(xaprintf)(text, " %s:%s\n", component, name); HChar *xtra = NULL; SizeT xtra_size = 0; @@ -403,8 +430,11 @@ static void gen_suppression(const Error* err) do { xtra_size += 256; xtra = VG_(realloc)("errormgr.gen_suppression.2", xtra,xtra_size); - num_written = VG_TDICT_CALL(tool_get_extra_suppression_info, - err, xtra, xtra_size); + if (err->ekind >= 0) + num_written = VG_TDICT_CALL(tool_get_extra_suppression_info, + err, xtra, xtra_size); + else + num_written = core_get_extra_suppression_info(err, xtra, xtra_size); } while (num_written == xtra_size); // resize buffer and retry // Ensure buffer is properly terminated @@ -441,7 +471,7 @@ static void gen_suppression(const Error* err) VG_(printf_xml)(" \n"); VG_(printf_xml)(" %s\n", dummy_name); VG_(printf_xml)( - " %pS:%pS\n", VG_(details).name, name); + " %pS:%pS\n", component, name); if (num_written) VG_(printf_xml)(" %pS\n", xtra); @@ -980,6 +1010,49 @@ static UInt core_update_extra (const Error *err) } } +static const HChar *core_get_error_name(const Error *err) +{ + switch (err->ekind) { + case FdBadClose: + return "FdBadClose"; + case FdNotClosed: + return "FdNotClosed"; + default: + VG_(umsg)("FATAL: unknown core error kind: %d\n", err->ekind ); + VG_(exit)(1); + } +} + +static Bool core_error_matches_suppression(const Error* err, const Supp* su) +{ + switch (su->skind) { + case FdBadCloseSupp: + return err->ekind == FdBadClose; + case FdNotClosedSupp: + return err->ekind == FdNotClosed; + default: + VG_(umsg)("FATAL: unknown core suppression kind: %d\n", su->skind ); + VG_(exit)(1); + } +} + +static SizeT core_get_extra_suppression_info(const Error *err, + HChar* buf, Int nBuf) +{ + /* No core error has any extra suppression info at the moment. */ + buf[0] = '\0'; + return 0; +} + +static SizeT core_print_extra_suppression_use(const Supp* su, + HChar* buf, Int nBuf) +{ + /* No core error has any extra suppression info at the moment. */ + buf[0] = '\0'; + return 0; +} + + /*------------------------------------------------------------*/ /*--- Exported fns ---*/ /*------------------------------------------------------------*/ @@ -1015,8 +1088,12 @@ static Bool show_used_suppressions ( void ) do { xtra_size += 256; xtra = VG_(realloc)("errormgr.sus.1", xtra, xtra_size); - num_written = VG_TDICT_CALL(tool_print_extra_suppression_use, - su, xtra, xtra_size); + if (su->skind >= 0) + num_written = VG_TDICT_CALL(tool_print_extra_suppression_use, + su, xtra, xtra_size); + else + num_written = core_print_extra_suppression_use(su, + xtra, xtra_size); } while (num_written == xtra_size); // resize buffer and retry // Ensure buffer is properly terminated @@ -1433,12 +1510,14 @@ static void load_one_suppressions_file ( Int clo_suppressions_i ) tool_names = & buf[0]; supp_name = & buf[i+1]; - if (VG_(needs).core_errors && tool_name_present("core", tool_names)) { + if (VG_(needs).core_errors + && tool_name_present("CoreError", tool_names)) { // A core suppression - //(example code, see comment on CoreSuppKind above) - //if (VG_STREQ(supp_name, "Thread")) - // supp->skind = ThreadSupp; - //else + if (VG_STREQ(supp_name, "FdBadClose")) + supp->skind = FdBadCloseSupp; + else if (VG_STREQ(supp_name, "FdNotClosed")) + supp->skind = FdNotClosedSupp; + else BOMB("unknown core suppression type"); } else if (VG_(needs).tool_errors @@ -1472,6 +1551,8 @@ static void load_one_suppressions_file ( Int clo_suppressions_i ) BOMB("bad or missing extra suppression info"); } + // No core errors need to read extra suppression info + got_a_location_line_read_by_tool = buf[0] != 0 && is_location_line(buf); /* the main frame-descriptor reading loop */ @@ -2010,20 +2091,18 @@ static Bool supp_matches_callers(IPtoFunOrObjCompleter* ip2fo, static Bool supp_matches_error(const Supp* su, const Error* err) { - switch (su->skind) { - //(example code, see comment on CoreSuppKind above) - //case ThreadSupp: - // return (err->ekind == ThreadErr); - default: - if (VG_(needs).tool_errors) { - return VG_TDICT_CALL(tool_error_matches_suppression, err, su); - } else { - VG_(printf)( - "\nUnhandled suppression type: %u. VG_(needs).tool_errors\n" - "probably needs to be set.\n", - (UInt)err->ekind); - VG_(core_panic)("unhandled suppression type"); - } + if (su->skind >= 0) { + if (VG_(needs).tool_errors) { + return VG_TDICT_CALL(tool_error_matches_suppression, err, su); + } else { + VG_(printf)( + "\nUnhandled suppression type: %u. VG_(needs).tool_errors\n" + "probably needs to be set.\n", + (UInt)err->ekind); + VG_(core_panic)("unhandled suppression type"); + } + } else { + return core_error_matches_suppression(err, su); } } @@ -2083,7 +2162,9 @@ static Supp* is_suppressible_error ( const Error* err ) && supp_matches_callers(&ip2fo, su)) { /* got a match. */ /* Inform the tool that err is suppressed by su. */ - (void)VG_TDICT_CALL(tool_update_extra_suppression_use, err, su); + if (su->skind >= 0) + (void)VG_TDICT_CALL(tool_update_extra_suppression_use, err, su); + /* No core errors need to update extra suppression info */ /* Move this entry to the head of the list in the hope of making future searches cheaper. */ if (su_prev) { diff --git a/coregrind/m_main.c b/coregrind/m_main.c index ac9e3f76b..f3ad6c569 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -2321,7 +2321,8 @@ void shutdown_actions_NORETURN( ThreadId tid, the error management machinery. */ VG_TDICT_CALL(tool_fini, 0/*exitcode*/); - if (VG_(needs).core_errors || VG_(needs).tool_errors) { + if ((VG_(needs).core_errors && VG_(found_or_suppressed_errs)()) + || VG_(needs).tool_errors) { if (VG_(clo_verbosity) == 1 && !VG_(clo_xml) && !VG_(clo_show_error_list)) diff --git a/coregrind/m_tooliface.c b/coregrind/m_tooliface.c index de16ebfe8..58aa82ccf 100644 --- a/coregrind/m_tooliface.c +++ b/coregrind/m_tooliface.c @@ -221,9 +221,13 @@ Bool VG_(finish_needs_init)(const HChar** failmsg) // These ones don't require any tool-supplied functions NEEDS(libc_freeres) NEEDS(cxx_freeres) -NEEDS(core_errors) NEEDS(var_info) +void VG_(needs_core_errors)( Bool need ) +{ + VG_(needs).core_errors = need; +} + void VG_(needs_superblock_discards)( void (*discard)(Addr, VexGuestExtents) ) diff --git a/coregrind/pub_core_errormgr.h b/coregrind/pub_core_errormgr.h index 9133a119d..e365125ba 100644 --- a/coregrind/pub_core_errormgr.h +++ b/coregrind/pub_core_errormgr.h @@ -82,6 +82,8 @@ extern Bool VG_(showing_core_errors) ( void ); extern UInt VG_(get_n_errs_found) ( void ); extern UInt VG_(get_n_errs_shown) ( void ); +extern Bool VG_(found_or_suppressed_errs) ( void ); + extern void VG_(print_errormgr_stats) ( void ); #endif // __PUB_CORE_ERRORMGR_H diff --git a/coregrind/pub_core_options.h b/coregrind/pub_core_options.h index 97cc741aa..d8bc4736d 100644 --- a/coregrind/pub_core_options.h +++ b/coregrind/pub_core_options.h @@ -297,9 +297,6 @@ extern const HChar* VG_(clo_prefix_to_strip); wildcards. */ extern XArray *VG_(clo_req_tsyms); -/* Track open file descriptors? 0 = No, 1 = Yes, 2 = All (including std) */ -extern UInt VG_(clo_track_fds); - /* Should we run __libc_freeres at exit? Sometimes causes crashes. Default: YES. Note this is subservient to VG_(needs).libc_freeres; if the latter says False, then the setting of VG_(clo_run_libc_freeres) diff --git a/drd/drd_error.c b/drd/drd_error.c index c3f98bcb3..d7928f9b7 100644 --- a/drd/drd_error.c +++ b/drd/drd_error.c @@ -629,6 +629,7 @@ void drd_update_extra_suppresion_use(const Error* e, const Supp* supp) /** Tell the Valgrind core about DRD's error handlers. */ void DRD_(register_error_handlers)(void) { + VG_(needs_core_errors)(True); VG_(needs_tool_errors)(drd_compare_error_contexts, drd_tool_error_before_pp, drd_tool_error_pp, diff --git a/helgrind/hg_main.c b/helgrind/hg_main.c index 3ac5d6bc8..f292d2329 100644 --- a/helgrind/hg_main.c +++ b/helgrind/hg_main.c @@ -6063,7 +6063,7 @@ static void hg_pre_clo_init ( void ) hg_instrument, hg_fini); - VG_(needs_core_errors) (); + VG_(needs_core_errors) (True); VG_(needs_tool_errors) (HG_(eq_Error), HG_(before_pp_Error), HG_(pp_Error), diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h index 972e7546a..32345dc6a 100644 --- a/include/pub_tool_options.h +++ b/include/pub_tool_options.h @@ -416,6 +416,9 @@ extern Bool VG_(clo_show_below_main); allocated e.g. leaks of or accesses just outside a block. */ extern Bool VG_(clo_keep_debuginfo); +/* Track open file descriptors? 0 = No, 1 = Yes, 2 = All (including std) */ +extern UInt VG_(clo_track_fds); + /* Used to expand file names. "option_name" is the option name, eg. "--log-file". 'format' is what follows, eg. "cachegrind.out.%p". In diff --git a/include/pub_tool_tooliface.h b/include/pub_tool_tooliface.h index 6031d1328..6b361ef8b 100644 --- a/include/pub_tool_tooliface.h +++ b/include/pub_tool_tooliface.h @@ -270,7 +270,7 @@ extern void VG_(needs_cxx_freeres) ( void ); - invalid file descriptors to syscalls like read() and write() - bad signal numbers passed to sigaction() - attempt to install signal handler for SIGKILL or SIGSTOP */ -extern void VG_(needs_core_errors) ( void ); +extern void VG_(needs_core_errors) ( Bool need ); /* Booleans that indicate extra operations are defined; if these are True, the corresponding template functions (given below) must be defined. A diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c index ba8ff34c5..abd5d6888 100644 --- a/memcheck/mc_main.c +++ b/memcheck/mc_main.c @@ -8555,7 +8555,7 @@ static void mc_pre_clo_init(void) VG_(details_version) (NULL); VG_(details_description) ("a memory error detector"); VG_(details_copyright_author)( - "Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al."); + "Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al."); VG_(details_bug_reports_to) (VG_BUGS_TO); VG_(details_avg_translation_sizeB) ( 640 ); @@ -8566,7 +8566,7 @@ static void mc_pre_clo_init(void) VG_(needs_final_IR_tidy_pass) ( MC_(final_tidy) ); - VG_(needs_core_errors) (); + VG_(needs_core_errors) (True); VG_(needs_tool_errors) (MC_(eq_Error), MC_(before_pp_Error), MC_(pp_Error), diff --git a/none/nl_main.c b/none/nl_main.c index c4780a6f6..4ea1410d4 100644 --- a/none/nl_main.c +++ b/none/nl_main.c @@ -28,9 +28,14 @@ #include "pub_tool_basics.h" #include "pub_tool_tooliface.h" +#include "pub_tool_options.h" static void nl_post_clo_init(void) { + /* Unless we are actually tracking file descriptors we act as if we don't + handle any errors. */ + if (!VG_(clo_track_fds)) + VG_(needs_core_errors)(False); } static @@ -63,6 +68,7 @@ static void nl_pre_clo_init(void) nl_instrument, nl_fini); VG_(needs_xml_output) (); + VG_(needs_core_errors) (True); /* Yes, but... see nl_post_clo_init */ /* No needs, no core events to track */ } diff --git a/none/tests/fdleak_ipv4.stderr.exp b/none/tests/fdleak_ipv4.stderr.exp index 2ca7c291e..c7493fab9 100644 --- a/none/tests/fdleak_ipv4.stderr.exp +++ b/none/tests/fdleak_ipv4.stderr.exp @@ -26,3 +26,5 @@ Open AF_INET socket 3: 127.0.0.1:... <-> 127.0.0.1:... ... +For lists of detected and suppressed errors, rerun with: -s +ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)