This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are set at the same address
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
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;
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}
+ }
+}