This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]