From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24477 invoked by alias); 29 Apr 2011 17:46:02 -0000 Received: (qmail 24451 invoked by uid 22791); 29 Apr 2011 17:46:01 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate2.uk.ibm.com (HELO mtagate2.uk.ibm.com) (194.196.100.162) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 Apr 2011 17:45:46 +0000 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate2.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p3THjjFC006256 for ; Fri, 29 Apr 2011 17:45:45 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 p3THkp1i1638650 for ; Fri, 29 Apr 2011 18:46:51 +0100 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 p3THjimI030491 for ; Fri, 29 Apr 2011 11:45:44 -0600 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 p3THjh0k030472; Fri, 29 Apr 2011 11:45:43 -0600 Message-Id: <201104291745.p3THjh0k030472@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Fri, 29 Apr 2011 19:45:43 +0200 Subject: Re: [RFA 3/3] Implement support for PowerPC BookE masked watchpoints To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Fri, 29 Apr 2011 17:46:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches ml) In-Reply-To: <1303161770.1969.221.camel@hactar> from "Thiago Jung Bauermann" at Apr 18, 2011 06:22:50 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-04/txt/msg00564.txt.bz2 Thiago Jung Bauermann wrote: > > 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). > > -location is indeed a good fit for this patch. I don't think we need a > separate -masked-location keyword, so in this patch I made the mask > parameter imply -location. Do you think this addresses your concerns > regarding the semantics of masked watchpoints? OK, that's fine with me. I guess that ought to mentioned in the documentation somewhere ... > > 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. > > Indeed. With this new patch, the only place that evaluates the > watchpoint expression is update_watchpoint. This is necessary because > the resulting value chain is then used to create the bp_locations and > also by can_use_hardware_watchpoint when deciding whether to set a > software or hardware watchpoint. > > I could manually create a bp_location for masked watchpoints and > implement a version of can_use_hardware_watchpoint just for masked > watchpoints, but is it worth the extra special casing and complexity in > update_watchpoint? No, this is good. Evaluating the expression just for it's value's location(s) is fine ... > + { > + CORE_ADDR newaddr, start; > + > + if (is_masked_watchpoint (loc->owner)) > + { > + newaddr = addr & loc->owner->hw_wp_mask; > + start = loc->address & loc->owner->hw_wp_mask; > + } > + else > + { > + newaddr = addr; > + start = loc->address; > + } > + > + /* Exact match not required. Within range is > + sufficient. */ > + if (target_watchpoint_addr_within_range (¤t_target, > + newaddr, start, > + loc->length)) > + { > + b->watchpoint_triggered = watch_triggered_yes; > + break; > + } > + } Hmmm, so for masked watchpoints we implement the masking operation directly here, and then also call the target callback (which may use the length, and do some additional masking on powerpc)? That seems a bit odd to me ... Shouldn't we either: - just compare masked addresses for equality, directly (which implements that stated semantics) - or else pass the mask into target_watchpoint_addr_within_range and allow the target to implement whatever matching is appropriate > +static enum print_stop_action > +print_it_masked_watchpoint (struct breakpoint *b) > +{ > + struct ui_stream *stb; > + struct cleanup *old_chain; > + > + /* Masked watchpoints have only one location. */ > + gdb_assert (b->loc && b->loc->next == NULL); > + > + stb = ui_out_stream_new (uiout); > + old_chain = make_cleanup_ui_out_stream_delete (stb); These aren't used anywhere, are they? > +ppc_linux_masked_watch_num_registers (struct target_ops *target, > + CORE_ADDR addr, CORE_ADDR mask) > +{ > + if (!have_ptrace_booke_interface () > + || (booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_MASK) == 0) > + return -1; > + else if ((mask & 0xC0000000) != 0xC0000000) > + { > + warning (_("\ > +The given mask covers kernel address space and cannot be used.\n\ > +You have to delete the masked watchpoint to continue the debugging session.")); This second sentence shouldn't be neccessary now: the watchpoint with the invalid mask will never be successfully created, right? Otherwise this looks all good to me now. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com