Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints

[Eli Zaretskii <eliz at gnu dot org>]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 29 Oct 2010 23:59:12 -0200
> 
> The version of the patch you reviewed wouldn't allow other architectures
> to create ranged software watchpoints (it asked the hardware whether the
> capability was available), though that was an unnecessary restriction.
> This version allows ranged software watchpoints for all architectures.
> (The change was made in the watch_range_command_1 function).

That's good to hear.  I think this is NEWS worthy, btw.

> > And what about masked watchpoints -- can they also be used in software
> > mode?
> 
> AFAIK, to implement masked watchpoints in software GDB would need to
> decode each instruction to see which address it was trying to access.
> This patch doesn't implement that, it would be hard work. If the target
> doesn't allow masked hardware watchpoints, the watch command will error
> out.

That's fine with me, I just wanted to make sure that documentation
faithfully reflects the implementation.

> > > +    case bp_hardware_watchpoint:
> > > +      ui_out_text (uiout, "Masked hardware watchpoint ");
> > 
> > Isn't it better to say "Masked hardware (write) watchpoint"?
> 
> Here I'm following the current GDB behaviour. It says just "Hardware
> watchpoint" for a bp_hardware_watchpoint.

OK.

> > > +  /* Display one line of extra information about this breakpoint,
> > > +     for "info breakpoints".  */
> > > +  void (*print_one_detail) (const struct breakpoint *, struct ui_out *);
> > 
> > Examples of such "extras" would be beneficial here.  Also, are there
> > any limitations on this extra information, like maximum length?
> 
> I changed this part to:
> 
>   /* Display extra information about this breakpoint, below the normal
>      breakpoint description in "info breakpoints".  In the example below,
>      the line with "memory range: [0x10094354, 0x100943a2]" was printed
>      by print_one_detail_ranged_watchpoint.
> 
>      (gdb) info breakpoints
>      Num     Type           Disp Enb Address    What
>      2       hw watchpoint  keep y              b
>              memory range: [0x10094354, 0x100943a2]
> 
>      */
>   void (*print_one_detail) (const struct breakpoint *, struct ui_out *);
> 
> What do you think?

I'm happy now.  Thanks.

> > > +  /* The watchpoint will trigger if the address of the memory access is
> > > +     within the defined range, as follows: p.addr <= address < p.addr2.  */
> > > +  p.addr            = (uint64_t) addr;
> > > +  p.addr2           = (uint64_t) addr + len;
> > 
> > But the documentation says that the end address is included, not
> > excluded.  Can we eliminate this source of confusion?
> 
> This code is in ppc-linux-nat.c right before calling ptrace to set the
> watchpoint, so it just documents how the kernel/processor interprets the
> parameters.
> 
> The user-facing code respects the semantics explained in the
> documentation. Also, target_insert_ranged_watchpoint takes a start
> address and a length as arguments and IMHO doesn't have this confusion.

Well, maybe this is worth a comment in this place.  Something like
"Note that this just documents how ptrace interprets its arguments;
the watchpoint is set to watch the defined range _inclusively_, as
specified by the user interface."

> gdb/doc/
> 	* gdb.texinfo (Set Watchpoints): Document mask parameter.
> 	(PowerPC Embedded): Document masked watchpoints and ranged watchpoints.

This part is approved.  Thanks.