From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27368 invoked by alias); 28 Feb 2011 16:53:09 -0000 Received: (qmail 27356 invoked by uid 22791); 28 Feb 2011 16:53:07 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,TW_YP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate7.uk.ibm.com (HELO mtagate7.uk.ibm.com) (194.196.100.167) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 28 Feb 2011 16:53:00 +0000 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate7.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p1SGqvrl024723 for ; Mon, 28 Feb 2011 16:52:57 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p1SGr7EC1785986 for ; Mon, 28 Feb 2011 16:53:07 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p1SGqtfR021848 for ; Mon, 28 Feb 2011 09:52:55 -0700 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p1SGqseO021809; Mon, 28 Feb 2011 09:52:54 -0700 Message-Id: <201102281652.p1SGqseO021809@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Mon, 28 Feb 2011 17:52:54 +0100 Subject: Re: [RFA] Implement support for PowerPC BookE ranged breakpoints To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Mon, 28 Feb 2011 17:08:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches ml) In-Reply-To: <1298493988.3172.31.camel@hactar> from "Thiago Jung Bauermann" at Feb 23, 2011 05:46:28 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2011-02/txt/msg00914.txt.bz2 Thiago Jung Bauermann wrote: > The callers of breakpoint_address_match that I didn't change are: > > breakpoint_restore_shadows (only software breakpoints have shadow > contents), bpstat_check_location (it calls ops->breakpoint_hit if > available, which will do the right thing for the bp_location at hand), > single_step_breakpoint_inserted_here_p (it calls > breakpoint_address_match for something which isn't a bp_location, also a > comment there says it checks only software breakpoints). This seems reasonable to me. > > > @@ -3306,11 +3318,6 @@ print_it_typical (bpstat bs) > > > int bp_temp = 0; > > > enum print_stop_action result; > > > > > > - /* bs->breakpoint_at can be NULL if it was a momentary breakpoint > > > - which has since been deleted. */ > > > - if (bs->breakpoint_at == NULL) > > > - return PRINT_UNKNOWN; > > > - > > > gdb_assert (bs->bp_location_at != NULL); > > > > > > bl = bs->bp_location_at; > > > @@ -3516,11 +3523,15 @@ print_bp_stop_message (bpstat bs) > > > { > > > struct breakpoint *b = bs->breakpoint_at; > > > > > > + /* bs->breakpoint_at can be NULL if it was a momentary breakpoint > > > + which has since been deleted. */ > > > + if (b == NULL) > > > + return PRINT_UNKNOWN; > > > + > > > /* Normal case. Call the breakpoint's print_it method, or > > > print_it_typical. */ > > > - /* FIXME: how breakpoint can ever be NULL here? */ > > > > This seems to be an unrelated change, isn't it? > > I consider it to be related. In this patch series I'm revamping the > breakpoint_ops methods to make them more suitable for implementing > breakpoints and watchpoints. print_it_typical is part of that, so an > improvement in a function which calls the print_it method and > print_it_typcal seemed to have a place in this patch. > > I don't mind submitting this in a separate patch if you want. Yes, please do so. I think this could go in right away. > > This adds the SAY_WHERE parameter so that the caller will go ahead to > > add location information. However, for a range breakpoint, this will > > just be the start of the range, which may or may not be particularly > > useful ... Shouldn't we either: > > - output the full range (start and end), or > > - output nothing ... we'll see the actual location where we stopped anyway > > > > (Either way, the API change to add SAY_WHERE is no longer needed.) > > I think it's useful. You have a point in that it makes more sense to > print the end of the range as well. I'd have to extend struct breakpoint > to save the source file and line number of the end of the range if I > were to print them. I'm not sure it's worth it, so I changed > print_mention_ranged_breakpoint to print only the start and end address > of the range. And dropped the say_where argument. What do you think? Don't you have to know the end of the range anyway, in order to be able to re-set the breakpoint? > +static enum print_stop_action > +print_it_ranged_breakpoint (struct breakpoint *b, > + const struct value *old_val) Hmm ... this is not against current mainline, but assumes some of the other patches is applied as well, right? > +{ > + struct bp_location *bl = b->loc; > + struct ui_stream *stb; > + struct cleanup *old_chain; > + > + gdb_assert (b->type == bp_hardware_breakpoint); > + > + /* Ranged breakpoints have only one location. */ > + gdb_assert (bl && bl->next == NULL); > + > + stb = ui_out_stream_new (uiout); > + old_chain = make_cleanup_ui_out_stream_delete (stb); You never use this stream, what's the point of allocating it? > + annotate_breakpoint (b->number); > + if (b->disposition == disp_del) > + ui_out_text (uiout, "\nTemporary ranged breakpoint "); > + else > + ui_out_text (uiout, "\nHardware assisted ranged breakpoint "); Either both or none of these should be using "hardware assisted". I'd probably argue for "none": for regular breakpoint, this distinction is not made here either ... > +static void > +print_one_ranged_breakpoint (struct breakpoint *b, > + struct bp_location **last_loc, > + char *wrap_indent) > +{ > + struct bp_location *bl = b->loc; > + struct value_print_options opts; > + struct ui_stream *stb = ui_out_stream_new (uiout); > + struct cleanup *old_chain = make_cleanup_ui_out_stream_delete (stb); Again the unused stream ... > + /* Ranged breakpoints have only one location. */ > + gdb_assert (bl && bl->next == NULL); > + > + get_user_print_options (&opts); > + > + if (opts.addressprint) > + ui_out_field_skip (uiout, "addr"); > + annotate_field (5); > + if (bl->enabled) > + print_breakpoint_location (b, bl, wrap_indent, stb); I think you should not check bl->enabled here like that. This is done for regular breakpoints only because enabled breakpoints with a single disabled location are handled by special code in print_one_breakpoint which is not active for breakpoints with custom functions. > +static void > +print_one_detail_ranged_breakpoint (const struct breakpoint *b, > + struct ui_out *uiout) > +{ > + struct bp_location *bl = b->loc; > + > + gdb_assert (bl); > + > + ui_out_text (uiout, "\taddress range: "); > + ui_out_field_range_core_addr (uiout, "addr", bl->gdbarch, > + bl->address, bl->length); Do we really need to make a new ui_out_ function for this; this seems a bit of a special case for that. Why don't you just generate the output here? (Note that here you might want to use a temporary stream like the one you had in the above functions but never used there ...) > +static void > +break_range_command (char *arg, int from_tty) > +{ > + char *orig_arg; > + char **addr_string_start; > + int bp_count, can_use_bp, ret; > + CORE_ADDR start_addr, start, end; > + int length; > + struct breakpoint *b; > + struct symtabs_and_lines sals_start, sals_end; > + struct gdbarch *gdbarch = get_current_arch (); > + struct gdb_exception e; > + struct cleanup *cleanup_start, *cleanup_end, *cleanup_orig_arg; > + > + /* We don't support software ranged breakpoints. */ > + if (target_ranged_break_num_registers () < 0) > + error (_("This target does not support hardware ranged breakpoints.")); > + > + bp_count = hw_breakpoint_used_count (); > + bp_count += target_ranged_break_num_registers (); > + can_use_bp = target_can_use_hardware_watchpoint (bp_hardware_breakpoint, > + bp_count, 0); > + if (can_use_bp < 0) > + error (_("Hardware breakpoints used exceeds limit.")); > + > + if (arg == NULL || arg[0] == '\0') > + error(_("No address range specified.")); > + > + /* Save the original argument string for later use by > + print_recreate_ranged_hw_breakpoint. */ > + orig_arg = xstrdup (arg); > + /* We'll only dispose of it if this function is aborted. */ > + cleanup_orig_arg = make_cleanup (xfree, orig_arg); > + > + sals_start.sals = NULL; > + sals_start.nelts = 0; > + addr_string_start = NULL; > + > + while (*arg == ' ' || *arg == '\t') > + arg++; > + > + parse_breakpoint_sals (&arg, &sals_start, &addr_string_start, NULL); > + > + cleanup_start = make_cleanup (xfree, sals_start.sals); > + make_cleanup (xfree, addr_string_start); > + make_cleanup (xfree, addr_string_start[0]); > + > + if (arg[0] != ',') > + error (_("Too few arguments.")); > + else if (sals_start.nelts == 0) > + error (_("Could not find location of the beginning of the range.")); > + else if (sals_start.nelts != 1) > + error (_("Cannot create a ranged breakpoint with multiple locations.")); > + > + breakpoint_sals_to_pc (&sals_start); > + start_addr = sals_start.sals[0].pc; > + > + arg++; /* Skip the comma. */ > + while (*arg == ' ' || *arg == '\t') > + arg++; > + > + /* Parse the end location. */ > + > + sals_end.sals = NULL; > + sals_end.nelts = 0; > + > + sals_end = decode_line_1 (&arg, 1, sals_start.sals[0].symtab, > + sals_start.sals[0].line, 0, 0); It seems oddly asymmetrical to use parse_breakpoint_sals above but decode_line_1 here. Shouldn't start and end of the range use the same symtab defaulting rules and other extra treatment done by parse_breakpoint_sals? > @@ -598,6 +598,7 @@ update_current_target (void) > INHERIT (to_can_use_hw_breakpoint, t); > INHERIT (to_insert_hw_breakpoint, t); > INHERIT (to_remove_hw_breakpoint, t); > + /* Do not inherit to_ranged_break_num_registers. */ > INHERIT (to_insert_watchpoint, t); > INHERIT (to_remove_watchpoint, t); > /* Do not inherit to_insert_mask_watchpoint. */ Again the question why the inheritance logic for this function should be different than for all the other breakpoint-related functions ... Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com