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

Luis Gustavo
2012-02-24T12:52:10Z

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

On 02/24/2012 10:38 AM, Pedro Alves wrote:
> On 02/24/2012 12:17 PM, Luis Gustavo wrote:
>> + 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);
>> +}
>
> Shouldn't there be a
>
> if (err)
> return 1;
>
> somewhere?
>
> IOW, what is guaranteeing that value is != 0 when gdb_eval_agent_expr returns error?
>
>
> Otherwise looks good.
>

Doh... of course.

Fixed.

Luis

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

2012-02-24 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-24 10:08:59.850553984 -0200
+++ gdb/gdb/gdbserver/server.c 2012-02-24 10:12:04.074553986 -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,43 @@ 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);
+ }
+
+ /* Skip tokens until we find one that we recognize. */
+ while (*dataptr && *dataptr != 'X' && *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 +3187,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-24 10:08:59.838553985 -0200
+++ gdb/gdb/gdbserver/mem-break.c 2012-02-24 10:51:43.854553987 -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,126 @@ 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);
+
+ if (bp == NULL)
+ return 0;

- return (bp != NULL);
+ /* 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);
+ }
+
+ if (err)
+ return 1;
+
+ 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-24 10:08:59.862553984 -0200
+++ gdb/gdb/gdbserver/mem-break.h 2012-02-24 10:12:04.078553984 -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-24 10:08:59.878553985 -0200
+++ gdb/gdb/gdbserver/linux-low.c 2012-02-24 10:12:04.082553985 -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