From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23458 invoked by alias); 11 Feb 2010 18:24:06 -0000 Received: (qmail 23449 invoked by uid 22791); 11 Feb 2010 18:24:05 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_PASS 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; Thu, 11 Feb 2010 18:24:01 +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 o1BINwEf022483 for ; Thu, 11 Feb 2010 18:23:58 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 o1BINwgd1347594 for ; Thu, 11 Feb 2010 19:23:58 +0100 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 o1BINw7F026019 for ; Thu, 11 Feb 2010 19:23:58 +0100 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 o1BINvgj026014; Thu, 11 Feb 2010 19:23:57 +0100 Message-Id: <201002111823.o1BINvgj026014@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 11 Feb 2010 19:23:57 +0100 Subject: Re: [PATCH 2/4] Hardware accelerated watchpoint conditions To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Thu, 11 Feb 2010 18:24: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: <201002081801.05214.bauerman@br.ibm.com> from "Thiago Jung Bauermann" at Feb 08, 2010 06:01:05 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: 2010-02/txt/msg00294.txt.bz2 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 ;-) > + > + 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 ... ] (If this is too much effort for now, I'd at least ask to put the special-purpose helpers into a ppc-private file so as not to advertise their reuse elsewhere ...) 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. > + 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? Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com