From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64624 invoked by alias); 24 Feb 2016 14:46:14 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 64609 invoked by uid 89); 24 Feb 2016 14:46:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 spammy=infcall, joe, CMD, bull X-HELO: mail-wm0-f51.google.com Received: from mail-wm0-f51.google.com (HELO mail-wm0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 24 Feb 2016 14:46:08 +0000 Received: by mail-wm0-f51.google.com with SMTP id c200so273089034wme.0 for ; Wed, 24 Feb 2016 06:46:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-transfer-encoding; bh=f46BstKjFTfIwNc3AZM4+v8upphk9RoFCbWsYhKByWY=; b=FZwporr5FHSbgbrRvxIAS0zLVXfA07qXCZn4+4incCi9GVXjL8rKdN6VbFi0cAi7hF EOSYgtbhiISJokejpVGt9c6tnlAP5HxdwbdLo92Fw8eu9+HqIHBTCv2SRS9djN6a+EsF lGyNuw7vGqsD48RJrMZjDh/c29s/ItXDMYb2Zd3g/E+xTBwpA4TUxZdwdxP5Ilf0m2iM sqcurq/ZxRtH5YcislUcDKNKvl6XByxyKg96Dc1F2un3mJkdQtUdFkdVuG49ply7LyxC BHQgWbu8We/VWD38vF1kHrb907IThnKOEzSyuPkdWV0ikt5QIisL8THhs1BAQbaKATQV +M4w== X-Gm-Message-State: AG10YORga1PN0WQDq941F9798z+DkU5I3cfCqXoEaB6c2+ySE9Im6MciSbXmPgv2fm4ErJ2yYJynV8v/ugnYwQ== MIME-Version: 1.0 X-Received: by 10.28.23.75 with SMTP id 72mr25544127wmx.50.1456325165270; Wed, 24 Feb 2016 06:46:05 -0800 (PST) Received: by 10.28.6.213 with HTTP; Wed, 24 Feb 2016 06:46:05 -0800 (PST) In-Reply-To: References: <56602D7B.2010404@redhat.com> <56685ED4.3030909@redhat.com> Date: Wed, 24 Feb 2016 14:46:00 -0000 Message-ID: Subject: Re: Always run GDB command post-hook after pre-hook has been run From: Stephen Cross To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00731.txt.bz2 Hi Pedro, Here's a patch that adds 'hookerror-' to GDB. This passes the test suite (running 'make check' in the 'gdb' directory). Do you think this patch is acceptable? Are there any further changes that y= ou'd like me to make? Thanks, Stephen ---------------------------------------------------------------------------= ----- [PATCH] Add command error hooks using 'hookerror-'. * cli-decode.c: Add errorhook args to delete_cmd(). * cli-decode.h: Add hook_error field to cmd_list_element. * cli-script.c: Add execute_cmd_error_hook(). Match 'hookerror-' string. * command.h: Declare execute_cmd_error_hook(). * top.c: Call error hook if command throws exception. * testsuite/gdb.base/define.exp: Add test cases for 'hookerror-'. --- gdb/ChangeLog | 10 ++++++++ gdb/cli/cli-decode.c | 35 ++++++++++++++++++++++------ gdb/cli/cli-decode.h | 7 ++++++ gdb/cli/cli-script.c | 28 +++++++++++++++++++++- gdb/command.h | 5 ++-- gdb/doc/gdb.texinfo | 19 +++++++++++---- gdb/testsuite/gdb.base/define.exp | 49 +++++++++++++++++++++++++++++++++++= ++++ gdb/top.c | 18 +++++++++++++- 8 files changed, 156 insertions(+), 15 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index d6b4a6b..6eb0c13 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2016-02-24 Stephen Cross + + Add command error hooks using 'hookerror-'. + * cli-decode.c: Add errorhook args to delete_cmd(). + * cli-decode.h: Add hook_error field to cmd_list_element. + * cli-script.c: Add execute_cmd_error_hook(). Match 'hookerror-' strin= g. + * command.h: Declare execute_cmd_error_hook(). + * top.c: Call error hook if command throws exception. + * testsuite/gdb.base/define.exp: Add test cases for 'hookerror-'. + 2016-02-24 Joel Brobecker GDB 7.11 released. diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c index 04ed6ab..c143d5a 100644 --- a/gdb/cli/cli-decode.c +++ b/gdb/cli/cli-decode.c @@ -33,7 +33,9 @@ static struct cmd_list_element *delete_cmd (const char *n= ame, struct cmd_list_element **prehook, struct cmd_list_element **prehookee, struct cmd_list_element **posthook, - struct cmd_list_element **posthookee); + struct cmd_list_element **posthookee, + struct cmd_list_element **errorhook, + struct cmd_list_element **errorhookee); static struct cmd_list_element *find_cmd (const char *command, int len, @@ -198,7 +200,8 @@ add_cmd (const char *name, enum command_class theclass, cmd_cfunc_ftype *fun, /* Turn each alias of the old command into an alias of the new command. */ c->aliases =3D delete_cmd (name, list, &c->hook_pre, &c->hookee_pre, - &c->hook_post, &c->hookee_post); + &c->hook_post, &c->hookee_post, &c->hook_error, + &c->hookee_error); for (iter =3D c->aliases; iter; iter =3D iter->alias_chain) iter->cmd_pointer =3D c; if (c->hook_pre) @@ -209,6 +212,10 @@ add_cmd (const char *name, enum command_class theclass, cmd_cfunc_ftype *fun, c->hook_post->hookee_post =3D c; if (c->hookee_post) c->hookee_post->hook_post =3D c; + if (c->hook_error) + c->hook_error->hookee_error =3D c; + if (c->hookee_error) + c->hookee_error->hook_error =3D c; if (*list =3D=3D NULL || strcmp ((*list)->name, name) >=3D 0) { @@ -294,10 +301,12 @@ add_alias_cmd (const char *name, const char *oldname, enum command_class theclas if (old =3D=3D 0) { - struct cmd_list_element *prehook, *prehookee, *posthook, *posthookee; + struct cmd_list_element *prehook, *prehookee, *posthook, *posthookee, + *errorhook, *errorhookee; struct cmd_list_element *aliases =3D delete_cmd (name, list, &prehook, &prehookee, - &posthook, &posthookee); + &posthook, &posthookee, + &errorhook, &errorhookee); /* If this happens, it means a programmer error somewhere. */ gdb_assert (!aliases && !prehook && !prehookee @@ -778,7 +787,7 @@ add_setshow_zuinteger_cmd (const char *name, enum command_class theclass, /* Remove the command named NAME from the command list. Return the list commands which were aliased to the deleted command. If the command had no aliases, return NULL. The various *HOOKs are set to - the pre- and post-hook commands for the deleted command. If the + the pre-, post- and error-hook commands for the deleted command. If the command does not have a hook, the corresponding out parameter is set to NULL. */ @@ -787,7 +796,9 @@ delete_cmd (const char *name, struct cmd_list_element **list, struct cmd_list_element **prehook, struct cmd_list_element **prehookee, struct cmd_list_element **posthook, - struct cmd_list_element **posthookee) + struct cmd_list_element **posthookee, + struct cmd_list_element **errorhook, + struct cmd_list_element **errorhookee) { struct cmd_list_element *iter; struct cmd_list_element **previous_chain_ptr; @@ -797,6 +808,8 @@ delete_cmd (const char *name, struct cmd_list_element **list, *prehookee =3D NULL; *posthook =3D NULL; *posthookee =3D NULL; + *errorhook =3D NULL; + *errorhookee =3D NULL; previous_chain_ptr =3D list; for (iter =3D *previous_chain_ptr; iter; iter =3D *previous_chain_ptr) @@ -815,6 +828,10 @@ delete_cmd (const char *name, struct cmd_list_element **list, xfree ((char *) iter->doc); *posthook =3D iter->hook_post; *posthookee =3D iter->hookee_post; + if (iter->hookee_error) + iter->hookee_error->hook_error =3D 0; + *errorhook =3D iter->hook_error; + *errorhookee =3D iter->hookee_error; /* Update the link. */ *previous_chain_ptr =3D iter->next; @@ -992,7 +1009,7 @@ help_cmd (const char *command, struct ui_file *stream) if (c->func =3D=3D NULL) help_list (cmdlist, "", c->theclass, stream); - if (c->hook_pre || c->hook_post) + if (c->hook_pre || c->hook_post || c->hook_error) fprintf_filtered (stream, "\nThis command has a hook (or hooks) defined:\n"); @@ -1004,6 +1021,10 @@ help_cmd (const char *command, struct ui_file *strea= m) fprintf_filtered (stream, "\tThis command is run before : %s (post hook)\n", c->hook_post->name); + if (c->hook_error) + fprintf_filtered (stream, + "\tThis command is run before : %s (error hook)\n", + c->hook_error->name); } /* diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h index 4ea8063..32ea902 100644 --- a/gdb/cli/cli-decode.h +++ b/gdb/cli/cli-decode.h @@ -147,6 +147,9 @@ struct cmd_list_element /* Hook for another command to be executed after this command. */ struct cmd_list_element *hook_post; + /* Hook for another command to be executed if this command fails. */ + struct cmd_list_element *hook_error; + /* Nonzero identifies a prefix command. For them, the address of the variable containing the list of subcommands. */ struct cmd_list_element **prefixlist; @@ -209,6 +212,10 @@ struct cmd_list_element so the hook can be removed when this one is deleted. */ struct cmd_list_element *hookee_post; + /* Pointer to command that is hooked by this one, (by hook_error) + so the hook can be removed when this one is deleted. */ + struct cmd_list_element *hookee_error; + /* Pointer to command that is aliased by this one, so the aliased command can be located in case it has been hooked. */ struct cmd_list_element *cmd_pointer; diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c index 7302330..9545571 100644 --- a/gdb/cli/cli-script.c +++ b/gdb/cli/cli-script.c @@ -337,6 +337,19 @@ execute_cmd_post_hook (struct cmd_list_element *c) } } +void +execute_cmd_error_hook (struct cmd_list_element *c) +{ + if ((c->hook_error) && (!c->hook_in)) + { + struct cleanup *cleanups =3D make_cleanup (clear_hook_in_cleanup, c); + + c->hook_in =3D 1; /* Prevent recursive hooking. */ + execute_user_command (c->hook_error, (char *) 0); + do_cleanups (cleanups); + } +} + /* Execute the command in CMD. */ static void do_restore_user_call_depth (void * call_depth) @@ -1496,7 +1509,8 @@ define_command (char *comname, int from_tty) { CMD_NO_HOOK =3D 0, CMD_PRE_HOOK, - CMD_POST_HOOK + CMD_POST_HOOK, + CMD_ERROR_HOOK }; struct command_line *cmds; struct cmd_list_element *c, *newc, *hookc =3D 0, **list; @@ -1510,6 +1524,8 @@ define_command (char *comname, int from_tty) #define HOOK_LEN 5 #define HOOK_POST_STRING "hookpost-" #define HOOK_POST_LEN 9 +#define HOOK_ERROR_STRING "hookerror-" +#define HOOK_ERROR_LEN 10 comfull =3D comname; list =3D validate_comname (&comname); @@ -1546,6 +1562,11 @@ define_command (char *comname, int from_tty) hook_type =3D CMD_POST_HOOK; hook_name_size =3D HOOK_POST_LEN; } + else if (!strncmp (comname, HOOK_ERROR_STRING, HOOK_ERROR_LEN)) + { + hook_type =3D CMD_ERROR_HOOK; + hook_name_size =3D HOOK_ERROR_LEN; + } if (hook_type !=3D CMD_NO_HOOK) { @@ -1599,6 +1620,11 @@ define_command (char *comname, int from_tty) newc->hookee_post =3D hookc; /* We are marked as hooking target cmd. */ break; + case CMD_ERROR_HOOK: + hookc->hook_error =3D newc; /* Target gets hooked. */ + newc->hookee_error =3D hookc; /* We are marked as hooking + target cmd. */ + break; default: /* Should never come here as hookc would be 0. */ internal_error (__FILE__, __LINE__, _("bad switch")); diff --git a/gdb/command.h b/gdb/command.h index ab62601..b8771df 100644 --- a/gdb/command.h +++ b/gdb/command.h @@ -180,11 +180,12 @@ extern void set_cmd_context (struct cmd_list_element = *cmd, extern void *get_cmd_context (struct cmd_list_element *cmd); -/* Execute CMD's pre/post hook. Throw an error if the command fails. - If already executing this pre/post hook, or there is no pre/post +/* Execute CMD's pre/post/error hook. Throw an error if the command fails. + If already executing this pre/post/error hook, or there is no pre/post/= error hook, the call is silently ignored. */ extern void execute_cmd_pre_hook (struct cmd_list_element *cmd); extern void execute_cmd_post_hook (struct cmd_list_element *cmd); +extern void execute_cmd_error_hook (struct cmd_list_element *cmd); /* Return the type of the command. */ extern enum cmd_types cmd_type (struct cmd_list_element *cmd); diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 4ec0ec1..c2447db 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -24133,8 +24133,19 @@ pre-execution hooks, for the same command. It is valid for a hook to call the command which it hooks. If this occurs, the hook is not re-executed, thereby avoiding infinite recursion. -@c It would be nice if hookpost could be passed a parameter indicating -@c if the command it hooks executed properly or not. FIXME! +@emph{Note}: The post-execution hook is not run if the command fails; error +hooks can be used to run code in this case. + +@cindex hooks, error-command +@kindex hookerror +A hook may be defined which is run after the command you executed if the command +fails. Whenever you run the command @samp{foo} and @samp{foo} fails, if the +user-defined command @samp{hookerror-foo} exists, it is executed (with no +arguments) after that command. Error hooks may exist simultaneously with +pre-execution and post-execution hooks, for the same command. + +It is valid for a hook to call the command which it hooks. If this +occurs, the hook is not re-executed, thereby avoiding infinite recursion. @kindex stop@r{, a pseudo-command} In addition, a pseudo-command, @samp{stop} exists. Defining @@ -24184,8 +24195,8 @@ not for command aliases; you should define a hook for the basic command name, e.g.@: @code{backtrace} rather than @code{bt}. @c FIXME! So how does Joe User discover whether a command is an alias @c or not? -You can hook a multi-word command by adding @code{hook-} or -@code{hookpost-} to the last word of the command, e.g.@: +You can hook a multi-word command by adding @code{hook-}, @code{hookpost-}= or +@code{hookerror-} to the last word of the command, e.g.@: @samp{define target hook-remote} to add a hook to @samp{target remote}. If an error occurs during the execution of your hook, execution of diff --git a/gdb/testsuite/gdb.base/define.exp b/gdb/testsuite/gdb.base/define.exp index 8177449..50fd8b3 100644 --- a/gdb/testsuite/gdb.base/define.exp +++ b/gdb/testsuite/gdb.base/define.exp @@ -249,8 +249,57 @@ gdb_test_multiple "define target hookpost-testsuite" "= " { } } +gdb_test_multiple "define target hookerror-testsuite" "" { + -re "Type commands for definition of \"target hookerror-testsuite\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"three\\n\"\nend" "" "define target hookerror-testsu= ite" + } +} + gdb_test "target testsuite" "one\r\nhello\r\ntwo" "target testsuite with h= ooks" +# Test creation of an additional target subcommand that raises an error. +gdb_test_multiple "define target testsuite-error" "" { + -re "Type commands for definition of \"target testsuite-error\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"hello\\n\"\nprint (1/0)\nend" "" "define target testsuite-error" + } +} + +# Add pre-, post- and error-hooks for the testsuite-error target. +gdb_test_multiple "define target hook-testsuite-error" "" { + -re "Type commands for definition of \"target hook-testsuite-error\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"one\\n\"\nend" "" "define target hook-testsuite-err= or" + } +} + +gdb_test_multiple "define target hookpost-testsuite-error" "" { + -re "Type commands for definition of \"target hookpost-testsuite-error\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"two\\n\"\nend" "" "define target hookpost-testsuite-error" + } +} + +gdb_test_multiple "define target hookerror-testsuite-error" "" { + -re "Type commands for definition of \"target hookerror-testsuite-error\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"three\\n\"\nend" "" "define target hookerror-testsuite-error" + } +} + +gdb_test "target testsuite-error" "one\r\nhello\r\nthree\r\nDivision by zero" "target testsuite-error with hooks" + +# Test creation of an additional target subcommand that raises an error in its error hook. +gdb_test_multiple "define target testsuite-errorinhook" "" { + -re "Type commands for definition of \"target testsuite-errorinhook\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"hello\\n\"\nprint (1/0)\nend" "" "define target testsuite-errorinhook" + } +} + +gdb_test_multiple "define target hookerror-testsuite-errorinhook" "" { + -re "Type commands for definition of \"target hookerror-testsuite-errorinhook\".\r\nEnd with a line saying just \"end\".\r\n>$" { + gdb_test "printf \"three\\n\"\nprint (1/0)\nend" "" "define target hookerror-testsuite-errorinhook" + } +} + +gdb_test "target testsuite-errorinhook" "hello\r\nthree\r\nDivision by zero\r\nDivision by zero" "target testsuite-errorinhook with hooks" + # This is a quasi-define command: Verify that the user can redefine # GDB's gdb_prompt. # diff --git a/gdb/top.c b/gdb/top.c index ed200ba..9535849 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -388,13 +388,27 @@ maybe_wait_sync_command_done (int was_sync) wait_sync_command_done (); } +static void +call_error_hook_cleanup(void* p) +{ + TRY + { + execute_cmd_error_hook (p); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_print (gdb_stderr, ex); + } + END_CATCH +} + /* Execute the line P as a command, in the current user context. Pass FROM_TTY as second argument to the defining function. */ void execute_command (char *p, int from_tty) { - struct cleanup *cleanup_if_error, *cleanup; + struct cleanup *cleanup_if_error, *cleanup, *cmd_cleanup; struct cmd_list_element *c; char *line; @@ -456,6 +470,7 @@ execute_command (char *p, int from_tty) /* If this command has been pre-hooked, run the hook first. */ execute_cmd_pre_hook (c); + cmd_cleanup =3D make_cleanup (call_error_hook_cleanup, c); if (c->deprecated_warn_user) deprecated_cmd_warning (line); @@ -478,6 +493,7 @@ execute_command (char *p, int from_tty) /* If this command has been post-hooked, run the hook last. */ execute_cmd_post_hook (c); + discard_cleanups (cmd_cleanup); } On Wed, Jan 13, 2016 at 3:21 PM, Stephen Cross w= rote: > Sorry for the delay in my response. > >> It may happen to work in practice on currently supported hosts, >> but it's undefined C/C++. Wrapper is the way to go, just like the >> comment says. > > Thanks for clarifying. > >> Do you support debugging with MI? Or users scripting gdb with Python? > > We do support MI and some GDB Python scripting (i.e. that doesn't > interfere with our own scripting; we've tried to minimise our own > scripting for this reason). > >> Seems to me you'll still end up with problems with e.g., varobjs with >> function calls, -data-evaluate-expression, etc., which you'd end up >> solving probably with events around expression evaluation, similar to >> the infcall ones. > > Yes, this is right and that's another part of this that we're working on. > >> If you determined that up until recently, gdb used to call the >> hookpost even on error, and this was a recent regression, then it'd >> shine a different light on the idea. OTOH, if this has always been >> this way, then I'm more inclined to have some way to distinguish >> normal hookpost vs error. > > My testing suggests this behaviour (not calling hookpost on error) has > been consistent since at least GDB 7.0. So it's not a recent > regression. > > So yes, hookerror- seems like a sensible addition. I'll start > putting together a patch for this. > >> Doesn't have to be literal "hookerror", could be something less >> confusing if you find it. > > I think this is a clear name. > >> Hmm, perhaps we should at least print the original error before >> losing it? > > I think this is a problem more generally with GDB's internal > exceptions, since a new exception overrides the current exception > being handled (these semantics are similar to throwing from a finally > block in Java). > > I imagine we could force it to print the original error by adding > another 'print_error_cleanup' that is run when the call to > 'execute_cmd_post_hook' ends up throwing inside > 'call_post_hook_cleanup'. > > Alternatively, we could have a catch around the > 'execute_cmd_post_hook' that would print the (new) error that came > from running the 'hookerror' and then we'd return normally from the > cleanup so the original error would be processed as normal. If there > was no original error it would still prevent the 'hookerror' exception > from leaking out of the cleanup. I think these semantics are possibly > more intuitive. > > Thanks, > Stephen > > On Wed, Dec 9, 2015 at 5:03 PM, Pedro Alves wrote: >> On 12/03/2015 04:11 PM, Stephen Cross wrote: >>> Hi Pedro, >>> >>>> Implementation approach, or on the idea in the first place? >>> >>> I'm mostly looking for input on the idea, because I know that the >>> proposed change affects the behaviour of post-hooks. >>> >>> The only relevant point about the implementation was that I added: >>> >>> +static void >>> +call_post_hook_cleanup(void* p) >>> +{ >>> + execute_cmd_post_hook (p); >>> +} >>> >>> I added this in response to a comment in 'cleanups.h' that says >>> (emphasis on the last line): >>> >>> /* NOTE: cagney/2000-03-04: This typedef is strictly for the >>> make_cleanup function declarations below. Do not use this typedef >>> as a cast when passing functions into the make_cleanup() code. >>> Instead either use a bounce function or add a wrapper function. >>> Calling a f(char*) function with f(void*) is non-portable. */ >>> >>> I had thought that calling f(char*) via f(void*) would be portable, >>> but I've added the wrapper function just in case. What do you think? >> >> It may happen to work in practice on currently supported hosts, >> but it's undefined C/C++. Wrapper is the way to go, just like the >> comment says. >> >>> >>>> I think it'd help if you told us the motivation. What's the intent >>>> of running the hookpost even on error? What are you trying to use the >>>> hooks for? >>> >>> Our focus here is on commands that can perform inferior calls. We have >>> a replacement for gdbserver that by default doesn't support inferior >>> calls, since we can be part way through a debuggee's history (our core >>> product is a reversible debugger). So for inferior calls we have to >>> issue a command to tell our server to fork the current debuggee >>> process and then GDB can make arbitrary modifications to the fork >>> child process; once the inferior call is complete we then issue >>> another command to tell the server to drop the fork child process and >>> switch back to the parent process. >>> >>> We're currently using the inferior call events added by my colleague >>> (Nick Bull) and these work for most cases. However we've found that if >>> GDB performs an inferior call which returns a pointer and then prints >>> that, GDB will access the memory *after* issuing the inferior call end >>> event. If the inferior call returns a buffer it allocated/modified >>> then this can cause us to print the old value of the buffer. >>> >>> Hooks solve this problem because we can keep the fork child process >>> around long enough for GDB to read the correct value. Unfortunately >>> this means that if the 'print' command fails for any reason then we >>> won't have been notified to drop the child process, affecting the rest >>> of the debug session. >> >> Thanks for the explanation. >> >> Do you support debugging with MI? Or users scripting gdb with Python? >> Seems to me you'll still end up with problems with e.g., varobjs with >> function calls, -data-evaluate-expression, etc., which you'd end up >> solving probably with events around expression evaluation, similar to >> the infcall ones. >> >>> >>>> Playing devil's advocate, isn't it reasonable to say that existing >>>> hookpost scripts out there may be assuming that they only run if >>>> the hooked command finished successfully? >>> >>> The online docs say "Whenever you run the command =E2=80=98foo=E2=80=99= , if the >>> user-defined command =E2=80=98hookpost-foo=E2=80=99 exists, it is execu= ted (with no >>> arguments) after that command.". They don't seem to mention that >>> sometimes the post-hook might not be run. Having said that, users may >>> have observed this happening in practice. >> >> If you determined that up until recently, gdb used to call the >> hookpost even on error, and this was a recent regression, then it'd >> shine a different light on the idea. OTOH, if this has always been >> this way, then I'm more inclined to have some way to distinguish >> normal hookpost vs error. >> >>> >>>> Wonder whether we should have that. Alternatively, guess we could have >>>> a new hookerror hook, that would run on error instead of hookpost. >>> >>> Yes, I think having a new 'hookerror' hook would be reasonable. This >>> would ensure existing users wouldn't be affected, but the naming might >>> cause confusion, so hookpost would need to be clearly documented as >>> only being run in the success case. I'm happy to augment the patch to >>> do this. >> >> Doesn't have to be literal "hookerror", could be something less >> confusing if you find it. >> >>> >>>> What happens / should happen if the hookpost itself throws an error? = Do >>>> we lose the original hooked-command's error? Is that OK? >>> >>> I previously tested this case with the patch applied and it appears >>> that the error in the post-hook is what appears. So yes, we do lose >>> the original hooked command's error. It looks like this is because the >>> throw_exception() function inside GDB first calls 'do_cleanups >>> (all_cleanups ());' and do_cleanups() allows cleanup functions to >>> throw (and it updates the list of cleanups before running each >>> cleanup). >>> >>> This behaviour seems OK to me, particularly if we added a new >>> 'hookerror' and warned in the documentation that this occurs. >>> Presumably this should also have a testcase. >> >> Hmm, perhaps we should at least print the original error before >> losing it? >> >> Thanks, >> Pedro Alves >> > > > > -- > Stephen Cross > > Software Engineer at Undo Software --=20 Stephen Cross Software Engineer at Undo Software