On Fri 05 Feb 2010 03:15:55 Joel Brobecker wrote: > Good news (see below). Great. :-) > > > I think, from your patch, that GDB will still evaluate the condition > > > once after the watchpoint and its condition trigger. I think that > > > we might want to fix that eventually, but I am actually more than > > > happy to ignore this minor issue for now. I like baby steps :). > > > > If I'm following your thought here, I added that on purpose. Before > > letting GDB evaluate the condition one more time, GDB wouldn't know > > which watchpoint triggered if there were two at the same location, > > with different conditions. > > I knew that! (if the name DiNozzo comes to mind, then I don't > know what you're talking about ;-) ) Oh, sorry then! > Just a few minor nits - pre-approved after the comments are addressed. I addressed your comments. I won't commit the patch yet because the kernel patch enabling this code to work hasn't been committed upstream yet. I expect it will be accepted soon, so I hope to get this in in time for 7.1. The same applies to the first patch in this series (adapting ppc-linux-nat.c to the new ptrace interface), which you approved a while ago... > > +/* This function is used to determine whether the condition associated > > + with bp_location B is of the form: > > + > > + watch *
if == > > + > > + If it is, then it sets DATA_VALUE to LITERAL and returns 1. > > + Otherwise, it returns 0. */ > > Can you add a note that VAR must be stored at ADDRESS too? Same for > similar functions... Done. > > + /* At this point, all verifications were positive, so we can use > > + hardware-assisted data-matching. Set the data value, and return > > + non-zero. */ > > The comment here is a bit too purpose-specific. It betrays its origin :), > but I think we should either drop it (I think that the function description > is sufficently clear), or change it to something more neutral. Again, > same for all other functions that used the same kind of comment. Dropped. > > +/* Return greater than zero if the condition associated with > > + the watchpoint `b' can be treated by the hardware; zero otherwise. > > + > > + Also stores the data value in `data_value'. */ > > I'd drop this comment in breakpoint.h. It duplicates the descriptions > in breakpoint.c... Dropped. > > +static int > > +ppc_linux_can_use_watchpoint_cond_accel (void) > > This function now needs a short description. It's kind of obvious, > what this function does, I agree, but we're trying to be consistent > in our style, and provide always function descriptions unless the > function implements a routine (function pointer) that's already > documented (eg a gdbarch method, or a target_ops routine). I added this concise description: /* Check whether we have at least one free DVC register. */ > > + if (p->hw_breaks[i].hw_break != NULL > > + && p->hw_breaks[i].hw_break->condition_mode > > + != PPC_BREAKPOINT_CONDITION_NONE) > > I'm not quite sure, here, but I think that the GNU Coding Style asks > us to parenthesize your not-equal condition, purely for the benefit of > GNU indent: > > if (p->hw_breaks[i].hw_break != NULL > && (p->hw_breaks[i].hw_break->condition_mode > != PPC_BREAKPOINT_CONDITION_NONE)) > > Can you double-check for me? It does mention that. Changed. > > + if (cond && ppc_linux_can_use_watchpoint_cond_accel () > > + && (watch_address_if_address_equal_literal (exp, cond, &cond_addr) > > + || watch_var_if_var_equal_literal (exp, cond, &cond_addr) > > + || watch_var_if_address_equal_literal (exp, cond, &cond_addr) > > + || watch_address_if_var_equal_literal (exp, cond, &cond_addr))) { > > Formatting nit: The curly brace should be on the next line. There are > a few instances where this needs to be fixed. Fixed this and the other instances. > > + p.condition_mode = PPC_BREAKPOINT_CONDITION_AND > > + | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable); > > Similar to above, I think you'll need to parenthesize the rhs. > Fixed this and the other instance. > > diff --git a/gdb/target.c b/gdb/target.c > > index e6659c9..7adc96e 100644 > > --- a/gdb/target.c > > +++ b/gdb/target.c > > @@ -44,6 +44,8 @@ > > #include "exec.h" > > #include "inline-frame.h" > > > > +struct expression; > > + > > This change is unnecessary (already declared in target.h)... Dropped. > > diff --git a/gdb/target.h b/gdb/target.h > > index 7103ab2..a65e900 100644 > > --- a/gdb/target.h > > +++ b/gdb/target.h > > @@ -36,6 +36,10 @@ struct trace_status; > > struct uploaded_tsv; > > struct uploaded_tp; > > > > +struct bp_location; > > +struct breakpoint; > > +struct expression; > > I don't think that declaring struct bp_location is necessary. > I just see that struct breakpoint should probably have been declared > before your patch - so OK. Dropped struct bp_location, kept the other two. > > + int (*to_remove_watchpoint) (CORE_ADDR, int, int, struct expression > > *, + struct expression *); > > + int (*to_insert_watchpoint) (CORE_ADDR, int, int, struct expression > > *, + struct expression *); > > Would you mind adding a comment explaining that documentation of what > these routines are expected to do is provided with the corresponding > target_* macros? I personally think that the complete description > of what these routines are supposed to do should be right there rather > than with the target_ macros, but that's only a very minor matter and > nothing to do with your patch. Added: /* Documentation of what the two routines below are expected to do is provided with the corresponding target_* macros. */ > > +#ifndef target_region_ok_for_hw_watchpoint > > #define target_region_ok_for_hw_watchpoint(addr, len) \ > > (*current_target.to_region_ok_for_hw_watchpoint) (addr, len) > > - > > +#endif > > This is not right - we no longer allow a macro to be overridden. Ok, dropped. > > /* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes. > > TYPE is 0 for write, 1 for read, and 2 for read/write accesses. Returns > > 0 for success, non-zero for failure. */ > > > > -#define target_insert_watchpoint(addr, len, type) \ > > - (*current_target.to_insert_watchpoint) (addr, len, type) > > +#define target_insert_watchpoint(addr, len, type, watch_exp, cond) \ > > + (*current_target.to_insert_watchpoint) (addr, len, type, watch_exp, > > cond) > > The macro description needs to be updated (to mention the two new > parameters). Updated. > > -#define target_remove_watchpoint(addr, len, type) \ > > - (*current_target.to_remove_watchpoint) (addr, len, type) > > +#define target_remove_watchpoint(addr, len, type, watch_exp, cond) \ > > + (*current_target.to_remove_watchpoint) (addr, len, type, watch_exp, > > cond) > > Can you add a short documentation for this macro? It is already documented, in a comment above target_insert_watchpoint (which is defined right before target_remove_watchpoint): /* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes. TYPE is 0 for write, 1 for read, and 2 for read/write accesses. WATCH_EXP is the watchpoint's expression and COND is the expression for its condition, or NULL if there's none. Returns 0 for success, non-zero for failure. */ -- []'s Thiago Jung Bauermann IBM Linux Technology Center 2010-02-08 Sergio Durigan Junior Thiago Jung Bauermann * target.h: Add opaque declarations for bp_location, breakpoint and expression. (struct target_ops) , : Add new arguments to pass the watchpoint expression and its condition. Update all callers and implementations. (target_region_ok_for_hw_watchpoint): Surround with ifndef. * breakpoint.c (exp_is_address): New function. (exp_is_var): Ditto. (exp_is_address_equal_literal): Ditto. (exp_is_var_equal_literal): Ditto. (watch_address_if_var_equal_literal): Ditto. (watch_var_if_address_equal_literal): Ditto. (watch_var_if_var_equal_literal): Ditto. (watch_address_if_address_equal_literal): Ditto. * breakpoint.h (watch_address_if_address_equal_literal): Declare. (watch_var_if_var_equal_literal): Ditto. (watch_address_if_var_equal_literal): Ditto. (watch_var_if_address_equal_literal): Ditto. * ppc-linux-nat.c (ppc_linux_can_use_watchpoint_cond_accel): New function. (ppc_linux_insert_watchpoint): Check if it is possible to use hardware-accelerated condition checking. (ppc_linux_remove_watchpoint): Check if the watchpoint to delete has hardware-accelerated condition checking.