[2/3] gdb/record: add support to vmovd and vmovq instructions

Message ID 20240521202800.2865871-3-blarsen@redhat.com
State New
Headers
Series Small step in supporting AVX instructions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Guinevere Larsen May 21, 2024, 8:27 p.m. UTC
  This commit adds support to the x86_64 AVX instructions vmovd and vmovq.
The programmers manuals for Intel and AMD describe these 2 instructions
as being almost the same, but my local testing, using gcc 13.2 on Fedora
39, showed several differences and inconsistencies.

The instruction is supposed to always use the 3-byte VEX prefix, but I
could only find 2-byte versions. The instructions aren't differentiated
by the VEX.w bit, but by opcodes and VEX.pp.

This patch adds a test with many different uses for both vmovd and
vmovq.
---
 gdb/i386-tdep.c                               |  69 ++++++-
 gdb/testsuite/gdb.reverse/i386-avx-reverse.c  |  90 +++++++++
 .../gdb.reverse/i386-avx-reverse.exp          | 171 ++++++++++++++++++
 3 files changed, 329 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.reverse/i386-avx-reverse.c
 create mode 100644 gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
  

Comments

Tom Tromey June 5, 2024, 4:13 p.m. UTC | #1
>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:

Guinevere> This commit adds support to the x86_64 AVX instructions vmovd and vmovq.
Guinevere> The programmers manuals for Intel and AMD describe these 2 instructions
Guinevere> as being almost the same, but my local testing, using gcc 13.2 on Fedora
Guinevere> 39, showed several differences and inconsistencies.

Guinevere>  static bool
Guinevere> -i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, uint8_t rex_r,
Guinevere> +i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, uint8_t vex_r,
Guinevere>  		 int opcode, struct gdbarch *gdbarch)
Guinevere>  {
...
Guinevere> +	error ("Unrecognized VEX.PP value %d at address %s.",
Guinevere> +	       ir->pp, paddress(gdbarch, ir->orig_addr));

I don't know anything about record, really, but it seems weird to have a
function that has two error return mechanisms -- either calling error or
printing a message and returning some value:

Guinevere>      default:
Guinevere>        gdb_printf (gdb_stderr,
Guinevere>  		  _("Process record does not support VEX instruction 0x%02x "

What's the deal with that?

Tom
  
Guinevere Larsen June 5, 2024, 6:24 p.m. UTC | #2
On 6/5/24 13:13, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
> Guinevere> This commit adds support to the x86_64 AVX instructions vmovd and vmovq.
> Guinevere> The programmers manuals for Intel and AMD describe these 2 instructions
> Guinevere> as being almost the same, but my local testing, using gcc 13.2 on Fedora
> Guinevere> 39, showed several differences and inconsistencies.
>
> Guinevere>  static bool
> Guinevere> -i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, uint8_t rex_r,
> Guinevere> +i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, uint8_t vex_r,
> Guinevere>  		 int opcode, struct gdbarch *gdbarch)
> Guinevere>  {
> ...
> Guinevere> +	error ("Unrecognized VEX.PP value %d at address %s.",
> Guinevere> +	       ir->pp, paddress(gdbarch, ir->orig_addr));
>
> I don't know anything about record, really, but it seems weird to have a
> function that has two error return mechanisms -- either calling error or
> printing a message and returning some value:
>
> Guinevere>      default:
> Guinevere>        gdb_printf (gdb_stderr,
> Guinevere>  		  _("Process record does not support VEX instruction 0x%02x "
>
> What's the deal with that?

Christmas vacation in between, mostly.

gdb_printf is what i386_process_record already uses, so felt reasonable, 
then I chose "error" int the other situation because it felt more 
appropriate for me to have a strong error when we find something that, 
to the best of my knowledge, breaks the ISA. To me it marks a difference 
between "we're not equipped to handle" versus "up to most recent ISA, 
this is straight up incorrect", but if you think it's better to only use 
one error mechanism, I can switch to gdb_printf everywhere.
  
Guinevere Larsen June 5, 2024, 7:53 p.m. UTC | #3
On 6/5/24 15:24, Guinevere Larsen wrote:
> On 6/5/24 13:13, Tom Tromey wrote:
>>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
>> Guinevere> This commit adds support to the x86_64 AVX instructions 
>> vmovd and vmovq.
>> Guinevere> The programmers manuals for Intel and AMD describe these 2 
>> instructions
>> Guinevere> as being almost the same, but my local testing, using gcc 
>> 13.2 on Fedora
>> Guinevere> 39, showed several differences and inconsistencies.
>>
>> Guinevere>  static bool
>> Guinevere> -i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, 
>> uint8_t rex_r,
>> Guinevere> +i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, 
>> uint8_t vex_r,
>> Guinevere>           int opcode, struct gdbarch *gdbarch)
>> Guinevere>  {
>> ...
>> Guinevere> +    error ("Unrecognized VEX.PP value %d at address %s.",
>> Guinevere> +           ir->pp, paddress(gdbarch, ir->orig_addr));
>>
>> I don't know anything about record, really, but it seems weird to have a
>> function that has two error return mechanisms -- either calling error or
>> printing a message and returning some value:
>>
>> Guinevere>      default:
>> Guinevere>        gdb_printf (gdb_stderr,
>> Guinevere>            _("Process record does not support VEX 
>> instruction 0x%02x "
>>
>> What's the deal with that?
>
> Christmas vacation in between, mostly.
>
> gdb_printf is what i386_process_record already uses, so felt 
> reasonable, then I chose "error" int the other situation because it 
> felt more appropriate for me to have a strong error when we find 
> something that, to the best of my knowledge, breaks the ISA. To me it 
> marks a difference between "we're not equipped to handle" versus "up 
> to most recent ISA, this is straight up incorrect", but if you think 
> it's better to only use one error mechanism, I can switch to 
> gdb_printf everywhere.
>
Interestingly enough, I just double checked what happens, and for the 
end user, the gdb_printf option looks stronger because gdbarch will add

 > Process record: failed to record execution log.

and error will not print anything but the message given. I'll 
standardize on the gdb_printf version instead.  Thanks for the pointer!
  

Patch

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 93a0926c4bc..d2848970ec4 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -4985,11 +4985,78 @@  static int i386_record_floats (struct gdbarch *gdbarch,
    with VEX prefix.  */
 
 static bool
-i386_record_vex (struct i386_record_s *ir, uint8_t rex_w, uint8_t rex_r,
+i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, uint8_t vex_r,
 		 int opcode, struct gdbarch *gdbarch)
 {
   switch (opcode)
     {
+    case 0x6e:	/* VMOVD XMM, reg/mem  */
+      /* This is moving from a regular register or memory region into an
+	 XMM register. */
+      i386_record_modrm (ir);
+      /* ModR/M only has the 3 least significant bits of the destination
+	 register, the last one is indicated by VEX.R (stored inverted).  */
+      record_full_arch_list_add_reg (ir->regcache,
+				     ir->regmap[X86_RECORD_XMM0_REGNUM]
+				       + ir->reg + vex_r * 8);
+      break;
+    case 0x7e:	/* VMOV(D/Q)  */
+      i386_record_modrm (ir);
+      /* Both the intel and AMD manual are wrong about this.  According to
+	 it, the only difference between vmovq and vmovd should be the rex_w
+	 bit, but in empirical testing, it seems that they share this opcode,
+	 and the way to differentiate it here is looking at VEX.PP.  */
+      if (ir->pp == 2)
+	{
+	  /* This is vmovq moving from a regular register or memory
+	     into an XMM register.  As above, VEX.R is the final bit for
+	     destination register.  */
+	  record_full_arch_list_add_reg (ir->regcache,
+					 ir->regmap[X86_RECORD_XMM0_REGNUM]
+					 + ir->reg + vex_r * 8);
+	}
+      else if (ir->pp == 1)
+	{
+	  /* This is the vmovd version that stores into a regular register
+	     or memory region.  */
+	  /* If ModRM.mod is 11 we are saving into a register.  */
+	  if (ir->mod == 3)
+	    record_full_arch_list_add_reg (ir->regcache, ir->regmap[ir->rm]);
+	  else
+	    {
+	      /* Calculate the size of memory that will be modified
+		 and store it in the form of 1 << ir->ot, since that
+		 is how the function uses it.  In theory, VEX.W is supposed
+		 to indicate the size of the memory. In practice, I only
+		 ever seen it set to 0, and for 16 bytes, 0xD6 opcode
+		 is used.  */
+	      if (vex_w)
+		ir->ot = 4;
+	      else
+		ir->ot = 3;
+
+	      i386_record_lea_modrm (ir);
+	    }
+	}
+      else
+	error ("Unrecognized VEX.PP value %d at address %s.",
+	       ir->pp, paddress(gdbarch, ir->orig_addr));
+      break;
+    case 0xd6: /* VMOVQ reg/mem XMM  */
+      i386_record_modrm (ir);
+      /* This is the vmovq version that stores into a regular register
+	 or memory region.  */
+      /* If ModRM.mod is 11 we are saving into a register.  */
+      if (ir->mod == 3)
+	record_full_arch_list_add_reg (ir->regcache, ir->regmap[ir->rm]);
+      else
+	{
+	  /* We know that this operation is always 64 bits.  */
+	  ir->ot = 4;
+	  i386_record_lea_modrm (ir);
+	}
+      break;
+
     default:
       gdb_printf (gdb_stderr,
 		  _("Process record does not support VEX instruction 0x%02x "
diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
new file mode 100644
index 00000000000..216b593736b
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
@@ -0,0 +1,90 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+/* Architecture tests for intel i386 platform.  */
+
+#include <stdlib.h>
+
+char global_buf0[] = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
+		      0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f};
+char global_buf1[] = {0, 0, 0, 0, 0, 0, 0, 0,
+		      0, 0, 0, 0, 0, 0, 0, 0};
+char *dyn_buf0;
+char *dyn_buf1;
+
+void
+vmov_test ()
+{
+  char buf0[] = {0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
+		 0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f};
+  char buf1[] = {0, 0, 0, 0, 0, 0, 0, 0,
+		 0, 0, 0, 0, 0, 0, 0, 0};
+
+  /* Operations on registers.  */
+  asm volatile ("mov $0, %rcx");
+  asm volatile ("mov $0xbeef, %rax");
+  asm volatile ("vmovd %rax, %xmm0");
+  asm volatile ("vmovd %xmm0, %rcx");
+
+  /* Operations based on local buffers.  */
+  asm volatile ("vmovd %0, %%xmm0": : "m"(buf0));
+  asm volatile ("vmovd %%xmm0, %0": "=m"(buf1));
+  asm volatile ("vmovq %0, %%xmm0": : "m"(buf0));
+  asm volatile ("vmovq %%xmm0, %0": "=m"(buf1));
+
+  /* Operations based on global buffers.  */
+  asm volatile ("vmovd %0, %%xmm0": : "m"(global_buf0));
+  asm volatile ("vmovd %%xmm0, %0": "=m"(global_buf1));
+  asm volatile ("vmovq %0, %%xmm0": : "m"(global_buf0));
+  asm volatile ("vmovq %%xmm0, %0": "=m"(global_buf1));
+
+  /* Operations based on dynamic buffers.  */
+  asm volatile ("vmovd %0, %%xmm0": : "m"(*dyn_buf0));
+  asm volatile ("vmovd %%xmm0, %0": "=m"(*dyn_buf1));
+  asm volatile ("vmovq %0, %%xmm0": : "m"(*dyn_buf0));
+  asm volatile ("vmovq %%xmm0, %0": "=m"(*dyn_buf1));
+
+  /* Reset all relevant buffers. */
+  asm volatile ("vmovq %%xmm15, %0": "=m" (buf1));
+  asm volatile ("vmovq %%xmm15, %0": "=m" (global_buf1));
+  asm volatile ("vmovq %%xmm15, %0": "=m" (*dyn_buf1));
+
+  /* Quick test for a different xmm register.  */
+  asm volatile ("vmovd %0, %%xmm15": "=m" (buf0));
+  asm volatile ("vmovd %0, %%xmm15": "=m" (buf1));
+  asm volatile ("vmovq %0, %%xmm15": "=m" (buf0));
+  asm volatile ("vmovq %0, %%xmm15": "=m" (buf1));
+} /* end vmov_test */
+
+int
+main ()
+{
+  dyn_buf0 = (char *) malloc(sizeof(char) * 16);
+  dyn_buf1 = (char *) malloc(sizeof(char) * 16);
+  for (int i =0; i < 16; i++)
+    {
+      dyn_buf0[i] = 0x20 + i;
+      dyn_buf1[i] = 0;
+    }
+  /* Zero relevant xmm registers, se we know what to look for.  */
+  asm volatile ("vmovq %0, %%xmm0": : "m" (global_buf1));
+  asm volatile ("vmovq %0, %%xmm15": : "m" (global_buf1));
+
+  /* Start recording. */
+  vmov_test ();
+  return 0;	/* end of main */
+}
diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
new file mode 100644
index 00000000000..42ddc3a6526
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
@@ -0,0 +1,171 @@ 
+# Copyright 2009-2023 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/>.
+
+# This file is part of the gdb testsuite.
+
+#
+# This test tests some i386 general instructions for reverse execution.
+#
+
+require supports_reverse
+
+if {![istarget "*86*-*linux*"]} {
+    verbose "Skipping i386 reverse tests."
+    return
+}
+
+standard_testfile
+
+# some targets have leading underscores on assembly symbols.
+set additional_flags [gdb_target_symbol_prefix_flags]
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 [list debug $additional_flags]]} {
+    return -1
+}
+
+# Shorthand to test reversing through one instruction and
+# testing if a register has the expected value.
+# Prefix, if included, should end with a colon and space.
+
+proc test_one_register {insn register value {prefix ""}} {
+    gdb_test "reverse-step" "$insn.*" \
+	"${prefix}reverse-step from $insn to test register $register"
+
+    gdb_test "info register $register" \
+	"$register.*uint128 = $value.*" \
+	"${prefix}verify $register before $insn"
+}
+
+# Shorthand to test reversing through one instruction and
+# testing if a variable has the expected value.
+# Prefix, if used, should end with a colon and space.
+
+proc test_one_memory {insn mem value {dynamic false} {prefix ""}} {
+    gdb_test "reverse-step" "$insn.*" \
+	"${prefix}reverse-step from $insn to test memory $mem"
+
+    # For the dynamic buffer, we have to cast and dereference the pointer
+    set cast ""
+    if {$dynamic == true} {
+	set cast {(char [16]) *}
+    }
+
+    gdb_test "p/x $cast$mem" \
+	".*$value.*" \
+	"${prefix}verify $mem before $insn"
+
+}
+
+# Record the execution for the whole function, and stop at its end
+# to check if we can correctly reconstruct the state.
+# In the source code, the function must be FUNCTION_test, and
+# at the end, it must have a comment in the form:
+# /* end FUNCTION_test */
+# Returns true if the full function could be recorded, false otherwise.
+proc record_full_function {function} {
+    set end [gdb_get_line_number "end ${function}_test "]
+    gdb_breakpoint $end temporary
+
+    if [supports_process_record] {
+	# Activate process record/replay.
+	gdb_test_no_output "record" "${function}: turn on process record"
+    }
+
+    gdb_test_multiple "continue" "continue to end of ${function}_test" {
+	-re " end ${function}_test .*\r\n$::gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+	-re " Illegal instruction.*\r\n$::gdb_prompt $" {
+	    fail $gdb_test_name
+	    return false
+	}
+	-re "Process record does not support VEX instruction.*" {
+	    fail $gdb_test_name
+	    return false
+	}
+    }
+    return true
+}
+
+set end_of_main          [gdb_get_line_number " end of main "]
+set rec_start            [gdb_get_line_number " Start recording"]
+
+runto_main
+
+gdb_breakpoint $rec_start
+gdb_continue_to_breakpoint "vmov_test" ".*vmov_test.*"
+
+global hex
+global decimal
+
+# Record all the execution for vmov tests first.
+
+if {[record_full_function "vmov"] == true} {
+    # Now execute backwards, checking all instructions.
+    # First we test all instructions handling global buffers.
+
+    test_one_register "vmovq" "xmm15" "0x3736353433323130" "reg_reset: "
+    test_one_register "vmovq" "xmm15" "0x0"
+    test_one_register "vmovd" "xmm15" "0x33323130" "reg_reset: "
+    test_one_register "vmovd" "xmm15" "0x0"
+
+    with_test_prefix buffer_reset {
+	test_one_memory "vmovq" "dyn_buf1" \
+	    "0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x0" true
+	test_one_memory "vmovq" "global_buf1" \
+	    "0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x0"
+	test_one_memory "vmovq" "buf1" \
+	    "0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x0"
+    }
+
+    with_test_prefix dynamic_buffers {
+	test_one_memory "vmovq" "dyn_buf1" "0x20, 0x21, 0x22, 0x23, 0x0" true
+	test_one_register "vmovq" "xmm0" "0x23222120"
+	test_one_memory "vmovd" "dyn_buf1" "0x0 .repeats 16 times" true
+	test_one_register "vmovd" "xmm0" "0x1716151413121110"
+    }
+
+    with_test_prefix global_buffers {
+	test_one_memory "vmovq" "global_buf1" "0x10, 0x11, 0x12, 0x13, 0x0"
+	test_one_register "vmovq" "xmm0" "0x13121110"
+	test_one_memory "vmovd" "global_buf1" "0x0 .repeats 16 times"
+	test_one_register "vmovd" "xmm0" "0x3736353433323130"
+    }
+
+    with_test_prefix local_buffers {
+	test_one_memory "vmovq" "buf1" "0x30, 0x31, 0x32, 0x33, 0x0"
+	test_one_register "vmovq" "xmm0" "0x33323130"
+	test_one_memory "vmovd" "buf1" "0x0 .repeats 16 times"
+	test_one_register "vmovd" "xmm0" "0xbeef"
+    }
+
+    # regular registers don't have uint128 members, so do it manually
+    with_test_prefix registers {
+	gdb_test "reverse-step" "vmovd %xmm0, %rcx.*" \
+	    "reverse step to check rcx recording"
+	gdb_test "print/x \$rcx" "= 0x0" "rcx was recorded"
+
+	test_one_register "vmovd" "xmm0" "0x0"
+    }
+} else {
+    untested "could not record vmov_test"
+}
+
+# Move to the end of vmov_test to set up next.
+# Stop recording in case of recording errors.
+gdb_test "record stop" "Process record is stopped.*" \
+    "delete history for vmov_test"
+gdb_test "finish" "Run till exit from.*vmov_test.*" "leaving vmov_test"