Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes

Pedro Alves
2012-02-09T12:33:14Z

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