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

[Yao Qi <yao@codesourcery.com>]

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

On 03/09/2012 12:26 AM, Pedro Alves wrote:
>> >   - remove downloaded tracepoint in target when failed to install,
> We should also check what happens when we do "enable TRACEPOINT_FOO", and
> that tracepoints fails to be likewise inserted.  Following this direction,
> we should make sure it stays disabled in the target, but _not_ deleted.
> 

AFAIK, tracepoint is not inserted when "enable tracepoint", so we don't
have to worry about the fails of inserting tracepoints in "enable
tracepoint".  Tracepoints are downloaded and installed in QTDP or
QTStart.  For QTenable/QTDisable, it simply changes the flag `enable' in
tracepoint.  Of course, we may get fails during QTenable/QTDisable, but
fails are not caused by inserting/installing tracepoint.

>> > +/* 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;
> 
> This doesn't look correct.  If there's no tp_prev, then TP must be
> TRACEPOINTS, and so you need to do 'tracepoints = tp->next;'.
> 

Oops, sorry.  I should stop coding at mid-night.  Fixed.

Regression tested on x86_64-linux.  Committed.
http://sourceware.org/ml/gdb-cvs/2012-03/msg00146.html

-- 
Yao (齐尧)

[-- Attachment #2: 0001-fix-pr13392-check-offset-of-jmp-insn.patch --]
[-- Type: text/x-patch, Size: 12261 bytes --]


2012-03-08  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-08  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    |   90 ++++++++++++++++++++++---------
 4 files changed, 166 insertions(+), 46 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..7bf576b 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 = 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 (strcmp (own_buf, "OK") != 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..a5a982f 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..4e7dc31 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"
-
-    gdb_test_multiple "continue" "continue to marker" {
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker"
+    set test "start trace experiment"
+    gdb_test_multiple "tstart" $test {
+	-re "^tstart\r\n$gdb_prompt $" {
+	    pass $test
 	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker"
-	    return
+	-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" "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