From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30819 invoked by alias); 17 Feb 2011 14:40:31 -0000 Received: (qmail 30810 invoked by uid 22791); 17 Feb 2011 14:40:29 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,TW_XF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate4.uk.ibm.com (HELO mtagate4.uk.ibm.com) (194.196.100.164) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 17 Feb 2011 14:40:23 +0000 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate4.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p1HEeKgk014321 for ; Thu, 17 Feb 2011 14:40:20 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p1HEeQgn1470692 for ; Thu, 17 Feb 2011 14:40:26 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p1HEeJoj012383 for ; Thu, 17 Feb 2011 07:40:19 -0700 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p1HEeIBJ012343; Thu, 17 Feb 2011 07:40:18 -0700 Message-Id: <201102171440.p1HEeIBJ012343@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 17 Feb 2011 15:40:18 +0100 Subject: Re: [RFA] Implement support for PowerPC BookE masked watchpoints To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Thu, 17 Feb 2011 15:10:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches ml) In-Reply-To: <1294949185.32616.75.camel@hactar> from "Thiago Jung Bauermann" at Jan 13, 2011 06:06:25 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-02/txt/msg00419.txt.bz2 Thiago Jung Bauermann wrote: > I'm almost done with the series adding suport for PowerPC BookE > features... There's only this patch adding masked watchpoints, and > another one adding ranged hardware breakpoints (to be posted later). So > please bear with me a little longer. :-) I have one basic issue with this patch: the semantics of the new masked watchpoint commands seems not quite fully defined. The semantics of a GDB watchpoint usually is: stop execution whenever the value of an expression changes. To implement this, GDB will parse the expression, and create a set of low-level watchpoints to be implemented by hardware (which are usually along the lines of: stop execution whenever a store to a contiguous region of memory happens). Now, the "masked breakpoints" feature is at its core defined at that lower level: stop execution whenever a store to a memory region happens, where that region of memory is defined in a more complex manner (via masked address matching). The problem with the patch in its current form seems to me that it confuses the two levels. On the GDB command line, the user specifies an expression, just like with normal watchpoints, and a mask, which is passed directly to the hardware feature. The effect is that GDB will parse the expression, create the list of memory regions it needs to watch to implement its high-level semantics. Then, it will just add the mask to each of the addresses generated in that manner. It seems to me that in general, the effects of what will actually happen now are hard to predict, and will generally not be what the user expects. Of course, the feature *can* be used in ways the user understands, like in the example you show in the documentation: watch *0xdeadbeef mask 0xffffff00 But if used in a more complex way, like watch p->x mask 0xffffff00 for some local variable p, this doesn't seem to have very useful (or even well-defined) semantics ... Now, I'm not sure how exactly the GDB command line syntax ought to be modified to implement this. But most fundamentally, we should go away from specifying "the value of an expression", and rather go to specifying a memory region via address and mask. We've recently added the -location feature that already prevents GDB from fully tracking the expression, but rather just evaluate it one time for its address. Maybe one simple fix could be to restrict the "mask" feature to watch -location (or have the presence of "mask" automatically imply -location, or maybe have instead a new keyword -masked-location or so). However, even so there are still differences: even with -location, GDB watchpoint code will still treat a watchpoint as refering to one value, and track it (e.g. remember the value we saw last time; re-evaluate it each time execution stops etc.). This is really not appropriate for the masked watchpoint case, because we do not *have* any contiguous region that would define any single value. Therefore, it seems to me that all this code ought to be disabled for masked watchpoints. A couple of more specific comments on the patch: > @@ -1518,10 +1522,26 @@ update_watchpoint (struct breakpoint *b, int reparse) > target_resources_ok = target_can_use_hardware_watchpoint > (bp_hardware_watchpoint, i, other_type_used); > if (target_resources_ok <= 0) > - b->type = bp_watchpoint; > + { > + if (orig_type == bp_hardware_watchpoint > + && b->ops && b->ops->works_in_software_mode > + && !b->ops->works_in_software_mode (b)) > + error (_("This watchpoint cannot be used " > + "in software mode.")); > + else > + b->type = bp_watchpoint; > + } > } > else > - b->type = bp_watchpoint; > + { > + if (b->type == bp_hardware_watchpoint > + && b->ops && b->ops->works_in_software_mode > + && !b->ops->works_in_software_mode (b)) > + error (_("This watchpoint cannot be used " > + "in software mode.")); > + else > + b->type = bp_watchpoint; > + } What is the point of checking the original type? If works_in_software_mode returns false, the original type couldn't have been a software watchpoint anyway, could it? > print_it_typical. */ > /* FIXME: how breakpoint can ever be NULL here? */ > if (b != NULL && b->ops != NULL && b->ops->print_it != NULL) > - return b->ops->print_it (b); > + return b->ops->print_it (b, bs->old_val); > else > return print_it_typical (bs); > } I don't understand this change. You add the "old_val" argument to the print_it routine, and the new instance of print_it for masked watchpoints then uses it (for what purpose?) ... But for masked watchpoints, the whole concept of its "value", new or old, doesn't make sense to start with ... > @@ -3761,6 +3796,11 @@ watchpoint_check (void *p) > b->val_valid = 1; > return WP_VALUE_CHANGED; > } > + else if (is_masked_watchpoint (b)) > + /* Since we don't know the exact trigger address (from > + stopped_data_address), just tell the user we've triggered > + a mask watchpoint. */ > + return WP_VALUE_CHANGED; This goes back to the issue discussed above: this should not even attempt to "compute the old value" which doesn't make sense for a masked watchpoint. Instead, it should simply report a stop to the user whenever the hardware reports a hit. > +static void > +print_one_detail_masked_watchpoint (const struct breakpoint *b, > + struct ui_out *uiout) > +{ > + ui_out_text (uiout, "\tmask "); > + > + /* I don't know whether it's possible to get here without a b->loc, > + but we can handle the situation. */ > + if (b->loc) > + ui_out_field_core_addr (uiout, "mask", b->loc->gdbarch, b->hw_wp_mask); > + else > + ui_out_field_string (uiout, "mask", core_addr_to_string (b->hw_wp_mask)); I think we should enforce that there is always exactly one location on masked watchpoints, just like we do e.g. for catchpoints. > +/* Implement the "print_recreate" breakpoint_ops method for > + masked hardware watchpoints. */ > + > +static void > +print_recreate_masked_watchpoint (struct breakpoint *b, struct ui_file *fp) > +{ > + char tmp[40]; > + > + switch (b->type) > + { > + case bp_hardware_watchpoint: > + fprintf_unfiltered (fp, "watch"); > + break; > + case bp_read_watchpoint: > + fprintf_unfiltered (fp, "rwatch"); > + break; > + case bp_access_watchpoint: > + fprintf_unfiltered (fp, "awatch"); > + break; > + default: > + internal_error (__FILE__, __LINE__, > + _("Invalid hardware watchpoint type.")); > + } > + > + sprintf_vma (tmp, b->hw_wp_mask); > + fprintf_unfiltered (fp, " %s mask 0x%s", b->exp_string, tmp); Depending on the discussion on command syntax this may need to change anyway, but even with the current syntax, it ignores any -location flag that may have been given initially ... > @@ -8540,7 +8832,14 @@ watch_command_1 (char *arg, int accessflag, int from_tty, > b->exp_string = savestring (exp_start, exp_end - exp_start); > b->val = val; > b->val_valid = 1; > - b->ops = &watchpoint_breakpoint_ops; > + > + if (use_mask) > + { > + b->hw_wp_mask = hw_wp_mask; > + b->ops = &masked_watchpoint_breakpoint_ops; > + } > + else > + b->ops = &watchpoint_breakpoint_ops; We shouldn't compute an intial value or set val_valid for masked watchpoints. > +/* Insert a new masked watchpoint at ADDR using the mask MASK. > + RW may be hw_read for a read watchpoint, hw_write for a write watchpoint > + or hw_access for an access watchpoint. Returns 0 on success and throws > + an error on failure. */ > + > +static int > +ppc_linux_insert_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr, > + CORE_ADDR mask, int rw) > +{ > + ptid_t ptid; > + struct lwp_info *lp; > + struct ppc_hw_breakpoint p; > + > + gdb_assert (have_ptrace_booke_interface ()); > + > + if ((mask & 0xC0000000) != 0xC0000000) > + error (_("\ > +The given mask covers kernel address space and cannot be used.\n\ > +You have to delete the masked watchpoint to continue the debugging session.")); Huh, this interface seems a bit odd. Shouldn't such watchpoints be rejected right from the start, using something like the region_ok_for_watchpoint callback? > +/* Return the number of registers needed for a masked hardware watchpoint. */ > + > +static int > +ppc_linux_masked_watch_num_registers (struct target_ops *target) > +{ > + return ((have_ptrace_booke_interface () > + && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_MASK)? > + 2 : -1); > +} So maybe this interface ought to get parameters to be able to check whether specific watchpoints are valid? > INHERIT (to_insert_watchpoint, t); > INHERIT (to_remove_watchpoint, t); > + /* Do not inherit to_insert_mask_watchpoint. */ > + /* Do not inherit to_remove_mask_watchpoint. */ Why should this differ between regular and masked watchpoints? Shouldn't they either both use inheritance or none? Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com