Re: [PATCH 2/2] Reset trace local state when opening trace file

[Yao Qi <yao at codesourcery dot com>]

On 06/05/2013 07:04 PM, Pedro Alves wrote:
> This is really doing what the existing:
> 
> /* This function handles the details of what to do about an ongoing
>     tracing run if the user has asked to detach or otherwise disconnect
>     from the target.  */
> 
> void
> disconnect_tracing (void)
> {
>    /* Also we want to be out of tfind mode, otherwise things can get
>       confusing upon reconnection.  Just use these calls instead of
>       full tfind_1 behavior because we're in the middle of detaching,
>       and there's no point to updating current stack frame etc.  */
>    set_current_traceframe (-1);
>    set_tracepoint_num (-1);
>    set_traceframe_context (NULL);
> }
> 
> is supposed to do, though disconnect_tracing seems to miss doing a
> couple things the new function does.  I think they should be merged.

Yes, think a little more, I was even wondering disconnect_tracing may
be removed completely.  Since we call trace_reset_local_state in
remote_close to clear tracing state, we don't need to call function
disconnect_tracing.

As we can see, disconnect_tracing is called from three places,
detach_command, disconnect_command, and force_quit, and remote_close is
called afterwards.  We can remove disconnect_tracing and, in effect,
postpone the clearing local tracing-related state.

With this change, GDB may remove some breakpoints before clear local
state.  An extreme case is that a breakpoint is set at address FOO, and
action in a tracepoint collects the memory at FOO.  In tfind mode, when
detach to the remote target, remove breakpoints,  GDB probably read
from traceframe instead of the live inferior.  This is OK, but GDB
can't write because writing to traceframes is forbidden.  It's unsafe
to remove disconnect_tracing.

The patch below is to call trace_reset_local_state in disconnect_tracing.

-- 
Yao (éå)

gdb:

2013-06-06  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c (start_tracing): Move code to ...
	(trace_reset_local_state): ... here.  New.
	(disconnect_tracing): Don't call set_current_traceframe,
	set_tracepoint_num, and set_traceframe_context. Call
	trace_reset_local_state instead.
	(tfile_close): Call trace_reset_local_state.
	* ctf.c (ctf_close): Likewise.
	* remote.c (remote_close): Likewise.
	* tracepoint.h (trace_reset_local_state): Declare.
---
 gdb/ctf.c        |    2 ++
 gdb/remote.c     |    2 ++
 gdb/tracepoint.c |   21 ++++++++++++++-------
 gdb/tracepoint.h |    1 +
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/gdb/ctf.c b/gdb/ctf.c
index 13df089..278f950 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1212,6 +1212,8 @@ ctf_close (void)
   ctf_destroy ();
   xfree (trace_dirname);
   trace_dirname = NULL;
+
+  trace_reset_local_state ();
 }
 
 /* This is the implementation of target_ops method to_files_info.
diff --git a/gdb/remote.c b/gdb/remote.c
index d8854ae..5da7eb9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3050,6 +3050,8 @@ remote_close (void)
     delete_async_event_handler (&remote_async_inferior_event_token);
 
   remote_notif_unregister_async_event_handler ();
+
+  trace_reset_local_state ();
 }
 
 /* Query the remote side for the text, data and bss offsets.  */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index d6ff051..5bad3e8 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1700,6 +1700,16 @@ process_tracepoint_on_disconnect (void)
 	       " GDB is disconnected\n"));
 }
 
+/* Reset local state of tracing.  */
+
+void
+trace_reset_local_state (void)
+{
+  set_traceframe_num (-1);
+  set_tracepoint_num (-1);
+  set_traceframe_context (NULL);
+  clear_traceframe_info ();
+}
 
 void
 start_tracing (char *notes)
@@ -1822,11 +1832,8 @@ start_tracing (char *notes)
   target_trace_start ();
 
   /* Reset our local state.  */
-  set_traceframe_num (-1);
-  set_tracepoint_num (-1);
-  set_traceframe_context (NULL);
+  trace_reset_local_state ();
   current_trace_status()->running = 1;
-  clear_traceframe_info ();
 }
 
 /* The tstart command requests the target to start a new trace run.
@@ -2234,9 +2241,7 @@ disconnect_tracing (void)
      confusing upon reconnection.  Just use these calls instead of
      full tfind_1 behavior because we're in the middle of detaching,
      and there's no point to updating current stack frame etc.  */
-  set_current_traceframe (-1);
-  set_tracepoint_num (-1);
-  set_traceframe_context (NULL);
+  trace_reset_local_state ();
 }
 
 /* Worker function for the various flavors of the tfind command.  */
@@ -4657,6 +4662,8 @@ tfile_close (void)
   trace_fd = -1;
   xfree (trace_filename);
   trace_filename = NULL;
+
+  trace_reset_local_state ();
 }
 
 static void
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index d7ebc16..3b09ca8 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -389,6 +389,7 @@ extern void merge_uploaded_trace_state_variables (struct uploaded_tsv **utsvp);
 
 extern void query_if_trace_running (int from_tty);
 extern void disconnect_tracing (void);
+extern void trace_reset_local_state (void);
 
 extern void start_tracing (char *notes);
 extern void stop_tracing (char *notes);
-- 
1.7.7.6