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

[Pedro Alves]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace_break_test.diff
Type: text/x-patch
Size: 10560 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111027/674bc305/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace_break_fix.diff
Type: text/x-patch
Size: 3511 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111027/674bc305/attachment-0001.bin>