|
From: Eyal S. <eya...@gm...> - 2021-03-02 22:04:25
|
When I run Valgrind memcheck, I can specify which errors are not cause for returning the error exitcode but using the error-for-leak-kinds flag. But when --exit-on-first-error is set to yes, Valgrind will exit early with an error code even though it would not have caused an error had that option been absent. This will confuse users. It also makes --exit-on-first-error useless for integration tests. The patch fixes it. https://github.com/eyal0/valgrind/pull/1/commits/6f06799abd8d1e51764d86ee239fd7fce07374ea Eyal >From 6f06799abd8d1e51764d86ee239fd7fce07374ea Mon Sep 17 00:00:00 2001 From: eyal0 <109...@us...> Date: Tue, 2 Mar 2021 14:42:38 -0700 Subject: [PATCH] Only quit on the first error if that error is "counted". It's possible for the user to specify in the memcheck that some errors should not cause an exit with the error-exitcode value. For those errors which would not cause an error exitcode, do not count them for the purposes of exiting after the first error. --- coregrind/m_errormgr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c index 52505ba5b2..55c049cde1 100644 --- a/coregrind/m_errormgr.c +++ b/coregrind/m_errormgr.c @@ -518,7 +518,7 @@ Bool VG_(is_action_requested) ( const HChar* action, Bool* clo ) * possibly, generate a suppression. */ static -void do_actions_on_error(const Error* err, Bool allow_db_attach) +void do_actions_on_error(const Error* err, Bool allow_db_attach, Bool count_error) { Bool still_noisy = True; @@ -541,7 +541,7 @@ void do_actions_on_error(const Error* err, Bool allow_db_attach) if (VG_(clo_gen_suppressions) == 1 && !still_noisy) VG_(clo_gen_suppressions) = 0; - if (VG_(clo_exit_on_first_error)) { + if (count_error && VG_(clo_exit_on_first_error)) { if (VG_(clo_xml)) VG_(printf_xml)("</valgrindoutput>\n"); VG_(umsg)("\n"); @@ -579,7 +579,7 @@ void do_actions_on_error(const Error* err, Bool allow_db_attach) * calls do_actions_on_error. This optionally does a gdbserver call and optionally prints a suppression; both of these may require user input. */ -static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml ) +static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool count_error ) { /* If this fails, you probably specified your tool's method dictionary incorrectly. */ @@ -640,7 +640,7 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml ) } - do_actions_on_error(err, allow_db_attach); + do_actions_on_error(err, allow_db_attach, count_error); } @@ -847,7 +847,7 @@ void VG_(maybe_record_error) ( ThreadId tid, n_errs_found++; n_errs_shown++; /* Actually show the error; more complex than you might think. */ - pp_Error( p, /*allow_db_attach*/True, VG_(clo_xml) ); + pp_Error( p, /*allow_db_attach*/True, VG_(clo_xml), /* count_error */ True ); } else { n_supp_contexts++; n_errs_suppressed++; @@ -897,7 +897,7 @@ Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s, /* update stats */ n_errs_shown++; /* Actually show the error; more complex than you might think. */ - pp_Error(&err, allow_db_attach, VG_(clo_xml)); + pp_Error(&err, allow_db_attach, VG_(clo_xml), count_error); } return False; @@ -1023,7 +1023,7 @@ void VG_(show_all_errors) ( Int verbosity, Bool xml ) VG_(umsg)("\n"); VG_(umsg)("%d errors in context %d of %u:\n", p_min->count, i+1, n_err_contexts); - pp_Error( p_min, False/*allow_db_attach*/, False /* xml */ ); + pp_Error( p_min, False/*allow_db_attach*/, False /* xml */, True /* count_error */ ); // We're not printing XML -- we'd have exited above if so. vg_assert(! xml); @@ -1066,7 +1066,7 @@ void VG_(show_last_error) ( void ) return; } - pp_Error( errors, False/*allow_db_attach*/, False/*xml*/ ); + pp_Error( errors, False/*allow_db_attach*/, False/*xml*/, True/*count_error*/ ); } |
|
From: Mark W. <ma...@kl...> - 2021-03-03 13:54:28
|
Hi Eyal, BTW. I did see your other patch for bug #432801 but haven't had time to dive into that yet. The expensive compare stuff is slightly voodoo to me. I'll get to it eventually if I dare and nobody else beats me to it. This patch seems more easy to understand. I added Philippe to CC to see if he has an opinion on how this should/shouldn't interact with vgdb. On Tue, 2021-03-02 at 15:04 -0700, Eyal Soha wrote: > When I run Valgrind memcheck, I can specify which errors are not cause for > returning the error exitcode but using the error-for-leak-kinds flag. But > when --exit-on-first-error is set to yes, Valgrind will exit early with an > error code even though it would not have caused an error had that option > been absent. This will confuse users. It also makes --exit-on-first-error > useless for integration tests. > > The patch fixes it. > https://github.com/eyal0/valgrind/pull/1/commits/6f06799abd8d1e51764d86ee239fd7fce07374ea BTW. The patch as attached doesn't really apply, best to sent it with git send-email. Or create a bugzilla and attach it there. I did fix up the line lengths (see attached). So for this to work you pass count_error from VG_(unique_error) to pp_error, which passes it onto do_actions_on_error. The other pp_error errors are adjusted to always pass True for count_error. VG_(unique_error) is called by MC_(record_leak_error) which passes count_error to true only for errors-for-leak-kinds (and print_errors to true for show_leak_kinds). So I think you patch is correct. I just wonder if this should just impact clo_exit_on_first_error or also the other actions in do_actions_on_error. Does it make sense to invoke vgdb if the error doesn't count, or to generate a suppression? Maybe we should simply not call do_actions_on_error when count_error is false? Also in my attached patch, but I like to hear other opinions, maybe my assumption that a non-counted error is not interesting for vgdb (or suppression generation) is flawed. Cheers, Mark |
|
From: Eyal S. <eya...@gm...> - 2021-03-03 18:00:05
|
On Wed, Mar 3, 2021 at 6:54 AM Mark Wielaard <ma...@kl...> wrote: > Hi Eyal, > > BTW. I did see your other patch for bug #432801 but haven't had time to > dive into that yet. The expensive compare stuff is slightly voodoo to > me. I'll get to it eventually if I dare and nobody else beats me to it. > Sure, when you're ready. The bug report is rather long but has a lot of details: https://bugs.kde.org/show_bug.cgi?id=432801 Also, there is a comment in the patch itself that is pretty long and describes the technique used. I was looking through memcheck again and saw another place where a similar technique is used so I will refactor the code so that they two can share more of the logic. > BTW. The patch as attached doesn't really apply, best to sent it with > git send-email. Or create a bugzilla and attach it there. > I'll do that from now on. > So I think you patch is correct. I just wonder if this should just > impact clo_exit_on_first_error or also the other actions in > do_actions_on_error. Does it make sense to invoke vgdb if the error > doesn't count, or to generate a suppression? > Good question. We could think about that and maybe adjust it, too. Or, we could just make this change and then do the vgdb part in a future commit. One advantage of doing it separately is that if we change our minds about just one or the other then it will be easier to revert in the future. Eyal > Maybe we should simply not call do_actions_on_error when count_error is > false? Also in my attached patch, but I like to hear other opinions, > maybe my assumption that a non-counted error is not interesting for > vgdb (or suppression generation) is flawed. > > Cheers, > > Mark > |
|
From: Mark W. <ma...@kl...> - 2021-03-04 14:31:27
|
Hi Eyal, On Wed, 2021-03-03 at 10:59 -0700, Eyal Soha wrote: > On Wed, Mar 3, 2021 at 6:54 AM Mark Wielaard <ma...@kl...> wrote: > So I think you patch is correct. I just wonder if this should just > > impact clo_exit_on_first_error or also the other actions in > > do_actions_on_error. Does it make sense to invoke vgdb if the error > > doesn't count, or to generate a suppression? > > Good question. We could think about that and maybe adjust it, too. Or, we > could just make this change and then do the vgdb part in a future commit. > One advantage of doing it separately is that if we change our minds about > just one or the other then it will be easier to revert in the future. You might be right. For --errors-for-leak-kinds=xxx it is kind of obvious that you don't want exit_on_first_error to trigger. That is the whole point of the option. For suppression generation and vgdb invocation it might be debatable. If there are errors that are NOT in the errors-for-leak-kinds set, but are in the show-leak-kinds set (so ppError will be called, but with count_error set to False) the user might want to invoke vgdb or generate a suppression? I am not really sure they do. But maybe they do? If nobody else has an opinion then lets go with you variant. So, any opinions, anybody? Cheers, Mark |