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

Ulrich Weigand
2011-02-28T16:53:09Z

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