Re: [RFA] Implement support for PowerPC BookE ranged breakpoints

Ulrich Weigand
2011-02-17T15:30:23Z

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