This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
- From: Pedro Alves <palves at redhat dot com>
- To: "Gustavo, Luis" <luis_gustavo at mentor dot com>
- Cc: Stan Shebs <stanshebs at earthlink dot net>, gdb-patches at sourceware dot org
- Date: Thu, 09 Feb 2012 12:32:41 +0000
- 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>
Some comments below, but overall looks good.
On 02/08/2012 11:14 PM, Luis Gustavo wrote:
>
> 2012-02-08 Luis Machado <lgustavo@codesourcery>
>
> 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) <cond_list>: 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