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

Luis Gustavo
2012-02-22T15:17:56Z

[-- Attachment #1: Type: text/plain, Size: 4271 bytes --]

On 02/09/2012 10:32 AM, Pedro Alves wrote:
> Some comments below, but overall looks good.

Thanks for the input.

>
> 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...

This probably means all Z packets, but only breakpoints are being
implemented now on both gdbserver's and gdb's side.

>
>
>> +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)

Fixed.

>
>> + {
>> + /* 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. :-)

Silently ignore. I thought further about the "conditions" marker and i
decided to drop it. We may want to pass more attributes in the future,
and these markers will be an overhead and will possibly get in the way.

I'm passing plain expressions now, with the first char identifying the
action. This is in synch with how tracepoint actions/attributes are
passed down to the target.

This also makes it easier to ignore unknown tokens, which in turn allows
people to easily extend the list of attributes in Z packets in the future.

What do you think?

>> + /* 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?
>

I agree. Thanks for pointing this out.

All the other things were fixed.

Luis

[-- Attachment #2: 0004-break_condition_gdbserver.diff --]
[-- Type: text/x-patch, Size: 11124 bytes --]

2012-02-22 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-22 12:31:03.950553987 -0200
+++ gdb/gdb/gdbserver/server.c 2012-02-22 12:31:54.594553988 -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+");
+
return;
}

@@ -2825,6 +2828,40 @@ main (int argc, char *argv[])
}
}

+/* Process options coming from Z packets for *point at address
+ POINT_ADDR. PACKET is the packet buffer. *PACKET is updated
+ to point to the first char after the last processed option. */
+
+static void
+process_point_options (CORE_ADDR point_addr, char **packet)
+{
+ char *dataptr = *packet;
+
+ /* Check if data has the correct format. */
+ if (*dataptr != ';')
+ return;
+
+ dataptr++;
+
+ while (*dataptr)
+ {
+ switch (*dataptr)
+ {
+ case 'X':
+ /* Conditional expression. */
+ fprintf (stderr, "Found breakpoint condition.\n");
+ add_breakpoint_condition (point_addr, &dataptr);
+ break;
+ default:
+ /* Unrecognized token, just skip it. */
+ fprintf (stderr, "Unknown token %c, ignoring.\n",
+ *dataptr);
+ dataptr++;
+ }
+ }
+ *packet = dataptr;
+}
+
/* Event loop callback that handles a serial event. The first byte in
the serial buffer gets us here. We expect characters to arrive at
a brisk pace, so we read the rest of the packet with a blocking
@@ -3147,7 +3184,22 @@ process_serial_event (void)
case '4': /* access watchpoint */
require_running (own_buf);
if (insert && the_target->insert_point != NULL)
- res = (*the_target->insert_point) (type, addr, len);
+ {
+ /* Insert the breakpoint. If it is already inserted, nothing
+ will take place. */
+ res = (*the_target->insert_point) (type, addr, len);
+
+ /* GDB may have sent us a list of *point parameters to be
+ evaluated on the target's side. Read such list here. If we
+ already have a list of parameters, GDB is telling us to drop
+ that list and use this one instead. */
+ if (!res && (type == '0' || type == '1'))
+ {
+ /* Remove previous conditions. */
+ clear_gdb_breakpoint_conditions (addr);
+ process_point_options (addr, &dataptr);
+ }
+ }
else if (!insert && the_target->remove_point != NULL)
res = (*the_target->remove_point) (type, addr, len);
break;
Index: gdb/gdb/gdbserver/mem-break.c
===================================================================
--- gdb.orig/gdb/gdbserver/mem-break.c 2012-02-22 12:31:03.938553986 -0200
+++ gdb/gdb/gdbserver/mem-break.c 2012-02-22 12:31:18.374553987 -0200
@@ -20,6 +20,8 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */

#include "server.h"
+#include "regcache.h"
+#include "ax.h"

const unsigned char *breakpoint_data;
int breakpoint_len;
@@ -85,6 +87,16 @@ enum bkpt_type
other_breakpoint,
};

+struct point_cond_list
+{
+ /* Pointer to the agent expression that is the breakpoint's
+ conditional. */
+ struct agent_expr *cond;
+
+ /* Pointer to the next condition. */
+ struct point_cond_list *next;
+};
+
/* 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 *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);
}

-static struct breakpoint *
+struct breakpoint *
find_gdb_breakpoint_at (CORE_ADDR where)
{
struct process_info *proc = current_process ();
@@ -692,6 +710,9 @@ delete_gdb_breakpoint_at (CORE_ADDR addr
if (bp == NULL)
return -1;

+ /* Before deleting the breakpoint, make sure to free
+ its condition list. */
+ clear_gdb_breakpoint_conditions (addr);
err = delete_breakpoint (bp);
if (err)
return -1;
@@ -699,12 +720,123 @@ delete_gdb_breakpoint_at (CORE_ADDR addr
return 0;
}

+/* 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 *cond, **cond_p;
+
+ if (bp == NULL || bp->cond_list == NULL)
+ return;
+
+ cond = bp->cond_list;
+ cond_p = &bp->cond_list->next;
+
+ while (cond != NULL)
+ {
+ free (cond->cond);
+ free (cond);
+ cond = *cond_p;
+ cond_p = &cond->next;
+ }
+
+ bp->cond_list = NULL;
+}
+
+/* Add condition CONDITION to GDBserver's breakpoint BP. */
+
+void
+add_condition_to_breakpoint (struct breakpoint *bp,
+ struct agent_expr *condition)
+{
+ struct point_cond_list *new_cond;
+
+ /* Create new condition. */
+ new_cond = xcalloc (1, sizeof (*new_cond));
+ new_cond->cond = condition;
+
+ /* Add condition to the list. */
+ new_cond->next = bp->cond_list;
+ bp->cond_list = new_cond;
+}
+
+/* Add a target-side condition CONDITION to the breakpoint at ADDR. */
+
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 = *condition;
+ struct agent_expr *cond;
+
+ if (bp == NULL)
+ return 1;
+
+ if (condition == NULL)
+ return 1;
+
+ cond = gdb_parse_agent_expr (&actparm);
+
+ if (cond == NULL)
+ {
+ fprintf (stderr, "Condition evaluation failed. "
+ "Assuming unconditional.\n");
+ return 0;
+ }
+
+ add_condition_to_breakpoint (bp, cond);
+
+ *condition = actparm;
+
+ return 0;
+}
+
+/* Evaluate condition (if any) at breakpoint BP. Return 1 if
+ true and 0 otherwise. */
+
+int
+gdb_condition_true_at_breakpoint (CORE_ADDR where)
+{
+ /* Fetch registers for the current inferior. */
struct breakpoint *bp = find_gdb_breakpoint_at (where);
+ ULONGEST value = 0;
+ struct point_cond_list *cl;
+ int err = 0;
+
+ struct regcache *regcache = get_thread_regcache (current_inferior, 1);

- return (bp != NULL);
+ if (bp == NULL)
+ return 0;
+
+ /* Check if the breakpoint is unconditional. If it is,
+ the condition always evaluates to TRUE. */
+ if (bp->cond_list == NULL)
+ return 1;
+
+ /* Evaluate each condition in the breakpoint's list of conditions.
+ Return true if any of the conditions evaluates to TRUE.
+
+ If we failed to evaluate the expression, TRUE is returned. This
+ forces GDB to reevaluate the conditions. */
+ for (cl = bp->cond_list;
+ cl && !value && !err; cl = cl->next)
+ {
+ /* Evaluate the condition. */
+ err = gdb_eval_agent_expr (regcache, NULL, cl->cond, &value);
+ }
+
+ return (value != 0);
+}
+
+/* Return 1 if there is a breakpoint inserted in address WHERE
+ and if its condition, if it exists, is true. */
+
+int
+gdb_breakpoint_here (CORE_ADDR where)
+{
+ return (find_gdb_breakpoint_at (where) != NULL);
}

void
Index: gdb/gdb/gdbserver/mem-break.h
===================================================================
--- gdb.orig/gdb/gdbserver/mem-break.h 2012-02-22 12:31:03.962553985 -0200
+++ gdb/gdb/gdbserver/mem-break.h 2012-02-22 12:31:18.378553986 -0200
@@ -25,6 +25,11 @@
struct breakpoint;
struct fast_tracepoint_jump;

+/* Locate a breakpoint placed at address WHERE and return a pointer
+ to its structure. */
+
+struct breakpoint *find_gdb_breakpoint_at (CORE_ADDR where);
+
/* Create a new GDB breakpoint at WHERE. Returns -1 if breakpoints
are not supported on this target, 0 otherwise. */

@@ -39,6 +44,19 @@ int breakpoint_here (CORE_ADDR addr);

int breakpoint_inserted_here (CORE_ADDR addr);

+/* Clear all breakpoint conditions associated with this address. */
+
+void clear_gdb_breakpoint_conditions (CORE_ADDR addr);
+
+/* Set target-side condition CONDITION to the breakpoint at ADDR. */
+
+int add_breakpoint_condition (CORE_ADDR addr, char **condition);
+
+/* Evaluation condition (if any) at breakpoint BP. Return 1 if
+ true and 0 otherwise. */
+
+int gdb_condition_true_at_breakpoint (CORE_ADDR where);
+
/* Returns TRUE if there's a GDB breakpoint set at ADDR. */

int gdb_breakpoint_here (CORE_ADDR where);
Index: gdb/gdb/gdbserver/linux-low.c
===================================================================
--- gdb.orig/gdb/gdbserver/linux-low.c 2012-02-22 12:31:03.978553985 -0200
+++ gdb/gdb/gdbserver/linux-low.c 2012-02-22 12:31:18.382553984 -0200
@@ -2398,7 +2398,8 @@ Check if we're already there.\n",
|| event_child->stopped_by_watchpoint
|| (!step_over_finished
&& !bp_explains_trap && !trace_event)
- || gdb_breakpoint_here (event_child->stop_pc));
+ || (gdb_breakpoint_here (event_child->stop_pc)
+ && gdb_condition_true_at_breakpoint (event_child->stop_pc)));

/* We found no reason GDB would want us to stop. We either hit one
of our own breakpoints, or finished an internal step GDB
@@ -3261,8 +3262,10 @@ need_step_over_p (struct inferior_list_e