This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Fix PR 13392 : check offset of JMP insn


On 03/07/2012 01:03 AM, Pedro Alves wrote:
> Hmm, what does that error on "continue" mean?  With just your patch, I see in gdb.log:
> 
> continue
> Continuing.
> Target returns error code '01'.
> 
> That's another generic error that could be made to return "E." instead.

This problem is what I planed to fix in next step.

> 
> So I enabled remote debug, and we see:
> 
> continue
> Continuing.
> Sending packet: $QPassSignals:#f3...Packet received: OK
> Sending packet: $vCont;s:p3e0d.3e0d#e8...Packet received: T0506:80d6ffffff7f0000;07:58d6ffffff7f0000;10:6a07400000000000;thread:p3e0d.3e0d;core:3;
> Sending packet: $qTStatus#49...Packet received: T1;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:001331049107617717;stoptime:00000
> 0000;username::;notes::
> Sending packet: $qTMinFTPILen#3b...Packet received: 5
> Sending packet: $m7ffff7bf05c6,1#96...Packet received: e8
> Sending packet: $m7ffff7bf05c7,4#9a...Packet received: f1ffffff
> Sending packet: $QTDP:4:00007ffff7bf05c6:E:0:0:F5#75...Packet received: E.duplicate
> Target returns error code '01'.
> 
> So we're failing in QTDP handling, as expected.  But where exactly?  It's not when
> trying to install the fast tracepoint to target memory.  It's here:
> 
> --- c/gdb/gdbserver/tracepoint.c
> +++ w/gdb/gdbserver/tracepoint.c
> @@ -2307,7 +2307,8 @@ cmd_qtdp (char *own_buf)
>           trace_debug ("Tracepoint error: tracepoint %d"
>                        " at 0x%s already exists",
>                        (int) num, paddress (addr));
> -         write_enn (own_buf);
> +         sprintf (own_buf, "E.duplicate");
> +         //write_enn (own_buf);
>           return;
>         }
> 
> continue
> Continuing.
> Sending packet: $QPassSignals:#f3...Packet received: OK
> Sending packet: $vCont;s:p3e0d.3e0d#e8...Packet received: T0506:80d6ffffff7f0000;07:58d6ffffff7f0000;10:6a07400000000000;thread:p3e0d.3e0d;core:3;
> Sending packet: $qTStatus#49...Packet received: T1;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:001331049107617717;stoptime:00000
> 0000;username::;notes::
> Sending packet: $qTMinFTPILen#3b...Packet received: 5
> Sending packet: $m7ffff7bf05c6,1#96...Packet received: e8
> Sending packet: $m7ffff7bf05c7,4#9a...Packet received: f1ffffff
> Sending packet: $QTDP:4:00007ffff7bf05c6:E:0:0:F5#75...Packet received: E.duplicate
> Target returns error code '.duplicate'.
> 
> So something doesn't look exactly right.  The target knows about this fast
> tracepoint already, but it isn't really installed yet (so we have 3 levels of "inserted",
> not just 2: never downloaded; downloaded; downloaded and installed).  Maybe GDB

Yes, there are 3 levels so far.  Do you think target should keep fast
tracepoints that "downloaded but not installed yet"?  I am inclined to
teach target to remove downloaded fast tracepoints if they are not
installed successfully.  Does it sounds good?  The QTDP is like a
transaction, returns OK when both download and installation is
successful.  If installation failed, roll back to cleanup downloaded
fast tracepoint.

> should have disabled the tracepoint on "tstart", or deleted it from the target, or...
> Anyway, that'll need some further thinking.  In the mean time, I've made gdbserver

Agreed.  GDB tracepoint/breakpoint module should exposes some interfaces
for other modules, such as target, to delete or disable breakpoint
locations when needed.

> send a clearer error back to GDB, and we end up with:
> 
> Target returns error code '.Tracepoint 2 at 0x7ffff67c2579 already exists'.

It is OK for me to let remote target to reports a duplicated tracepoint.

In this version, some changes compared with last one,

  - move `tracepoint already exist' patch out, which can be a separate one,
  - remove downloaded tracepoint in target when failed to install,

-- 
Yao (éå)
2012-03-07  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	Fix PR server/13392.
	* linux-x86-low.c (amd64_install_fast_tracepoint_jump_pad): Check
	offset of JMP insn.
	* tracepoint.c (remove_tracepoint): New.
	(cmd_qtdp): Call remove_tracepoint when failed to install.

2012-03-07  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	Fix PR server/13392.
	* gdb.trace/change-loc.exp (tracepoint_change_loc_1): Remove kfail.
	(tracepoint_change_loc_2): Remove kfail.  Return if failed to
	download tracepoints.
	* gdb.trace/pending.exp (pending_tracepoint_works): Likewise.
	(pending_tracepoint_resolved_during_trace): Likewise.
	(pending_tracepoint_installed_during_trace): Likewise.
	(pending_tracepoint_with_action_resolved): Likewise.
---
 gdb/gdbserver/linux-x86-low.c          |   27 +++++++++-
 gdb/gdbserver/tracepoint.c             |   24 ++++++++
 gdb/testsuite/gdb.trace/change-loc.exp |   71 ++++++++++++++++++------
 gdb/testsuite/gdb.trace/pending.exp    |   94 ++++++++++++++++++++++----------
 4 files changed, 168 insertions(+), 48 deletions(-)

diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 58aaf9a..ed1f8a8 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -20,6 +20,7 @@
 #include <stddef.h>
 #include <signal.h>
 #include <limits.h>
+#include <inttypes.h>
 #include "server.h"
 #include "linux-low.h"
 #include "i387-fp.h"
@@ -1200,6 +1201,8 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
 {
   unsigned char buf[40];
   int i, offset;
+  int64_t loffset;
+
   CORE_ADDR buildaddr = *jump_entry;
 
   /* Build the jump pad.  */
@@ -1323,7 +1326,17 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
   *adjusted_insn_addr_end = buildaddr;
 
   /* Finally, write a jump back to the program.  */
-  offset = (tpaddr + orig_size) - (buildaddr + sizeof (jump_insn));
+
+  loffset = (tpaddr + orig_size) - (buildaddr + sizeof (jump_insn));
+  if (loffset > INT_MAX || loffset < INT_MIN)
+    {
+      sprintf (err,
+	       "E.Jump back from jump pad too far from tracepoint "
+	       "(offset 0x%" PRIx64 " > int32).", loffset);
+      return 1;
+    }
+
+  offset = (int) loffset;
   memcpy (buf, jump_insn, sizeof (jump_insn));
   memcpy (buf + 1, &offset, 4);
   append_insns (&buildaddr, sizeof (jump_insn), buf);
@@ -1332,7 +1345,17 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
      is always done last (by our caller actually), so that we can
      install fast tracepoints with threads running.  This relies on
      the agent's atomic write support.  */
-  offset = *jump_entry - (tpaddr + sizeof (jump_insn));
+  loffset = *jump_entry - (tpaddr + sizeof (jump_insn));
+  if (loffset > INT_MAX || loffset < INT_MIN)
+    {
+      sprintf (err,
+	       "E.Jump pad too far from tracepoint "
+	       "(offset 0x%" PRIx64 " > int32).", loffset);
+      return 1;
+    }
+
+  offset = (int) loffset;
+
   memcpy (buf, jump_insn, sizeof (jump_insn));
   memcpy (buf + 1, &offset, 4);
   memcpy (jjump_pad_insn, buf, sizeof (jump_insn));
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 21e58ff..6ec322a 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -1684,6 +1684,28 @@ find_tracepoint (int id, CORE_ADDR addr)
   return NULL;
 }
 
+/* Remove TPOINT from global list.  */
+
+static void
+remove_tracepoint (struct tracepoint *tpoint)
+{
+  struct tracepoint *tp, *tp_prev;
+
+  for (tp = tracepoints, tp_prev = NULL; tp && tp != tpoint;
+       tp_prev = tp, tp = tp->next)
+    ;
+
+  if (tp)
+    {
+      if (tp_prev)
+	tp_prev->next = tp->next;
+      else
+	tracepoints->next = tp->next;
+
+      xfree (tp);
+    }
+}
+
 /* There may be several tracepoints with the same number (because they
    are "locations", in GDB parlance); return the next one after the
    given tracepoint, or search from the beginning of the list if the
@@ -2391,6 +2413,8 @@ cmd_qtdp (char *own_buf)
 
       download_tracepoint (tpoint);
       install_tracepoint (tpoint, own_buf);
+      if (strncmp (own_buf, "OK", 2) != 0)
+	remove_tracepoint (tpoint);
 
       unpause_all (1);
       return;
diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
index d6062fc..71890d7 100644
--- a/gdb/testsuite/gdb.trace/change-loc.exp
+++ b/gdb/testsuite/gdb.trace/change-loc.exp
@@ -98,7 +98,21 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" {
     gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
 	"continue to marker 1"
     # Set a tracepoint during tracing.
-    gdb_test "${trace_type} set_tracepoint" ".*" "set tracepoint on set_tracepoint"
+    set test "set tracepoint on set_tracepoint"
+    gdb_test_multiple "${trace_type} set_tracepoint" $test {
+	-re "Target returns error code .* too far .*$gdb_prompt $" {
+	    if [string equal $trace_type "ftrace"] {
+		# The target was unable to install the fast tracepoint
+		# (e.g., jump pad too far from tracepoint).
+		pass "$test (too far)"
+	    } else {
+		fail $test
+	    }
+	}
+	-re "\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
 
     gdb_trace_setactions "set action for tracepoint" "" \
 	"collect \$$pcreg" "^$"
@@ -109,15 +123,27 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" {
 \[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*4\.1.* in func4.*4\.2.* in func4.*" \
 	"tracepoint with two locations"
 
-    gdb_test_multiple "continue" "continue to marker 2" {
-	-re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker 2"
-	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 2"
-	    return
-	}
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .* too far .*$gdb_prompt $" {
+            if [string equal $trace_type "ftrace"] {
+		# Expected if the target was unable to install the
+		# fast tracepoint (e.g., jump pad too far from
+		# tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+                return
+            } else {
+		fail "continue to marker 2"
+		fail $test
+            }
+
+       }
+       -re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+           pass "continue to marker 2"
+       }
     }
+
     # tracepoint has three locations after shlib change-loc-2 is loaded.
     gdb_test "info trace" \
 	"Num     Type\[ \]+Disp Enb Address\[ \]+What.*
@@ -198,19 +224,28 @@ proc tracepoint_change_loc_2 { trace_type } { with_test_prefix "2 $trace_type" {
 	"breakpoint on marker"
 
     # tracepoint with two locations will be downloaded and installed.
-    gdb_test_no_output "tstart"
-
-    gdb_test_multiple "continue" "continue to marker 1" {
-	-re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker 1"
-	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 1"
-	    return
+    set test "tstart"
+    gdb_test_multiple "tstart" $test {
+        -re "^tstart\r\n$gdb_prompt $" {
+	    pass "tstart"
+        }
+	-re "Target returns error code .* too far .*$gdb_prompt $" {
+            if [string equal $trace_type "ftrace"] {
+		# The target was unable to install the fast tracepoint
+		# (e.g., jump pad too far from tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+                return
+            } else {
+		fail $test
+            }
 	}
     }
 
     gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+	"continue to marker 1"
+
+    gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
 	"continue to marker 2"
 
     # tracepoint has three locations after shlib change-loc-2 is loaded.
diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp
index 017aea9..7a41b2f 100644
--- a/gdb/testsuite/gdb.trace/pending.exp
+++ b/gdb/testsuite/gdb.trace/pending.exp
@@ -132,18 +132,28 @@ proc pending_tracepoint_works { trace_type } { with_test_prefix "$trace_type wor
     gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \
 	"breakpoint on marker"
 
-    gdb_test_no_output "tstart" "start trace experiment"
+    set test "start trace experiment"
+    gdb_test_multiple "tstart" $test {
+        -re "^tstart\r\n$gdb_prompt $" {
+	    pass $test
+        }
+	-re "Target returns error code .* too far .*$gdb_prompt $" {
+            if [string equal $trace_type "ftrace"] {
+		# The target was unable to install the fast tracepoint
+		# (e.g., jump pad too far from tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+                return
+            } else {
+		fail $test
+            }
+        }
 
-    gdb_test_multiple "continue" "continue to marker" {
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker"
-	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker"
-	    return
-	}
     }
 
+    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*" \
+	"continue to marker"
+
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
 
     gdb_test "tfind start" "#0 .*" "tfind test frame 0"
@@ -189,13 +199,22 @@ proc pending_tracepoint_resolved_during_trace { trace_type } \
     gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \
 	"continue to marker 1"
 
-    gdb_test_multiple "continue" "continue to marker 2" {
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker 2"
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .* too far .*$gdb_prompt $" {
+	    if [string equal $trace_type "ftrace"] {
+		# Expected if the target was unable to install the
+		# fast tracepoint (e.g., jump pad too far from
+		# tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+		return
+	    } else {
+		fail $test
+	    }
 	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 2"
-	    return
+	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	    pass $test
 	}
     }
 
@@ -253,14 +272,23 @@ proc pending_tracepoint_installed_during_trace { trace_type } \
 \[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \t\]+keep y.*PENDING.*set_point2.*" \
 	"single pending tracepoint on set_point2"
 
-    gdb_test_multiple "continue" "continue to marker 2" {
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker 2"
-	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 2"
-	    return
-	}
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	    -re "Target returns error code .* too far .*$gdb_prompt $" {
+	    if [string equal $trace_type "ftrace"] {
+               # Expected if the target was unable to install the
+		# fast tracepoint (e.g., jump pad too far from
+		# tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+                return
+            } else {
+		fail $test
+            }
+        }
+       -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+           pass $test
+       }
     }
 
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
@@ -423,14 +451,24 @@ proc pending_tracepoint_with_action_resolved { trace_type } \
     gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \
 	"continue to marker 1"
 
-    gdb_test_multiple "continue" "continue to marker 2" {
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	    -re "Target returns error code .* too far .*$gdb_prompt $" {
+            if [string equal $trace_type "ftrace"] {
+		# Expected if the target was unable to install the
+		# fast tracepoint (e.g., jump pad too far from
+		# tracepoint).
+		pass "$test (too far)"
+		# Skip the rest of the tests.
+                return
+            } else {
+		fail $test
+            }
+	}
 	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
 	    pass "continue to marker 2"
 	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 2"
-	    return
-	}
+
     }
 
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
-- 
1.7.0.4


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]