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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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