This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCH, gdbserver] Fix a bug on uploading tracepoint actions.
- From: Yao Qi <yao at codesourcery dot com>
- To: <gdb-patches at sourceware dot org>
- Date: Thu, 7 Mar 2013 16:33:16 +0800
- Subject: [PATCH, gdbserver] Fix a bug on uploading tracepoint actions.
When I test saving tracepoint actions to CTF data, I find tracepoint
actions are not uploaded from GDBserver. The bug is caused by a
comparison between signed and unsigned below:
else if (cur_action < cur_tpoint->numactions - 1)
numactions was a signed integer, and was changed to unsigned by this
(my) patch,
[PATCH 2/2] Use sized types in tracepoint.
http://sourceware.org/ml/gdb-patches/2012-04/msg00335.html
and 'cur_action' is initialized to -1, so the condition can't be
true. This patch fixes this problem by changing the type to
unsigned. Regression tested on x86_64-linux.
I tried to write a test case to expose this regression, but realize
that the uploaded actions and step actions are not used in GDB except
in breakpoint.c:create_tracepoint_from_upload
else if (!VEC_empty (char_ptr, utp->actions)
|| !VEC_empty (char_ptr, utp->step_actions))
warning (_("Uploaded tracepoint %d actions "
"have no source form, ignoring them"),
utp->number);
We can't tell from GDB's behaviors about actions are uploaded or not,
unless we check the RSP packets. So no test case is written.
OTH, the uploaded actions will/may be used in
tracepoint.c:find_matching_tracepoint_location
if (b->type == utp->type
&& t->step_count == utp->step
&& t->pass_count == utp->pass
&& cond_string_is_same (t->base.cond_string, utp->cond_string)
/* FIXME also test actions. */
Do we really need testing uploaded actions here? Is it a
good (or bad) idea to stop uploading actions in GDBserver and writing
actions into tfile? This change shouldn't bring any compatibility
issues. What do you think?
gdb/gdbserver:
2013-03-07 Yao Qi <yao@codesourcery.com>
* tracepoint.c (cur_action, cur_step_action): Make them unsigned.
(cmd_qtfp): Initialize cur_action and cur_step_action 0 instead
of -1.
(cmd_qtsp): Adjust condition. Do post increment.
Set cur_action and cur_step_action back to 0.
---
gdb/gdbserver/tracepoint.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index c6f4d54..7980457 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3684,8 +3684,8 @@ cmd_qtp (char *own_buf)
/* State variables to help return all the tracepoint bits. */
static struct tracepoint *cur_tpoint;
-static int cur_action;
-static int cur_step_action;
+static unsigned int cur_action;
+static unsigned int cur_step_action;
static struct source_string *cur_source_string;
static struct trace_state_variable *cur_tsv;
@@ -3759,7 +3759,7 @@ cmd_qtfp (char *packet)
trace_debug ("Returning first tracepoint definition piece");
cur_tpoint = tracepoints;
- cur_action = cur_step_action = -1;
+ cur_action = cur_step_action = 0;
cur_source_string = NULL;
if (cur_tpoint)
@@ -3784,17 +3784,17 @@ cmd_qtsp (char *packet)
GDB misbehavior. */
strcpy (packet, "l");
}
- else if (cur_action < cur_tpoint->numactions - 1)
+ else if (cur_action < cur_tpoint->numactions)
{
- ++cur_action;
response_action (packet, cur_tpoint,
cur_tpoint->actions_str[cur_action], 0);
+ ++cur_action;
}
- else if (cur_step_action < cur_tpoint->num_step_actions - 1)
+ else if (cur_step_action < cur_tpoint->num_step_actions)
{
- ++cur_step_action;
response_action (packet, cur_tpoint,
cur_tpoint->step_actions_str[cur_step_action], 1);
+ ++cur_step_action;
}
else if ((cur_source_string
? cur_source_string->next
@@ -3809,7 +3809,7 @@ cmd_qtsp (char *packet)
else
{
cur_tpoint = cur_tpoint->next;
- cur_action = cur_step_action = -1;
+ cur_action = cur_step_action = 0;
cur_source_string = NULL;
if (cur_tpoint)
response_tracepoint (packet, cur_tpoint);
--
1.7.7.6