From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32597 invoked by alias); 13 Sep 2011 12:55:52 -0000 Received: (qmail 32569 invoked by uid 22791); 13 Sep 2011 12:55:49 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Sep 2011 12:55:16 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R3SWB-0003fI-Gv from pedro_alves@mentor.com ; Tue, 13 Sep 2011 05:55:15 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Tue, 13 Sep 2011 13:55:13 +0100 From: Pedro Alves To: Edjunior Barbosa Machado Subject: Re: Watchpoint resource accounting broken (Re: [5/6] breakpoints_ops for all kinds of breakpoints: new watchpoints instance type) Date: Tue, 13 Sep 2011 14:16:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.0; x86_64; ; ) Cc: Ulrich Weigand , gdb-patches@sourceware.org References: <201109121503.p8CF3JCD003396@d06av02.portsmouth.uk.ibm.com> <4E6E2265.8050102@linux.vnet.ibm.com> In-Reply-To: <4E6E2265.8050102@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201109131355.11485.pedro@codesourcery.com> 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: 2011-09/txt/msg00210.txt.bz2 On Monday 12 September 2011 16:03:18, Ulrich Weigand wrote: > However, update_watchpoint relies on having the watchpoint under > investigation be on the breakpoint list; see the comment: > > /* We need to determine how many resources are already > used for all other hardware watchpoints plus this one > to see if we still have enough resources to also fit > this watchpoint in as well. To guarantee the > hw_watchpoint_used_count call below counts this > watchpoint, make sure that it is marked as a hardware > watchpoint. */ > if (b->base.type == bp_watchpoint) > b->base.type = bp_hardware_watchpoint; > > i = hw_watchpoint_used_count (b->base.type, &other_type_used); > target_resources_ok = target_can_use_hardware_watchpoint > (b->base.type, i, other_type_used); > > Note how just "i", the result of hw_watchpoint_used_count, is passed to > target_can_use_hardware_watchpoint -- this works only if the current > watchpoint is on the list that hw_watchpoint_used_count iterates over. > > (You cannot just consider the current watchpoint in addition to the > result of hw_watchpoint_used_count either, because for other callers > to update_watchpoint, the current watchpoint *is* on the list.) > > I'm sure sure how best to fix this; maybe go back to adding the > watchpoint to the breakpoint list earlier? Does the patch below work? Never consider the current watchpoint when going over the breakpoint list counting resources, and then add the resources of the current watchpoint on top. This way we don't have to care of the current watchpoint being on the list yet or not. As bonus, we no longer have to frog the watchpoint's type before knowing if it'll fit. I've tested it on x86_64-linux and saw no regressions. On Monday 12 September 2011 16:16:53, Edjunior Barbosa Machado wrote: > On 09/12/2011 12:03 PM, Ulrich Weigand wrote: > > Note how just "i", the result of hw_watchpoint_used_count, is passed to > > target_can_use_hardware_watchpoint -- this works only if the current > > watchpoint is on the list that hw_watchpoint_used_count iterates over. > > I noticed this problem too and was considering use "i + reg_cnt" instead > of only "i" when calling hw_watchpoint_used_count() (actually, I saw gdb > used to use this previously). > However, with this change, due to the same problem with the watchpoint > added to breakpoint list that Ulrich mentioned, watchpoints added before > run the inferior will not work. Can you expand on this? I don't think I'm understanding that problem. -- Pedro Alves 2011-09-13 Pedro Alves gdb/ * breakpoint.c (update_watchpoint): Handle the case of the watchpoint to update not being the in the breakpoint list yet. (hw_watchpoint_use_count): New, factored out from hw_watchpoint_used_count. (hw_watchpoint_used_count): Rename to ... (hw_watchpoint_used_count_others): ... this. Add `except' parameter. Don't count resources of `except'. Use hw_watchpoint_use_count. --- gdb/breakpoint.c | 99 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 75 insertions(+), 24 deletions(-) Index: src/gdb/breakpoint.c =================================================================== --- src.orig/gdb/breakpoint.c 2011-08-29 17:36:34.000000000 +0100 +++ src/gdb/breakpoint.c 2011-09-13 13:47:49.421051499 +0100 @@ -176,7 +176,11 @@ static void maintenance_info_breakpoints static int hw_breakpoint_used_count (void); -static int hw_watchpoint_used_count (enum bptype, int *); +static int hw_watchpoint_use_count (struct breakpoint *); + +static int hw_watchpoint_used_count_others (struct breakpoint *except, + enum bptype type, + int *other_type_used); static void hbreak_command (char *, int); @@ -1458,6 +1462,7 @@ update_watchpoint (struct watchpoint *b, if (reg_cnt) { int i, target_resources_ok, other_type_used; + enum bptype type; /* Use an exact watchpoint when there's only one memory region to be watched, and only one debug register is needed to watch it. */ @@ -1466,16 +1471,29 @@ update_watchpoint (struct watchpoint *b, /* We need to determine how many resources are already used for all other hardware watchpoints plus this one to see if we still have enough resources to also fit - this watchpoint in as well. To guarantee the - hw_watchpoint_used_count call below counts this - watchpoint, make sure that it is marked as a hardware - watchpoint. */ - if (b->base.type == bp_watchpoint) - b->base.type = bp_hardware_watchpoint; - - i = hw_watchpoint_used_count (b->base.type, &other_type_used); - target_resources_ok = target_can_use_hardware_watchpoint - (b->base.type, i, other_type_used); + this watchpoint in as well. */ + + /* If this is a software watchpoint, we try to turn it + to a hardware one -- count resources as if B was of + hardware watchpoint type. */ + type = b->base.type; + if (type == bp_watchpoint) + type = bp_hardware_watchpoint; + + /* This watchpoint may or may not have been placed on + the list yet at this point (it won't be in the list + if we're trying to create it for the first time, + through watch_command), so always account for it + manually. */ + + /* Count resources used by all watchpoints except B. */ + i = hw_watchpoint_used_count_others (&b->base, type, &other_type_used); + + /* Add in the resources needed for B. */ + i += hw_watchpoint_use_count (&b->base); + + target_resources_ok + = target_can_use_hardware_watchpoint (type, i, other_type_used); if (target_resources_ok <= 0) { int sw_mode = b->base.ops->works_in_software_mode (&b->base); @@ -1486,8 +1504,17 @@ update_watchpoint (struct watchpoint *b, else if (target_resources_ok < 0 && !sw_mode) error (_("There are not enough available hardware " "resources for this watchpoint.")); - else - b->base.type = bp_watchpoint; + + /* Downgrade to software watchpoint. */ + b->base.type = bp_watchpoint; + } + else + { + /* If this was a software watchpoint, we've just + found we have enough resources to turn it to a + hardware watchpoint. Otherwise, this is a + nop. */ + b->base.type = type; } } else if (!b->base.ops->works_in_software_mode (&b->base)) @@ -6846,28 +6873,52 @@ hw_breakpoint_used_count (void) return i; } +/* Returns the resources B would use if it were a hardware + watchpoint. */ + static int -hw_watchpoint_used_count (enum bptype type, int *other_type_used) +hw_watchpoint_use_count (struct breakpoint *b) { int i = 0; - struct breakpoint *b; struct bp_location *bl; + if (!breakpoint_enabled (b)) + return 0; + + for (bl = b->loc; bl; bl = bl->next) + { + /* Special types of hardware watchpoints may use more than + one register. */ + i += b->ops->resources_needed (bl); + } + + return i; +} + +/* Returns the sum the used resources of all hardware watchpoints of + type TYPE in the breakpoints list. Also returns in OTHER_TYPE_USED + the sum of the used resources of all hardware watchpoints of other + types _not_ TYPE. */ + +static int +hw_watchpoint_used_count_others (struct breakpoint *except, + enum bptype type, int *other_type_used) +{ + int i = 0; + struct breakpoint *b; + *other_type_used = 0; ALL_BREAKPOINTS (b) { + if (b == except) + continue; if (!breakpoint_enabled (b)) continue; - if (b->type == type) - for (bl = b->loc; bl; bl = bl->next) - { - /* Special types of hardware watchpoints may use more than - one register. */ - i += b->ops->resources_needed (bl); - } - else if (is_hardware_watchpoint (b)) - *other_type_used = 1; + if (b->type == type) + i += hw_watchpoint_use_count (b); + else if (is_hardware_watchpoint (b)) + *other_type_used = 1; } return i;