From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8846 invoked by alias); 30 Jan 2004 04:13:26 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 8542 invoked from network); 30 Jan 2004 04:13:21 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 30 Jan 2004 04:13:21 -0000 Received: from drow by nevyn.them.org with local (Exim 4.30 #1 (Debian)) id 1AmQ1t-0007g9-04 for ; Thu, 29 Jan 2004 23:13:17 -0500 Date: Fri, 30 Jan 2004 04:13:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: Re: [RFA]: pending breakpoint support [1/3] Message-ID: <20040130041316.GA28843@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com References: <400EE69A.5030902@redhat.com> <20040122222741.GA17425@nevyn.them.org> <4016CCE0.5060900@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4016CCE0.5060900@redhat.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2004-01/txt/msg00756.txt.bz2 On Tue, Jan 27, 2004 at 03:41:04PM -0500, J. Johnston wrote: > Anyway, I have a new and improved patch attached that > addresses the issues below. I have included answers to questions below as > well. Thanks. I have just one question and a couple of cosmetic issues left. I'm also still curious about pending breakpoints which resolve to multiple functions, et cetera. We can revisit at a later date. > >When reading the testsuite diffs I asked: > > Might deferred be a better name for this concept anyway? > > > >Comments? > > > > This is a case of "you can't please everybody all the time". I am > certainly happy with it. OK. I have some ideas for better wording of the message, but I can take care of it later. > Well, it only gets there on a "not-found" error from decode_line_1 so the > onus is on decode_line_1 and it's children to do proper cleanups which is > outside of this patch. Gotcha. About the new patch: > + /* Flag value for pending breakpoint. > + first bit : 0 non-temporary, 1 temporary. > + second bit : 0 normal breakpoint, 1 hardware breakpoint. */ > + int flag; OK, now I can see what flag is used for. This raises a new question. Should temporary breakpoints be allowed to be pending? Oh, I guess the answer is yes. tbreak my_dso_initialization; run. So this is fine. > +/* Try and resolve a pending breakpoint. */ > +static int > +resolve_pending_breakpoint (struct breakpoint *b) I love the new version of this function... > + input_radix = b->input_radix; > + rc = break_command_1 (b->addr_string, b->flag, b->from_tty, b); > + > + if (rc == GDB_RC_OK) > + /* Pending breakpoint has been resolved. */ > + printf_filtered ("Resolves pending breakpoint \"%s\"\n", b->addr_string); Spelling error, though. > @@ -4779,6 +4888,26 @@ create_breakpoints (struct symtabs_and_l > b->ignore_count = ignore_count; > b->enable_state = bp_enabled; > b->disposition = disposition; > + /* If resolving a pending breakpoint, a check must be made to see if > + the user has specified a new condition or commands for the > + breakpoint. A new condition will override any condition that was > + initially specified with the initial breakpoint command. */ > + if (pending_bp) > + { > + char *arg; > + if (pending_bp->cond_string) > + { > + arg = pending_bp->cond_string; > + b->cond_string = savestring (arg, strlen (arg)); > + b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0); > + if (*arg) > + error ("Junk at end of pending breakpoint condition expression"); > + } > + /* If there are commands associated with the breakpoint, they should > + be copied too. */ > + if (pending_bp->commands) > + b->commands = copy_command_lines (pending_bp->commands); > + } > mention (b); > } > } Here's the one question. The only way to get here with a PENDING_BP is from break_command_1 from resolve_pending_breakpoint. So I don't think it is possible for there to be a condition already set on B, which makes the comment about "overriding" such a condition a little strange. Am I right, or is there some other way to get a condition to here? > -static void > -break_command_1 (char *arg, int flag, int from_tty) > + PENDING_BP is non-NULL when this function is being called to resolve > + a pending breakpoint. */ > + > +static int > +break_command_1 (char *arg, int flag, int from_tty, struct breakpoint *pending_bp) > { > int tempflag, hardwareflag; > struct symtabs_and_lines sals; > struct expression **cond = 0; > + struct symtab_and_line pending_sal; > /* Pointers in arg to the start, and one past the end, of the > condition. */ Aha! That's where that baffling comment came from, it was already present in break_command_1. Here we have a var named ARG, so it makes more sense, but the variables cond_start and cond_end that it refers to have walked down about a hundred lines. > + if (bpt->pending) > + { > + if (bpt->enable_state != bp_enabled) > + { > + /* When enabling a pending breakpoint, we need to check if the breakpoint > + is resolvable since shared libraries could have been loaded > + after the breakpoint was disabled. */ > + struct breakpoint *new_bp; > + breakpoints_changed (); > + if (resolve_pending_breakpoint (bpt) == GDB_RC_OK) new_bp is no longer used, you can remove it. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer