From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1635 invoked by alias); 28 Dec 2010 20:15:45 -0000 Received: (qmail 1623 invoked by uid 22791); 28 Dec 2010 20:15:43 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e24smtp02.br.ibm.com (HELO e24smtp02.br.ibm.com) (32.104.18.86) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Dec 2010 20:15:37 +0000 Received: from mailhub1.br.ibm.com (mailhub1.br.ibm.com [9.18.232.109]) by e24smtp02.br.ibm.com (8.14.4/8.13.1) with ESMTP id oBSKZ4oE014445 for ; Tue, 28 Dec 2010 18:35:04 -0200 Received: from d24av01.br.ibm.com (d24av01.br.ibm.com [9.8.31.91]) by mailhub1.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oBSKMqWX688256 for ; Tue, 28 Dec 2010 18:22:57 -0200 Received: from d24av01.br.ibm.com (loopback [127.0.0.1]) by d24av01.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oBSKFDPd016680 for ; Tue, 28 Dec 2010 18:15:14 -0200 Received: from [9.8.10.210] ([9.8.10.210]) by d24av01.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id oBSKFA9v016604; Tue, 28 Dec 2010 18:15:11 -0200 Subject: Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints From: Thiago Jung Bauermann To: Pedro Alves Cc: gdb-patches@sourceware.org, Jan Kratochvil , Joel Brobecker , Eli Zaretskii In-Reply-To: <201012281529.18162.pedro@codesourcery.com> References: <1290549100.3164.47.camel@hactar> <201012232017.11120.pedro@codesourcery.com> <1293484743.1544.78.camel@hactar> <201012281529.18162.pedro@codesourcery.com> Content-Type: text/plain; charset="UTF-8" Date: Wed, 29 Dec 2010 01:00:00 -0000 Message-ID: <1293567319.2946.87.camel@hactar> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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-12/txt/msg00524.txt.bz2 On Tue, 2010-12-28 at 15:29 +0000, Pedro Alves wrote: > On Monday 27 December 2010 21:19:03, Thiago Jung Bauermann wrote: > > > @@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse) > > if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint) > > && reparse) > > { > > - int i, mem_cnt, other_type_used; > > - > > - /* We need to determine how many resources are already used > > - for all other hardware watchpoints to see if we still have > > - enough resources to also fit this watchpoint in as well. > > - To avoid the hw_watchpoint_used_count call below from counting > > - this watchpoint, make sure that it is marked as a software > > - watchpoint. */ > > - b->type = bp_watchpoint; > > - i = hw_watchpoint_used_count (bp_hardware_watchpoint, > > - &other_type_used); > > - mem_cnt = can_use_hardware_watchpoint (val_chain); > > - > > - if (!mem_cnt) > > + int reg_cnt; > > + > > + reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact); > > + > > + if (!reg_cnt) > > b->type = bp_watchpoint; > > else > > { > > - int target_resources_ok = target_can_use_hardware_watchpoint > > - (bp_hardware_watchpoint, i + mem_cnt, other_type_used); > > + int i, other_type_used, target_resources_ok; > > + enum bptype orig_type; > > + > > + if (b->ops && b->ops->extra_resources_needed) > > + reg_cnt += b->ops->extra_resources_needed (b, NULL); > > I'm taking a look at the resource accounting changes, which I > mostly ignored before, and I'm confused. I don't blame you. What I need is beyond what the current resource accounting code in GDB is capable of doing, and I'm trying to extend it in the least intrusive way possible. I don't think it's worth rewriting it, especially after your hints that it should just be removed. This leads to some hacks in my patch... > can_use_hardware_watchpoint > (above) calls target_region_ok_for_hw_watchpoint, which has been changed > to return the number of resources normally necessary. Yet, > extra_resources_needed_watchpoint calls target_region_ok_for_hw_watchpoint > as well (discounting 1, it seems). Aren't we ending up with gdb > thinking it needs more resources than are really necessary? What's going on here is this: extra_resources_needed_watchpoint is called at two slightly different contexts: In hw_watchpoint_used_count, where it is asked to answer about each breakpoint location in all the existing watchpoints. This is easy, it just calls target_region_ok_for_hw_watchpoint (discounting one because of the convention where I make GDB assume that each bp_location consumes one resource, so any additional ones are "extra" (I'm not very happy about this convention, but it allows me not to rewrite the resource accounting code)). In update_watchpoint, where it needs to answer about the watchpoint currently being examined by update_watchpoint. The problem here is that the watchpoint doesn't have any bp_location yet. The bp_locations will be created just a few moments later. So at this point extra_resources_needed_watchpoint just says that no extra resources are needed (because bl is NULL). This hack is needed because the resource accounting code examines high-level breakpoints to decide about resource utilization, but it should be using breakpoint locations instead. To make it examine breakpoint locations, the resource accounting functions would have to be called at different places from where they are called now, where changing from a hardware watchpoint to a software one, or refusing to create a hardware breakpoint (for example) would be cumbersome, since breakpoint locations are created late in the breakpoint/watchpoint creation process. It can be done, but it's a fair amount of work which may not be worthwhile given that the current disposition is to get rid of this mechanism altogether. Perhaps I should just change ppc-linux-nat.c to always accept hardware breakpoints and watchpoints, and then just fail later if it overcommitted (as other ports like i386 do)? Then I believe this patch would be simpler in this regard. > > + > > + /* We need to determine how many resources are already used > > + for all other hardware watchpoints to see if we still have > > + enough resources to also fit this watchpoint in as well. > > + To avoid the hw_watchpoint_used_count call below from > > + counting this watchpoint, make sure that it is marked as a > > + software watchpoint. */ > > + orig_type = b->type; > > + b->type = bp_watchpoint; > > Something I notice: It sounds like if you remove this hack above, > > > + i = hw_watchpoint_used_count (bp_hardware_watchpoint, > > + &other_type_used); > > then this above accounts for the new watchpoint too, so... > > > + > > + target_resources_ok = target_can_use_hardware_watchpoint > > + (bp_hardware_watchpoint, i + reg_cnt, other_type_used) > > ... this would just be ", i,", instead of ", i + reg_cnt,". It wouldn't be exactly the same. In this patch I changed hw_watchpoint_used_count to iterate over the breakpoint locations instead of the breakpoints (which is what actually makes sense). But since the watchpoint currently being examined by update_watchpoint doesn't have any location, then it wouldn't be accounted for. I could change update_watchpoint to create the breakpoint locations before doing the resource accounting so that I could remove the hacks described here, but it was not a trivial change (but not magic either) and I wondered whether it was worth it. > > if (target_resources_ok <= 0) > > b->type = bp_watchpoint; > > else > > > > What does the "TYPE_NFIELDS (t) == 1" do ? > > > > TYPE_NFIELDS (t) == 1 && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE > > is a way to determine that the given array type has known bounds. > > Didn't know about that. I thought TYPE_NFIELDS only made sense > for structs/classes/unions. My grep foo isn't finding other similar uses. I looked at different places in GDB which determined the array size, if available, and there are different ways to do it. I chose to use the code from c-lang.c:c_get_string, which I wrote. :-) gdbtypes.c:create_array_type says: TYPE_NFIELDS (result_type) = 1; TYPE_FIELDS (result_type) = (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field)); TYPE_INDEX_TYPE (result_type) = range_type; So I believe it is correct. > I tried: > > compunit1.c: > extern int array[]; > > void foo () > { > ... > } > > compunit2.c: > > int array[]; > > and in foo, ptype array gave me array[0], TYPE_NFIELDS==1. That information is wrong, since the array is actually of an incomplete type. But it would not have been a problem for is_scalar_type_recursive since it only considers arrays with exactly one element. -- []'s Thiago Jung Bauermann IBM Linux Technology Center