[PATCH] Fix instruction relocation for fast tracepoints

[PATCH] Fix instruction relocation for fast tracepoints

[Kwok Cheung Yeung]

Hello

There is a problem with the instruction relocation facility in GDB, which is
triggered when fast tracepoints are set on call or jump instructions.  The third
and fourth arguments of calls to store_signed_integer (defined in findvar.c) in
the i386 and amd64 implementations of relocate_instruction are the wrong way
around, which results in a relative jump one byte forward.  In the context of
fast tracepoints, this results in a jump to the address portion of another jump
instruction, which typically causes the program to crash when the tracepoint is
reached.

This patch fixes the ordering of the arguments to store_signed_integer.  It also
adds debug messages to the case where a call instruction is relocated (identical
to the ones for jump instructions), and tidies up the presentation of the
messages by removing the duplicate '0x0x' prefix of hexadecimal numbers (since
hex_string and paddress already output the prefix).

Kwok Cheung Yeung


2011-03-17  Kwok Cheung Yeung  <kcy@codesourcery.com>

        * amd64-tdep.c (amd64_relocate_instruction): Fix ordering of arguments
        to store_signed_integer.  Add debug message when relocating CALL
        instructions.  Fix formatting of debug message.
        * i386-tdep.c (i386_relocate_instruction): Ditto.

--- gdb-7.2/gdb/amd64-tdep.c	2010-05-26 19:19:26.000000000 +0100
+++ gdb-7.2.new/gdb/amd64-tdep.c	2011-03-17 18:28:11.390662621 +0000
@@ -1588,7 +1588,14 @@ amd64_relocate_instruction (struct gdbar
       /* Adjust the destination offset.  */
       rel32 = extract_signed_integer (insn + 1, 4, byte_order);
       newrel = (oldloc - *to) + rel32;
-      store_signed_integer (insn + 1, 4, newrel, byte_order);
+      store_signed_integer (insn + 1, 4, byte_order, newrel);
+
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Adjusted insn rel32=%s at %s to"
+			    " rel32=%s at %s\n",
+			    hex_string (rel32), paddress (gdbarch, oldloc),
+			    hex_string (newrel), paddress (gdbarch, *to));

       /* Write the adjusted jump into its displaced location.  */
       append_insns (to, 5, insn);
@@ -1611,11 +1618,11 @@ amd64_relocate_instruction (struct gdbar
     {
       rel32 = extract_signed_integer (insn + offset, 4, byte_order);
       newrel = (oldloc - *to) + rel32;
-      store_signed_integer (insn + offset, 4, newrel, byte_order);
+      store_signed_integer (insn + offset, 4, byte_order, newrel);
       if (debug_displaced)
 	fprintf_unfiltered (gdb_stdlog,
-			    "Adjusted insn rel32=0x%s at 0x%s to"
-			    " rel32=0x%s at 0x%s\n",
+			    "Adjusted insn rel32=%s at %s to"
+			    " rel32=%s at %s\n",
 			    hex_string (rel32), paddress (gdbarch, oldloc),
 			    hex_string (newrel), paddress (gdbarch, *to));
     }
--- gdb-7.2/gdb/i386-tdep.c	2010-06-22 03:15:45.000000000 +0100
+++ gdb-7.2.new/gdb/i386-tdep.c	2011-03-17 18:28:11.398608604 +0000
@@ -747,7 +747,14 @@ i386_relocate_instruction (struct gdbarc
       /* Adjust the destination offset.  */
       rel32 = extract_signed_integer (insn + 1, 4, byte_order);
       newrel = (oldloc - *to) + rel32;
-      store_signed_integer (insn + 1, 4, newrel, byte_order);
+      store_signed_integer (insn + 1, 4, byte_order, newrel);
+
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Adjusted insn rel32=%s at %s to"
+			    " rel32=%s at %s\n",
+			    hex_string (rel32), paddress (gdbarch, oldloc),
+			    hex_string (newrel), paddress (gdbarch, *to));

       /* Write the adjusted jump into its displaced location.  */
       append_insns (to, 5, insn);
@@ -766,11 +773,11 @@ i386_relocate_instruction (struct gdbarc
     {
       rel32 = extract_signed_integer (insn + offset, 4, byte_order);
       newrel = (oldloc - *to) + rel32;
-      store_signed_integer (insn + offset, 4, newrel, byte_order);
+      store_signed_integer (insn + offset, 4, byte_order, newrel);
       if (debug_displaced)
 	fprintf_unfiltered (gdb_stdlog,
-			    "Adjusted insn rel32=0x%s at 0x%s to"
-			    " rel32=0x%s at 0x%s\n",
+			    "Adjusted insn rel32=%s at %s to"
+			    " rel32=%s at %s\n",
 			    hex_string (rel32), paddress (gdbarch, oldloc),
 			    hex_string (newrel), paddress (gdbarch, *to));
     }

Re: [PATCH] Fix instruction relocation for fast tracepoints

[Tom Tromey]

>>>>> ">" == Kwok Cheung Yeung <kcy@codesourcery.com> writes:

>> 2011-03-17  Kwok Cheung Yeung  <kcy@codesourcery.com>
>>         * amd64-tdep.c (amd64_relocate_instruction): Fix ordering of arguments
>>         to store_signed_integer.  Add debug message when relocating CALL
>>         instructions.  Fix formatting of debug message.
>>         * i386-tdep.c (i386_relocate_instruction): Ditto.

Ok.  Thanks.

Tom

Re: [PATCH] Fix instruction relocation for fast tracepoints

[Pedro Alves]

On Thursday 17 March 2011 19:29:31, Kwok Cheung Yeung wrote:
> Hello
> 
> There is a problem with the instruction relocation facility in GDB, which is
> triggered when fast tracepoints are set on call or jump instructions.  The third
> and fourth arguments of calls to store_signed_integer (defined in findvar.c) in
> the i386 and amd64 implementations of relocate_instruction are the wrong way
> around, which results in a relative jump one byte forward.  In the context of
> fast tracepoints, this results in a jump to the address portion of another jump
> instruction, which typically causes the program to crash when the tracepoint is
> reached.
> 
> This patch fixes the ordering of the arguments to store_signed_integer.  It also
> adds debug messages to the case where a call instruction is relocated (identical
> to the ones for jump instructions), and tidies up the presentation of the
> messages by removing the duplicate '0x0x' prefix of hexadecimal numbers (since
> hex_string and paddress already output the prefix).

Thanks!  This is okay for both mainline and the 7.2 branch.  We should get
you cvs write access so you can apply your own patches.  Follow the instructions
at http://sourceware.org/, and mention me as approver.  Then add yourself
in the Write After Approval section of gdb/MAINTAINERS.

> 
> Kwok Cheung Yeung
> 
> 
> 2011-03-17  Kwok Cheung Yeung  <kcy@codesourcery.com>
> 
>         * amd64-tdep.c (amd64_relocate_instruction): Fix ordering of arguments
>         to store_signed_integer.  Add debug message when relocating CALL
>         instructions.  Fix formatting of debug message.
>         * i386-tdep.c (i386_relocate_instruction): Ditto.
> 
> --- gdb-7.2/gdb/amd64-tdep.c	2010-05-26 19:19:26.000000000 +0100
> +++ gdb-7.2.new/gdb/amd64-tdep.c	2011-03-17 18:28:11.390662621 +0000
> @@ -1588,7 +1588,14 @@ amd64_relocate_instruction (struct gdbar
>        /* Adjust the destination offset.  */
>        rel32 = extract_signed_integer (insn + 1, 4, byte_order);
>        newrel = (oldloc - *to) + rel32;
> -      store_signed_integer (insn + 1, 4, newrel, byte_order);
> +      store_signed_integer (insn + 1, 4, byte_order, newrel);
> +
> +      if (debug_displaced)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "Adjusted insn rel32=%s at %s to"
> +			    " rel32=%s at %s\n",
> +			    hex_string (rel32), paddress (gdbarch, oldloc),
> +			    hex_string (newrel), paddress (gdbarch, *to));
> 
>        /* Write the adjusted jump into its displaced location.  */
>        append_insns (to, 5, insn);
> @@ -1611,11 +1618,11 @@ amd64_relocate_instruction (struct gdbar
>      {
>        rel32 = extract_signed_integer (insn + offset, 4, byte_order);
>        newrel = (oldloc - *to) + rel32;
> -      store_signed_integer (insn + offset, 4, newrel, byte_order);
> +      store_signed_integer (insn + offset, 4, byte_order, newrel);
>        if (debug_displaced)
>  	fprintf_unfiltered (gdb_stdlog,
> -			    "Adjusted insn rel32=0x%s at 0x%s to"
> -			    " rel32=0x%s at 0x%s\n",
> +			    "Adjusted insn rel32=%s at %s to"
> +			    " rel32=%s at %s\n",
>  			    hex_string (rel32), paddress (gdbarch, oldloc),
>  			    hex_string (newrel), paddress (gdbarch, *to));
>      }
> --- gdb-7.2/gdb/i386-tdep.c	2010-06-22 03:15:45.000000000 +0100
> +++ gdb-7.2.new/gdb/i386-tdep.c	2011-03-17 18:28:11.398608604 +0000
> @@ -747,7 +747,14 @@ i386_relocate_instruction (struct gdbarc
>        /* Adjust the destination offset.  */
>        rel32 = extract_signed_integer (insn + 1, 4, byte_order);
>        newrel = (oldloc - *to) + rel32;
> -      store_signed_integer (insn + 1, 4, newrel, byte_order);
> +      store_signed_integer (insn + 1, 4, byte_order, newrel);
> +
> +      if (debug_displaced)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "Adjusted insn rel32=%s at %s to"
> +			    " rel32=%s at %s\n",
> +			    hex_string (rel32), paddress (gdbarch, oldloc),
> +			    hex_string (newrel), paddress (gdbarch, *to));
> 
>        /* Write the adjusted jump into its displaced location.  */
>        append_insns (to, 5, insn);
> @@ -766,11 +773,11 @@ i386_relocate_instruction (struct gdbarc
>      {
>        rel32 = extract_signed_integer (insn + offset, 4, byte_order);
>        newrel = (oldloc - *to) + rel32;
> -      store_signed_integer (insn + offset, 4, newrel, byte_order);
> +      store_signed_integer (insn + offset, 4, byte_order, newrel);
>        if (debug_displaced)
>  	fprintf_unfiltered (gdb_stdlog,
> -			    "Adjusted insn rel32=0x%s at 0x%s to"
> -			    " rel32=0x%s at 0x%s\n",
> +			    "Adjusted insn rel32=%s at %s to"
> +			    " rel32=%s at %s\n",
>  			    hex_string (rel32), paddress (gdbarch, oldloc),
>  			    hex_string (newrel), paddress (gdbarch, *to));
>      }
> 

-- 
Pedro Alves