From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16359 invoked by alias); 17 Feb 2011 15:30:23 -0000 Received: (qmail 16329 invoked by uid 22791); 17 Feb 2011 15:30:20 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate5.uk.ibm.com (HELO mtagate5.uk.ibm.com) (194.196.100.165) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 17 Feb 2011 15:30:12 +0000 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate5.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p1HFU9WE003334 for ; Thu, 17 Feb 2011 15:30:09 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 p1HFUGbw1859750 for ; Thu, 17 Feb 2011 15:30:16 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 p1HFU5bY004021 for ; Thu, 17 Feb 2011 08:30:05 -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 p1HFU3mE003981; Thu, 17 Feb 2011 08:30:04 -0700 Message-Id: <201102171530.p1HFU3mE003981@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 17 Feb 2011 16:30:03 +0100 Subject: Re: [RFA] Implement support for PowerPC BookE ranged breakpoints To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Thu, 17 Feb 2011 15:49:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches ml) In-Reply-To: <1296177985.2843.82.camel@hactar> from "Thiago Jung Bauermann" at Jan 27, 2011 11:26:25 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/msg00422.txt.bz2 Thiago Jung Bauermann wrote: > This is (finally!) the last patch in my series to support BookE hardware > debug features. It adds the following command: > > (gdb) help break-range > Set a breakpoint for an address range. > break-range START-LOCATION, END-LOCATION > where START-LOCATION and END-LOCATION can be one of the following: > LINENUM, for that line in the current file, > FILE:LINENUM, for that line in that file, > +OFFSET, for that number of lines after the current line > or the start of the range > FUNCTION, for the first line in that function, > FILE:FUNCTION, to distinguish among like-named static functions. > *ADDRESS, for the instruction at that address. > > The breakpoint will stop execution of the inferior whenever it > executes any address within the [start-address, end-address] range > (including START-LOCATION and END-LOCATION). The user interface for this command seems fine to me. A couple of detail comments: > @@ -2771,11 +2779,15 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc) > && bl->loc_type != bp_loc_hardware_breakpoint) > continue; > > - /* ALL_BP_LOCATIONS bp_location has bl->OWNER always non-NULL. */ > + /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ > if ((breakpoint_enabled (bl->owner) > || bl->owner->enable_state == bp_permanent) > - && breakpoint_address_match (bl->pspace->aspace, bl->address, > - aspace, pc)) > + && (breakpoint_address_match (bl->pspace->aspace, bl->address, > + aspace, pc) > + || (is_ranged_breakpoint (bl->owner) > + && breakpoint_address_match_range (bl->pspace->aspace, > + bl->address, bl->length, > + aspace, pc)))) It might make sense to have a helper routine that does the right thing for both breakpoint types by matching an aspace/pc pair against a breakpoint location ... More problematically, while you've changed this use of breakpoint_address_match, there are many more of them in breakpoint.c, and it seems at least some of those would also need to be updated to handle range breakpoints (what about the one in breakpoint_thread_match for example?). > @@ -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? > - if (b != NULL && b->ops != NULL && b->ops->print_it != NULL) > - return b->ops->print_it (b, bs->old_val); > + if (b->ops != NULL && b->ops->print_it != NULL) > + return b->ops->print_it (bs->bp_location_at, bs->old_val); I'm not sure this API change is necessary. Yes, you need to get at the location to print range breakpoints, but those will always have exactly one location ... The print_it routine could instead just make the same assumption e.g. print_exception_catchpoint already does, and just access b->loc. > +/* Implement the "print_it" breakpoint_ops method for > + ranged breakpoints. */ > + > +static enum print_stop_action > +print_it_ranged_breakpoint (const struct bp_location *bl, > + const struct value *old_val) > +{ > + const struct breakpoint *b = bl->owner; > + struct ui_stream *stb; > + struct cleanup *old_chain; > + > + gdb_assert (b->type == bp_breakpoint || b->type == bp_hardware_breakpoint); > + > + stb = ui_out_stream_new (uiout); > + old_chain = make_cleanup_ui_out_stream_delete (stb); > + > + if (bl->address != bl->requested_address) > + breakpoint_adjustment_warning (bl->requested_address, > + bl->address, > + b->number, 1); Can this happen? Does it make sense for a range breakpoint to have its address adjusted? If so, does that affect the end as well? > + annotate_breakpoint (b->number); > + if (b->disposition == disp_del) > + ui_out_text (uiout, "\nTemporary ranged breakpoint "); > + else if (b->type == bp_hardware_breakpoint) > + ui_out_text (uiout, "\nHardware assisted ranged breakpoint "); > + else > + ui_out_text (uiout, "\nRanged breakpoint "); > + if (ui_out_is_mi_like_p (uiout)) > + { > + ui_out_field_string (uiout, "reason", > + async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT)); > + ui_out_field_string (uiout, "disp", bpdisp_text (b->disposition)); > + } > + ui_out_field_int (uiout, "bkptno", b->number); > + ui_out_text (uiout, ", "); > + > + do_cleanups (old_chain); > + > + return PRINT_SRC_AND_LOC; > +} > +/* Implement the "print_one" breakpoint_ops method for > + ranged breakpoints. */ > + > +static void > +print_one_ranged_breakpoint (struct breakpoint *b, > + struct bp_location **last_loc, > + char *wrap_indent, struct ui_stream *stb) > +{ > + struct value_print_options opts; > + > + /* We're prepared to deal with only one location. */ > + gdb_assert (b->loc && !b->loc->next); > + > + get_user_print_options (&opts); > + > + if (opts.addressprint) > + ui_out_field_skip (uiout, "addr"); > + annotate_field (5); > + if (b->loc->enabled) > + print_breakpoint_location (b, b->loc, wrap_indent, stb); > + if (b->loc) > + *last_loc = b->loc; > +} I don't like the API change just to pass through those variables, which don't hold any state information and could just as well be recreated by the print_breakpoint_location routine itself ... > +/* Implement the "print_mention" breakpoint_ops method for > + ranged breakpoints. */ > + > +static void > +print_mention_ranged_breakpoint (struct breakpoint *b, int *say_where) > +{ > + switch (b->type) > + { > + case bp_breakpoint: > + if (ui_out_is_mi_like_p (uiout)) > + { > + *say_where = 0; > + break; > + } > + if (b->disposition == disp_del) > + printf_filtered (_("Temporary ranged breakpoint")); > + else > + printf_filtered (_("Ranged breakpoint")); > + printf_filtered (_(" %d"), b->number); > + *say_where = 1; > + break; > + case bp_hardware_breakpoint: > + if (ui_out_is_mi_like_p (uiout)) > + { > + *say_where = 0; > + break; > + } > + > + printf_filtered (_("Hardware assisted ranged breakpoint %d"), b->number); > + *say_where = 1; > + break; > + default: > + internal_error (__FILE__, __LINE__, _("Invalid breakpoint type.")); > + } > +} 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.) > + /* We need to use parse_to_comma_and_eval but decode_line_1 uses > + parse_and_eval_address_1 (see decode_indirect), so we just call it > + directly if the user provided an explicit PC. */ > + if (arg[0] == '*') > + { > + arg++; > + start_addr = value_as_address (parse_to_comma_and_eval (&arg)); > + > + sals_start.sals = (struct symtab_and_line *) > + xmalloc (sizeof (struct symtab_and_line)); > + sals_start.nelts = 1; > + sals_start.sals[0] = find_pc_line (start_addr, 0); > + sals_start.sals[0].pc = start_addr; > + sals_start.sals[0].section = find_pc_overlay (start_addr); > + sals_start.sals[0].explicit_pc = 1; > + > + cleanup_start = make_cleanup (xfree, sals_start.sals); > + } > + else > + { > + > + 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]); > + } Hmm. This should be fixed in decode_line_1 itself. The routine currently only says that it allows a trailing "if ...", but it should probably be modified to also stop parsing at the comma ... > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index 3195124..0b19c35 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -218,6 +219,10 @@ struct bp_target_info > is used to determine the type of breakpoint to insert. */ > CORE_ADDR placed_address; > > + /* If this is a ranged breakpoint, then this field contains the > + length of the range that will be watched for execution. */ > + ULONGEST length; > + > /* If the breakpoint lives in memory and reading that memory would > give back the breakpoint, instead of the original contents, then > the original contents are cached here. Only SHADOW_LEN bytes of > @@ -325,8 +330,8 @@ struct bp_location > bp_loc_other. */ > CORE_ADDR address; > > - /* For hardware watchpoints, the size of data ad ADDRESS being > - watches. */ > + /* For hardware watchpoints, the size of the memory region being watched. > + For hardware ranged breakpoints, the size of the breakpoint range. */ > int length; Why is it ULONGEST above, and int here? Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com