Re: [patch/testsuite/gdbserver] unsuspend lwps after step over

[Yao Qi <yao at codesourcery dot com>]

On 02/29/2012 12:19 AM, Pedro Alves wrote:
> I don't object to the test, but I'm wondering if you found all that
> baggage really necessary, such as testing fast tracepoints, or setting a
> breakpoint on top of the tracepoint?  I think the test only really needs:
> 
> - more than one thread
> - all-stop
> - setting a tracepoint at $pc
> - tstart
> - stepi
> 
> All other combinations that don't end up as this would pass, AFAICS.
> 

These steps expose this bug, and they are quite simple!

(gdb) trace *$pc^M
Note: breakpoint 2 also set at pc 0x80484ac.^M
Tracepoint 3 at 0x80484ac: file
../../../src/gdb/testsuite/gdb.trace/trace-mt.c, line 36.^M
(gdb) tstart^M
(gdb) stepi^M
Remote connection closed^M
../../../src/gdb/gdbserver/linux-low.c:2821: A problem internal to
GDBserver has been detected.^M
stuck_in_jump_pad_callback: Assertion `lwp->suspended == 0' failed.

I create a new proc step_over_tracepoint in test case to automate these
steps.

>> > +      /* If we just finished a step-over, all other threads had been
>> > +	 paused in function start_step_over.  We have to unsuspend
>> > +	 them, say, decrement `suspended' flag, otherwise, we'll get
>> > +	 an assertion failure else where.  */
> Suggest tweaking this a bit:
> 
>     /* If we were going a step-over, all other threads but the stepping one
>        had been paused in start_step_over, with their suspend counts
>        incremented.  We don't want to do a full unstop/unpause, because we're
>        in all-stop mode (so we want threads stopped), but we still need to
>        unsuspend the other threads, to decrement their `suspended' count back.  */
> 

It looks good, copied it into my patch. :)

>> > +# Set breakpoint and tracepoint at the same address.
>> > +
>> > +proc break_trace_same_addr_1 { trace_type option } {
>> > +    global executable
>> > +    global pf_prefix
>> > +    global hex
>> > +
>> > +    set old_pf_prefix $pf_prefix
>> > +    set pf_prefix "$pf_prefix 1 $trace_type $option:"
> Can you please adjust the test to use the new with_test_prefix
> mechanism instead?  Sorry about that.  What that "1" in the prefix
> mean, btw?

"1" makes no sense anymore, because the code originally had
break_trace_same_addr_2, which was removed later.

In this version, test case is renamed to gdb.trace/trace-mt.exp, ideally
for tracepoint on multi-threaded env, and with_test_prefix is used.

-- 
Yao (éå)

gdb/gdbserver:

2012-02-29  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	* linux-low.c (linux_wait_1): Call unsuspend_all_lwps when
	`step_over_finished' is true.

gdb/testsuite:

2012-02-29  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	* gdb.trace/trace-mt.c: New
	* gdb.trace/trace-mt.exp: New.
---
 gdb/gdbserver/linux-low.c            |    9 ++
 gdb/testsuite/gdb.trace/trace-mt.c   |   63 ++++++++++++++++
 gdb/testsuite/gdb.trace/trace-mt.exp |  136 ++++++++++++++++++++++++++++++++++
 3 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/trace-mt.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-mt.exp

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8a83231..5df6ad7 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2482,6 +2482,15 @@ Check if we're already there.\n",
 	 why.  */
       find_inferior (&all_lwps, cancel_breakpoints_callback, event_child);
 
+      /* If we were going a step-over, all other threads but the stepping one
+	 had been paused in start_step_over, with their suspend counts
+	 incremented.  We don't want to do a full unstop/unpause, because we're
+	 in all-stop mode (so we want threads stopped), but we still need to
+	 unsuspend the other threads, to decrement their `suspended' count
+	 back.  */
+      if (step_over_finished)
+	unsuspend_all_lwps (event_child);
+
       /* Stabilize threads (move out of jump pads).  */
       stabilize_threads ();
     }
diff --git a/gdb/testsuite/gdb.trace/trace-mt.c b/gdb/testsuite/gdb.trace/trace-mt.c
new file mode 100644
index 0000000..90a5b54
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-mt.c
@@ -0,0 +1,63 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 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/>.  */
+
+#include <pthread.h>
+
+#ifdef SYMBOL_PREFIX
+#define SYMBOL(str)     SYMBOL_PREFIX #str
+#else
+#define SYMBOL(str)     #str
+#endif
+/* Called from asm.  */
+static void __attribute__((used))
+func (void)
+{}
+
+static void *
+thread_function(void *arg)
+{
+  /* `set_point1' is the label at which to set a fast tracepoint.  The
+     insn at the label must be large enough to fit a fast tracepoint
+     jump.  */
+  asm ("    .global " SYMBOL(set_point1) "\n"
+       SYMBOL(set_point1) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+       "    call " SYMBOL(func) "\n"
+#endif
+       );
+}
+
+static void
+end (void)
+{}
+
+int
+main (int argc, char *argv[], char *envp[])
+{
+  pthread_t threads[2];
+  int i;
+
+  for (i = 0; i < 2; i++)
+    pthread_create (&threads[i], NULL, thread_function, NULL);
+
+  for (i = 0; i < 2; i++)
+    pthread_join (threads[i], NULL);
+
+  end ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/trace-mt.exp b/gdb/testsuite/gdb.trace/trace-mt.exp
new file mode 100644
index 0000000..b840590
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-mt.exp
@@ -0,0 +1,136 @@
+# Copyright 2012 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 "trace-mt"
+set executable $testfile
+set srcfile $testfile.c
+set binfile $objdir/$subdir/$testfile
+set expfile $testfile.exp
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [gdb_target_symbol_prefix_flags]
+
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile \
+	  executable [list debug $additional_flags] ] != "" } {
+    untested itset.exp
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+proc step_over_tracepoint { trace_type } \
+{with_test_prefix "step over $trace_type" \
+{
+    global executable
+    global hex
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    # Make sure inferior is running in all-stop mode.
+    gdb_test_no_output "set non-stop 0"
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    gdb_test "break set_point1" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+.*Breakpoint.*" "continue to set_point1"
+
+    gdb_test "${trace_type} *\$pc" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+    gdb_test_no_output "tstart"
+
+    gdb_test "stepi" ".*"
+    gdb_test_no_output "tstop"
+}}
+
+# Set breakpoint and tracepoint at the same address.
+
+proc break_trace_same_addr { trace_type option } \
+{with_test_prefix "$trace_type $option" \
+{
+    global executable
+    global hex
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    gdb_test_no_output "set breakpoint always-inserted ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point1" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point1" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+.*Breakpoint.*" "continue to set_point1 1"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+.*Breakpoint.*" "continue to set_point1 2"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+.*Breakpoint.*" "continue to end"
+    gdb_test_no_output "tstop"
+
+    gdb_test "tfind" "Found trace frame 0, tracepoint .*" "tfind frame 0"
+    gdb_test "tfind" "Found trace frame 1, tracepoint .*" "tfind frame 1"
+
+    gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+}}
+
+foreach break_always_inserted { "on" "off" } {
+    break_trace_same_addr "trace" ${break_always_inserted}
+}
+
+step_over_tracepoint "trace"
+
+set libipa $objdir/../gdbserver/libinproctrace.so
+gdb_load_shlibs $libipa
+
+# Compile test case again with IPA.
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile \
+	  executable [list debug $additional_flags shlib=$libipa] ] != "" } {
+    untested "failed to compile ftrace tests"
+    return -1
+}
+clean_restart ${executable}
+
+if ![runto_main] {
+    fail "Can't run to main for ftrace tests"
+    return 0
+}
+
+gdb_reinitialize_dir $srcdir/$subdir
+if { [gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "IPA loaded"] != 0 } {
+    untested "Could not find IPA lib loaded"
+} else {
+    foreach break_always_inserted { "on" "off" } {
+	break_trace_same_addr "ftrace" ${break_always_inserted}
+    }
+
+    step_over_tracepoint "ftrace"
+}
-- 
1.7.0.4