From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Buettner To: Andrew Cagney , Don Howard Cc: , Fernando Nasser , Michael Snyder Subject: Re: [RFA] deleting breakpoints inside of 'commands' [Repost] Date: Mon, 24 Sep 2001 17:10:00 -0000 Message-id: <1010925001014.ZM30380@ocotillo.lan> References: X-SW-Source: 2001-09/msg00324.html On Sep 21, 4:53pm, Don Howard wrote: > One more try. =) This patch adds a new field to struct command_line: > 'executing'. If this field is non-zero, free_command_lines() will not > delete that struct command_line. Instead, it increments the value of > 'executing'. bpstats_do_action() uses this behavior to see if it needs > to delete the command_line after executing all the statements in the > list. I've looked your patch over, and it looks correct to me. Having said that, I think that the correctness of this patch is much less obvious than the version that made a copy of the command chain associated with a breakpoint. I don't fault you for this; the changes in your current patch are somewhat more distributed which means that there's more code to consider (and more ways for something to get fouled up later on). I do have some other comments though; see below... > * defs.h: (struct > command_line): Added new field 'executing'. Why was this broken up between to lines? (Sorry for nit-picking.) Also, I wonder if there's a better name for this field? It is true that ``executing'' will be non-zero when the command is executing, but one might be mislead into thinking that it's a simple boolean when in fact it's a (sort of) counter. Also, the point of this field has more to do with determining when it's okay to delete a command list... > Index: breakpoint.c > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.c,v > retrieving revision 1.53 > diff -p -u -r1.53 breakpoint.c > --- breakpoint.c 2001/09/18 05:00:48 1.53 > +++ breakpoint.c 2001/09/21 23:49:48 > @@ -1800,6 +1800,7 @@ bpstat_do_actions (bpstat *bsp) > bpstat bs; > struct cleanup *old_chain; > struct command_line *cmd; > + struct command_line *cmd_head; > > /* Avoid endless recursion if a `source' command is contained > in bs->commands. */ > @@ -1824,6 +1825,10 @@ top: > breakpoint_proceeded = 0; > for (; bs != NULL; bs = bs->next) > { > + cmd_head = bs->commands; > + if (cmd_head) > + cmd_head->executing = 1; Upon first looking at this portion of your patch, I was thinking of ``executing'' as a sort of reference count, and it seemed to me that the above line ought to be ``cmd_head->executing++;''. But now that I think about it some more, I see that ``executing'' isn't really a reference count, but rather a sort of two-purpose flag which tells free_command_lines() that a command is executing; however, it may also be changed by free_command_lines(), so it's second purpose is to let the latter parts of bpstat_do_actions() know if a command chain deletion was deferred by free_command_lines. Anyway, I found this somewhat surprising. I think I would've been less surprised if ``executing'' was more of a conventional reference count. > + > cmd = bs->commands; > while (cmd != NULL) > { > @@ -1834,6 +1839,15 @@ top: > else > cmd = cmd->next; > } > + I'd like to see a comment right here describing what's going on. I understand it because I get to see all the logic wrapped up in the nice tidy patch to which I'm now replying, but I'm thinking it might not be so obvious to someone encountering this code later on... > + if (cmd_head->executing != 1) > + { > + cmd_head->executing = 0; > + free_command_lines (&cmd_head); > + } > + else > + cmd_head->executing = 0; > + > if (breakpoint_proceeded) > /* The inferior is proceeded by the command; bomb out now. > The bpstat chain has been blown away by wait_for_inferior. > Index: defs.h > =================================================================== > RCS file: /cvs/src/src/gdb/defs.h,v > retrieving revision 1.63 > diff -p -u -r1.63 defs.h > --- defs.h 2001/09/07 21:33:08 1.63 > +++ defs.h 2001/09/21 23:49:51 > @@ -832,6 +832,7 @@ struct command_line > enum command_control_type control_type; > int body_count; > struct command_line **body_list; A comment right here describing what the member (below) is about would really aid in understanding the code. You should make it clear that the real purpose of this field is in deciding whether a particular command chain may be deleted immediately or if it must be deferred if the command chain winds up being self-deleting. > + int executing; > }; > > extern struct command_line *read_command_lines (char *, int); > Index: cli/cli-script.c [...] The rest of your patch is fine. Kevin