From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4273 invoked by alias); 9 Feb 2012 12:33:14 -0000 Received: (qmail 4263 invoked by uid 22791); 9 Feb 2012 12:33:13 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 09 Feb 2012 12:33:00 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q19CWhgn012803 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 9 Feb 2012 07:32:43 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q19CWfVG002231; Thu, 9 Feb 2012 07:32:42 -0500 Message-ID: <4F33BCE9.8080900@redhat.com> Date: Thu, 09 Feb 2012 12:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 MIME-Version: 1.0 To: "Gustavo, Luis" CC: Stan Shebs , gdb-patches@sourceware.org Subject: Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes References: <4F230A29.3060404@mentor.com> <4F31A249.9000800@earthlink.net> <4F3301C1.9070400@mentor.com> In-Reply-To: <4F3301C1.9070400@mentor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-02/txt/msg00138.txt.bz2 Some comments below, but overall looks good. On 02/08/2012 11:14 PM, Luis Gustavo wrote: > > 2012-02-08 Luis Machado > > gdbserver/ > * server.c (handle_query): Advertise support for target-side > breakpoint condition evaluation. > (process_point_options): New function. > (process_serial_event): When inserting a breakpoint, check for > a target-side condition that should be evaluated. > > * mem-break.c: Include regcache.h and ax.h. > (point_cond_list_t): New data structure. > (breakpoint) : New field. > (find_gdb_breakpoint_at): Make non-static. > (delete_gdb_breakpoint_at): Clear any target-side > conditions. > (clear_gdb_breakpoint_conditions): New function. > (add_condition_to_breakpoint): Likewise. > (add_breakpoint_condition): Likewise. > (gdb_condition_true_at_breakpoint): Likewise. > (gdb_breakpoint_here): Return result directly instead > of going through a local variable. > > * mem-break.h (find_gdb_breakpoint_at): New prototype. > (clear_gdb_breakpoint_conditions): Likewise. > (add_breakpoint_condition): Likewise. > (gdb_condition_true_at_breakpoint): Likewise. > > * linux-low.c (linux_wait_1): Evaluate target-side breakpoint condition. > (need_step_over_p): Take target-side breakpoint condition into > consideration. > > Index: gdb/gdb/gdbserver/server.c > =================================================================== > --- gdb.orig/gdb/gdbserver/server.c 2012-02-08 15:57:07.399075002 -0200 > +++ gdb/gdb/gdbserver/server.c 2012-02-08 15:57:33.139074999 -0200 > @@ -1621,6 +1621,9 @@ handle_query (char *own_buf, int packet_ > strcat (own_buf, ";tracenz+"); > } > > + /* Support target-side breakpoint conditions. */ > + strcat (own_buf, ";ConditionalBreakpoints+"); I still think it's a shame this doesn't mean all Z packets understand target side conditionals... > +static void > +process_point_options (CORE_ADDR point_addr, char **packet) > +{ > + char *dataptr = *packet; > + > + while (dataptr[0] == ';') > + { > + dataptr++; > + > + if (!strncmp (dataptr, "conditions=", strlen ("conditions="))) strncmp's return is not a boolean. Please write as if (strncmp (dataptr, "conditions=", strlen ("conditions=")) == 0) > + { > + /* We have conditions to parse. */ > + dataptr += strlen ("conditions="); > + > + /* Insert conditions. */ > + do > + { > + add_breakpoint_condition (point_addr, &dataptr); > + } while (*dataptr == 'X'); > + } > + } Should we silently ignore unknown options, or error? If the former, then you should skip to the next `;' and go from there. If the latter, well, and error is missing. :-) > +typedef struct point_cond_list_t > +{ > + /* Pointer to the agent expression that is the breakpoint's > + conditional. */ > + struct agent_expr *cond; > + > + /* Pointer to the next condition. */ > + struct point_cond_list_t *next; > +} point_cond_list_t; I know where the `typedef struct foo_t {} foo_t' style comes from, but it's not gdb's or gdbserver's style. Please drop the typedef and the _t. > + > /* A high level (in gdbserver's perspective) breakpoint. */ > struct breakpoint > { > @@ -93,6 +105,12 @@ struct breakpoint > /* The breakpoint's type. */ > enum bkpt_type type; > > + /* Pointer to the condition list that should be evaluated on > + the target or NULL if the breakpoint is unconditional or > + if GDB doesn't want us to evaluate the conditionals on the > + target's side. */ > + struct point_cond_list_t *cond_list; > + > /* Link to this breakpoint's raw breakpoint. This is always > non-NULL. */ > struct raw_breakpoint *raw; > @@ -632,7 +650,7 @@ delete_breakpoint (struct breakpoint *to > return delete_breakpoint_1 (proc, todel); > } > > +/* Clear all conditions associated with this breakpoint address. */ > + > +void > +clear_gdb_breakpoint_conditions (CORE_ADDR addr) > +{ > + struct breakpoint *bp = find_gdb_breakpoint_at (addr); > + struct point_cond_list_t *cond, **cond_p; > + > + if (!bp || (bp && !bp->cond_list)) > + return; The "bp && " check is redundant. if (bp == NULL || bp->cond_list == NULL) return; > +/* Add condition CONDITION to GDBServer's breakpoint BP. */ GDBserver > int > -gdb_breakpoint_here (CORE_ADDR where) > +add_breakpoint_condition (CORE_ADDR addr, char **condition) > +{ > + struct breakpoint *bp = find_gdb_breakpoint_at (addr); > + char *actparm = (char *) *condition; Why the cast? > + struct agent_expr *cond; > + > + if (!bp) > + return 1; I'd much rather NULL check styles were consistent. Can you make all those be explicit == NULL checks, like you have right below? > + > + if (condition == NULL) > + return 1; > + > + cond = gdb_parse_agent_expr (&actparm); > + > + if (!cond) > + { > + fprintf (stderr, "Condition evaluation failed. " > + "Assuming unconditional.\n"); > + return 0; > + } > + > + add_condition_to_breakpoint (bp, cond); > + > + *condition = actparm; > + > + return 0; > +} > + > + /* Evaluate each condition in the breakpoint's list of conditions. > + Return true if any of the conditions evaluate to TRUE. */ > + for (cl = bp->cond_list; cl && !value; cl = cl->next) > + { > + /* Evaluate the condition. */ > + gdb_eval_agent_expr (regcache, NULL, cl->cond, &value); > + } This is ignoring gdb_eval_agent_expr's return code. If the expression for example touches unreadable memory and errors out, I think we should still inform gdb of the breakpoint hit, and let it re-evaluate the condition on its side (which reinforces the idea that gdb should always reevaluate the conditions). WDYT? > + > + return (value != 0); > +} > + -- Pedro Alves