From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7004 invoked by alias); 2 Jul 2010 13:20:09 -0000 Received: (qmail 6989 invoked by uid 22791); 2 Jul 2010 13:20:08 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate7.de.ibm.com (HELO mtagate7.de.ibm.com) (195.212.17.167) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 02 Jul 2010 13:20:02 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate7.de.ibm.com (8.13.1/8.13.1) with ESMTP id o62DJxAA023024 for ; Fri, 2 Jul 2010 13:19:59 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o62DJxTt1765500 for ; Fri, 2 Jul 2010 15:19:59 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id o62DJw3n018910 for ; Fri, 2 Jul 2010 15:19:58 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id o62DJvv4018879; Fri, 2 Jul 2010 15:19:57 +0200 Message-Id: <201007021319.o62DJvv4018879@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Fri, 02 Jul 2010 15:19:57 +0200 Subject: Re: [PATCH 2/4] Hardware accelerated watchpoint conditions To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Fri, 02 Jul 2010 13:20:00 -0000 From: "Ulrich Weigand" Cc: brobecker@adacore.com (Joel Brobecker), gdb-patches@sourceware.org, luisgpm@linux.vnet.ibm.com (Luis Machado), tyrlik@us.ibm.com (Matt Tyrlik) In-Reply-To: <1277995817.4087.22.camel@hactar.cps.virtua.com.br> from "Thiago Jung Bauermann" at Jul 01, 2010 11:50:16 AM 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: 2010-07/txt/msg00039.txt.bz2 Thiago Jung Bauermann wrote: > I changed the comment to read: > > /* Return non-zero if the target is capable of using hardware to evaluate > the condition expression. In this case, if the condition is false when > the watched memory location changes, execution may continue without the > debugger being notified. > > Due to limitations in the hardware implementation, it may be capable of > avoiding triggering the watchpoint in some cases where the condition > expression is false, but may report some false positives as well. > For this reason, GDB will still evaluate the condition expression when > the watchpoint triggers. */ > > What do you think? Looks good to me. > > > + for (; v; v = value_next (v)) > > > + { > > > + if (VALUE_LVAL (v) == lval_memory) > > > + { > > > + /* A lazy memory lvalue is one that GDB never needed to fetch; > > > + we either just used its address (e.g., `a' in `a.b') or > > > + we never needed it at all (e.g., `a' in `a,b'). */ > > > + if (!value_lazy (v)) > > > + found_memory_cnt++; > > > + } > > > + else if (VALUE_LVAL (v) != not_lval > > > + && deprecated_value_modifiable (v) == 0) > > > + return -1; /* ??? What does this represent? */ > > > > These are values from the history (e.g. $1). In this context, they > > should be treated as non-lvalues, so they should be fine ... > > I removed that else if then. I'll submit a separate patch to replace > that comment in can_use_hardware_watchpoint. Actually, thinking about this a bit more, the logic still does not seem quite correct. What we really want is: - not_lval values: these are fine - deprecated_value_modifiable (v) == 0 (history) values: treat just like not_lval, no matter what their VALUE_LVAL says - lval_memory values: check whether they are lazy or not and count them - all other values (lval_computed, lval_internalvar ...): *not* OK In particular, the missing check on lval_computed may become more important now that DWARF multi-piece values are always represented as lval_computed ... (This may also affect watchpoint code in breakpoint.c, but I guess that's a different issue.) > 2010-07-01 Sergio Durigan Junior > Thiago Jung Bauermann > > * breakpoint.c (fetch_watchpoint_value): Rename to fetch_subexp_value > and move to eval.c. Change callers. > (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. > * eval.c: Include wrapper.h. > (fetch_subexp_value): Moved from breakpoint.c. > * ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): > Formatting fix. > (can_use_watchpoint_cond_accel): New function. > (calculate_dvc): Likewise. > (num_memory_accesses): 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 declaration 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. > * value.c (free_value_chain): New function. > * value.h (fetch_subexp_value): New prototype. > (free_value_chain): Likewise. Except for the num_memory_accesses logic discussed above, the patch is otherwise OK now. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com