On Tue 12 Jan 2010 08:50:45 Joel Brobecker wrote: > Sorry for the delay looking into this; I needed a little bit of time > to let this brew a little... On my part, sorry for the delay in taking this long to rework the patch and send it back. I was on vacation... Thanks for the in-depth review! > 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. > Conceptually, I think that the target now needs to be able to look > at the watchpoint expression, and the watchpoint condition, in order > to determine whether the condition evaluation can be evaluated > by hardware. I propose that we expand the insert_watchpoint routine > to pass this data, as 2 extra parameters; and let these routines > decide whether to use hardware acceleration or not for the condition. I liked the idea. This patches implements that approach. > One of the downsides of this approach is that GDB will no longer > be able to print that the condition will be hardware-accelerated > when the user creates the watchpoint. I think that's OK for now. Yeah, I would like to have that message printed though. Wil have to think about it. > I am also a bit suspicious of adding the following bits to the bp_location > > structure directly: > > + /* Flag to indicate if the condition is going to be accelerated > > + by hardware. If its value is non-zero, then GDB checks the > > + condition using hardware acceleration; otherwise it uses the > > + regular software-based checking. */ > > + int cond_hw_accel : 1; > > + > > + /* If the condition can be hardware-accelerated, then we must > > + get the condition's variable address so that GDB can > > + properly set the evaluation via hardware. */ > > + CORE_ADDR cond_hw_addr; > > I think that they are too target specific. Initially, we can do without, > with our not-so-efficient approach, by recomputing these address every > time we insert the watchpoint. I think that'd still be an interesting > first step. Then we can look at storing that info as a private data > structure, allocated by the target. I think that this would be doable > without too many changes to the current framework - we just need to > be a little careful as watchpoint expressions and conditions get > re-evaluated sometimes. Those are gone. I don't think recomputing the address everytime GDB stops will get in our way at this point. Parsing the condition expression should be a quick enough operation I think. > > +static int > > +exp_is_address_p (struct breakpoint *b) > > If you want, you can drop the _p prefix in this type of situation. > I think the use of the verb _is_ in the names makes it obvious that > this function is a predicate. I didn't like those _p suffixes much either. They're all gone. > > +int > > +default_watch_address_if_var_equal_literal_p (struct bp_location *b, > > + CORE_ADDR *data_value) > > This is nitpicking on function names, but I don't understand the > "default" here. Would there be some non-default implementations > of the same routine? Perhaps: watch_address_if_var_equal_literal_p > is just going to be sufficient? Not sure either. I removed the default_ prefix from those function names. > > + /* Make sure that the two addresses are the same. */ > > + if (exp_address != cond_address) > > + { > > + printf_filtered (_("\ > > +Memory location for the watchpoint expression and its condition need\n\ > > +to be the same.\n")); > > + return 0; > > + } > > Not sure whether you really meant to keep this message. It sounds like > an error, when in fact this could occur in a perfectly legitimate > situation, no? For instance, if the user entered: > > (gdb) watch A if B == 3 > > The message could be kept as a debug trace, but it should be more > neutral, IMO, something like: > > address of variable in condition does not match expression being > watched. > > would provide all the information without suggesting that something > might have gone wrong. I agree. In this version GDB silently uses software condition checking. But there should be a way to let the user know that the watchpoint he created is having hardware-accelerated condition checking or not (hence my comment up there that I'd like some message to be shown when the watchpoint is created). > Let me know what you think. I realize that I'm significantly reducing > the scope of the first attempt at getting h/w-accelerated watchpoint > conditions, but I'm hoping it'll help focusing on each issue separately. > I'm perfectly happy to make this feature another patch series on its own, > in parallel to the other hardware-breakpoint patches. We can also take it > one patch at a time. Whatever works best for you. Mmm.... perhaps I'm too thick today, but I don't see how you are significantly reducing the scope here, except for the hardware-accelerated message not being shown at watchpoint creation time. > BTW: > > @@ -186,6 +186,11 @@ struct bp_target_info > > is used to determine the type of breakpoint to insert. */ > > CORE_ADDR placed_address; > > > > + /* If this is a ranged hardware breakpoint, then we can use this > > + field in order to store the length of the range that will be > > + watched for execution. */ > > + ULONGEST length; > > + > > Oops! This should be part of another patch ;-). Ooops. Gone. -- []'s Thiago Jung Bauermann IBM Linux Technology Center 2010-02-04 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.