This is the mail archive of the
mailing list for the GDB project.
Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints
- From: Eli Zaretskii <eliz at gnu dot org>
- To: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sat, 30 Oct 2010 09:12:56 +0200
- Subject: Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints
- References: <1282074110.2606.703.camel@hactar> <1287807761.10521.423.camel@hactar> <firstname.lastname@example.org> <1288403952.2598.58.camel@hactar>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> From: Thiago Jung Bauermann <email@example.com>
> Cc: firstname.lastname@example.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
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.
> > > + /* 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
> 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.texinfo (Set Watchpoints): Document mask parameter.
> (PowerPC Embedded): Document masked watchpoints and ranged watchpoints.
This part is approved. Thanks.