From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20006 invoked by alias); 22 Jan 2004 22:27:43 -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 19993 invoked from network); 22 Jan 2004 22:27:41 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 22 Jan 2004 22:27:41 -0000 Received: from drow by nevyn.them.org with local (Exim 4.30 #1 (Debian)) id 1AjnIb-0004kN-BI; Thu, 22 Jan 2004 17:27:41 -0500 Date: Thu, 22 Jan 2004 22:27:00 -0000 From: Daniel Jacobowitz To: Jeff Johnston Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: pending breakpoint support [1/3] Message-ID: <20040122222741.GA17425@nevyn.them.org> Mail-Followup-To: Jeff Johnston , gdb-patches@sources.redhat.com References: <400EE69A.5030902@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <400EE69A.5030902@redhat.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2004-01/txt/msg00616.txt.bz2 On Wed, Jan 21, 2004 at 03:52:42PM -0500, Jeff Johnston wrote: > This is the first of the last 3 patches for pending breakpoint support. > This patch adds the actual support. The other two patches are the > documentation (previously approved, but before code was accepted) and > changes to the testsuite. > > This patch accounts for previous comments on the breakpoint code. > > Ok to commit? Sorry, but a number of my comments from last time still apply, and I have some others. The concept is looking great now, though! > Index: breakpoint.h > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.h,v > retrieving revision 1.26 > diff -u -p -r1.26 breakpoint.h > --- breakpoint.h 6 Nov 2003 18:24:55 -0000 1.26 > +++ breakpoint.h 21 Jan 2004 02:34:10 -0000 > @@ -385,6 +385,15 @@ struct breakpoint > > /* Methods associated with this breakpoint. */ > struct breakpoint_ops *ops; > + > + /* Initial from_tty value. */ > + int from_tty; > + > + /* Initial flag value. */ > + int flag; > + > + /* Is breakpoint pending on shlib loads? */ > + int pending; Can you read this comment and figure out from it what flag is, or even where to look for more information? I can't. Please indicate where they come from, and that they are used for pending breakpoints. Something more descriptive than "flag" would be good. > @@ -3458,7 +3463,15 @@ print_one_breakpoint (struct breakpoint > if (addressprint) > { > annotate_field (4); > - ui_out_field_core_addr (uiout, "addr", b->loc->address); > + if (b->pending) > + { > + if (TARGET_ADDR_BIT <= 32) > + ui_out_field_string (uiout, "addr", " "); > + else > + ui_out_field_string (uiout, "addr", " "); > + } > + else > + ui_out_field_core_addr (uiout, "addr", b->loc->address); > } > annotate_field (5); > *last_addr = b->loc->address; Last time I said: I'm curious what effect this will have on MI consumers. Also, the resulting MI output will be somewhat gruesome with the embedded spaces. Maybe: ui_out_field_string (uiout, "addr", ""); ui_out_text (uiout, "addr", " "); but I'm not sure that works without testing it. The other possible alternative is not outputting addr at all for MI-like uiouts. May be better to do this (more flexible). I still don't know what to do about the latter, but please try the former. > @@ -4288,23 +4316,139 @@ disable_breakpoints_in_shlibs (int silen > } > } > > +struct captured_parse_breakpoint_args > + { > + char **arg_p; > + struct symtabs_and_lines *sals_p; > + char ***addr_string_p; > + int *not_found_ptr; > + }; > + > +struct lang_and_radix > + { > + enum language lang; > + int radix; > + }; > + > +/* Cleanup helper routine to restore the current language and > + input radix. */ > +static void > +do_restore_lang_radix_cleanup (void *old) > +{ > + struct lang_and_radix *p = old; > + set_language (p->lang); > + input_radix = p->radix; > +} > + > +/* Try and resolve a pending breakpoint. */ > +static struct breakpoint * > +resolve_pending_breakpoint (struct breakpoint *b) > +{ > + /* Try and reparse the breakpoint in case the shared library > + is now loaded. */ > + struct symtabs_and_lines sals; > + struct symtab_and_line pending_sal; > + /* Pointers in arg to the start, and one past the end, of the > + condition. */ Last time I read this patch I read this comment and said "Huh?" That still applies :) What pointers in what arg to the end of what condition? > + char **cond_string = (char **) NULL; > + char *copy_arg = b->addr_string; > + char **addr_string; > + char *errmsg; > + struct captured_parse_breakpoint_args parse_args; > + int rc; > + int not_found = 0; > + struct ui_file *old_gdb_stderr; > + > + sals.sals = NULL; > + sals.nelts = 0; > + addr_string = NULL; > + > + parse_args.arg_p = ©_arg; > + parse_args.sals_p = &sals; > + parse_args.addr_string_p = &addr_string; > + parse_args.not_found_ptr = ¬_found; > + > + rc = catch_exceptions (uiout, do_captured_parse_breakpoint, > + &parse_args, NULL, RETURN_MASK_ALL); > + > + if (rc == GDB_RC_OK) > + { > + struct lang_and_radix old_lr; > + struct cleanup *old_chain; > + char *arg; > + struct breakpoint *b1; > + > + printf_filtered ("Pending breakpoint \"%s\" resolved\n", b->addr_string); > + > + /* Set language, input-radix, then reissue breakpoint command. > + Ensure the language and input-radix are restored afterwards. */ > + old_lr.lang = current_language->la_language; > + old_lr.radix = input_radix; > + old_chain = make_cleanup (do_restore_lang_radix_cleanup, &old_lr); Thanks for adding the cleanup. > + > + set_language (b->language); > + input_radix = b->input_radix; > + break_command_1 (b->addr_string, b->flag, b->from_tty); > + b1 = breakpoint_chain; > + while (b1->next) > + b1 = b1->next; I would really prefer that you not walk the breakpoint chain looking for the last breakpoint. > + /* If there is condition specified, it should be copied over. */ > + if (b->cond_string) > + { > + arg = b->cond_string; > + b1->cond_string = savestring (arg, strlen (arg)); > + b1->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0); > + if (*arg) > + error ("Junk at end of expression"); > + } > + /* If there are commands associated with the breakpoint, they should > + be copied too. */ > + if (b->commands) > + { > + b1->commands = copy_command_lines (b->commands); > + } No, I've already asked you to do entire section differently: This, on the other hand, is not OK. First of all you're wasting work - you parse the breakpoint location twice. Then you go grubbing around in the breakpoint chain looking for it, which assumes it will be added at the end of the chain - it will, but that sort of detail shouldn't be exposed. And break_command_1 can currently create multiple breakpoints, so you'll get that wrong too. decode_line_1 can end up prompting the user; you should have a testcase for that and make sure it does something sane. Easiest way is probably to put it in gdb.cp using an overloaded function. All set breakpoints should get the condition and commands. Can you arrange for most of this function to happen inside break_command_1, possibly by giving it a pending breakpoint as an optional argument? Other ideas? Also, I'm 99% sure you mean b1->loc->address above. b->loc->address is the pending breakpoint's address, which is unset. > + > + do_cleanups (old_chain); > + return b1; /* Pending breakpoint resolved. */ > + } > + > + /* Otherwise, we didn't successfully resolve pending breakpoint. */ > + return NULL; > +} > + > /* Try to reenable any breakpoints in shared libraries. */ > void > re_enable_breakpoints_in_shlibs (void) > { > struct breakpoint *b; > + struct breakpoint *del_b = NULL; > > ALL_BREAKPOINTS (b) > + { > + if (del_b) > + { > + delete_breakpoint (del_b); > + del_b = NULL; > + } I said: No, use ALL_BREAKPOINTS_SAFE and you can get rid of del_b. > @@ -4923,19 +5094,51 @@ break_command_1 (char *arg, int flag, in > sals.sals = NULL; > sals.nelts = 0; > addr_string = NULL; > - parse_breakpoint_sals (&arg, &sals, &addr_string); > > - if (!sals.nelts) > + parse_args.arg_p = &arg; > + parse_args.sals_p = &sals; > + parse_args.addr_string_p = &addr_string; > + parse_args.not_found_ptr = ¬_found; > + > + rc = catch_exceptions_with_msg (uiout, do_captured_parse_breakpoint, > + &parse_args, NULL, &err_msg, > + RETURN_MASK_ALL); Awesome, the need to play with the text of the error message is gone. > + > + if (rc != GDB_RC_OK) > + { > + /* Check for file or function not found. */ > + if (not_found) > + { > + error_output_message (NULL, err_msg); > + xfree (err_msg); > + if (!query ("Make breakpoint pending on future shared library load? ")) > + return; I said: I'm still not really happy with this wording. What does that mean? Plus, it'll look really out of place if we don't have shared library support. When reading the testsuite diffs I asked: Might deferred be a better name for this concept anyway? Comments? > + copy_arg = (char *)xmalloc (strlen (addr_start)); > + strcpy (copy_arg, addr_start); I said: The cast is not necessary nowadays. There are also a number of functions for doing this, which don't suffer from the off-by-one error above :) Try xstrdup instead. > + addr_string = ©_arg; I said: Is addr_string guaranteed to still be NULL at this point, or are we leaking? The answer to this question is somewhere in the depths of decode_line_1, so I'm not sure what the answer is. > + sals.nelts = 1; > + sals.sals = &pending_sal; > + pending_sal.pc = 0; > + pending = 1; > + } > + else > + return; > + } > + else if (!sals.nelts) > return; > > + > /* Create a chain of things that always need to be cleaned up. */ > old_chain = make_cleanup (null_cleanup, 0); > > - /* Make sure that all storage allocated to SALS gets freed. */ > - make_cleanup (xfree, sals.sals); > - > - /* Cleanup the addr_string array but not its contents. */ > - make_cleanup (xfree, addr_string); > + if (!pending) > + { > + /* Make sure that all storage allocated to SALS gets freed. */ > + make_cleanup (xfree, sals.sals); > + > + /* Cleanup the addr_string array but not its contents. */ > + make_cleanup (xfree, addr_string); > + } > > /* Allocate space for all the cond expressions. */ > cond = xcalloc (sals.nelts, sizeof (struct expression *)); Yes, you don't want to free addr_string, but a few lines further down you probably want to add a cleanup to free copy_arg on the breakpoint_chain. > @@ -7363,70 +7600,92 @@ do_enable_breakpoint (struct breakpoint > error ("Hardware breakpoints used exceeds limit."); > } > > - if (bpt->enable_state != bp_permanent) > - bpt->enable_state = bp_enabled; > - bpt->disposition = disposition; > - check_duplicates (bpt); > - breakpoints_changed (); > - > - if (bpt->type == bp_watchpoint || > - bpt->type == bp_hardware_watchpoint || > - bpt->type == bp_read_watchpoint || > - bpt->type == bp_access_watchpoint) > - { > - if (bpt->exp_valid_block != NULL) > - { > - struct frame_info *fr = > - fr = frame_find_by_id (bpt->watchpoint_frame); > - if (fr == NULL) > + 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 (); I said: Sure this is necessary? If we resolve the breakpoint successfully, this will get called from set_raw_breakpoint. > + if ((new_bp = resolve_pending_breakpoint (bpt)) != NULL) I said: Might as well not use assignment-in-if-statement in new code. I think this is in the ARI. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer