Re: [PATCH 3/3] MI notification on trace stop: triggered by remote

[Yao Qi <yao at codesourcery dot com>]

On 01/08/2013 03:21 AM, Pedro Alves wrote:
> I don't think trace_stop_notes is correct here.  The user
> could have changed GDB's notes since they were set it
> in the target, or the target could have decided to report
> different notes, right?  I think ts->notes instead gives you
> the notes as reported by the target, so that'd be better.
> 

When user changes 'trace_stop_notes', the stop notes in remote target
should be sync'ed, because target_set_trace_notes is called when 
executing command 'set trace-stop-notes'.  If target reports different 
notes, it is not correct to use trace_stop_notes, as you said.

The correct stop notes got from the remote target should be in 
ts->stop_desc instead of ts->notes.  Further reading gives me more 
thoughts, that, trace stop notes make sense when trace is stopped by 
command 'tstop', IIUC.  In this case, trace stop is triggered by 
remote, we shouldn't use trace stop notes at all.  In the new version 
of patch, I call 'observer_notify_trace_stopped (NULL)'.  WDYT?

> BTW, isn't the frontend always going to fetch the trace status
> afterwards anyway?  IOW is putting the notes on the MI
> notification actually useful?  Or rather, why are notes in
> the notification while other status bits are not?
> 

The reason I put trace notes and stop notes into trace started/stopped 
notification is that notes are sort of parameters of commands 'tstart' 
and 'tstop'.

GDB emits many qtstatus packets to get trace status in sync with the 
remote target.  As GDB has async remote notification on trace, the 
trace status in GDB side can be updated by notification, and save some 
qtstatus packets to query.  I'll post a follow-up patch to reduce the 
number of qtstatus packets.

>> >+    }
>> >    else
> 
> On 12/19/2012 10:54 AM, Yao Qi wrote:
>> >+	gdb_expect {
>> >+	    -re ".*${mi_gdb_prompt}$" {
>> >+		pass $test
>> >+	    }
>> >+	    timeout {
>> >+		fail "$test (timeout)"
>> >+	    }
>> >+	}
>> >+	# No =trace-started notification.
>> >+	mi_gdb_test "-trace-start" "-trace-start\r\n=breakpoint-modified\[^\n\]+\r\n\\^done" \
>> >+	    "start trace without notification"
>> >+	mi_gdb_test "-break-insert end" {.*\^done,bkpt=.*} \
>> >+	    "insert breakpoint on end"
>> >+
>> >+	mi_send_resuming_command "exec-continue" \
>> >+	    "continuing execution to end"
>> >+	set test "trace-stopped triggered by bufferfull"
> Is "bufferfull" (instead of "buffer full") a typo?

Fixed.

-- 
Yao (éå)

gdb:

2013-01-08  Yao Qi  <yao@codesourcery.com>

	* remote-notif-trace.c: Include "observer.h".
	 (remote_notif_parse_trace): Call
	 'observer_notify_trace_stopped'.

gdb/testsuite:

2013-01-08  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/mi-trace-changed.exp (test_trace_buffer_full): New.
---
 gdb/remote-notif-trace.c                     |    4 +
 gdb/testsuite/gdb.trace/mi-trace-changed.exp |   80 ++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
index c74d9cb..5374394 100644
--- a/gdb/remote-notif-trace.c
+++ b/gdb/remote-notif-trace.c
@@ -22,6 +22,7 @@
 #include "remote.h"
 #include "tracepoint.h"
 #include "remote-notif.h"
+#include "observer.h"
 
 static void
 remote_notif_trace_parse (struct notif_client *self, char *buf,
@@ -34,6 +35,9 @@ remote_notif_trace_parse (struct notif_client *self, char *buf,
       /* Skip "stop:T" in BUF.  */
       parse_trace_status (buf + 5 + 1, ts);
       gdb_assert (!ts->running);
+      /* Notify observer 'trace_stopped' without stop notes,
+	 because trace is not stopped by a command.  */
+      observer_notify_trace_stopped (NULL);
     }
   /* Ignore the trace notifications that GDB does not understand.  */
 }
diff --git a/gdb/testsuite/gdb.trace/mi-trace-changed.exp b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
index 7654638..9b54202 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
@@ -82,4 +82,84 @@ proc test_normal_tstart_stop { } {
 
 test_normal_tstart_stop
 
+# Verify that MI notification '=trace-stopped' is emitted when trace
+# buffer is full.
+
+proc test_trace_buffer_full { } {
+    with_test_prefix "tracebuffer full" {
+	global mi_gdb_prompt
+
+	if [mi_gdb_start] {
+	    return
+	}
+	mi_run_to_main
+
+	mi_gdb_test "-break-insert -a func2" {.*\^done,bkpt=.*} \
+	    "insert tracepoint on func2"
+
+	send_gdb "actions\n"
+	gdb_expect {
+	    -re "End with" {
+	    }
+	}
+
+	send_gdb "collect buf\nend\n"
+	set test "define actions"
+	gdb_expect {
+	    -re ".*${mi_gdb_prompt}$" {
+		pass $test
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	}
+	# Set stop notes.
+	mi_gdb_test "set trace-stop-notes bar" \
+	    ".*=cmd-param-changed,param=\"trace-stop-notes\",value=\"bar\".*\\^done" \
+	    "set trace stop notes"
+
+	# No =trace-started notification.
+	mi_gdb_test "-trace-start" "-trace-start\r\n=breakpoint-modified\[^\n\]+\r\n\\^done" \
+	    "start trace without notification"
+	mi_gdb_test "-break-insert end" {.*\^done,bkpt=.*} \
+	    "insert breakpoint on end"
+
+	mi_send_resuming_command "exec-continue" \
+	    "continuing execution to end"
+	set test "trace-stopped triggered by buffer full"
+	gdb_expect {
+	    # Get notification without stop notes, although stop notes
+	    # were set.  Stop notes are not shown because the trace
+	    # was not stopped with a command.
+	    -re "=trace-stopped\\\\n" {
+		pass "$test"
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	}
+
+	global async
+	# In sync mode, eat all the output.  Don't have to do so in
+	# async mode.
+	if {!$async} {
+	    gdb_expect {
+		-re ".*${mi_gdb_prompt}$" {
+		}
+	    }
+	}
+	# GDB has got the rsp notifcation from remote stub that trace
+	# is stopped.
+	mi_gdb_test "tstop" ".*Trace is not running.*" \
+	    "tstop on stopped"
+
+	mi_gdb_test "-trace-status" ".*\\^done.*stop-reason=\"overflow\".*" \
+	    "trace-status"
+
+	mi_gdb_exit
+    }
+}
+
+test_trace_buffer_full
+
 return 0
-- 
1.7.7.6