Re: Patch ping

Jim Blandy
2002-05-16T22:54:36Z

I'm sorry it's taken a long time to get to this patch.

Scanning for commands that could delete breakpoints doesn't seem very
robust. If I add a new command that happens to delete breakpoints,
this won't catch that, right?

Why not add a reference count field to struct command_line? It would
be set to 1 when allocated. You'd increment it when you begin
executing a command list, and decrement it when you were done, or
aborted execution due to an error. You'd also decrement it when the
breakpoint gets deleted.

I think it might be less code overall.

Don Howard <dhoward@redhat.com> writes:

> I still need reveiw/approval for the breakpoint portion of this.
>
>
>
> --
> dhoward@redhat.com
> gdb engineering
>
>
> ---------- Forwarded message ----------
> Date: Sun, 28 Apr 2002 12:06:11 -0400
> From: Fernando Nasser <fnasser@redhat.com>
> To: Don Howard <dhoward@redhat.com>
> Cc: Andrew Cagney <ac131313@cygnus.com>, Michael Snyder <msnyder@cygnus.com>,
> gdb-patches@sources.redhat.com
> Subject: Re: [RFA] deleting breakpoints inside of 'commands'
>
> The CLI part is approved. This is somewhat mixed with breakpoints code
> (in the breakpoint.c file) so Michael Snyder would have to approve it as
> well.
>
> Regards,
> Fernando
>
> Don Howard wrote:
> >
> > I'm revisiting this again, as I've not received feedback the last two
> > times that I've reposted it.
> >
> > This patch addresses a crash that can occur when a breakpoint's command
> > list deletes it's own breakpoint. In this solution, breakpoint command
> > lists are walked (recursively, so as to examine compound statements and
> > user defined commands). If a 'clear' or 'delete' command is found
> > anywhere in the definition of the command list, then the command list is
> > duplicated and that duplicate is executed. Once execution of the command
> > list is complete, the duplicate is freed.
> >
> > This patch does not address any situation where a user defined command
> > deletes itself. (is that even possible? I don't see a way to remove a
> > user-defined command, only a way to redefine one.)
> >
> > This version of contains 2 corrections to
> > bpstat_actions_delete_breakpoints()
> >
> > 1) The call to lookup_cmd() allows unknown commands.
> >
> > 2) The body of the command is examined regardless of the result of
> > lookup_cmd().
> >
> > Testing on i686 RH 7.2 shows no regressions:
> >
> > === gdb Summary ===
> >
> > # of expected passes 8272
> > # of unexpected failures 31
> > # of unexpected successes 11
> > # of expected failures 170
> > # of untested testcases 7
> > /home/dhoward/work/sources/build/gdb/testsuite/../../gdb/gdb version
> > 2002-04-27-cvs -nx
> >
> > After:
> > === gdb Summary ===
> >
> > # of expected passes 8272
> > # of unexpected failures 31
> > # of unexpected successes 11
> > # of expected failures 170
> > # of untested testcases 7
> > /home/dhoward/work/sources/rebuild/gdb/testsuite/../../gdb/gdb version
> > 2002-04-27-cvs -nx
> >
> > Comments?
> >
> > Ok to commit?
> >
> > 2002-04-27 Don Howard <dhoward@redhat.com>
> >
> > * breakpoint.c (bpstat_do_actions): Avoid deleting a 'commands'
> > list while executing that list. Thanks to Pierre Muller
> > <muller@ics.u-strasbg.fr> for suggesting this implementation.
> > (cleanup_dup_command_lines): New function.
> > (bpstat_actions_delete_breakpoints): Ditto.
> > * cli/cli-script.c (dup_command_lines): New function.
> > * defs.h: Added declaration of new function.
> >
> > Index: gdb/breakpoint.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/breakpoint.c,v
> > retrieving revision 1.74
> > diff -p -u -w -r1.74 breakpoint.c
> > --- gdb/breakpoint.c 24 Apr 2002 16:28:15 -0000 1.74
> > +++ gdb/breakpoint.c 27 Apr 2002 19:18:57 -0000
> > @@ -47,6 +47,7 @@
> > #include "ui-out.h"
> >
> > #include "gdb-events.h"
> > +#include "cli/cli-decode.h"
> >
> > /* Prototypes for local functions. */
> >
> > @@ -1851,6 +1852,100 @@ cleanup_executing_breakpoints (PTR ignor
> > executing_breakpoint_commands = 0;
> > }
> >
> > +static void
> > +cleanup_dup_command_lines (PTR cmds)
> > +{
> > + free_command_lines (cmds);
> > +}
> > +
> > +
> > +/* Walk a list of struct command_lines and try to determine if any
> > + command deletes breakpoints */
> > +
> > +static int
> > +bpstat_actions_delete_breakpoints (struct command_line * cmd)
> > +{
> > + for (; cmd; cmd = cmd->next)
> > + {
> > + struct cmd_list_element *ce;
> > + char *line = cmd->line;
> > + int i;
> > + int ret;
> > +
> > + ce = lookup_cmd (&line, cmdlist, "", 1, 1);
> > +
> > + if (ce != NULL)
> > + {
> > + /* Check the command */
> > + if (ce->class == class_user && !ce->hook_in)
> > + {
> > + ce->hook_in = 1;
> > + ret = bpstat_actions_delete_breakpoints (ce->user_commands);
> > + ce->hook_in = 0;
> > +
> > + if (ret)
> > + return 1;
> > + }
> > + else if (strcmp (ce->name, "delete") == 0
> > + || strcmp (ce->name, "clear") == 0)
> > + {
> > + return 1;
> > + }
> > +
> > +
> > + /* Check the pre-hook */
> > + if (ce->hook_pre)
> > + {
> > + if (ce->hook_pre->class == class_user && !ce->hook_in)
> > + {
> > + ce->hook_in = 1;
> > + ret = bpstat_actions_delete_breakpoints (ce->hook_pre->user_commands);
> > + ce->hook_in = 0;
> > +
> > + if (ret)
> > + return 1;
> > + }
> > + else if (strcmp (ce->hook_pre->name, "delete") == 0
> > + || strcmp (ce->hook_pre->name, "clear") == 0)
> > + {
> > + return 1;
> > + }
> > + }
> > +
> > +
> > + /* Check the post-hook */
> > + if (ce->hook_post)
> > + {
> > + if (ce->hook_post->class == class_user && !ce->hook_in)
> > + {
> > + ce->hook_in = 1;
> > + ret = bpstat_actions_delete_breakpoints (ce->hook_post->user_commands);
> > + ce->hook_in = 0;
> > +
> > + if (ret)
> > + return 1;
> > + }
> > + else if (strcmp (ce->hook_post->name, "delete") == 0
> > + || strcmp (ce->hook_post->name, "clear") == 0)
> > + {
> > + return 1;
> > + }
> > + }
> > + }
> > +
> > + /* If this is a multi-part command (while, if, etc), check the
> > + body. */
> > + for (i=0; i<cmd->body_count; i++)
> > + if (bpstat_actions_delete_breakpoints (cmd->body_list[i]))
> > + return 1;
> > +
> > + }
> > +
> > + return 0;
> > +
> > +}
> > +
> > +
> > /* Execute all the commands associated with all the breakpoints at this
> > location. Any of these commands could cause the process to proceed
> > beyond this point, etc. We look out for such changes by checking
> > @@ -1861,7 +1956,6 @@ bpstat_do_actions (bpstat *bsp)
> > {
> > bpstat bs;
> > struct cleanup *old_chain;
> > - struct command_line *cmd;
> >
> > /* Avoid endless recursion if a `source' command is contained
> > in bs->commands. */
> > @@ -1886,16 +1980,37 @@ top:
> > breakpoint_proceeded = 0;
> > for (; bs != NULL; bs = bs->next)
> > {
> > - cmd = bs->commands;
> > - while (cmd != NULL)
> > + struct command_line *cmd;
> > + struct cleanup *new_old_chain;
> > +
> > + cmd = 0;
> > + new_old_chain = 0;
> > +
> > + /* If the command list for this breakpoint includes a statement
> > + that deletes breakpoints, we assume that the target may be
> > + this breakpoint, so we make a copy of the command list to
> > + avoid walking a list that has been deleted. */
> > +
> > + for (cmd = bs->commands; cmd; cmd = cmd->next)
> > + {
> > + if (!new_old_chain && bpstat_actions_delete_breakpoints (cmd))
> > {
> > + cmd = dup_command_lines (cmd);
> > + new_old_chain = make_cleanup (cleanup_dup_command_lines, cmd);
> > + }
> > +
> > execute_control_command (cmd);
> >
> > if (breakpoint_proceeded)
> > break;
> > - else
> > - cmd = cmd->next;
> > }
> > +
> > + if (new_old_chain)
> > + {
> > + free_command_lines (&cmd);
> > + discard_cleanups (new_old_chain);
> > + }
> > +
> > if (breakpoint_proceeded)
> > /* The inferior is proceeded by the command; bomb out now.
> > The bpstat chain has been blown away by wait_for_inferior.
> > @@ -1905,6 +2020,7 @@ top:
> > else
> > bs->commands = NULL;
> > }
> > +
> >
> > executing_breakpoint_commands = 0;
> > discard_cleanups (old_chain);
> > Index: gdb/defs.h
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/defs.h,v
> > retrieving revision 1.88
> > diff -p -u -w -r1.88 defs.h
> > --- gdb/defs.h 18 Apr 2002 18:08:59 -0000 1.88
> > +++ gdb/defs.h 27 Apr 2002 19:18:57 -0000
> > @@ -641,6 +641,7 @@ struct command_line
> > extern struct command_line *read_command_lines (char *, int);
> >
> > extern void free_command_lines (struct command_line **);
> > +extern struct command_line * dup_command_lines (struct command_line *);
> >
> > /* To continue the execution commands when running gdb asynchronously.
> > A continuation structure contains a pointer to a function to be called
> > Index: gdb/cli/cli-script.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
> > retrieving revision 1.12
> > diff -p -u -w -r1.12 cli-script.c
> > --- gdb/cli/cli-script.c 12 Apr 2002 22:31:23 -0000 1.12
> > +++ gdb/cli/cli-script.c 27 Apr 2002 19:18:58 -0000
> > @@ -974,6 +974,59 @@ read_command_lines (char *prompt_arg, in
> > return (head);
> > }
> >
> > +/* Duplicate a chain of struct command_line's */
> > +
> > +struct command_line *
> > +dup_command_lines (struct command_line *l)
> > +{
> > + struct command_line *dup = NULL;
> > + register struct command_line *next = NULL;
> > +
> > +
> > + for (; l ; l = l->next)
> > + {
> > + if (next == NULL)
> > + {
> > + dup = next = (struct command_line *)
> > + xmalloc (sizeof (struct command_line));
> > + }
> > + else
> > + {
> > + next->next = (struct command_line *)
> > + xmalloc (sizeof (struct command_line));
> > +
> > + next = next->next;
> > + }
> > +
> > +
> > + if (next == NULL)
> > + return NULL;
> > +
> > +
> > + next->next = NULL;
> > + next->line = xstrdup (l->line);
> > + next->control_type = l->control_type;
> > + next->body_count = l->body_count;
> > +
> > +
> > + if (l->body_count > 0)
> > + {
> > + int i;
> > + struct command_line **blist = l->body_list;
> > +
> > + next->body_list =
> > + (struct command_line **) xmalloc (sizeof (struct command_line *)
> > + * l->body_count);
> > +
> > + for (i = 0; i < l->body_count; i++, blist++)
> > + next->body_list[i] = dup_command_lines (*blist);
> > + }
> > + }
> > +
> > + return dup;
> > +}
> > +
> > +
> > /* Free a chain of struct command_line's. */
> >
> > void
> >
> > --
> > dhoward@redhat.com
> > gdb engineering
>
> --
> Fernando Nasser
> Red Hat Canada Ltd. E-Mail: fnasser@redhat.com
> 2323 Yonge Street, Suite #300
> Toronto, Ontario M4P 2C9