[PATCH] Fix tracepoint tstart again get gdb_assert

[PATCH] Fix tracepoint tstart again get gdb_assert

[Hui Zhu]

Hi,

I use a gdb tstart again in a section an got:

(gdb) tstart
(gdb) tstop
(gdb) tstart
../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
Assertion `!loc->inserted' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

The reason is:
start_tracing:
      for (loc = b->loc; loc; loc = loc->next)
	{
	  /* Since tracepoint locations are never duplicated, `inserted'
	     flag should be zero.  */
	  gdb_assert (!loc->inserted);

	  target_download_tracepoint (loc);

	  loc->inserted = 1;
	}

But in stop_tracing and trace_status_command don't have code to set
inserted back to 0.

So I make a patch for  it.

Thanks,
Hui

2011-12-06  Hui Zhu  <teawater@gmail.com>

	* tracepoint.c (stop_tracing): Set loc->inserted if need.
	(trace_status_command): Ditto.
---
 tracepoint.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

--- a/tracepoint.c
+++ b/tracepoint.c
@@ -1847,6 +1847,9 @@ void
 stop_tracing (char *note)
 {
   int ret;
+  VEC(breakpoint_p) *tp_vec = NULL;
+  int ix;
+  struct breakpoint *b;

   target_trace_stop ();

@@ -1859,6 +1862,22 @@ stop_tracing (char *note)

   /* Should change in response to reply?  */
   current_trace_status ()->running = 0;
+
+  tp_vec = all_tracepoints ();
+  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+    {
+      struct tracepoint *t = (struct tracepoint *) b;
+      struct bp_location *loc;
+
+      if ((b->type == bp_fast_tracepoint
+	   ? !may_insert_fast_tracepoints
+	   : !may_insert_tracepoints))
+	continue;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	loc->inserted = 0;
+    }
+  VEC_free (breakpoint_p, tp_vec);
 }

 /* tstatus command */
@@ -1868,7 +1887,7 @@ trace_status_command (char *args, int fr
   struct trace_status *ts = current_trace_status ();
   int status, ix;
   VEC(breakpoint_p) *tp_vec = NULL;
-  struct breakpoint *t;
+  struct breakpoint *b;

   status = target_get_trace_status (ts);

@@ -2013,8 +2032,24 @@ trace_status_command (char *args, int fr
   /* Now report any per-tracepoint status available.  */
   tp_vec = all_tracepoints ();

-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
-    target_get_tracepoint_status (t, NULL);
+  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+    {
+      struct tracepoint *t = (struct tracepoint *) b;
+      struct bp_location *loc;
+
+      target_get_tracepoint_status (b, NULL);
+
+      if (!ts->running)
+	{
+	  if ((b->type == bp_fast_tracepoint
+	      ? !may_insert_fast_tracepoints
+	      : !may_insert_tracepoints))
+	    continue;
+
+	  for (loc = b->loc; loc; loc = loc->next)
+	    loc->inserted = 0;
+	}
+    }

   VEC_free (breakpoint_p, tp_vec);
 }

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Yao Qi]

On 12/06/2011 11:56 PM, Hui Zhu wrote:
> Hi,
> 
> I use a gdb tstart again in a section an got:
> 
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
> 

Thanks for this patch!  It is related to my previous commit

  [patch 4/8] Download tracepoint locations and track its status
  http://sourceware.org/ml/gdb-patches/2011-11/msg00337.html

> The reason is:
> start_tracing:
>       for (loc = b->loc; loc; loc = loc->next)
> 	{
> 	  /* Since tracepoint locations are never duplicated, `inserted'
> 	     flag should be zero.  */
> 	  gdb_assert (!loc->inserted);
> 
> 	  target_download_tracepoint (loc);
> 
> 	  loc->inserted = 1;
> 	}
> 
> But in stop_tracing and trace_status_command don't have code to set
> inserted back to 0.
> 

Right, I agree that loc->inserted should be cleared on stop_tracing.
However, I don't know why we have to clear loc->inserted in
trace_status_command.  A few comments below.

> So I make a patch for  it.
> 
> Thanks,
> Hui
> 
> 2011-12-06  Hui Zhu  <teawater@gmail.com>
> 
> 	* tracepoint.c (stop_tracing): Set loc->inserted if need.
> 	(trace_status_command): Ditto.
> ---
>  tracepoint.c |   41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -1847,6 +1847,9 @@ void
>  stop_tracing (char *note)
>  {
>    int ret;
> +  VEC(breakpoint_p) *tp_vec = NULL;
> +  int ix;
> +  struct breakpoint *b;
> 
>    target_trace_stop ();
> 
> @@ -1859,6 +1862,22 @@ stop_tracing (char *note)
> 
>    /* Should change in response to reply?  */
>    current_trace_status ()->running = 0;
> +
> +  tp_vec = all_tracepoints ();
> +  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +    {
> +      struct tracepoint *t = (struct tracepoint *) b;
> +      struct bp_location *loc;
> +
> +      if ((b->type == bp_fast_tracepoint
> +	   ? !may_insert_fast_tracepoints
> +	   : !may_insert_tracepoints))
> +	continue;
> +

I don't think this check above is needed here.  In start_tracing, we
need may_insert_fast_tracepoints and may_insert_tracepoints to download
desired type of tracepoint.  However, these two variables may be changed
in a trace experiment, so we don't have to check them and clear
loc->inserted unconditionally.

> +      for (loc = b->loc; loc; loc = loc->next)
> +	loc->inserted = 0;
> +    }
> +  VEC_free (breakpoint_p, tp_vec);
>  }
> 


>  /* tstatus command */
> @@ -1868,7 +1887,7 @@ trace_status_command (char *args, int fr
>    struct trace_status *ts = current_trace_status ();
>    int status, ix;
>    VEC(breakpoint_p) *tp_vec = NULL;
> -  struct breakpoint *t;
> +  struct breakpoint *b;
> 
>    status = target_get_trace_status (ts);
> 
> @@ -2013,8 +2032,24 @@ trace_status_command (char *args, int fr
>    /* Now report any per-tracepoint status available.  */
>    tp_vec = all_tracepoints ();
> 
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
> -    target_get_tracepoint_status (t, NULL);
> +  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +    {
> +      struct tracepoint *t = (struct tracepoint *) b;
> +      struct bp_location *loc;
> +
> +      target_get_tracepoint_status (b, NULL);
> +
> +      if (!ts->running)
> +	{
> +	  if ((b->type == bp_fast_tracepoint
> +	      ? !may_insert_fast_tracepoints
> +	      : !may_insert_tracepoints))
> +	    continue;
> +
> +	  for (loc = b->loc; loc; loc = loc->next)
> +	    loc->inserted = 0;
> +	}
> +    }
> 
>    VEC_free (breakpoint_p, tp_vec);
>  }

As I said at the beginning, I don't understand why we have to clear
loc->inserted in trace_status_command when ts->running is 0.

-- 
Yao (齐尧)

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Stan Shebs]

On 12/6/11 8:08 PM, Yao Qi wrote:
> On 12/06/2011 11:56 PM, Hui Zhu wrote:
>> Hi,
>>
>> I use a gdb tstart again in a section an got:
>>
>> (gdb) tstart
>> (gdb) tstop
>> (gdb) tstart
>> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
>> Assertion `!loc->inserted' failed.
>> A problem internal to GDB has been detected,
>> further debugging may prove unreliable.
>> Quit this debugging session? (y or n)
>>
> Thanks for this patch!  It is related to my previous commit
>
>    [patch 4/8] Download tracepoint locations and track its status
>    http://sourceware.org/ml/gdb-patches/2011-11/msg00337.html
>
>> The reason is:
>> start_tracing:
>>        for (loc = b->loc; loc; loc = loc->next)
>> 	{
>> 	  /* Since tracepoint locations are never duplicated, `inserted'
>> 	     flag should be zero.  */
>> 	  gdb_assert (!loc->inserted);
>>
>> 	  target_download_tracepoint (loc);
>>
>> 	  loc->inserted = 1;
>> 	}
>>
>> But in stop_tracing and trace_status_command don't have code to set
>> inserted back to 0.
>>
> Right, I agree that loc->inserted should be cleared on stop_tracing.
> However, I don't know why we have to clear loc->inserted in
> trace_status_command.  A few comments below.
>
>

I agree, stop_tracing should clear loc->inserted, and tstatus should 
not; in general an information-reading/displaying command should not be 
making state changes.  If the assert triggers without the tstatus patch, 
then we should understand how and why.  (We might be missing a 
reconnection case or some such?)

Also, I notice that while the testsuite has lots of tstart/tstop 
sequences, it normally starts a fresh GDB each time, so it was unable to 
catch this failure.  The patch to fix should add a sequence of a couple 
trace runs in a row to one of the existing test files.

Stan

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Hui Zhu]

Hi guys,

I am sorry that I didn't talk clear about the issue of tstatus.
When we use tstatus, it can auto set the tracepoint back to stop when
it full or got error.  Then user can use tstart without tstop that set
 loc->inserted.  So we will still meet that issue if we just set
loc->inserted in tstop.

For examp:
This gdb is just set  loc->inserted in tstop but not tstatus.
(gdb) tstart
(gdb) tstop
(gdb) tstart
(gdb) tstop
(gdb) tstart
xxx
xxx
(gdb) tstatus
Trace stopped because the buffer was full.
Buffer contains 0 trace frames (of 49072 created total).
Trace buffer has 908408 bytes of 10414080 bytes free (91% full).
Trace will stop if GDB disconnects.
Not looking at any trace frame.
(gdb) tstart
../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
Assertion `!loc->inserted' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n

../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
Assertion `!loc->inserted' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n


Thanks,
Hui

On Thu, Dec 8, 2011 at 07:50, Stan Shebs <stanshebs@earthlink.net> wrote:
> On 12/6/11 8:08 PM, Yao Qi wrote:
>>
>> On 12/06/2011 11:56 PM, Hui Zhu wrote:
>>>
>>> Hi,
>>>
>>> I use a gdb tstart again in a section an got:
>>>
>>> (gdb) tstart
>>> (gdb) tstop
>>> (gdb) tstart
>>> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
>>> Assertion `!loc->inserted' failed.
>>> A problem internal to GDB has been detected,
>>> further debugging may prove unreliable.
>>> Quit this debugging session? (y or n)
>>>
>> Thanks for this patch!  It is related to my previous commit
>>
>>   [patch 4/8] Download tracepoint locations and track its status
>>   http://sourceware.org/ml/gdb-patches/2011-11/msg00337.html
>>
>>> The reason is:
>>> start_tracing:
>>>       for (loc = b->loc; loc; loc = loc->next)
>>>        {
>>>          /* Since tracepoint locations are never duplicated, `inserted'
>>>             flag should be zero.  */
>>>          gdb_assert (!loc->inserted);
>>>
>>>          target_download_tracepoint (loc);
>>>
>>>          loc->inserted = 1;
>>>        }
>>>
>>> But in stop_tracing and trace_status_command don't have code to set
>>> inserted back to 0.
>>>
>> Right, I agree that loc->inserted should be cleared on stop_tracing.
>> However, I don't know why we have to clear loc->inserted in
>> trace_status_command.  A few comments below.
>>
>>
>
> I agree, stop_tracing should clear loc->inserted, and tstatus should not; in
> general an information-reading/displaying command should not be making state
> changes.  If the assert triggers without the tstatus patch, then we should
> understand how and why.  (We might be missing a reconnection case or some
> such?)
>
> Also, I notice that while the testsuite has lots of tstart/tstop sequences,
> it normally starts a fresh GDB each time, so it was unable to catch this
> failure.  The patch to fix should add a sequence of a couple trace runs in a
> row to one of the existing test files.
>
> Stan
>
>
>

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

On 12/08/2011 11:40 AM, Hui Zhu wrote:
> Hi guys,
> 
> I am sorry that I didn't talk clear about the issue of tstatus.
> When we use tstatus, it can auto set the tracepoint back to stop when
> it full or got error.  Then user can use tstart without tstop that set
>  loc->inserted.  So we will still meet that issue if we just set
> loc->inserted in tstop.
> 
> For examp:
> This gdb is just set  loc->inserted in tstop but not tstatus.
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> xxx
> xxx
> (gdb) tstatus
> Trace stopped because the buffer was full.
> Buffer contains 0 trace frames (of 49072 created total).
> Trace buffer has 908408 bytes of 10414080 bytes free (91% full).
> Trace will stop if GDB disconnects.
> Not looking at any trace frame.
> (gdb) tstart
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n
> 
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n

This internal-error is caused by two consequent `tstart' commands, and
`inserted' flag is not cleared before calling start_tracing for the 2nd
`tstart' command.

My patch (on base of Hui's version) fixes this problem by 1) clearing
`inserted' flag at stop_tracing, and 2) calling stop_tracing in
start_tracing when existing tracing is still running.

There is no existing test case fit for this test, so I create a new test
case gdb.trace/tstart.exp.

Patch is tested on x86_64-linux, OK for mainline and 7.4 branch?

-- 
Yao (齐尧)

[-- Attachment #2: 0001-clear-inserted-in-stop_tracing.patch --]
[-- Type: text/x-patch, Size: 1289 bytes --]

2011-12-08  Hui Zhu  <teawater@gmail.com>
	    Yao Qi  <yao@codesourcery.com>

	* tracepoint.c (trace_start_command): Call sto_tracing.
        (stop_tracing): Clear `inserted' flag in each location.
---
 gdb/tracepoint.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index e00538c..ac6d112 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1824,6 +1824,8 @@ trace_start_command (char *args, int from_tty)
       if (from_tty
 	  && !query (_("A trace is running already.  Start a new run? ")))
 	error (_("New trace run not started."));
+
+      stop_tracing (NULL);
     }
 
   start_tracing (args);
@@ -1847,6 +1849,9 @@ void
 stop_tracing (char *note)
 {
   int ret;
+  VEC(breakpoint_p) *tp_vec = NULL;
+  int ix;
+  struct breakpoint *b;
 
   target_trace_stop ();
 
@@ -1859,6 +1864,16 @@ stop_tracing (char *note)
 
   /* Should change in response to reply?  */
   current_trace_status ()->running = 0;
+
+  tp_vec = all_tracepoints ();
+  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+    {
+      struct bp_location *loc;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	loc->inserted = 0;
+    }
+  VEC_free (breakpoint_p, tp_vec);
 }
 
 /* tstatus command */
-- 
1.7.0.4


[-- Attachment #3: 0002-tstart.exp.patch --]
[-- Type: text/x-patch, Size: 3060 bytes --]


2011-12-08  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/tstart.exp: New.
---
 gdb/testsuite/gdb.trace/tstart.exp |   88 ++++++++++++++++++++++++++++++++++++
 1 files changed, 88 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/tstart.exp

diff --git a/gdb/testsuite/gdb.trace/tstart.exp b/gdb/testsuite/gdb.trace/tstart.exp
new file mode 100644
index 0000000..2690dad
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/tstart.exp
@@ -0,0 +1,88 @@
+# Copyright 2011 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp";
+
+set testfile "tstart"
+set executable $testfile
+set srcfile collection.c
+set binfile $objdir/$subdir/$testfile
+set expfile $testfile.exp
+
+
+if [prepare_for_testing $expfile $executable $srcfile \
+        {debug nowarnings}] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+# Verify that the sequence of commands "tstart tstop tstart" works well.
+
+proc test_tstart_tstop_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstop_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace args_test_func" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+
+    gdb_test_no_output "tstop"
+
+    gdb_test_no_output "tstart"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Verify the sequence of commands "tstart tstart" works well.
+
+proc test_tstart_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace args_test_func" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "tstart" "" "tstart again" "A trace is running already.  Start a new run\\? \\(y or n\\) " "y"
+
+}
+
+test_tstart_tstop_tstart
+
+test_tstart_tstart
\ No newline at end of file
-- 
1.7.0.4

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On 12/08/2011 11:40 AM, Hui Zhu wrote:
> Hi guys,
> 
> I am sorry that I didn't talk clear about the issue of tstatus.
> When we use tstatus, it can auto set the tracepoint back to stop when
> it full or got error.  Then user can use tstart without tstop that set
>  loc->inserted.  So we will still meet that issue if we just set
> loc->inserted in tstop.
> 
> For examp:
> This gdb is just set  loc->inserted in tstop but not tstatus.
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> xxx
> xxx
> (gdb) tstatus
> Trace stopped because the buffer was full.
> Buffer contains 0 trace frames (of 49072 created total).
> Trace buffer has 908408 bytes of 10414080 bytes free (91% full).
> Trace will stop if GDB disconnects.
> Not looking at any trace frame.
> (gdb) tstart
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n
> 
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> 

This is the 2nd version of my patch.  Compared with 1st version (sent
four hours ago), this version covers more cases.  In this patch, a new
observer `trace_stopped' is added.  In stop_tracing and
target_get_trace_status, observer `trace_stopped' is notified, and then,
`inserted' flag is cleared.  This looks more clear than version 1.

Tested again on x86_64-linux.  No regression.

-- 
Yao (齐尧)

[-- Attachment #2: 0001-clear-inserted-in-stop_tracing.patch --]
[-- Type: text/x-patch, Size: 3828 bytes --]


2011-12-09  Hui Zhu  <teawater@gmail.com>
	    Yao Qi  <yao@codesourcery.com>

	* target.c (target_get_trace_status): New function.  Call
	observer_notify_trace_stopped if trace is stopped.
	* target.h (target_get_trace_status): Remove macro.
	* tracepoint.c (stop_tracing): Call observer_notify_trace_stopped.
	(trace_start_command): Call stop_tracing.
        (trace_stopped): New.
	(_initialize_tracepoint): Install trace_stopped to observer
	`trace_stopped'.

gdb/doc:

2011-12-09  Yao Qi  <yao@codesourcery.com>

	* observer.texi (trace_stopped): New observer.
---
 gdb/doc/observer.texi |    4 ++++
 gdb/target.c          |   12 ++++++++++++
 gdb/target.h          |    3 +--
 gdb/tracepoint.c      |   25 ++++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 689b303..1daf75a 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -188,6 +188,10 @@ A tracepoint has been modified in some way.  The argument @var{tpnum}
 is the number of the modified tracepoint.
 @end deftypefun
 
+@deftypefun void trace_stopped (void)
+A trace run has been stopped.
+@end deftypefun
+
 @deftypefun void architecture_changed (struct gdbarch *@var{newarch})
 The current architecture has changed.  The argument @var{newarch} is
 a pointer to the new architecture.
diff --git a/gdb/target.c b/gdb/target.c
index 5700066..e5bac02 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -44,6 +44,7 @@
 #include "exec.h"
 #include "inline-frame.h"
 #include "tracepoint.h"
+#include "observer.h"
 
 static void target_info (char *, int);
 
@@ -505,6 +506,17 @@ target_terminal_inferior (void)
   (*current_target.to_terminal_inferior) ();
 }
 
+int
+target_get_trace_status(struct trace_status *ts)
+{
+  int ret = (*current_target.to_get_trace_status) (ts);
+
+  if (!ts->running)
+    observer_notify_trace_stopped ();
+
+  return ret;
+}
+
 static int
 nomemory (CORE_ADDR memaddr, char *myaddr, int len, int write,
 	  struct target_ops *t)
diff --git a/gdb/target.h b/gdb/target.h
index fd488d6..8a6dce9 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1517,8 +1517,7 @@ extern int target_search_memory (CORE_ADDR start_addr,
 #define target_trace_set_readonly_regions() \
   (*current_target.to_trace_set_readonly_regions) ()
 
-#define target_get_trace_status(ts) \
-  (*current_target.to_get_trace_status) (ts)
+extern int target_get_trace_status(struct trace_status *ts);
 
 #define target_get_tracepoint_status(tp,utp)		\
   (*current_target.to_get_tracepoint_status) (tp, utp)
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index e00538c..7b4c56a 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1824,6 +1824,8 @@ trace_start_command (char *args, int from_tty)
       if (from_tty
 	  && !query (_("A trace is running already.  Start a new run? ")))
 	error (_("New trace run not started."));
+
+      stop_tracing (NULL);
     }
 
   start_tracing (args);
@@ -1857,8 +1859,27 @@ stop_tracing (char *note)
   if (!ret && note)
     warning ("Target does not support trace notes, note ignored");
 
-  /* Should change in response to reply?  */
+  observer_notify_trace_stopped ();
+}
+
+static void
+trace_stopped (void)
+{
+  VEC(breakpoint_p) *tp_vec = NULL;
+  int ix;
+  struct breakpoint *b;
+
   current_trace_status ()->running = 0;
+
+  tp_vec = all_tracepoints ();
+  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+    {
+      struct bp_location *loc;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	loc->inserted = 0;
+    }
+  VEC_free (breakpoint_p, tp_vec);
 }
 
 /* tstatus command */
@@ -5327,5 +5348,7 @@ Show the notes string to use for future tstop commands"), NULL,
 
   init_tfile_ops ();
 
+  observer_attach_trace_stopped (trace_stopped);
+
   add_target (&tfile_ops);
 }
-- 
1.7.0.4


[-- Attachment #3: 0002-status-stop.patch --]
[-- Type: text/x-patch, Size: 5501 bytes --]


2011-12-09  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/status-stop.exp: New.
	* gdb.trace/status-stop.c: New.
---
 gdb/testsuite/gdb.trace/status-stop.c   |   48 ++++++++++++
 gdb/testsuite/gdb.trace/status-stop.exp |  124 +++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/status-stop.c
 create mode 100644 gdb/testsuite/gdb.trace/status-stop.exp

diff --git a/gdb/testsuite/gdb.trace/status-stop.c b/gdb/testsuite/gdb.trace/status-stop.c
new file mode 100644
index 0000000..9ff69d6
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/status-stop.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static void
+func1 (void)
+{}
+
+int buf[1024];
+
+static void
+func2 (void)
+{}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  int i;
+
+  func1 ();
+
+  /* We call func2 as many times as possible to make sure that trace is
+     stopped due to trace buffer is full.  */
+  for (i = 0; i < 10000; i++)
+    {
+      func2 ();
+    }
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/status-stop.exp b/gdb/testsuite/gdb.trace/status-stop.exp
new file mode 100644
index 0000000..6c92b75
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/status-stop.exp
@@ -0,0 +1,124 @@
+# Copyright 2011 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp";
+
+set testfile "status-stop"
+set executable $testfile
+set srcfile ${testfile}.c
+set binfile $objdir/$subdir/$testfile
+set expfile $testfile.exp
+
+
+if [prepare_for_testing $expfile $executable $srcfile \
+        {debug nowarnings}] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+# Verify that the sequence of commands "tstart tstop tstart" works well.
+
+proc test_tstart_tstop_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstop_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func1" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+
+    gdb_test_no_output "tstop"
+
+    gdb_test_no_output "tstart"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Verify the sequence of commands "tstart tstart" works well.
+
+proc test_tstart_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func1" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "tstart" "" "tstart again" "A trace is running already.  Start a new run\\? \\(y or n\\) " "y"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Verify that trace stops clearly when trace buffer is full.
+
+proc test_buffer_full_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix buffer_full_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func2" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_trace_setactions "collect buf: define actions" \
+	"" \
+	"collect buf" "^$"
+
+    gdb_test_no_output "tstart"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+
+    gdb_test "tstatus" ".*buffer was full.*"
+    gdb_test_no_output "tstart"
+
+    set old_pf_prefix $pf_prefix
+}
+
+test_tstart_tstop_tstart
+
+test_tstart_tstart
+
+test_buffer_full_tstart
-- 
1.7.0.4

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Pedro Alves]

On Friday 09 December 2011 09:53:23, Yao Qi wrote:
> On 12/08/2011 11:40 AM, Hui Zhu wrote:
> > Hi guys,
> > 
> > I am sorry that I didn't talk clear about the issue of tstatus.
> > When we use tstatus, it can auto set the tracepoint back to stop when
> > it full or got error.  Then user can use tstart without tstop that set
> >  loc->inserted.  So we will still meet that issue if we just set
> > loc->inserted in tstop.
> > 
> > For examp:
> > This gdb is just set  loc->inserted in tstop but not tstatus.
> > (gdb) tstart
> > (gdb) tstop
> > (gdb) tstart
> > (gdb) tstop
> > (gdb) tstart
> > xxx
> > xxx
> > (gdb) tstatus
> > Trace stopped because the buffer was full.
> > Buffer contains 0 trace frames (of 49072 created total).
> > Trace buffer has 908408 bytes of 10414080 bytes free (91% full).
> > Trace will stop if GDB disconnects.
> > Not looking at any trace frame.
> > (gdb) tstart
> > ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> > Assertion `!loc->inserted' failed.
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Quit this debugging session? (y or n) n
> > 
> > ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> > Assertion `!loc->inserted' failed.
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Create a core file of GDB? (y or n) n
> > 
> 
> This is the 2nd version of my patch.  Compared with 1st version (sent
> four hours ago), this version covers more cases.  In this patch, a new
> observer `trace_stopped' is added.  In stop_tracing and
> target_get_trace_status, observer `trace_stopped' is notified, and then,
> `inserted' flag is cleared.  This looks more clear than version 1.

I think this is overkill.  It's just impossible to be absolutely
certain a trace run is currently ongoing.  If you do "tstatus;tstart",
the trace can still stop between the two commands.  Is there
anything that we benefit from clearing the inserted flag
on tstatus?  I think that if we clear them only, and
always, at "tstart" time, we're good.  We already clear all
the inserted flag of all breakpoints and tracepoints on target
open ("target remote ...", etc.), from within
breakpoint.c:breakpoint_init_inferior -- that's why you don't
see this assertion if you disconnect and reconnect, and find
the target was tracing.

So I think we should just clear the inserted flag of all
tracepoints in start_tracing, right after the target_trace_init
call.  Possibly even merge the new loop with the existing loop
that downloads tracepoints.

-- 
Pedro Alves

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On 12/09/2011 10:33 PM, Pedro Alves wrote:
> I think this is overkill.  It's just impossible to be absolutely
> certain a trace run is currently ongoing.  If you do "tstatus;tstart",
> the trace can still stop between the two commands.  Is there

This is a very good example.

> anything that we benefit from clearing the inserted flag
> on tstatus?  I think that if we clear them only, and

No, I don't think we benefit from clearing `inserted' flag on tstatus.
The intention of patch v2 is trying to make gdb's trace status as closed
to remote stub's as possible.  Your example above prove that it may make
no sense.

> always, at "tstart" time, we're good.  We already clear all
> the inserted flag of all breakpoints and tracepoints on target
> open ("target remote ...", etc.), from within
> breakpoint.c:breakpoint_init_inferior -- that's why you don't
> see this assertion if you disconnect and reconnect, and find
> the target was tracing.
> 
> So I think we should just clear the inserted flag of all
> tracepoints in start_tracing, right after the target_trace_init
> call.  Possibly even merge the new loop with the existing loop
> that downloads tracepoints.

This sounds right, and makes patch quite simpler :)

-- 
Yao (齐尧)

[-- Attachment #2: 0001-clear-inserted-flag.patch --]
[-- Type: text/x-patch, Size: 717 bytes --]

2011-12-09  Hui Zhu  <teawater@gmail.com>
	    Yao Qi  <yao@codesourcery.com>

	* tracepoint.c (start_tracing): Clear `inserted' flag.
---
 gdb/tracepoint.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index e00538c..da7298c 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1756,6 +1756,10 @@ start_tracing (char *notes)
       struct tracepoint *t = (struct tracepoint *) b;
       struct bp_location *loc;
 
+      /* Clear `inserted' flag.  */
+      for (loc = b->loc; loc; loc = loc->next)
+	loc->inserted = 0;
+
       if ((b->type == bp_fast_tracepoint
 	   ? !may_insert_fast_tracepoints
 	   : !may_insert_tracepoints))
-- 
1.7.0.4


[-- Attachment #3: 0002-status-stop.patch --]
[-- Type: text/x-patch, Size: 5529 bytes --]


2011-12-09  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/status-stop.exp: New.
	* gdb.trace/status-stop.c: New.
---
 gdb/testsuite/gdb.trace/status-stop.c   |   48 ++++++++++++
 gdb/testsuite/gdb.trace/status-stop.exp |  124 +++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/status-stop.c
 create mode 100644 gdb/testsuite/gdb.trace/status-stop.exp

diff --git a/gdb/testsuite/gdb.trace/status-stop.c b/gdb/testsuite/gdb.trace/status-stop.c
new file mode 100644
index 0000000..9ff69d6
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/status-stop.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static void
+func1 (void)
+{}
+
+int buf[1024];
+
+static void
+func2 (void)
+{}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  int i;
+
+  func1 ();
+
+  /* We call func2 as many times as possible to make sure that trace is
+     stopped due to trace buffer is full.  */
+  for (i = 0; i < 10000; i++)
+    {
+      func2 ();
+    }
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/status-stop.exp b/gdb/testsuite/gdb.trace/status-stop.exp
new file mode 100644
index 0000000..6c92b75
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/status-stop.exp
@@ -0,0 +1,124 @@
+# Copyright 2011 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp";
+
+set testfile "status-stop"
+set executable $testfile
+set srcfile ${testfile}.c
+set binfile $objdir/$subdir/$testfile
+set expfile $testfile.exp
+
+
+if [prepare_for_testing $expfile $executable $srcfile \
+        {debug nowarnings}] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+# Verify that the sequence of commands "tstart tstop tstart" works well.
+
+proc test_tstart_tstop_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstop_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func1" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+
+    gdb_test_no_output "tstop"
+
+    gdb_test_no_output "tstart"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Verify the sequence of commands "tstart tstart" works well.
+
+proc test_tstart_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix tstart_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func1" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "tstart" "" "tstart again" "A trace is running already.  Start a new run\\? \\(y or n\\) " "y"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Verify that trace stops clearly when trace buffer is full.
+
+proc test_buffer_full_tstart { } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix buffer_full_tstart:"
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+        fail "Can't run to main"
+        set pf_prefix $old_pf_prefix
+        return -1
+    }
+
+    gdb_test "trace func2" "Tracepoint \[0-9\] at $hex: file.*"
+    gdb_trace_setactions "collect buf: define actions" \
+	"" \
+	"collect buf" "^$"
+
+    gdb_test_no_output "tstart"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+
+    gdb_test "tstatus" ".*buffer was full.*"
+    gdb_test_no_output "tstart"
+
+    set old_pf_prefix $pf_prefix
+}
+
+test_tstart_tstop_tstart
+
+test_tstart_tstart
+
+test_buffer_full_tstart
\ No newline at end of file
-- 
1.7.0.4

Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Pedro Alves]

Okay, thanks.

On Friday 09 December 2011 15:53:43, Yao Qi wrote:
> +test_buffer_full_tstart
> \ No newline at end of file

But please add one.

-- 
Pedro Alves