* [rfc target-side break conditions 5/5 v2] GDBServer-side changes
@ 2012-01-27 20:34 Luis Gustavo
2012-02-07 22:14 ` Stan Shebs
0 siblings, 1 reply; 10+ messages in thread
From: Luis Gustavo @ 2012-01-27 20:34 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 236 bytes --]
This updated patch addresses all the comments made before and also
handles the updated Z packet format with the "conditions" marker.
I've added a function that parses (a generic set of) options passed
through the Z packets.
Luis
[-- Attachment #2: 0004-break_condition_gdbserver.diff --]
[-- Type: text/x-patch, Size: 10888 bytes --]
2012-01-26 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-01-27 15:36:31.897821938 -0200
+++ gdb/gdb/gdbserver/server.c 2012-01-27 15:36:44.613821938 -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,36 @@ 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;
+
+ while (dataptr[0] == ',')
+ {
+ dataptr++;
+
+ if (!strncmp (dataptr, "conditions=", strlen ("conditions=")))
+ {
+ /* We have conditions to parse. */
+ dataptr += strlen ("conditions");
+
+ /* Insert conditions. */
+ do
+ {
+ dataptr++;
+ add_breakpoint_condition (point_addr, &dataptr);
+ } while (*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 +3180,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-01-27 15:36:31.885821938 -0200
+++ gdb/gdb/gdbserver/mem-break.c 2012-01-27 15:36:44.613821938 -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,
};
+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;
+
/* 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);
}
-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,119 @@ 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_t *cond, **cond_p;
+
+ if (!bp || (bp && !bp->cond_list))
+ 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)
+{
+ point_cond_list_t *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 = (char *) *condition;
+ struct agent_expr *cond;
+
+ if (!bp)
+ return 1;
+
+ 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 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_t *cl;
- return (bp != NULL);
+ struct regcache *regcache = get_thread_regcache (current_inferior, 1);
+
+ if (!bp)
+ return 0;
+
+ /* Check if the breakpoint is unconditional. If it is,
+ the condition always evaluates to TRUE. */
+ if (!bp->cond_list)
+ return 1;
+
+ /* 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 != NULL && !value; cl = cl->next)
+ {
+ /* Evaluate the condition. */
+ gdb_eval_agent_expr (regcache, NULL, cl->cond, &value);
+ value = value? 1 : 0;
+ }
+
+ 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-01-27 15:36:31.909821938 -0200
+++ gdb/gdb/gdbserver/mem-break.h 2012-01-27 15:36:44.665821938 -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-01-27 15:36:31.925821938 -0200
+++ gdb/gdb/gdbserver/linux-low.c 2012-01-27 15:36:44.669821938 -0200
@@ -2390,7 +2390,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
@@ -3253,8 +3254,10 @@ need_step_over_p (struct inferior_list_e
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
{
/* Don't step over a breakpoint that GDB expects to hit
- though. */
- if (gdb_breakpoint_here (pc))
+ though. If the condition is being evaluated on the target's side
+ and it evaluate to false, step over this breakpoint as well. */
+ if (gdb_breakpoint_here (pc)
+ && gdb_condition_true_at_breakpoint (pc))
{
if (debug_threads)
fprintf (stderr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-01-27 20:34 [rfc target-side break conditions 5/5 v2] GDBServer-side changes Luis Gustavo
@ 2012-02-07 22:14 ` Stan Shebs
2012-02-08 23:14 ` Luis Gustavo
0 siblings, 1 reply; 10+ messages in thread
From: Stan Shebs @ 2012-02-07 22:14 UTC (permalink / raw)
To: gdb-patches
On 1/27/12 12:33 PM, Luis Gustavo wrote:
> This updated patch addresses all the comments made before and also
> handles the updated Z packet format with the "conditions" marker.
>
> I've added a function that parses (a generic set of) options passed
> through the Z packets.
This all looks ready to go in, modulo my syntax characters comment from
earlier.
Incidentally, don't we officially say "GDBserver" rather than "GDBServer"?
Stan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-07 22:14 ` Stan Shebs
@ 2012-02-08 23:14 ` Luis Gustavo
2012-02-09 12:33 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Luis Gustavo @ 2012-02-08 23:14 UTC (permalink / raw)
To: Stan Shebs; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 643 bytes --]
On 02/07/2012 08:14 PM, Stan Shebs wrote:
> On 1/27/12 12:33 PM, Luis Gustavo wrote:
>> This updated patch addresses all the comments made before and also
>> handles the updated Z packet format with the "conditions" marker.
>>
>> I've added a function that parses (a generic set of) options passed
>> through the Z packets.
>
> This all looks ready to go in, modulo my syntax characters comment from
> earlier.
>
> Incidentally, don't we officially say "GDBserver" rather than "GDBServer"?
The code that handles parsing of conditions in the packet has been updated.
I fixed the NEWS and other places to use GDBserver instead.
Thanks.
Luis
[-- Attachment #2: 0004-break_condition_gdbserver.diff --]
[-- Type: text/x-patch, Size: 10833 bytes --]
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+");
+
return;
}
@@ -2825,6 +2828,35 @@ 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;
+
+ while (dataptr[0] == ';')
+ {
+ dataptr++;
+
+ if (!strncmp (dataptr, "conditions=", strlen ("conditions=")))
+ {
+ /* We have conditions to parse. */
+ dataptr += strlen ("conditions=");
+
+ /* Insert conditions. */
+ do
+ {
+ add_breakpoint_condition (point_addr, &dataptr);
+ } while (*dataptr == 'X');
+ }
+ }
+
+ *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 +3179,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-08 15:57:07.387074997 -0200
+++ gdb/gdb/gdbserver/mem-break.c 2012-02-08 15:57:33.143074994 -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,
};
+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;
+
/* 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);
}
-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,118 @@ 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_t *cond, **cond_p;
+
+ if (!bp || (bp && !bp->cond_list))
+ 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)
+{
+ point_cond_list_t *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 = (char *) *condition;
+ struct agent_expr *cond;
+
+ if (!bp)
+ return 1;
+
+ 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 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_t *cl;
- return (bp != NULL);
+ struct regcache *regcache = get_thread_regcache (current_inferior, 1);
+
+ if (!bp)
+ return 0;
+
+ /* Check if the breakpoint is unconditional. If it is,
+ the condition always evaluates to TRUE. */
+ if (!bp->cond_list)
+ return 1;
+
+ /* 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);
+ }
+
+ 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-08 15:57:07.411075000 -0200
+++ gdb/gdb/gdbserver/mem-break.h 2012-02-08 15:57:33.143074994 -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-08 15:57:07.427075000 -0200
+++ gdb/gdb/gdbserver/linux-low.c 2012-02-08 15:57:33.147074994 -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
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
{
/* Don't step over a breakpoint that GDB expects to hit
- though. */
- if (gdb_breakpoint_here (pc))
+ though. If the condition is being evaluated on the target's side
+ and it evaluate to false, step over this breakpoint as well. */
+ if (gdb_breakpoint_here (pc)
+ && gdb_condition_true_at_breakpoint (pc))
{
if (debug_threads)
fprintf (stderr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-08 23:14 ` Luis Gustavo
@ 2012-02-09 12:33 ` Pedro Alves
2012-02-22 15:25 ` Luis Gustavo
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2012-02-09 12:33 UTC (permalink / raw)
To: Gustavo, Luis; +Cc: Stan Shebs, gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-09 12:33 ` Pedro Alves
@ 2012-02-22 15:25 ` Luis Gustavo
2012-02-23 17:25 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Luis Gustavo @ 2012-02-22 15:25 UTC (permalink / raw)
To: Pedro Alves; +Cc: Stan Shebs, gdb-patches
[-- 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
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
{
/* Don't step over a breakpoint that GDB expects to hit
- though. */
- if (gdb_breakpoint_here (pc))
+ though. If the condition is being evaluated on the target's side
+ and it evaluate to false, step over this breakpoint as well. */
+ if (gdb_breakpoint_here (pc)
+ && gdb_condition_true_at_breakpoint (pc))
{
if (debug_threads)
fprintf (stderr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-22 15:25 ` Luis Gustavo
@ 2012-02-23 17:25 ` Pedro Alves
2012-02-24 12:19 ` Luis Gustavo
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2012-02-23 17:25 UTC (permalink / raw)
To: Gustavo, Luis; +Cc: Stan Shebs, gdb-patches
Hi Luis,
On 02/22/2012 03:17 PM, Luis Gustavo wrote:
> On 02/09/2012 10:32 AM, Pedro Alves wrote:
>>> 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.
There's no probably :-). GDB will need to be able to tell.
Either it does, or it doesn't. If watchpoints aren't supported now, we'll need
a new qSupported bit latter when we support them. (*) No biggie, but the
documentation should be clear (may it already is though, haven't checked
for that).
(*) I thought about it a bit, and I had a moment of "damn, this isn't
going to work", thinking about the fact that we're assuming the stubs
must ignore Z packets for the same addresses, and that with watchpoints
that might not work, given that the remote side does reference counting
of resources, to handle overlapping watchpoints. Then I remembered that
GDB will never send exact duplicates of watchpoints, so it can send
1 watchpoint location for ADDR1;LEN1 and another for ADDR1;LEN2, but never
two for ADDR1;LEN1 (assuming same type). So we're good, and supporting
this for watchpoints shouldn't be hard.
>
>>
>>
>>> +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?
Hmm, thinking more, I don't think GDB should ever send tokens the
stub didn't report support for in qSupported, so in practice it
doesn't matter that much either way, but in any case:
> +/* 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++;
That's not the proper way to skip an unknown token. You need to skip
until some known marker (`;' or `,' or some such).
> + }
> + }
> + *packet = dataptr;
> +}
Everything else looked good to me now. Thanks.
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-23 17:25 ` Pedro Alves
@ 2012-02-24 12:19 ` Luis Gustavo
2012-02-24 12:52 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Luis Gustavo @ 2012-02-24 12:19 UTC (permalink / raw)
To: Pedro Alves; +Cc: Stan Shebs, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]
On 02/23/2012 02:32 PM, Pedro Alves wrote:
> Hi Luis,
>
> On 02/22/2012 03:17 PM, Luis Gustavo wrote:
>> On 02/09/2012 10:32 AM, Pedro Alves wrote:
>
>>>> 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.
>
> There's no probably :-). GDB will need to be able to tell.
> Either it does, or it doesn't. If watchpoints aren't supported now, we'll need
> a new qSupported bit latter when we support them. (*) No biggie, but the
> documentation should be clear (may it already is though, haven't checked
> for that).
Let's stick with the new qSupported bit later then.
>
> (*) I thought about it a bit, and I had a moment of "damn, this isn't
> going to work", thinking about the fact that we're assuming the stubs
> must ignore Z packets for the same addresses, and that with watchpoints
> that might not work, given that the remote side does reference counting
> of resources, to handle overlapping watchpoints. Then I remembered that
> GDB will never send exact duplicates of watchpoints, so it can send
> 1 watchpoint location for ADDR1;LEN1 and another for ADDR1;LEN2, but never
> two for ADDR1;LEN1 (assuming same type). So we're good, and supporting
> this for watchpoints shouldn't be hard.
Good! You scared me... The breakpoint code already made me dizzy.
If all the Zx packets honor what the documentation says, we should be OK.
>
>>
>>>
>>>
>>>> +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?
>
> Hmm, thinking more, I don't think GDB should ever send tokens the
> stub didn't report support for in qSupported, so in practice it
> doesn't matter that much either way, but in any case:
>
>> +/* 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++;
>
> That's not the proper way to skip an unknown token. You need to skip
> until some known marker (`;' or `,' or some such).
This section of the Z packet should contain only 0~9 chars and commas. A
semicolon may be seen in the future if someones decides to add a new
section. In any case, better be safe than sorry.
What about the following patch?
Luis
[-- Attachment #2: 0004-break_condition_gdbserver.diff --]
[-- Type: text/x-patch, Size: 11247 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:12:04.078553984 -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-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
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
{
/* Don't step over a breakpoint that GDB expects to hit
- though. */
- if (gdb_breakpoint_here (pc))
+ though. If the condition is being evaluated on the target's side
+ and it evaluate to false, step over this breakpoint as well. */
+ if (gdb_breakpoint_here (pc)
+ && gdb_condition_true_at_breakpoint (pc))
{
if (debug_threads)
fprintf (stderr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-24 12:19 ` Luis Gustavo
@ 2012-02-24 12:52 ` Pedro Alves
2012-02-24 12:59 ` Luis Gustavo
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2012-02-24 12:52 UTC (permalink / raw)
To: Gustavo, Luis; +Cc: Stan Shebs, gdb-patches
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.
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-24 12:52 ` Pedro Alves
@ 2012-02-24 12:59 ` Luis Gustavo
2012-02-24 13:01 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Luis Gustavo @ 2012-02-24 12:59 UTC (permalink / raw)
To: Pedro Alves; +Cc: Stan Shebs, gdb-patches
[-- 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
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
{
/* Don't step over a breakpoint that GDB expects to hit
- though. */
- if (gdb_breakpoint_here (pc))
+ though. If the condition is being evaluated on the target's side
+ and it evaluate to false, step over this breakpoint as well. */
+ if (gdb_breakpoint_here (pc)
+ && gdb_condition_true_at_breakpoint (pc))
{
if (debug_threads)
fprintf (stderr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes
2012-02-24 12:59 ` Luis Gustavo
@ 2012-02-24 13:01 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2012-02-24 13:01 UTC (permalink / raw)
To: Gustavo, Luis; +Cc: Stan Shebs, gdb-patches
On 02/24/2012 12:52 PM, Luis Gustavo wrote:
>
> 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.
Okay.
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-02-24 13:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-27 20:34 [rfc target-side break conditions 5/5 v2] GDBServer-side changes Luis Gustavo
2012-02-07 22:14 ` Stan Shebs
2012-02-08 23:14 ` Luis Gustavo
2012-02-09 12:33 ` Pedro Alves
2012-02-22 15:25 ` Luis Gustavo
2012-02-23 17:25 ` Pedro Alves
2012-02-24 12:19 ` Luis Gustavo
2012-02-24 12:52 ` Pedro Alves
2012-02-24 12:59 ` Luis Gustavo
2012-02-24 13:01 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox