Re: [PATCH] Fix error when gdb connect to a stub that tracepoint is running[1/2] reset current_trace_status in the begin of remote_start_remote

[Yao Qi <yao at codesourcery dot com>]

On 01/31/2012 10:04 AM, Hui Zhu wrote:
> #9  0x00000000006cc8e5 in post_create_inferior (target=0x1d1e860,
> from_tty=1) at ../../src/gdb/infcmd.c:431
> #10 0x00000000006d479d in start_remote (from_tty=1) at
> ../../src/gdb/infrun.c:2309
> #11 0x00000000005d80c9 in remote_start_remote (from_tty=1,
> target=0x1cefce0, extended_p=0)
>     at ../../src/gdb/remote.c:3367
> 
> About this patch, what I do is:
> 1. If we try to connect an new stub, reset current_trace_status to unknown.
> 2. Change remote_can_download_tracepoint to if the current_trace_status
> is unknown, return false.
> After this patch, the tracepoint insered after:
>   /* Possibly the target has been engaged in a trace run started
>      previously; find out where things are at.  */
>   if (remote_get_trace_status (current_trace_status ()) != -1)
>     {
> I think that will make us handle tracepoint issue more easy.

It is caused by setting `inserted' flag improperly, but I have some
different understanding on the cause of improper-set-of-inserted-flag.

As you posted, the call stack is,

remote_start_remote
        |
        +--> start_remote
        |          |
        |          `--> .... [1]
        `--> merge_uploaded_tracepoints

Error occurs in the callees of [1].  In fact, the `inserted' flag will
be set in merge_uploaded_tracepoints, if it has been in remote target
(See merge_uploaded_tracepoints).  The intention of these bits is to
mark tracepoint bp_locations as `inserted' to avoid inserting them
again.  The root cause is that we use the `inserted' flag (in calleees
of [1]) prior to setting it properly (in merge_uploaded_tracepoints), so
the fix to this problem is moving merge_uploaded_tracepoints prior to
start_remote.  I drafted a patch in this way, and fixed the problem you
posted.

Patch attached is used to illustrate my thought to fix this problem.  I
am not confident on it because I don't know it is correct to change the
order of function calls in remote_start_remote.  The "non stop" path in
remote_start_remote is not affected by this patch.  In the "stop" path,
the order of some functions call is changed, but don't know they are
equivalent.

   Original                            Patched

   start_remote                  merge_uploaded_tracepoints
   remote_check_symbols          start_remote
   merge_uploaded_tracepoints    remote_check_symbols

-- 
Yao (éå)

	* remote.c (remote_upload_merge_tp_tsv): New.
	(remote_start_remote):  Move some bits to
	remote_upload_merge_tp_tsv and call it.
---
 gdb/remote.c |   52 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 60d7ecd..090f68c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3150,6 +3150,32 @@ send_interrupt_sequence (void)
 }
 
 static void
+remote_upload_merge_tp_tsv (void)
+{
+  /* Possibly the target has been engaged in a trace run started
+     previously; find out where things are at.  */
+  if (remote_get_trace_status (current_trace_status ()) != -1)
+    {
+      struct uploaded_tp *uploaded_tps = NULL;
+      struct uploaded_tsv *uploaded_tsvs = NULL;
+
+      if (current_trace_status ()->running)
+	printf_filtered (_("Trace is already running on the target.\n"));
+
+      /* Get trace state variables first, they may be checked when
+	 parsing uploaded commands.  */
+
+      remote_upload_trace_state_variables (&uploaded_tsvs);
+
+      merge_uploaded_trace_state_variables (&uploaded_tsvs);
+
+      remote_upload_tracepoints (&uploaded_tps);
+
+      merge_uploaded_tracepoints (&uploaded_tps);
+    }
+}
+
+static void
 remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 {
   struct remote_state *rs = get_remote_state ();
@@ -3313,6 +3339,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	  target_find_description ();
 	}
 
+      remote_upload_merge_tp_tsv ();
+
       /* Use the previously fetched status.  */
       gdb_assert (wait_status != NULL);
       strcpy (rs->buf, wait_status);
@@ -3387,6 +3415,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 
       /* Report all signals during attach/startup.  */
       remote_pass_signals (0, NULL);
+
+      remote_upload_merge_tp_tsv ();
     }
 
   /* If we connected to a live target, do some additional setup.  */
@@ -3396,28 +3426,6 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	remote_check_symbols (symfile_objfile);
     }
 
-  /* Possibly the target has been engaged in a trace run started
-     previously; find out where things are at.  */
-  if (remote_get_trace_status (current_trace_status ()) != -1)
-    {
-      struct uploaded_tp *uploaded_tps = NULL;
-      struct uploaded_tsv *uploaded_tsvs = NULL;
-
-      if (current_trace_status ()->running)
-	printf_filtered (_("Trace is already running on the target.\n"));
-
-      /* Get trace state variables first, they may be checked when
-	 parsing uploaded commands.  */
-
-      remote_upload_trace_state_variables (&uploaded_tsvs);
-
-      merge_uploaded_trace_state_variables (&uploaded_tsvs);
-
-      remote_upload_tracepoints (&uploaded_tps);
-
-      merge_uploaded_tracepoints (&uploaded_tps);
-    }
-
   /* If breakpoints are global, insert them now.  */
   if (gdbarch_has_global_breakpoints (target_gdbarch)
       && breakpoints_always_inserted_mode ())
-- 
1.7.0.4