Hi, On Thu, 2010-02-11 at 19:23 +0100, Ulrich Weigand wrote: > Thiago Jung Bauermann wrote: > > > @@ -1666,15 +1697,33 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw) > > > > if (have_ptrace_new_debug_booke) > > { > > + int byte_to_enable; > > struct ppc_hw_breakpoint p; > > + CORE_ADDR cond_addr; > > The naming confused me as cond_addr turns out to hold the *data* value > to be compared against, not any address ;-) Indeed. Changed name to data_value. > > + > > + 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))) > > This logic, together with the variety of special-purpose subroutines, > strikes me as overly restrictive on the syntactical format of the > condition expression, for example: > - why shouldn't "if VAR.MEMBER == LITERAL" be allowed? > - why shouldn't "if VAR == " be allowed? > > What we really need here is a variant of expression evaluation that > is "extremely lazy" and stops as soon as any reference to target > memory or register contents would be made. If we run both sides > of the BINOP_EQUAL through this new evaluator, and one side can > be fully evaluated, and the other can be evaluated to a lazy > lval_memory value, we should be in business. > > This new evaluator might possibly take the form of a new "enum noside" > value as argument to evaluate_subexp, or might be a completely separate > routine (like e.g. gen_eval_for_expr). [ Or maybe even the regular > evaluator after temporarily resetting current_target to a target that > throws an exception on every memory / register access? That may be > a bit ugly though ... ] I ended up doing the following to validate the watchpoint condition: - verify whether the expression is an equality - evaluate each side with evaluate_subexp (with EVAL_NORMAL, i.e., with side-effects) - one of the resulting values should be not_lval, and the other should be lval_memory I don't think fully evaluating the expression is a problem because GDB already evaluates it each time the watchpoint triggers, so it should be harmless to do the same here. What do you think? > Also, why do we need to evaluate the expression (as opposed to the > condition) again here? We know we're going to set the hardware > watchpoint at ADDRESS. As long as the memory access implied in COND > happens at that address, it does not matter at all how the expression > was formulated, right? > > In fact, I'm not sure why we're passing the expression into the > insert/remove watchpoint routine in the first place, as we already > get the address/length. Indeed. I changed the patch to pass only the condition expression. > > + byte_to_enable = addr % 4; > > + cond_addr >>= (byte_to_enable * 8); > > + p.condition_mode = (PPC_BREAKPOINT_CONDITION_AND > > + | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable)); > > + p.condition_value = (uint64_t) cond_addr; > > I'm not really familiar with the details of the hardware here, but > is the byte-enable stuff really correct? You always set just one > single bit in the mask; shouldn't we have one bit for every byte > that is to be enabled? > > Also, don't we at some point need to consider the length; e.g. if > we have a byte-sized variable that happens to start at an address > that is a multiple of 4? The logic above is indeed very simplified, but works for the scenarios we tested (watchpoints on various lengths of data types (char, short, int, long...). For completeness, I changed the code to calculate the DVC taking all aspects in consideration. It became a bit complicated, so I put many comments to explain what it is doing. One important thing about this patch is that I had to add a new target method: target_can_accel_watchpoint_condition. It is called in watchpoint_locations_match to avoid making GDB insert only one of two bp_locations which are at the same address but with different conditions. When the target supports evaluating the watchpoint expression in hardware, GDB needs to insert both hardware watchpoints even if they are at the same address, so that their different conditions have a chance to trigger. Ok? 2010-06-08 Sergio Durigan Junior Thiago Jung Bauermann * breakpoint.c (insert_bp_location): Pass watchpoint condition in target_insert_watchpoint. (remove_breakpoint_1) Pass watchpoint condition in target_remove_watchpoint. (watchpoint_locations_match): Call target_can_accel_watchpoint_condition. * ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): Formatting fix. (can_use_watchpoint_cond_accel): New function. (calculate_dvc): Likewise. (check_condition): Likewise. (ppc_linux_can_accel_watchpoint_condition): Likewise (ppc_linux_insert_watchpoint): Call can_use_watchpoint_cond_accel, check_condition and calculate_dvc. (ppc_linux_remove_watchpoint): Likewise. (_initialize_ppc_linux_nat): Set to_can_accel_watchpoint_condition to ppc_linux_can_accel_watchpoint_condition * target.c (debug_to_insert_watchpoint): Add argument for watchpoint condition. (debug_to_remove_watchpoint): Likewise. (debug_to_can_accel_watchpoint_condition): New function. (update_current_target): Set to_can_accel_watchpoint_condition. (setup_target_debug): Set to_can_accel_watchpoint_condition. * target.h: Add opaque declarations for struct expression. (struct target_ops) , : Add new arguments to pass the watchpoint : New member. condition. Update all callers and implementations. (target_can_accel_watchpoint_condition): New macro.