[patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Yao Qi]

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

Hi,
I find a program will receive segv fault when I set a regular tracepoint
and a fast tracepoint at the same address, start tracing and resume program.

gdbserver has taken care of this situation in many places of the code,
when uninserting breakpoint or fast tracepoint, write_inferior_memory is
called to take care of layering breakpoints on top of fast tracepoints.
 However, it is not right to me.  Here is an example to illustrate this
problem.

Supposing I set a regular tracepoint and a fast tracepoint on 0x080484fc,

    0x080484fc <+3>:     e8 f3 ff ff ff  call   0x80484f4 <func>

During insertion, trap insn (for regular tracepoint) and jmp insn (for
fast tracepoint) are inserted, and gdbserver takes care of them to make
sure trap insn is *always* inserted on top of jmp insn.  In my example,
gdbserver will first insert a jmp insn (0xe9XXXXXXXX), save original
insn in jump shadow (0xe8f3ffffff), and then insert trap insn on top of
jump insn.  So, gdbserver writes 0xcc to 0x080484fc and save the first
byte (0xe9) in bp->old_data.  Memory content on 0x080484fc is
0xccXXXXXXXX.  Note that bp->old_data saves the first byte of jmp insn
rather than original insn.

When program hits trap insn, gdbserver will step-over trap insn.
gdbserver will uninsert trap and uninsert fast tracepoint jumps (see
linux-low.c:start_step_over).  In uninsert_raw_breakpoint, bp->old_data
will be written back to bp->pc, in my example, 0xe9 is written back to
0x080484fc.  check_mem_write will be called, and check the overlap of
memory to be write and breakpoint/tracepoint insn memory range.  In my
example, there is an overlap with fast tracepoint jump insn and trap
tracepoint insn.  So, 1) 0xe9 is written to the first byte of jump
shadow and shadow becomes 0xe9f3ffffff, which is clobbered, 2) insn on
0x080484fc becomes 0xe9XXXXXXXX (right jump insn).  Later, when
uninserting fast tracepoint jumps, jump shadow will be written back, so
0xe9f3ffffff is written back, it is a wrong insn.

Generally, when writing to memory, we need check_mem_write to take care
of layering breakpoint and tracepoint, however, when writing memory
during breakpoint/tracepoint uninsertion, we don't have to.  Because 1)
trap breakpoint is *always* inserted on top of fast tracepoint jump, 2)
trap insn is not longer than jmp insn.  During uninsertion, we don't
have to worry about layering, and uninsert trap breakpoint and fast
tracepoint jump one by one with simple *the_target->write_memory.

Test case attached is about testing setting breakpoint and different
kinds of tracepoint at the same address.  In current trunk, there are 3
fails on x86_64-linux,

  FAIL: gdb.trace/trace-break.exp: 1 ftrace on: continue to end
  FAIL: gdb.trace/trace-break.exp: 2 trace ftrace on: continue to end
  FAIL: gdb.trace/trace-break.exp: 2 trace ftrace off: continue to end

I also run trace-break.exp on commit
de642393026eee797efdd1355c1913f8054dab64, which is the first revision
for fast tracepoint.

commit de642393026eee797efdd1355c1913f8054dab64
Author: Pedro Alves <pedro@codesourcery.com>
Date:   Tue Jun 1 13:20:49 2010 +0000

    gdb/gdbserver/
    2010-06-01  Pedro Alves  <pedro@codesourcery.com>
            Stan Shebs  <stan@codesourcery.com>

[...]
        * mem-break.c (set_raw_breakpoint_at): Use read_inferior_memory.
        (struct fast_tracepoint_jump): New.
        (fast_tracepoint_jump_insn): New.
        (fast_tracepoint_jump_shadow): New.
        (find_fast_tracepoint_jump_at): New.
        (fast_tracepoint_jump_here): New.
        (delete_fast_tracepoint_jump): New.
        (set_fast_tracepoint_jump): New.
        (uninsert_fast_tracepoint_jumps_at): New.
        (reinsert_fast_tracepoint_jumps_at): New.
        (set_breakpoint_at): Use write_inferior_memory.
        (uninsert_raw_breakpoint): Use write_inferior_memory.
        (check_mem_read): Mask out fast tracepoint jumps.
        (check_mem_write): Mask out fast tracepoint jumps.
[...]

  FAIL: gdb.trace/trace-break.exp: 1 ftrace on: continue to end
  FAIL: gdb.trace/trace-break.exp: 2 trace ftrace on: continue to end
  FAIL: gdb.trace/trace-break.exp: 3 ftrace on: continue to end
  FAIL: gdb.trace/trace-break.exp: 2 trace ftrace off: continue to end
  FAIL: gdb.trace/trace-break.exp: 3 ftrace off: continue to end

Test case fails as well, so these fails are not regression.

Regression tested on x86_64-linux, no regression and fails in
gdb.trace/trace-break.exp are fixed.  OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: 0003-new-testcase-trace-break.patch --]
[-- Type: text/x-patch, Size: 9815 bytes --]

2011-10-26  Yao Qi  <yao@codesourcery.com>

        * gdb.trace/trace-break.c: New.
        * gdb.trace/trace-break.exp: New.
---
 gdb/testsuite/gdb.trace/trace-break.c   |   46 ++++++
 gdb/testsuite/gdb.trace/trace-break.exp |  233 +++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.exp

diff --git a/gdb/testsuite/gdb.trace/trace-break.c b/gdb/testsuite/gdb.trace/trace-break.c
new file mode 100644
index 0000000..0eda029
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-break.c
@@ -0,0 +1,46 @@
+/* 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
+func ()
+{}
+
+static void
+marker ()
+{
+  int a = 0;
+  int b = a;
+
+  asm (".global set_point");
+  asm (" set_point:"); /* The lable that we'll set tracepoint on.  */
+#if (defined __x86_64__ || defined __i386__)
+  /* It is a five-byte insn, so that fast trace point can be set on it.  */
+  asm ("call func");
+#endif
+}
+
+static void
+end ()
+{}
+
+int main()
+{
+  marker ();
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
new file mode 100644
index 0000000..6f8d9aa
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -0,0 +1,233 @@
+# 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";
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile "trace-break"
+
+set srcfile $testfile.c
+set binfile $objdir/$subdir/$testfile
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable [list debug nowarnings] ] != "" } {
+    untested trace-break.exp
+    return -1
+}
+
+gdb_start
+gdb_load ${binfile}
+
+runto_main
+gdb_reinitialize_dir $srcdir/$subdir
+
+# We generously give ourselves one "pass" if we successfully 
+# detect that this test cannot be run on this target!
+if { ![gdb_target_supports_trace] } then {
+    pass "Current target does not support trace"
+    return 1;
+}
+
+# Set breakpoint and tracepoint at the same address.
+
+proc break_trace_same_addr_1 { trace_type option } {
+    global srcdir
+    global subdir
+    global binfile
+    global pf_prefix
+    global srcfile
+
+    # Start with a fresh gdb.
+    gdb_exit
+    gdb_start
+    gdb_load ${binfile}
+    runto_main
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 1 $trace_type $option:"
+
+    gdb_test_no_output "set breakpoint always-inserted  ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix 
+}
+
+# Set multiple tracepoints at the same address.
+
+proc break_trace_same_addr_2 { trace_type1 trace_type2 option } {
+    global srcdir
+    global subdir
+    global binfile
+    global pf_prefix
+    global srcfile
+
+    # Start with a fresh gdb.
+    gdb_exit
+    gdb_start
+    gdb_load ${binfile}
+    runto_main
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 2 $trace_type1 $trace_type2 $option:"
+
+    gdb_test_no_output "set breakpoint always-inserted  ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "${trace_type1} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "${trace_type2} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test_no_output "tstart"
+    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\\..*"
+
+    set pf_prefix $old_pf_prefix 
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete breakpoint, and verify
+# that tracepoint still works.
+
+proc break_trace_same_addr_3 { trace_type option } {
+    global srcdir
+    global subdir
+    global binfile
+    global pf_prefix
+    global srcfile
+
+    # Start with a fresh gdb.
+    gdb_exit
+    gdb_start
+    gdb_load ${binfile}
+    runto_main
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 3 $trace_type $option:"
+
+    gdb_test_no_output "set breakpoint always-inserted  ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    gdb_test "delete break 4"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix 
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete tracepoint, and verify
+# that breakpoint still works.
+
+proc break_trace_same_addr_4 { trace_type option } {
+    global srcdir
+    global subdir
+    global binfile
+    global pf_prefix
+    global srcfile
+
+    # Start with a fresh gdb.
+    gdb_exit
+    gdb_start
+    gdb_load ${binfile}
+    runto_main
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 4 $trace_type $option:"
+
+    gdb_test_no_output "set breakpoint always-inserted  ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    # Detele tracepoint set on set_point.
+    gdb_test "delete trace 5"
+
+    gdb_test "tstart" "No tracepoints defined, not starting trace.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+    gdb_test "tstop" "Trace is not running.*"
+
+    gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix 
+}
+
+foreach break_always_inserted { "on" "off" } {
+    break_trace_same_addr_1 "trace" ${break_always_inserted}
+    break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
+    break_trace_same_addr_3 "trace" ${break_always_inserted}
+    break_trace_same_addr_4 "trace" ${break_always_inserted}
+}
+
+gdb_exit
+
+set libipa $objdir/../gdbserver/libinproctrace.so
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable [list debug nowarnings shlib=$libipa] ] != "" } {
+    untested trace-break.exp
+    return -1
+}
+
+gdb_start
+gdb_load ${binfile}
+
+runto_main
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "check libinproctrace.so"
+
+foreach break_always_inserted { "on" "off" } {
+    break_trace_same_addr_1 "ftrace" ${break_always_inserted}
+    break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted}
+    break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted}
+    break_trace_same_addr_3 "ftrace" ${break_always_inserted}
+    break_trace_same_addr_4 "ftrace" ${break_always_inserted}
+}
-- 
1.7.0.4


[-- Attachment #3: 0002-don-t-call-write_inferior_memory.patch --]
[-- Type: text/x-patch, Size: 3160 bytes --]


	gdb/gdbserver
	* mem-break.c (uninsert_fast_tracepoint_jumps_at): Don't use
	write_inferior_memory.
	(delete_raw_breakpoint, uninsert_raw_breakpoint): Likewise.
---
 gdb/gdbserver/mem-break.c |   42 +++++++-----------------------------------
 1 files changed, 7 insertions(+), 35 deletions(-)

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 1a140f4..6ec2078 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -411,19 +411,9 @@ uninsert_fast_tracepoint_jumps_at (CORE_ADDR pc)
   if (jp->inserted)
     {
       jp->inserted = 0;
-
-      /* Since there can be trap breakpoints inserted in the same
-	 address range, we use use `write_inferior_memory', which
-	 takes care of layering breakpoints on top of fast
-	 tracepoints, and on top of the buffer we pass it.  This works
-	 because we've already marked the fast tracepoint fast
-	 tracepoint jump uninserted above.  Also note that we need to
-	 pass the current shadow contents, because
-	 write_inferior_memory updates any shadow memory with what we
-	 pass here, and we want that to be a nop.  */
-      err = write_inferior_memory (jp->pc,
-				   fast_tracepoint_jump_shadow (jp),
-				   jp->length);
+      err = (*the_target->write_memory) (jp->pc,
+					 fast_tracepoint_jump_shadow (jp),
+					 jp->length);
       if (err != 0)
 	{
 	  jp->inserted = 1;
@@ -526,18 +516,8 @@ delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
 	      struct raw_breakpoint *prev_bp_link = *bp_link;
 
 	      *bp_link = bp->next;
-
-	      /* Since there can be trap breakpoints inserted in the
-		 same address range, we use `write_inferior_memory',
-		 which takes care of layering breakpoints on top of
-		 fast tracepoints, and on top of the buffer we pass
-		 it.  This works because we've already unlinked the
-		 fast tracepoint jump above.  Also note that we need
-		 to pass the current shadow contents, because
-		 write_inferior_memory updates any shadow memory with
-		 what we pass here, and we want that to be a nop.  */
-	      ret = write_inferior_memory (bp->pc, bp->old_data,
-					   breakpoint_len);
+	      ret = (*the_target->write_memory) (bp->pc, bp->old_data,
+						 breakpoint_len);
 	      if (ret != 0)
 		{
 		  /* Something went wrong, relink the breakpoint.  */
@@ -747,16 +727,8 @@ uninsert_raw_breakpoint (struct raw_breakpoint *bp)
       int err;
 
       bp->inserted = 0;
-      /* Since there can be fast tracepoint jumps inserted in the same
-	 address range, we use `write_inferior_memory', which takes
-	 care of layering breakpoints on top of fast tracepoints, and
-	 on top of the buffer we pass it.  This works because we've
-	 already unlinked the fast tracepoint jump above.  Also note
-	 that we need to pass the current shadow contents, because
-	 write_inferior_memory updates any shadow memory with what we
-	 pass here, and we want that to be a nop.  */
-      err = write_inferior_memory (bp->pc, bp->old_data,
-				   breakpoint_len);
+      err = (*the_target->write_memory) (bp->pc, bp->old_data, breakpoint_len);
+
       if (err != 0)
 	{
 	  bp->inserted = 1;
-- 
1.7.0.4

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Pedro Alves]

[-- Attachment #1: Type: Text/Plain, Size: 13521 bytes --]

On Thursday 27 October 2011 03:56:17, Yao Qi wrote:

> I find a program will receive segv fault when I set a regular tracepoint
> and a fast tracepoint at the same address, start tracing and resume program.
> 
> gdbserver has taken care of this situation in many places of the code,
> when uninserting breakpoint or fast tracepoint, write_inferior_memory is
> called to take care of layering breakpoints on top of fast tracepoints.
>  However, it is not right to me.  Here is an example to illustrate this
> problem.
> 
> Supposing I set a regular tracepoint and a fast tracepoint on 0x080484fc,
> 
>     0x080484fc <+3>:     e8 f3 ff ff ff  call   0x80484f4 <func>
> 
> During insertion, trap insn (for regular tracepoint) and jmp insn (for
> fast tracepoint) are inserted, and gdbserver takes care of them to make
> sure trap insn is *always* inserted on top of jmp insn.  In my example,
> gdbserver will first insert a jmp insn (0xe9XXXXXXXX), save original
> insn in jump shadow (0xe8f3ffffff), and then insert trap insn on top of
> jump insn.  So, gdbserver writes 0xcc to 0x080484fc and save the first
> byte (0xe9) in bp->old_data.  Memory content on 0x080484fc is
> 0xccXXXXXXXX.  

> Note that bp->old_data saves the first byte of jmp insn
> rather than original insn.

_This_ is the bug.  I must have introduced it as a last minute change
before pushing fast tracepoints upstream, because I'm sure this used
to work, proof being all the comments in the code about it.  :-/

The idea is that the shadow of all breakpoints and fast tracepoint
jumps always has the contents of the memory as if no breakpoint
of fast tracepoint was inserted.

The problem is that check_mem_write does:

 - loop over fast tracepoint jumps, updating their shadow buffers,
   _and_ clobbers the input buffer.
 - loop over raw breakpoints, updating their shadow buffers, but
   at this point, the input buffer is already overwritten with
   fast tracepoint jumps...

The fix is to split updating the shadow buffers from stapling fast
tracepoint jumps and breakpoints on top of the input buffer.

While at it, I did several changes to the new tests.  Thanks a lot
for writing them BTW.  They're awesome.  I wish I had done so before...
I'll point out the main issues below.

> diff --git a/gdb/testsuite/gdb.trace/trace-break.c b/gdb/testsuite/gdb.trace/trace-break.c
> new file mode 100644
> index 0000000..0eda029
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/trace-break.c
> @@ -0,0 +1,46 @@
> +/* 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
> +func ()
> +{}

Only called from asm, should have __attribute__((used)), as some
compilers can get rid of it even without optimizations on.

> +
> +static void
> +marker ()
> +{
> +  int a = 0;
> +  int b = a;
> +
> +  asm (".global set_point");
> +  asm (" set_point:"); /* The lable that we'll set tracepoint on.  */

Some targets (cygwin/mingw, for example) need an underscore on asm
symbols to make them accessible to C.

> +#if (defined __x86_64__ || defined __i386__)
> +  /* It is a five-byte insn, so that fast trace point can be set on it.  */
> +  asm ("call func");
> +#endif
> +}
> +
> +static void
> +end ()
> +{}
> +
> +int main()
> +{
> +  marker ();
> +  end ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
> new file mode 100644
> index 0000000..6f8d9aa
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/trace-break.exp
> @@ -0,0 +1,233 @@
> +# 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";
> +
> +if $tracelevel then {
> +    strace $tracelevel
> +}
> +
> +set testfile "trace-break"
> +
> +set srcfile $testfile.c
> +set binfile $objdir/$subdir/$testfile
> +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
> +         executable [list debug nowarnings] ] != "" } {
> +    untested trace-break.exp
> +    return -1
> +}
> +
> +gdb_start
> +gdb_load ${binfile}
> +
> +runto_main
> +gdb_reinitialize_dir $srcdir/$subdir

We can use prepare_for_testing for most of this.  Failure on
runto_main should be handled.

> +
> +# We generously give ourselves one "pass" if we successfully 
> +# detect that this test cannot be run on this target!
> +if { ![gdb_target_supports_trace] } then {
> +    pass "Current target does not support trace"
> +    return 1;
> +}

We should go through all the gdb.trace tests and get rid of
this funny pass.  Should be `unsupported' instead.

> +
> +# Set breakpoint and tracepoint at the same address.
> +
> +proc break_trace_same_addr_1 { trace_type option } {
> +    global srcdir
> +    global subdir
> +    global binfile
> +    global pf_prefix
> +    global srcfile
> +
> +    # Start with a fresh gdb.
> +    gdb_exit
> +    gdb_start
> +    gdb_load ${binfile}
> +    runto_main
> +    gdb_reinitialize_dir $srcdir/$subdir

clean_restart does all this for us.  runto_main failures
be handled.

> +
> +    set old_pf_prefix $pf_prefix
> +    set pf_prefix "$pf_prefix 1 $trace_type $option:"

The prefix should be changed _before_ compiling/running to main,
so if the latter fails, the FAIL's will have the correct prefix.

> +
> +    gdb_test_no_output "set breakpoint always-inserted  ${option}"

Spurious extra space.

> +
> +    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"

We can use $hex.

> +
> +    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +
> +    gdb_test_no_output "tstart"
> +
> +    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
> +
> +    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" "Target failed to find requested trace frame\\..*"
> +
> +    set pf_prefix $old_pf_prefix 
> +}
> +
> +# Set multiple tracepoints at the same address.
> +
> +proc break_trace_same_addr_2 { trace_type1 trace_type2 option } {
> +    global srcdir
> +    global subdir
> +    global binfile
> +    global pf_prefix
> +    global srcfile
> +
> +    # Start with a fresh gdb.
> +    gdb_exit
> +    gdb_start
> +    gdb_load ${binfile}
> +    runto_main
> +    gdb_reinitialize_dir $srcdir/$subdir
> +
> +    set old_pf_prefix $pf_prefix
> +    set pf_prefix "$pf_prefix 2 $trace_type1 $trace_type2 $option:"
> +
> +    gdb_test_no_output "set breakpoint always-inserted  ${option}"
> +
> +    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "${trace_type1} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "${trace_type2} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"

These last two will result in the same message in gdb.sum when trace_type1 and
trace_type2 is the same.  Always do something like:

  $ cat testsuite/gdb.sum | grep PASS | sort | uniq -c | sort -n

to make sure all tests have unique messages.

> +
> +    gdb_test_no_output "tstart"
> +    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\\..*"
> +
> +    set pf_prefix $old_pf_prefix 
> +}
> +
> +# Set breakpoint and tracepoint at the same address.  Delete breakpoint, and verify
> +# that tracepoint still works.
> +
> +proc break_trace_same_addr_3 { trace_type option } {
> +    global srcdir
> +    global subdir
> +    global binfile
> +    global pf_prefix
> +    global srcfile
> +
> +    # Start with a fresh gdb.
> +    gdb_exit
> +    gdb_start
> +    gdb_load ${binfile}
> +    runto_main
> +    gdb_reinitialize_dir $srcdir/$subdir
> +
> +    set old_pf_prefix $pf_prefix
> +    set pf_prefix "$pf_prefix 3 $trace_type $option:"
> +
> +    gdb_test_no_output "set breakpoint always-inserted  ${option}"
> +    gdb_test "break marker" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +
> +    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +
> +    gdb_test_no_output "tstart"
> +
> +    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
> +    gdb_test "delete break 4"
> +
> +    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" "Target failed to find requested trace frame\\..*"
> +
> +    set pf_prefix $old_pf_prefix 
> +}
> +
> +# Set breakpoint and tracepoint at the same address.  Delete tracepoint, and verify
> +# that breakpoint still works.
> +
> +proc break_trace_same_addr_4 { trace_type option } {
> +    global srcdir
> +    global subdir
> +    global binfile
> +    global pf_prefix
> +    global srcfile
> +
> +    # Start with a fresh gdb.
> +    gdb_exit
> +    gdb_start
> +    gdb_load ${binfile}
> +    runto_main
> +    gdb_reinitialize_dir $srcdir/$subdir
> +
> +    set old_pf_prefix $pf_prefix
> +    set pf_prefix "$pf_prefix 4 $trace_type $option:"
> +
> +    gdb_test_no_output "set breakpoint always-inserted  ${option}"
> +    gdb_test "break marker" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "break end" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +
> +    gdb_test "break set_point" "Breakpoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at 0x\[0-9a-fA-F\]+: file.*"
> +
> +    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
> +    # Detele tracepoint set on set_point.
> +    gdb_test "delete trace 5"
> +
> +    gdb_test "tstart" "No tracepoints defined, not starting trace.*"
> +
> +    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
> +    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
> +    gdb_test "tstop" "Trace is not running.*"
> +
> +    gdb_test "tfind" "Target failed to find requested trace frame\\..*"
> +
> +    set pf_prefix $old_pf_prefix 
> +}
> +
> +foreach break_always_inserted { "on" "off" } {
> +    break_trace_same_addr_1 "trace" ${break_always_inserted}
> +    break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
> +    break_trace_same_addr_3 "trace" ${break_always_inserted}
> +    break_trace_same_addr_4 "trace" ${break_always_inserted}
> +}
> +
> +gdb_exit
> +
> +set libipa $objdir/../gdbserver/libinproctrace.so

Needs uploading to the remote host.

> +
> +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
> +         executable [list debug nowarnings shlib=$libipa] ] != "" } {
> +    untested trace-break.exp
> +    return -1
> +}

We can't use prepare_for_testing/build_executable for this one
(see comment in patch), but we can use clean_restart.

> +
> +gdb_start
> +gdb_load ${binfile}
> +
> +runto_main
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "check libinproctrace.so"

If this fails, we shouldn't proceed and run the test of the tests.  They'll
all fail/timeout.

> +
> +foreach break_always_inserted { "on" "off" } {
> +    break_trace_same_addr_1 "ftrace" ${break_always_inserted}
> +    break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted}
> +    break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted}
> +    break_trace_same_addr_3 "ftrace" ${break_always_inserted}
> +    break_trace_same_addr_4 "ftrace" ${break_always_inserted}
> +}

Several typos and spurious whitespace were fixed as well.

Below's my version of the patches.  WDYT?

-- 
Pedro Alves

[-- Attachment #2: trace_break_test.diff --]
[-- Type: text/x-patch, Size: 10560 bytes --]

2011-10-27  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
        * gdb.trace/trace-break.c: New.
        * gdb.trace/trace-break.exp: New.

---
 gdb/testsuite/gdb.trace/trace-break.c   |   58 +++++++
 gdb/testsuite/gdb.trace/trace-break.exp |  240 ++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.exp

Index: src/gdb/testsuite/gdb.trace/trace-break.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.trace/trace-break.c	2011-10-27 16:32:41.905141841 +0100
@@ -0,0 +1,58 @@
+/* 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/>.  */
+
+#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
+marker (void)
+{
+  /* Some code to make sure `b marker' and `b set_point' set
+     breakpoints at different addresses.  */
+  int a = 0;
+  int b = a;
+
+  /* `set_point' is the label where we'll set multiple tracepoints and
+     breakpoints at.  The insn at the label must the large enough to
+     fit a fast tracepoint jump.  */
+  asm ("    .global " SYMBOL(set_point) "\n"
+       SYMBOL(set_point) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+       "    call " SYMBOL(func) "\n"
+#endif
+       );
+}
+
+static void
+end (void)
+{}
+
+int
+main ()
+{
+  marker ();
+  end ();
+  return 0;
+}
Index: src/gdb/testsuite/gdb.trace/trace-break.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.trace/trace-break.exp	2011-10-27 16:39:03.545141899 +0100
@@ -0,0 +1,240 @@
+# 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 "trace-break"
+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 [prepare_for_testing $expfile $executable $srcfile \
+	[list debug $additional_flags]] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+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;
+}
+
+# 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:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set multiple tracepoints at the same address.
+
+proc break_trace_same_addr_2 { trace_type1 trace_type2 option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 2 $trace_type1 $trace_type2 $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "${trace_type1} set_point" \
+	"\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+	"${trace_type1} set_point (1)"
+
+    gdb_test "${trace_type2} set_point" \
+	"\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+	"${trace_type2} set_point (2)"
+
+    gdb_test_no_output "tstart"
+    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\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete breakpoint, and verify
+# that tracepoint still works.
+
+proc break_trace_same_addr_3 { trace_type option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 3 $trace_type $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    gdb_test "delete break 4"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete tracepoint, and verify
+# that breakpoint still works.
+
+proc break_trace_same_addr_4 { trace_type option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 4 $trace_type $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    # Delete tracepoint set on set_point.
+    gdb_test "delete trace 5"
+
+    gdb_test "tstart" "No tracepoints defined, not starting trace.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+    gdb_test "tstop" "Trace is not running.*"
+
+    gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+foreach break_always_inserted { "on" "off" } {
+    break_trace_same_addr_1 "trace" ${break_always_inserted}
+    break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
+    break_trace_same_addr_3 "trace" ${break_always_inserted}
+    break_trace_same_addr_4 "trace" ${break_always_inserted}
+}
+
+set libipa $objdir/../gdbserver/libinproctrace.so
+gdb_load_shlibs $libipa
+
+# Can't use prepare_for_testing, because that splits compiling into
+# building objects and then linking, and we'd fail with "linker input
+# file unused because linking not done" when building the object.
+
+if { [gdb_compile "$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_1 "ftrace" ${break_always_inserted}
+	break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted}
+	break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted}
+	break_trace_same_addr_3 "ftrace" ${break_always_inserted}
+	break_trace_same_addr_4 "ftrace" ${break_always_inserted}
+    }
+}

[-- Attachment #3: trace_break_fix.diff --]
[-- Type: text/x-patch, Size: 3511 bytes --]

2011-10-27  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* mem-break.c (check_mem_write): Split updating shadows and
	filling the write buffer into two passes.  Don't clobber the
	breakpoints' shadows with fast tracepoint jumps.

---
 gdb/gdbserver/mem-break.c |   68 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 7 deletions(-)

Index: src/gdb/gdbserver/mem-break.c
===================================================================
--- src.orig/gdb/gdbserver/mem-break.c	2011-10-27 15:40:38.195141376 +0100
+++ src/gdb/gdbserver/mem-break.c	2011-10-27 16:31:25.865141831 +0100
@@ -1032,14 +1032,15 @@ void
 check_mem_write (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
 {
   struct process_info *proc = current_process ();
-  struct raw_breakpoint *bp = proc->raw_breakpoints;
-  struct fast_tracepoint_jump *jp = proc->fast_tracepoint_jumps;
-  CORE_ADDR mem_end = mem_addr + mem_len;
+  struct raw_breakpoint *bp;
+  struct fast_tracepoint_jump *jp;
+  const CORE_ADDR mem_end = mem_addr + mem_len;
   int disabled_one = 0;
 
-  /* First fast tracepoint jumps, then breakpoint traps on top.  */
+  /* First update the shadows of breakpoints and fast tracepoint jumps
+     with the contents of BUF.  */
 
-  for (; jp != NULL; jp = jp->next)
+  for (jp = proc->fast_tracepoint_jumps; jp != NULL; jp = jp->next)
     {
       CORE_ADDR jp_end = jp->pc + jp->length;
       CORE_ADDR start, end;
@@ -1064,12 +1065,66 @@ check_mem_write (CORE_ADDR mem_addr, uns
 
       memcpy (fast_tracepoint_jump_shadow (jp) + copy_offset,
 	      buf + buf_offset, copy_len);
+    }
+
+  for (bp = proc->raw_breakpoints; bp != NULL; bp = bp->next)
+    {
+      CORE_ADDR bp_end = bp->pc + breakpoint_len;
+      CORE_ADDR start, end;
+      int copy_offset, copy_len, buf_offset;
+
+      if (mem_addr >= bp_end)
+	continue;
+      if (bp->pc >= mem_end)
+	continue;
+
+      start = bp->pc;
+      if (mem_addr > start)
+	start = mem_addr;
+
+      end = bp_end;
+      if (end > mem_end)
+	end = mem_end;
+
+      copy_len = end - start;
+      copy_offset = start - bp->pc;
+      buf_offset = start - mem_addr;
+
+      memcpy (bp->old_data + copy_offset, buf + buf_offset, copy_len);
+    }
+
+  /* Then fill in BUF with what we really want to write to memory.
+     First fast tracepoint jumps, then breakpoint traps on top.  */
+
+  for (jp = proc->fast_tracepoint_jumps; jp != NULL; jp = jp->next)
+    {
+      CORE_ADDR jp_end = jp->pc + jp->length;
+      CORE_ADDR start, end;
+      int copy_offset, copy_len, buf_offset;
+
+      if (mem_addr >= jp_end)
+	continue;
+      if (jp->pc >= mem_end)
+	continue;
+
+      start = jp->pc;
+      if (mem_addr > start)
+	start = mem_addr;
+
+      end = jp_end;
+      if (end > mem_end)
+	end = mem_end;
+
+      copy_len = end - start;
+      copy_offset = start - jp->pc;
+      buf_offset = start - mem_addr;
+
       if (jp->inserted)
 	memcpy (buf + buf_offset,
 		fast_tracepoint_jump_insn (jp) + copy_offset, copy_len);
     }
 
-  for (; bp != NULL; bp = bp->next)
+  for (bp = proc->raw_breakpoints; bp != NULL; bp = bp->next)
     {
       CORE_ADDR bp_end = bp->pc + breakpoint_len;
       CORE_ADDR start, end;
@@ -1092,7 +1147,6 @@ check_mem_write (CORE_ADDR mem_addr, uns
       copy_offset = start - bp->pc;
       buf_offset = start - mem_addr;
 
-      memcpy (bp->old_data + copy_offset, buf + buf_offset, copy_len);
       if (bp->inserted)
 	{
 	  if (validate_inserted_breakpoint (bp))

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Stan Shebs]

On 10/26/11 7:56 PM, Yao Qi wrote:
> Hi,
> I find a program will receive segv fault when I set a regular tracepoint
> and a fast tracepoint at the same address, start tracing and resume program.
>
> gdbserver has taken care of this situation in many places of the code,
> when uninserting breakpoint or fast tracepoint, write_inferior_memory is
> called to take care of layering breakpoints on top of fast tracepoints.
>   However, it is not right to me.  Here is an example to illustrate this
> problem.
>
> Supposing I set a regular tracepoint and a fast tracepoint on 0x080484fc,
>
>      0x080484fc<+3>:     e8 f3 ff ff ff  call   0x80484f4<func>
>
> During insertion, trap insn (for regular tracepoint) and jmp insn (for
> fast tracepoint) are inserted, and gdbserver takes care of them to make
> sure trap insn is *always* inserted on top of jmp insn.

I'm looking at this and wondering, why are we inserting the fast 
tracepoint jump insn at all?
Shouldn't be it sufficient to let the trap handler do the work of both 
slow and fast tracepoints at that location?  Since hitting the trap has 
already put us on the slow path, there's not going to any noticeable 
additional penalty for not going to the IPA and interpreting conditional 
bytecodes instead of compiled ones, etc.  We may not even need to sync 
trace buffers (I'm not sure about that, code is tricky).

Stan
stan@codesourcery.com

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Pedro Alves]

On Thursday 27 October 2011 19:38:31, Stan Shebs wrote:
> On 10/26/11 7:56 PM, Yao Qi wrote:
> > Hi,
> > I find a program will receive segv fault when I set a regular tracepoint
> > and a fast tracepoint at the same address, start tracing and resume program.
> >
> > gdbserver has taken care of this situation in many places of the code,
> > when uninserting breakpoint or fast tracepoint, write_inferior_memory is
> > called to take care of layering breakpoints on top of fast tracepoints.
> >   However, it is not right to me.  Here is an example to illustrate this
> > problem.
> >
> > Supposing I set a regular tracepoint and a fast tracepoint on 0x080484fc,
> >
> >      0x080484fc<+3>:     e8 f3 ff ff ff  call   0x80484f4<func>
> >
> > During insertion, trap insn (for regular tracepoint) and jmp insn (for
> > fast tracepoint) are inserted, and gdbserver takes care of them to make
> > sure trap insn is *always* inserted on top of jmp insn.
> 
> I'm looking at this and wondering, why are we inserting the fast 
> tracepoint jump insn at all?

I think the way we do things currently is simpler.  Consider:

 1 - ftrace foo (5 bytes)
 2 - tstart, installs fast tracepoint
 3 - b foo, sets breakpoint (would remove the jmp?)
 4 - del breakpoint $bkpt_foo (would re-insert insert the jmp?)

If in 3 the answer is no, then we have to handle the jmp being
inserted anyway.  If it is yes, then, it looks like extra
weird work.  Same for step 4.  This introduces unnecessary
coupling between different "kinds" of breakpoints.

As is, we think in terms of range of addresses we're
writting to / reading from.

> Shouldn't be it sufficient to let the trap handler do the work of both 
> slow and fast tracepoints at that location?  
> Since hitting the trap has 
> already put us on the slow path, there's not going to any noticeable 
> additional penalty for not going to the IPA and interpreting conditional 
> bytecodes instead of compiled ones, etc.  We may not even need to sync 
> trace buffers (I'm not sure about that, code is tricky).

That's actually kind of a separate issue, and we already do that.
gdbserver/tracepoint.c has this comment:

  /* Presently, gdbserver doesn't run compiled conditions, only the
     IPA does.  If the program stops at a fast tracepoint's address
     (e.g., due to a breakpoint, trap tracepoint, or stepping),
     gdbserver preemptively collect the fast tracepoint.  Later, on
     resume, gdbserver steps over the fast tracepoint like it steps
     over breakpoints, so that the IPA doesn't see that fast
     tracepoint.  This avoids double collects of fast tracepoints in
     that stopping scenario.  Having gdbserver itself handle the fast
     tracepoint gives the user a consistent view of when fast or trap
     tracepoints are collected, compared to an alternative where only
     trap tracepoints are collected on stop, and fast tracepoints on
     resume.  When a fast tracepoint is being processed by gdbserver,
     it is always the non-compiled condition expression that is
     used.  */

-- 
Pedro Alves

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Yao Qi]

On 10/27/2011 11:59 PM, Pedro Alves wrote:
>> > Note that bp->old_data saves the first byte of jmp insn
>> > rather than original insn.
> _This_ is the bug.  I must have introduced it as a last minute change
> before pushing fast tracepoints upstream, because I'm sure this used
> to work, proof being all the comments in the code about it.  :-/
> 
> The idea is that the shadow of all breakpoints and fast tracepoint
> jumps always has the contents of the memory as if no breakpoint
> of fast tracepoint was inserted.
> 

Usually, shadow of both breakpoints and fast tracepoint jumps is a copy
of *original* insn.  That is absolutely correct.

> The problem is that check_mem_write does:
> 
>  - loop over fast tracepoint jumps, updating their shadow buffers,
>    _and_ clobbers the input buffer.
>  - loop over raw breakpoints, updating their shadow buffers, but
>    at this point, the input buffer is already overwritten with
>    fast tracepoint jumps...
> 
> The fix is to split updating the shadow buffers from stapling fast
> tracepoint jumps and breakpoints on top of the input buffer.
> 

Your fix looks right to me, except one concern on performance.  Your
patch doubles the time on iterating over fast tracepoint jump list and
raw breakpoint list.  It is not a big deal when the number of fast
tracepoint and breakpoint is small.  I did an experiment on showing how
much time step-over breakpoint may cost as the number of tracepoint goes
up.  In my experiment, I set thousands (from 1*1296 to 6*1296) of
tracepoints at some different unreachable places to make sure the
breakpoint list in gdbserver is long enough, and set four breakpoints in
program.  GDB will step-over these breakpoint until the end of program,
and I'll calculate the time spent.  The result shows there is no notable
slowdown with your patch applied, it is good, and I am over-worried :)

Another thing I want you to help me to understand is what is wrong with
my patch?  In my patch, the interpretation of shadow is different from
yours.  In my patch, when raw breakpoint and fast tracepoint jump is set
at the same address, shadow of breakpoint is a copy of jump insn.  It
makes easier in uninsert breakpoint, because check_mem_write is not
needed, and we can simply write memory.  The only problem I can think of
is about removing fast tracepoint first, and leaving breakpoint still
there.  Except this problem, is there any more problem?

> While at it, I did several changes to the new tests.  Thanks a lot
> for writing them BTW.  They're awesome.  I wish I had done so before...
> I'll point out the main issues below.
> 

The tests after your modifications looks better.  Please check them in
with your patch.  Thanks for reviewing them.

-- 
Yao (齐尧)

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Yao Qi]

On 10/29/2011 08:15 PM, Yao Qi wrote:
> Another thing I want you to help me to understand is what is wrong with
> my patch?  In my patch, the interpretation of shadow is different from
> yours.  In my patch, when raw breakpoint and fast tracepoint jump is set
> at the same address, shadow of breakpoint is a copy of jump insn.  It
> makes easier in uninsert breakpoint, because check_mem_write is not
> needed, and we can simply write memory.  The only problem I can think of
> is about removing fast tracepoint first, and leaving breakpoint still
> there.  Except this problem, is there any more problem?

I find my patch caused some regression on my local tracepoint test
cases, and I figured out why my patch is wrong.  Please ignore this
question.

-- 
Yao (齐尧)

Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address

[Pedro Alves]

[-- Attachment #1: Type: Text/Plain, Size: 2598 bytes --]

On Saturday 29 October 2011 13:15:13, Yao Qi wrote:
> On 10/27/2011 11:59 PM, Pedro Alves wrote:
> >> > Note that bp->old_data saves the first byte of jmp insn
> >> > rather than original insn.
> > _This_ is the bug.  I must have introduced it as a last minute change
> > before pushing fast tracepoints upstream, because I'm sure this used
> > to work, proof being all the comments in the code about it.  :-/
> > 
> > The idea is that the shadow of all breakpoints and fast tracepoint
> > jumps always has the contents of the memory as if no breakpoint
> > of fast tracepoint was inserted.
> > 
> 
> Usually, shadow of both breakpoints and fast tracepoint jumps is a copy
> of *original* insn.  That is absolutely correct.
> 
> > The problem is that check_mem_write does:
> > 
> >  - loop over fast tracepoint jumps, updating their shadow buffers,
> >    _and_ clobbers the input buffer.
> >  - loop over raw breakpoints, updating their shadow buffers, but
> >    at this point, the input buffer is already overwritten with
> >    fast tracepoint jumps...
> > 
> > The fix is to split updating the shadow buffers from stapling fast
> > tracepoint jumps and breakpoints on top of the input buffer.
> > 
> 
> Your fix looks right to me, except one concern on performance.  Your
> patch doubles the time on iterating over fast tracepoint jump list and
> raw breakpoint list.  It is not a big deal when the number of fast
> tracepoint and breakpoint is small.  I did an experiment on showing how
> much time step-over breakpoint may cost as the number of tracepoint goes
> up.  In my experiment, I set thousands (from 1*1296 to 6*1296) of
> tracepoints at some different unreachable places to make sure the
> breakpoint list in gdbserver is long enough, and set four breakpoints in
> program.  GDB will step-over these breakpoint until the end of program,
> and I'll calculate the time spent.  The result shows there is no notable
> slowdown with your patch applied, it is good, and I am over-worried :)

Yeah, I wasn't worried about it, but looking again, 
it's actually easy to avoid.  We're already working with an xmalloc'ed copy
of the data to write to the inferior, so we can just pass both buffers
to check_mem_write.

> > While at it, I did several changes to the new tests.  Thanks a lot
> > for writing them BTW.  They're awesome.  I wish I had done so before...
> > I'll point out the main issues below.
> > 
> 
> The tests after your modifications looks better.  Please check them in
> with your patch.  Thanks for reviewing them.

Alright, I've checked this in.  Thanks!

-- 
Pedro Alves

[-- Attachment #2: trace_break_fix.diff --]
[-- Type: text/x-patch, Size: 3358 bytes --]

2011-10-31  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* mem-break.c (check_mem_write): Add `myaddr' parameter.  Don't
	clobber the breakpoints' shadows with fast tracepoint jumps.
	* mem-break.h (check_mem_write): Add `myaddr' parameter.
	* target.c (write_inferior_memory): Also pass MYADDR down to
	check_mem_write.

---
 gdb/gdbserver/mem-break.c |    7 ++++---
 gdb/gdbserver/mem-break.h |    6 ++++--
 gdb/gdbserver/target.c    |    2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

Index: src/gdb/gdbserver/mem-break.c
===================================================================
--- src.orig/gdb/gdbserver/mem-break.c	2011-10-31 12:44:19.000000000 +0000
+++ src/gdb/gdbserver/mem-break.c	2011-10-31 12:52:38.930047496 +0000
@@ -1029,7 +1029,8 @@ check_mem_read (CORE_ADDR mem_addr, unsi
 }
 
 void
-check_mem_write (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
+check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
+		 const unsigned char *myaddr, int mem_len)
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp = proc->raw_breakpoints;
@@ -1063,7 +1064,7 @@ check_mem_write (CORE_ADDR mem_addr, uns
       buf_offset = start - mem_addr;
 
       memcpy (fast_tracepoint_jump_shadow (jp) + copy_offset,
-	      buf + buf_offset, copy_len);
+	      myaddr + buf_offset, copy_len);
       if (jp->inserted)
 	memcpy (buf + buf_offset,
 		fast_tracepoint_jump_insn (jp) + copy_offset, copy_len);
@@ -1092,7 +1093,7 @@ check_mem_write (CORE_ADDR mem_addr, uns
       copy_offset = start - bp->pc;
       buf_offset = start - mem_addr;
 
-      memcpy (bp->old_data + copy_offset, buf + buf_offset, copy_len);
+      memcpy (bp->old_data + copy_offset, myaddr + buf_offset, copy_len);
       if (bp->inserted)
 	{
 	  if (validate_inserted_breakpoint (bp))
Index: src/gdb/gdbserver/mem-break.h
===================================================================
--- src.orig/gdb/gdbserver/mem-break.h	2011-10-31 12:44:19.000000000 +0000
+++ src/gdb/gdbserver/mem-break.h	2011-10-31 12:46:54.150047616 +0000
@@ -102,9 +102,11 @@ void check_mem_read (CORE_ADDR mem_addr,
 
 /* See if any breakpoints shadow the target memory area from MEM_ADDR
    to MEM_ADDR + MEM_LEN.  Update the data to be written to the target
-   (in BUF) if necessary, as well as the original data for any breakpoints.  */
+   (in BUF, a copy of MYADDR on entry) if necessary, as well as the
+   original data for any breakpoints.  */
 
-void check_mem_write (CORE_ADDR mem_addr, unsigned char *buf, int mem_len);
+void check_mem_write (CORE_ADDR mem_addr,
+		      unsigned char *buf, const unsigned char *myaddr, int mem_len);
 
 /* Set the byte pattern to insert for memory breakpoints.  This function
    must be called before any breakpoints are set.  */
Index: src/gdb/gdbserver/target.c
===================================================================
--- src.orig/gdb/gdbserver/target.c	2011-10-31 12:44:19.000000000 +0000
+++ src/gdb/gdbserver/target.c	2011-10-31 12:46:54.150047616 +0000
@@ -63,7 +63,7 @@ write_inferior_memory (CORE_ADDR memaddr
 
   buffer = xmalloc (len);
   memcpy (buffer, myaddr, len);
-  check_mem_write (memaddr, buffer, len);
+  check_mem_write (memaddr, buffer, myaddr, len);
   res = (*the_target->write_memory) (memaddr, buffer, len);
   free (buffer);
   buffer = NULL;

[-- Attachment #3: trace_break_test.diff --]
[-- Type: text/x-patch, Size: 10560 bytes --]

2011-10-31  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
        * gdb.trace/trace-break.c: New.
        * gdb.trace/trace-break.exp: New.

---
 gdb/testsuite/gdb.trace/trace-break.c   |   58 +++++++
 gdb/testsuite/gdb.trace/trace-break.exp |  240 ++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-break.exp

Index: src/gdb/testsuite/gdb.trace/trace-break.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.trace/trace-break.c	2011-10-31 12:46:57.470047614 +0000
@@ -0,0 +1,58 @@
+/* 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/>.  */
+
+#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
+marker (void)
+{
+  /* Some code to make sure `b marker' and `b set_point' set
+     breakpoints at different addresses.  */
+  int a = 0;
+  int b = a;
+
+  /* `set_point' is the label where we'll set multiple tracepoints and
+     breakpoints at.  The insn at the label must the large enough to
+     fit a fast tracepoint jump.  */
+  asm ("    .global " SYMBOL(set_point) "\n"
+       SYMBOL(set_point) ":\n"
+#if (defined __x86_64__ || defined __i386__)
+       "    call " SYMBOL(func) "\n"
+#endif
+       );
+}
+
+static void
+end (void)
+{}
+
+int
+main ()
+{
+  marker ();
+  end ();
+  return 0;
+}
Index: src/gdb/testsuite/gdb.trace/trace-break.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.trace/trace-break.exp	2011-10-31 12:46:57.470047614 +0000
@@ -0,0 +1,240 @@
+# 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 "trace-break"
+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 [prepare_for_testing $expfile $executable $srcfile \
+	[list debug $additional_flags]] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+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;
+}
+
+# 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:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set multiple tracepoints at the same address.
+
+proc break_trace_same_addr_2 { trace_type1 trace_type2 option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 2 $trace_type1 $trace_type2 $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "${trace_type1} set_point" \
+	"\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+	"${trace_type1} set_point (1)"
+
+    gdb_test "${trace_type2} set_point" \
+	"\(Fast t|T\)racepoint \[0-9\] at $hex: file.*" \
+	"${trace_type2} set_point (2)"
+
+    gdb_test_no_output "tstart"
+    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\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete breakpoint, and verify
+# that tracepoint still works.
+
+proc break_trace_same_addr_3 { trace_type option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 3 $trace_type $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test_no_output "tstart"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    gdb_test "delete break 4"
+
+    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" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+# Set breakpoint and tracepoint at the same address.  Delete tracepoint, and verify
+# that breakpoint still works.
+
+proc break_trace_same_addr_4 { trace_type option } {
+    global executable
+    global pf_prefix
+    global hex
+
+    set old_pf_prefix $pf_prefix
+    set pf_prefix "$pf_prefix 4 $trace_type $option:"
+
+    # 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_no_output "set breakpoint always-inserted ${option}"
+    gdb_test "break marker" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "break end" "Breakpoint \[0-9\] at $hex: file.*"
+
+    gdb_test "break set_point" "Breakpoint \[0-9\] at $hex: file.*"
+    gdb_test "${trace_type} set_point" "\(Fast t|T\)racepoint \[0-9\] at $hex: file.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to marker"
+    # Delete tracepoint set on set_point.
+    gdb_test "delete trace 5"
+
+    gdb_test "tstart" "No tracepoints defined, not starting trace.*"
+
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to set_point"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" "continue to end"
+    gdb_test "tstop" "Trace is not running.*"
+
+    gdb_test "tfind" "Target failed to find requested trace frame\\..*"
+
+    set pf_prefix $old_pf_prefix
+}
+
+foreach break_always_inserted { "on" "off" } {
+    break_trace_same_addr_1 "trace" ${break_always_inserted}
+    break_trace_same_addr_2 "trace" "trace" ${break_always_inserted}
+    break_trace_same_addr_3 "trace" ${break_always_inserted}
+    break_trace_same_addr_4 "trace" ${break_always_inserted}
+}
+
+set libipa $objdir/../gdbserver/libinproctrace.so
+gdb_load_shlibs $libipa
+
+# Can't use prepare_for_testing, because that splits compiling into
+# building objects and then linking, and we'd fail with "linker input
+# file unused because linking not done" when building the object.
+
+if { [gdb_compile "$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_1 "ftrace" ${break_always_inserted}
+	break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted}
+	break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted}
+	break_trace_same_addr_3 "ftrace" ${break_always_inserted}
+	break_trace_same_addr_4 "ftrace" ${break_always_inserted}
+    }
+}