From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5967 invoked by alias); 3 May 2011 04:56:13 -0000 Received: (qmail 5959 invoked by uid 22791); 3 May 2011 04:56:10 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e6.ny.us.ibm.com (HELO e6.ny.us.ibm.com) (32.97.182.146) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 May 2011 04:55:53 +0000 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by e6.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p434VdQM018192 for ; Tue, 3 May 2011 00:31:39 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p434tqWj675928 for ; Tue, 3 May 2011 00:55:52 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p434tq2J026047 for ; Tue, 3 May 2011 00:55:52 -0400 Received: from [9.12.229.75] ([9.12.229.75]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p434tops026019; Tue, 3 May 2011 00:55:51 -0400 Subject: Re: [RFA 2/3] Demote to sw watchpoint only in update_watchpoint From: Thiago Jung Bauermann To: Ulrich Weigand Cc: gdb-patches ml In-Reply-To: <201104291726.p3THQVaC029608@d06av02.portsmouth.uk.ibm.com> References: <201104291726.p3THQVaC029608@d06av02.portsmouth.uk.ibm.com> Content-Type: text/plain; charset="UTF-8" Date: Tue, 03 May 2011 04:56:00 -0000 Message-ID: <1304398546.2245.80.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: 2011-05/txt/msg00042.txt.bz2 On Fri, 2011-04-29 at 19:26 +0200, Ulrich Weigand wrote: > Thiago Jung Bauermann wrote: > > I decided that it was cleaner if watch_command_1 just delegated > > everything to update_watchpoint. This patch makes it create a > > watchpoint, call update_watchpoint and then delete it if some error is > > hit. The only drawback I can think of is that the aborted watchpoint > > will "consume" one breakpoint number, and thus GDB will skip one number > > when creating the next breakpoint/watchpoint. This doesn't seem > > important to me. > > Hmm, I don't really like that change. However, it should be easy to > fix by just moving the set_breakpoint_number call to down below where > update_watchpoint has succeeded, shouldn't it? Indeed, it's quite simple. I wish I had thought of that. :-) This version does what you say. > > (delete_breakpoint): Add announce argument to control whether > > observers are notified of the deletion. Update all callers. > > Ugh. Adding an extra argument that everybody must be aware of just > to take care of this special case isn't really nice ... I'd prefer > if you'd either split off the parts of delete_breakpoint that are > needed to undo partial creation into a separate function, or else, > if you use the set_breakpoint_number trick, identify partial > breakpoints by the fact that their b->number is still zero. Right. This version checks whether b->number is zero. > > @@ -1432,8 +1436,22 @@ update_watchpoint (struct breakpoint *b, int reparse) > > target_resources_ok = target_can_use_hardware_watchpoint > > (bp_hardware_watchpoint, i, other_type_used); > > This should now use b->type instead of hardcoding bp_hardware_watchpoint, > shouldn't it? No, because the line above it (on which the one you mention depends because of the i variable) hardcodes bp_hardware_watchpoint: i = hw_watchpoint_used_count (bp_hardware_watchpoint, &other_type_used); I hardcoded bp_hardware_watchpoint in an attempt to make read and access watchpoints also count as hardware watchpoints when determining the number of used and available debug resources (otherwise only watchpoints of the same type as the one being created are taken into account when counting how many debug resources are used). I just realised it doesn't work, and to fix it I'd have to slightly change the meaning of the other_type_used argument to be the number of other types of watchpoints, instead of just one or zero indicated whether there is any watchpoint of another type being used. I didn't make the change because I don't know if there's any target which would break. I couldn't find anywhere what other_type_used actually means... On the other hand, the current code is already broken in that regard, and creating several read and access watchpoints on today's GDB will confuse it and allow creating more hardware watchpoints (read, access or regular) than possible. Thus, this patch doesn't make the situation any worse. So now I just use b->type. The real fix IMHO would be to make the -tdep code manage the creation and deletion of bp_locations, so that it could always make an informed decision about what can and cannot be done with the available hardware debug resources. But that's out of the scope of this patch series. > Otherwise looks good to me. Nice! What about this version? -- []'s Thiago Jung Bauermann IBM Linux Technology Center Demote to sw watchpoint only in update_watchpoint. 2011-05-03 Thiago Jung Bauermann * breakpoint.c (update_watchpoint): Change between software and hardware watchpoint for all kinds of watchpoints, not just read/write ones. Determine b->exact value here instead of in watch_command_1. Error out if there are not enough resources for a read or access hardware watchpoint. (watch_command_1): Remove logic of checking whether there are enough resources available, since update_watchpoint will do that work now. Don't set b->exact here. Catch exceptions thrown by update_watchpoint and delete the watchpoint. (can_use_hardware_watchpoint): Remove exact_watchpoints argument. Use target_exact_watchpoints instead. (delete_breakpoint): Notify observers only if deleted watchpoint has a breakpoint number assigned to it. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e265135..8358dac 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -101,7 +101,7 @@ static void clear_command (char *, int); static void catch_command (char *, int); -static int can_use_hardware_watchpoint (struct value *, int); +static int can_use_hardware_watchpoint (struct value *); static void break_command_1 (char *, int, int); @@ -1404,19 +1404,22 @@ update_watchpoint (struct breakpoint *b, int reparse) an ordinary watchpoint depending on the hardware support and free hardware slots. REPARSE is set when the inferior is started. */ - if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint) - && reparse) + if (reparse) { int reg_cnt; enum bp_loc_type loc_type; struct bp_location *bl; - reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact); + reg_cnt = can_use_hardware_watchpoint (val_chain); if (reg_cnt) { int i, target_resources_ok, other_type_used; + /* Use an exact watchpoint when there's only one memory region to be + watched, and only one debug register is needed to watch it. */ + b->exact = target_exact_watchpoints && reg_cnt == 1; + /* 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 @@ -1424,16 +1427,29 @@ update_watchpoint (struct breakpoint *b, int reparse) hw_watchpoint_used_count call below counts this watchpoint, make sure that it is marked as a hardware watchpoint. */ - b->type = bp_hardware_watchpoint; - - i = hw_watchpoint_used_count (bp_hardware_watchpoint, - &other_type_used); + if (b->type == bp_watchpoint) + b->type = bp_hardware_watchpoint; + i = hw_watchpoint_used_count (b->type, &other_type_used); target_resources_ok = target_can_use_hardware_watchpoint - (bp_hardware_watchpoint, i, other_type_used); + (b->type, i, other_type_used); if (target_resources_ok <= 0) - b->type = bp_watchpoint; + { + if (target_resources_ok == 0 + && b->type != bp_hardware_watchpoint) + error (_("Target does not support this type of " + "hardware watchpoint.")); + else if (target_resources_ok < 0 + && b->type != bp_hardware_watchpoint) + error (_("Target can only support one kind " + "of HW watchpoint at a time.")); + else + b->type = bp_watchpoint; + } } + else if (b->type != bp_hardware_watchpoint) + error (_("Expression cannot be implemented with " + "read/access watchpoint.")); else b->type = bp_watchpoint; @@ -8783,6 +8799,7 @@ static void watch_command_1 (char *arg, int accessflag, int from_tty, int just_location, int internal) { + volatile struct gdb_exception e; struct breakpoint *b, *scope_breakpoint = NULL; struct expression *exp; struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL; @@ -8794,9 +8811,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty, int toklen; char *cond_start = NULL; char *cond_end = NULL; - int i, other_type_used, target_resources_ok = 0; enum bptype bp_type; - int reg_cnt = 0; int thread = -1; int pc = 0; @@ -8926,28 +8941,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty, else bp_type = bp_hardware_watchpoint; - reg_cnt = can_use_hardware_watchpoint (val, target_exact_watchpoints); - if (reg_cnt == 0 && bp_type != bp_hardware_watchpoint) - error (_("Expression cannot be implemented with read/access watchpoint.")); - if (reg_cnt != 0) - { - i = hw_watchpoint_used_count (bp_type, &other_type_used); - target_resources_ok = - target_can_use_hardware_watchpoint (bp_type, i + reg_cnt, - other_type_used); - if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint) - error (_("Target does not support this type of hardware watchpoint.")); - - if (target_resources_ok < 0 && bp_type != bp_hardware_watchpoint) - error (_("Target can only support one kind " - "of HW watchpoint at a time.")); - } - - /* Change the type of breakpoint to an ordinary watchpoint if a - hardware watchpoint could not be set. */ - if (!reg_cnt || target_resources_ok <= 0) - bp_type = bp_watchpoint; - frame = block_innermost_frame (exp_valid_block); /* If the expression is "local", then set up a "watchpoint scope" @@ -8985,7 +8978,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty, /* Now set up the breakpoint. */ b = set_raw_breakpoint_without_location (NULL, bp_type); - set_breakpoint_number (internal, b); b->thread = thread; b->disposition = disp_donttouch; b->exp = exp; @@ -9016,10 +9008,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty, b->val_valid = 1; b->ops = &watchpoint_breakpoint_ops; - /* Use an exact watchpoint when there's only one memory region to be - watched, and only one debug register is needed to watch it. */ - b->exact = target_exact_watchpoints && reg_cnt == 1; - if (cond_start) b->cond_string = savestring (cond_start, cond_end - cond_start); else @@ -9047,12 +9035,22 @@ watch_command_1 (char *arg, int accessflag, int from_tty, if (!just_location) value_free_to_mark (mark); - /* Finally update the new watchpoint. This creates the locations - that should be inserted. */ - update_watchpoint (b, 1); + TRY_CATCH (e, RETURN_MASK_ALL) + { + /* Finally update the new watchpoint. This creates the locations + that should be inserted. */ + update_watchpoint (b, 1); + } + if (e.reason < 0) + { + delete_breakpoint (b); + throw_exception (e); + } + + set_breakpoint_number (internal, b); /* Do not mention breakpoints with a negative number, but do - notify observers. */ + notify observers. */ if (!internal) mention (b); observer_notify_breakpoint_created (b); @@ -9061,14 +9059,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty, } /* Return count of debug registers needed to watch the given expression. - If EXACT_WATCHPOINTS is 1, then consider that only the address of - the start of the watched region will be monitored (i.e., all accesses - will be aligned). This uses less debug registers on some targets. - If the watchpoint cannot be handled in hardware return zero. */ static int -can_use_hardware_watchpoint (struct value *v, int exact_watchpoints) +can_use_hardware_watchpoint (struct value *v) { int found_memory_cnt = 0; struct value *head = v; @@ -9124,7 +9118,7 @@ can_use_hardware_watchpoint (struct value *v, int exact_watchpoints) int len; int num_regs; - len = (exact_watchpoints + len = (target_exact_watchpoints && is_scalar_type_recursive (vtype))? 1 : TYPE_LENGTH (value_type (v)); @@ -10483,7 +10477,12 @@ delete_breakpoint (struct breakpoint *bpt) bpt->related_breakpoint = bpt; } - observer_notify_breakpoint_deleted (bpt); + /* watch_command_1 creates a watchpoint but only sets its number if + update_watchpoint succeeds in creating its bp_locations. If there's + a problem in that process, we'll be asked to delete the half-created + watchpoint. In that case, don't announce the deletion. */ + if (bpt->number) + observer_notify_breakpoint_deleted (bpt); if (breakpoint_chain == bpt) breakpoint_chain = bpt->next;