From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32112 invoked by alias); 5 Feb 2010 05:16:15 -0000 Received: (qmail 32103 invoked by uid 22791); 5 Feb 2010 05:16:12 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 05 Feb 2010 05:16:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3A0832BAC13; Fri, 5 Feb 2010 00:16:04 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Faaf+AmWNFsh; Fri, 5 Feb 2010 00:16:04 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 998292BAA95; Fri, 5 Feb 2010 00:16:02 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id DC1F7F59A2; Fri, 5 Feb 2010 09:15:55 +0400 (RET) Date: Fri, 05 Feb 2010 05:16:00 -0000 From: Joel Brobecker To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org, Luis Machado , Matt Tyrlik Subject: Re: [PATCH 2/4] Hardware accelerated watchpoint conditions Message-ID: <20100205051555.GA4335@adacore.com> References: <200912232231.01798.bauerman@br.ibm.com> <200912311519.12125.bauerman@br.ibm.com> <20100112105045.GK2007@adacore.com> <201002041948.33792.bauerman@br.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201002041948.33792.bauerman@br.ibm.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00157.txt.bz2 Good news (see below). > > I think, from your patch, that GDB will still evaluate the condition > > once after the watchpoint and its condition trigger. I think that > > we might want to fix that eventually, but I am actually more than > > happy to ignore this minor issue for now. I like baby steps :). > > If I'm following your thought here, I added that on purpose. Before > letting GDB evaluate the condition one more time, GDB wouldn't know > which watchpoint triggered if there were two at the same location, > with different conditions. I knew that! (if the name DiNozzo comes to mind, then I don't know what you're talking about ;-) ) > > One of the downsides of this approach is that GDB will no longer > > be able to print that the condition will be hardware-accelerated > > when the user creates the watchpoint. I think that's OK for now. > > Yeah, I would like to have that message printed though. Wil have > to think about it. Me too. I think we're touching at a larger issue, there (already pre-discussed with Eli). We'd need to talk to the target sooner, rather than at insertion time. I think that's a desirable change anyway, but I'm unsure of the consequences... > 2010-02-04 Sergio Durigan Junior > Thiago Jung Bauermann > > * target.h: Add opaque declarations for bp_location, breakpoint and > expression. > (struct target_ops) , > : Add new arguments to pass the watchpoint > expression and its condition. Update all callers and implementations. > (target_region_ok_for_hw_watchpoint): Surround with ifndef. > * breakpoint.c (exp_is_address): New function. > (exp_is_var): Ditto. > (exp_is_address_equal_literal): Ditto. > (exp_is_var_equal_literal): Ditto. > (watch_address_if_var_equal_literal): Ditto. > (watch_var_if_address_equal_literal): Ditto. > (watch_var_if_var_equal_literal): Ditto. > (watch_address_if_address_equal_literal): Ditto. > * breakpoint.h (watch_address_if_address_equal_literal): Declare. > (watch_var_if_var_equal_literal): Ditto. > (watch_address_if_var_equal_literal): Ditto. > (watch_var_if_address_equal_literal): Ditto. > * ppc-linux-nat.c (ppc_linux_can_use_watchpoint_cond_accel): New > function. > (ppc_linux_insert_watchpoint): Check if it is possible to use > hardware-accelerated condition checking. > (ppc_linux_remove_watchpoint): Check if the watchpoint to delete > has hardware-accelerated condition checking. Just a few minor nits - pre-approved after the comments are addressed. > +/* This function is used to determine whether the condition associated > + with bp_location B is of the form: > + > + watch *
if == > + > + If it is, then it sets DATA_VALUE to LITERAL and returns 1. > + Otherwise, it returns 0. */ Can you add a note that VAR must be stored at ADDRESS too? Same for similar functions... > + /* At this point, all verifications were positive, so we can use > + hardware-assisted data-matching. Set the data value, and return > + non-zero. */ The comment here is a bit too purpose-specific. It betrays its origin :), but I think we should either drop it (I think that the function description is sufficently clear), or change it to something more neutral. Again, same for all other functions that used the same kind of comment. > +/* Return greater than zero if the condition associated with > + the watchpoint `b' can be treated by the hardware; zero otherwise. > + > + Also stores the data value in `data_value'. */ I'd drop this comment in breakpoint.h. It duplicates the descriptions in breakpoint.c... > +static int > +ppc_linux_can_use_watchpoint_cond_accel (void) This function now needs a short description. It's kind of obvious, what this function does, I agree, but we're trying to be consistent in our style, and provide always function descriptions unless the function implements a routine (function pointer) that's already documented (eg a gdbarch method, or a target_ops routine). > + if (p->hw_breaks[i].hw_break != NULL > + && p->hw_breaks[i].hw_break->condition_mode > + != PPC_BREAKPOINT_CONDITION_NONE) I'm not quite sure, here, but I think that the GNU Coding Style asks us to parenthesize your not-equal condition, purely for the benefit of GNU indent: if (p->hw_breaks[i].hw_break != NULL && (p->hw_breaks[i].hw_break->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)) Can you double-check for me? > + 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))) { Formatting nit: The curly brace should be on the next line. There are a few instances where this needs to be fixed. > + p.condition_mode = PPC_BREAKPOINT_CONDITION_AND > + | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable); Similar to above, I think you'll need to parenthesize the rhs. p.condition_mode = (PPC_BREAKPOINT_CONDITION_AND | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable)); > + p.condition_value = (uint64_t) cond_addr; > + } else { (curly braces) > + || watch_address_if_var_equal_literal (exp, cond, &cond_addr))) { (curly brace) > + p.condition_mode = PPC_BREAKPOINT_CONDITION_AND > + | PPC_BREAKPOINT_CONDITION_BE (byte_to_enable); parenthesize. > + p.condition_value = (uint64_t) cond_addr; > + } else { curly braces > diff --git a/gdb/target.c b/gdb/target.c > index e6659c9..7adc96e 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -44,6 +44,8 @@ > #include "exec.h" > #include "inline-frame.h" > > +struct expression; > + This change is unnecessary (already declared in target.h)... > diff --git a/gdb/target.h b/gdb/target.h > index 7103ab2..a65e900 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -36,6 +36,10 @@ struct trace_status; > struct uploaded_tsv; > struct uploaded_tp; > > +struct bp_location; > +struct breakpoint; > +struct expression; I don't think that declaring struct bp_location is necessary. I just see that struct breakpoint should probably have been declared before your patch - so OK. > + int (*to_remove_watchpoint) (CORE_ADDR, int, int, struct expression *, > + struct expression *); > + int (*to_insert_watchpoint) (CORE_ADDR, int, int, struct expression *, > + struct expression *); Would you mind adding a comment explaining that documentation of what these routines are expected to do is provided with the corresponding target_* macros? I personally think that the complete description of what these routines are supposed to do should be right there rather than with the target_ macros, but that's only a very minor matter and nothing to do with your patch. > +#ifndef target_region_ok_for_hw_watchpoint > #define target_region_ok_for_hw_watchpoint(addr, len) \ > (*current_target.to_region_ok_for_hw_watchpoint) (addr, len) > - > +#endif This is not right - we no longer allow a macro to be overridden. > /* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes. TYPE is 0 > for write, 1 for read, and 2 for read/write accesses. Returns 0 for > success, non-zero for failure. */ > > -#define target_insert_watchpoint(addr, len, type) \ > - (*current_target.to_insert_watchpoint) (addr, len, type) > +#define target_insert_watchpoint(addr, len, type, watch_exp, cond) \ > + (*current_target.to_insert_watchpoint) (addr, len, type, watch_exp, cond) The macro description needs to be updated (to mention the two new parameters). > -#define target_remove_watchpoint(addr, len, type) \ > - (*current_target.to_remove_watchpoint) (addr, len, type) > +#define target_remove_watchpoint(addr, len, type, watch_exp, cond) \ > + (*current_target.to_remove_watchpoint) (addr, len, type, watch_exp, cond) Can you add a short documentation for this macro? -- Joel