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

Luis Gustavo
2012-02-24T12:17:02Z

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