From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21733 invoked by alias); 12 Jan 2010 10:51:10 -0000 Received: (qmail 21714 invoked by uid 22791); 12 Jan 2010 10:51:07 -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; Tue, 12 Jan 2010 10:51:02 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id DE27F41613; Tue, 12 Jan 2010 05:51:00 -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 al6hMuJB7hhi; Tue, 12 Jan 2010 05:51:00 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 07F3841611; Tue, 12 Jan 2010 05:50:59 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 2CBB7F595E; Tue, 12 Jan 2010 14:50:45 +0400 (RET) Date: Tue, 12 Jan 2010 10:51: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: <20100112105045.GK2007@adacore.com> References: <200912232231.01798.bauerman@br.ibm.com> <200912311519.12125.bauerman@br.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200912311519.12125.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-01/txt/msg00297.txt.bz2 Global Maintainers: This is touching an area of the target vector that I think could be improved. Eli actually mentioned some of the issues in this area, a few days ago. I don't think that we should stall this patch pending a rework of the watchpoint support, but I would like to see an implementation that does not make the long term cleanup more difficult than it already is. So, feedback and ideas would be welcome. Thiago, Luis, Sorry for the delay looking into this; I needed a little bit of time to let this brew a little... > > In the case of BookE processors, the condition will be accelerated > > when the user types "watch A if A == B", where A and B are either > > address (e.g. "*0x12345678") or variables. I like the principle that detection of the situations where h/w acceleration of the condition can be done, so that the user does not have to do anything to get access to this feature. 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 :). In fact, let's worry about functionality only, even if it means writing inefficient code. We can implement caches and optimizations later, if it helps getting this in faster. My first general observation is that this patch introduces new target_ops routine in an area that, IMO, could use some simplification (refer to the first comment above). I think we can avoid that. Conceptually, I think that the target now needs to be able to look at the watchpoint expression, and the watchpoint condition, in order to determine whether the condition evaluation can be evaluated by hardware. I propose that we expand the insert_watchpoint routine to pass this data, as 2 extra parameters; and let these routines decide whether to use hardware acceleration or not for the condition. 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. The upside is that the code will not have to worry about this aspect of the watchpoint when inserting/removing it. Thus, the following hunk would be simplified: > - bpt->watchpoint_type); > + if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL) > + val = target_insert_cond_accel_watchpoint (bpt->address, > + bpt->length, > + bpt->watchpoint_type, > + bpt->cond_hw_addr); > + else > + val = target_insert_watchpoint (bpt->address, > + bpt->length, > + bpt->watchpoint_type); I am also a bit suspicious of adding the following bits to the bp_location structure directly: > + /* Flag to indicate if the condition is going to be accelerated > + by hardware. If its value is non-zero, then GDB checks the > + condition using hardware acceleration; otherwise it uses the > + regular software-based checking. */ > + int cond_hw_accel : 1; > + > + /* If the condition can be hardware-accelerated, then we must > + get the condition's variable address so that GDB can > + properly set the evaluation via hardware. */ > + CORE_ADDR cond_hw_addr; I think that they are too target specific. Initially, we can do without, with our not-so-efficient approach, by recomputing these address every time we insert the watchpoint. I think that'd still be an interesting first step. Then we can look at storing that info as a private data structure, allocated by the target. I think that this would be doable without too many changes to the current framework - we just need to be a little careful as watchpoint expressions and conditions get re-evaluated sometimes. > +static int > +exp_is_address_p (struct breakpoint *b) If you want, you can drop the _p prefix in this type of situation. I think the use of the verb _is_ in the names makes it obvious that this function is a predicate. > +int > +default_watch_address_if_var_equal_literal_p (struct bp_location *b, > + CORE_ADDR *data_value) This is nitpicking on function names, but I don't understand the "default" here. Would there be some non-default implementations of the same routine? Perhaps: watch_address_if_var_equal_literal_p is just going to be sufficient? > + /* Make sure that the two addresses are the same. */ > + if (exp_address != cond_address) > + { > + printf_filtered (_("\ > +Memory location for the watchpoint expression and its condition need\n\ > +to be the same.\n")); > + return 0; > + } Not sure whether you really meant to keep this message. It sounds like an error, when in fact this could occur in a perfectly legitimate situation, no? For instance, if the user entered: (gdb) watch A if B == 3 The message could be kept as a debug trace, but it should be more neutral, IMO, something like: address of variable in condition does not match expression being watched. would provide all the information without suggesting that something might have gone wrong. Let me know what you think. I realize that I'm significantly reducing the scope of the first attempt at getting h/w-accelerated watchpoint conditions, but I'm hoping it'll help focusing on each issue separately. I'm perfectly happy to make this feature another patch series on its own, in parallel to the other hardware-breakpoint patches. We can also take it one patch at a time. Whatever works best for you. BTW: > @@ -186,6 +186,11 @@ struct bp_target_info > is used to determine the type of breakpoint to insert. */ > CORE_ADDR placed_address; > > + /* If this is a ranged hardware breakpoint, then we can use this > + field in order to store the length of the range that will be > + watched for execution. */ > + ULONGEST length; > + Oops! This should be part of another patch ;-). -- Joel