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

[Yao Qi <yao at codesourcery dot com>]

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 (éå)

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

	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