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/06/2012 01:38 PM, Yao Qi wrote:

> Hi,
> Bug 13392 is caused by incorrect JMP instruction when offset exceeds
> integer limit.  This patch is to check the offset first to see if it is
> still within the integer range.  Return error if offset exceeds.
> 
> The changes to test cases is a little mechanical, simply remove kfail,
> and check the error messages of downloading tracepoints.  The test
> result changes like this,
> 
> -KFAIL: gdb.trace/change-loc.exp: 1 ftrace: continue to marker 2 (PRMS:
> gdb/13392)
> -PASS: gdb.trace/change-loc.exp: 2 ftrace: tstart
> -KFAIL: gdb.trace/change-loc.exp: 2 ftrace: continue to marker 1 (PRMS:
> gdb/13392)
> -PASS: gdb.trace/pending.exp: ftrace works: start trace experiment
> -KFAIL: gdb.trace/pending.exp: ftrace works: continue to marker (PRMS:
> gdb/13392)
> -KFAIL: gdb.trace/pending.exp: ftrace resolved_in_trace: continue to
> marker 2 (PRMS: gdb/13392)
> -KFAIL: gdb.trace/pending.exp: ftrace action_resolved: continue to
> marker 2 (PRMS: gdb/13392)
> -KFAIL: gdb.trace/pending.exp: ftrace installed_in_trace: continue to
> marker 2 (PRMS: gdb/13392)
> 
> Tested on x86_64-linux, OK to apply?
> 
> -- Yao (éå)
> 
> 
> 0001-fix-pr13392-check-offset-of-jmp-insn.patch
> 
> 
> 2012-03-06  Yao Qi  <yao@codesourcery.com>
> 
> 	Fix PR server/13392.
> 	* linux-x86-low.c (amd64_install_fast_tracepoint_jump_pad): Check
> 	offset of JMP insn.
> 	* tracepoint.c (install_fast_tracepoint): Fill in ERRBUF when
> 	gets error.
> 
> 2012-03-06  Yao Qi  <yao@codesourcery.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): Likwise.
> 	(pending_tracepoint_resolved_during_trace): Likewise.
> 	(pending_tracepoint_installed_during_trace): Likewise.
> 	(pending_tracepoint_with_action_resolved): Likewise.
> ---
>  gdb/gdbserver/linux-x86-low.c          |   25 +++++++++++++-
>  gdb/gdbserver/tracepoint.c             |    5 ++-
>  gdb/testsuite/gdb.trace/change-loc.exp |   38 +++++++++++++++-----
>  gdb/testsuite/gdb.trace/pending.exp    |   57 ++++++++++++++++++++++++--------
>  4 files changed, 98 insertions(+), 27 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
> index 58aaf9a..4647f5a 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,16 @@ 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)
> +    {
> +      warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n",
> +	       loffset);


We should send an error back to GDB with "E." instead of printing something
to gdbserver's console, and leaving the user with a generic and
unhelful error.  "4-byte" isn't strictly correct, as this is a signed
offset, and I think we can be a bit more clear.  So we end up with:

      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 +1344,16 @@ 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)
> +    {
> +      warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n",
> +	       loffset);


      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..1f85f9a 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -2853,7 +2853,10 @@ install_fast_tracepoint (struct tracepoint *tpoint, char *errbuf)
>  					  errbuf);
>  
>    if (err)
> -    return 1;
> +    {
> +      xsnprintf (errbuf, 50, "E.Failed to install fast tracepoint jumppad");


This would overwrite any error the install_fast_tracepoint target hook wrote to
errbuf (can be seen quoted just above).

> +      return 1;
> +    }
>  
>    /* Wire it in.  */
>    tpoint->handle = set_fast_tracepoint_jump (tpoint->address, fjump,
> diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
> index d6062fc..7f2fd91 100644
> --- a/gdb/testsuite/gdb.trace/change-loc.exp
> +++ b/gdb/testsuite/gdb.trace/change-loc.exp
> @@ -114,8 +114,16 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" {
>  	    pass "continue to marker 2"
>  	}
>  	-re ".*$gdb_prompt $" {
> -	    kfail "gdb/13392" "continue to marker 2"
> -	    return
> +
> +	    # It is possible to unable to create a jumppad for a fast tracepoint
> +	    # due to the limitation of instructions.  We simply return to skip
> +	    # the rest of tests.


I understand what you mean, but this needs copy/editing.  "possible to unable to create"
doesn't parse.  But, I think we can do better when the target sends back the real error
to GDB.  Since that essentially means every hunk in your patch needs redoing, I went
ahead and did the changes.  See patches attached.

> +	    if [string equal $trace_type "ftrace"] {
> +		return
> +	    } else {
> +		fail "continue to marker 2"
> +	    }
> +
>  	}
>      }
>      # tracepoint has three locations after shlib change-loc-2 is loaded.
> @@ -198,19 +206,29 @@ 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"
> -	}
> +    gdb_test_multiple "tstart" "tstart" {
> +        -re "^tstart\r\n$gdb_prompt $" {
> +	    if ![string match "" "tstart"] then {


???  When can [string match "" "tstart"] ever return something other than 0 ?

> +		pass "tstart"
> +            }
> +        }
>  	-re ".*$gdb_prompt $" {
> -	    kfail "gdb/13392" "continue to marker 1"
> -	    return
> +	    # It is possible to unable to create a jumppad for a fast tracepoint
> +	    # due to the limitation of instructions.  We simply return to skip
> +	    # the rest of tests.
> +
> +	    if [string equal $trace_type "ftrace"] {
> +		return
> +	    } else {
> +		fail "tstart"
> +	    }


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.

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
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
send a clearer error back to GDB, and we end up with:

Target returns error code '.Tracepoint 2 at 0x7ffff67c2579 already exists'.

>      # 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..1391d42 100644
> --- a/gdb/testsuite/gdb.trace/pending.exp
> +++ b/gdb/testsuite/gdb.trace/pending.exp


... more of the same.

Please find attached the interdiff (my changes on top of yours), and then
the final patch (both patches merged).

WDYT?

-- 
Pedro Alves
commit a7e67466d25e5f5172c08d8023290d0bb64fe4ef
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Mar 6 14:51:03 2012 +0000

    a

diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 4647f5a..ed1f8a8 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -1330,8 +1330,9 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
   loffset = (tpaddr + orig_size) - (buildaddr + sizeof (jump_insn));
   if (loffset > INT_MAX || loffset < INT_MIN)
     {
-      warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n",
-	       loffset);
+      sprintf (err,
+	       "E.Jump back from jump pad too far from tracepoint "
+	       "(offset 0x%" PRIx64 " > int32).", loffset);
       return 1;
     }
 
@@ -1347,8 +1348,9 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
   loffset = *jump_entry - (tpaddr + sizeof (jump_insn));
   if (loffset > INT_MAX || loffset < INT_MIN)
     {
-      warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n",
-	       loffset);
+      sprintf (err,
+	       "E.Jump pad too far from tracepoint "
+	       "(offset 0x%" PRIx64 " > int32).", loffset);
       return 1;
     }
 
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 1f85f9a..3a10daf 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -2304,10 +2304,8 @@ cmd_qtdp (char *own_buf)
       /* Duplicate tracepoints are never allowed.  */
       if (tpoint)
 	{
-	  trace_debug ("Tracepoint error: tracepoint %d"
-		       " at 0x%s already exists",
+	  sprintf (own_buf, "E.Tracepoint %d at 0x%s already exists",
 		       (int) num, paddress (addr));
-	  write_enn (own_buf);
 	  return;
 	}
 
@@ -2853,10 +2851,7 @@ install_fast_tracepoint (struct tracepoint *tpoint, char *errbuf)
 					  errbuf);
 
   if (err)
-    {
-      xsnprintf (errbuf, 50, "E.Failed to install fast tracepoint jumppad");
-      return 1;
-    }
+    return 1;
 
   /* Wire it in.  */
   tpoint->handle = set_fast_tracepoint_jump (tpoint->address, fjump,
diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
index dc9b8a9..a9bbbbc 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,21 +123,22 @@ 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 $" {
-
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint 4 at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# 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.
@@ -206,21 +221,20 @@ 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_multiple "tstart" "tstart" {
+    set test "tstart"
+    gdb_test_multiple "tstart" $test {
         -re "^tstart\r\n$gdb_prompt $" {
-	    if ![string match "" "tstart"] then {
-		pass "tstart"
-            }
+	    pass $test
         }
-	-re ".*$gdb_prompt $" {
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
-
+	-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 "tstart"
+		fail $test
 	    }
 	}
     }
diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp
index 88b294a..f2147cc 100644
--- a/gdb/testsuite/gdb.trace/pending.exp
+++ b/gdb/testsuite/gdb.trace/pending.exp
@@ -132,21 +132,20 @@ 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_multiple "tstart" "start trace experiment" {
+    set test "start trace experiment"
+    gdb_test_multiple "tstart" $test {
         -re "^tstart\r\n$gdb_prompt $" {
-	    if ![string match "" "tstart"] then {
-		pass "start trace experiment"
-            }
+	    pass $test
         }
-	-re ".*$gdb_prompt $" {
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
-
+	-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 "start trace experiment"
+		pass $test
 	    }
 	}
     }
@@ -199,21 +198,23 @@ 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"
-	}
-	-re ".*$gdb_prompt $" {
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
-
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# Skip the rest of the tests.
 		return
 	    } else {
-		fail "continue to marker 2"
+		fail $test
 	    }
 	}
+	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	    pass $test
+	}
     }
 
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
@@ -270,20 +271,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 $" {
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# Skip the rest of the tests.
 		return
 	    } else {
-		fail "continue to marker 2"
+		fail $test
 	    }
 	}
+	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	    pass "continue to marker 2"
+	}
     }
 
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
@@ -446,20 +450,23 @@ 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" {
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
-	    pass "continue to marker 2"
-	}
-	-re ".*$gdb_prompt $" {
-	    # It is possible to unable to create a jumppad for a fast tracepoint
-	    # due to the limitation of instructions.  We simply return to skip
-	    # the rest of tests.
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# Skip the rest of the tests.
 		return
 	    } else {
-		fail "continue to marker 2"
+		fail $test
 	    }
 	}
+	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	    pass $test
+	}
     }
 
     gdb_test "tstop" "\[\r\n\]+" "stop trace experiment"
2012-03-06  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	PR server/13392

	* linux-x86-low.c: Include inttypes.h.
	(amd64_install_fast_tracepoint_jump_pad): Check
	offset of JMP insn.
	* tracepoint.c (cmd_qtdp): Return an E. style error when a
	duplicate tracepoint is detected.

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

	PR server/13392

	* gdb.trace/change-loc.exp (tracepoint_change_loc_1): Remove
	kfail.  Expect target errors related to fast tracepoints being too
	far from the jump pad.
	(tracepoint_change_loc_2): Likewise.
	* 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.

diff --git c/gdb/gdbserver/linux-x86-low.c w/gdb/gdbserver/linux-x86-low.c
index 58aaf9a..ed1f8a8 100644
--- c/gdb/gdbserver/linux-x86-low.c
+++ w/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 c/gdb/gdbserver/tracepoint.c w/gdb/gdbserver/tracepoint.c
index 21e58ff..3a10daf 100644
--- c/gdb/gdbserver/tracepoint.c
+++ w/gdb/gdbserver/tracepoint.c
@@ -2304,10 +2304,8 @@ cmd_qtdp (char *own_buf)
       /* Duplicate tracepoints are never allowed.  */
       if (tpoint)
 	{
-	  trace_debug ("Tracepoint error: tracepoint %d"
-		       " at 0x%s already exists",
+	  sprintf (own_buf, "E.Tracepoint %d at 0x%s already exists",
 		       (int) num, paddress (addr));
-	  write_enn (own_buf);
 	  return;
 	}
 
diff --git c/gdb/testsuite/gdb.trace/change-loc.exp w/gdb/testsuite/gdb.trace/change-loc.exp
index d6062fc..a9bbbbc 100644
--- c/gdb/testsuite/gdb.trace/change-loc.exp
+++ w/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,14 +123,23 @@ 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" {
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint 4 at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# Skip the rest of the tests.
+		return
+	    } else {
+		fail $test
+	    }
+	}
 	-re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
 	    pass "continue to marker 2"
 	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker 2"
-	    return
-	}
     }
     # tracepoint has three locations after shlib change-loc-2 is loaded.
     gdb_test "info trace" \
@@ -198,19 +221,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 $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 "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 c/gdb/testsuite/gdb.trace/pending.exp w/gdb/testsuite/gdb.trace/pending.exp
index 017aea9..f2147cc 100644
--- c/gdb/testsuite/gdb.trace/pending.exp
+++ w/gdb/testsuite/gdb.trace/pending.exp
@@ -132,18 +132,27 @@ 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"
-	}
-	-re ".*$gdb_prompt $" {
-	    kfail "gdb/13392" "continue to marker"
-	    return
+    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 {
+		pass $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 +198,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 .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# 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 +271,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" {
+    set test "continue to marker 2"
+    gdb_test_multiple "continue" $test {
+	-re "Target returns error code .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# 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"
@@ -423,13 +450,22 @@ 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" {
-	-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 .*Tracepoint .* at .* already exists.*$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) earlier.
+		pass "$test (tracepoint already exists)"
+		# 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
 	}
     }
 

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