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

Eli Zaretskii
2010-10-30T07:13:01Z

> 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.