gdb/reverse: Fix recording vmov[u|a]p[s|d] instructions

Message ID 20250116123640.930813-1-guinevere@redhat.com
State New
Headers
Series gdb/reverse: Fix recording vmov[u|a]p[s|d] instructions |

Checks

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

Commit Message

Guinevere Larsen Jan. 16, 2025, 12:36 p.m. UTC
  Tromey reported that some of the test for the vmov[u|a]p[s|d] was
failing. In my machine xmm3 was consistently set to 0x54, but apparently
that is different depending on the system. This commit zeroes out xmm3
at the start of the test instead.

While debugging the test failures, I also noticed an issue where the
recording wasn't saving all the required memory. That happened because
vmovs[s|d] shares its opcode with vmovap[s|d], meaning they seem to
share code paths, but the latter encodes memory modification size on
VEX.L whereas the former encodes in VEX.pp. So this commit fixed that,
and made the relevant tests more robust and complete

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32561
---
 gdb/i386-tdep.c                               | 13 +++++++++----
 gdb/testsuite/gdb.reverse/i386-avx-reverse.c  | 17 +++++++++--------
 .../gdb.reverse/i386-avx-reverse.exp          | 19 +++++++++++--------
 3 files changed, 29 insertions(+), 20 deletions(-)
  

Comments

Tom de Vries Jan. 16, 2025, 12:49 p.m. UTC | #1
On 1/16/25 13:36, Guinevere Larsen wrote:
> Tromey

Hi Guinevere,

I found some nits in the commit message.

Did you mean me, or did Tromey also report this?

> reported that some of the test for the vmov[u|a]p[s|d] was

grammar: some of the tests were failing / part of the test was failing.

> failing. In my machine xmm3 was consistently set to 0x54, but apparently
> that is different depending on the system. This commit zeroes out xmm3
> at the start of the test instead.
> 
> While debugging the test failures, I also noticed an issue where the
> recording wasn't saving all the required memory. That happened because
> vmovs[s|d] shares its opcode with vmovap[s|d], meaning they seem to
> share code paths, but the latter encodes memory modification size on
> VEX.L whereas the former encodes in VEX.pp. So this commit fixed that,
> and made the relevant tests more robust and complete
>

Missing dot-at-end-of-line.

Thanks,
- Tom

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32561
> ---
>   gdb/i386-tdep.c                               | 13 +++++++++----
>   gdb/testsuite/gdb.reverse/i386-avx-reverse.c  | 17 +++++++++--------
>   .../gdb.reverse/i386-avx-reverse.exp          | 19 +++++++++++--------
>   3 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 5f585b2ca7e..219de3a6c0f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -4842,10 +4842,15 @@ i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, uint8_t vex_r,
>   	}
>         else
>   	{
> -	  /* VEX.pp stores whether we're moving a single or double precision
> -	     float.  It just happens that pp is exactly one less than
> -	     log2(size) so we can use that directly.  */
> -	  ir->ot = ir->pp;
> +	  /* Opcode 0x29 is trivial, the size of memory written is defined by
> +	     VEX.L.  Opcode 0x11 can refer to vmovs[s|d] or vmovup[s|d]; they
> +	     are differentiated by the most significant bit of VEX.pp, and the
> +	     latter works exactly like 0x29, but the former encodes the size
> +	     on VEX.pp itself.  */
> +	  if (opcode == 0x11 && (ir->pp & 2) != 0)
> +	    ir->ot = ir->pp;
> +	  else
> +	    ir->ot = 4 + ir->l;
>   	  i386_record_lea_modrm (ir);
>   	}
>         break;
> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
> index 0c26bcd8e85..815657594b0 100644
> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
> @@ -149,24 +149,24 @@ vmov_test ()
>        opcodes, meaning they'll need to be tested separately.  */
>   
>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(buf0));
> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(buf1));
> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(buf1));
>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(buf1));
> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(buf1));
> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(buf1));
>   
>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(global_buf0));
> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(global_buf1));
> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(global_buf1));
>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(global_buf1));
> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(global_buf1));
> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(global_buf1));
>   
>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(*dyn_buf0));
> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(*dyn_buf1));
> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(*dyn_buf1));
>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(*dyn_buf1));
> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(*dyn_buf1));
> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(*dyn_buf1));
>   
>     asm volatile ("vmovaps %0, %%xmm0"  : : "m"(*dyn_buf0));
> -  asm volatile ("vmovapd %0, %%xmm15" : : "m"(*dyn_buf1));
> +  asm volatile ("vmovapd %0, %%ymm15" : : "m"(*dyn_buf1));
>     asm volatile ("vmovapd %%xmm0, %0"  : : "m"(*dyn_buf1));
> -  asm volatile ("vmovaps %%xmm15, %0" : : "m"(*dyn_buf1));
> +  asm volatile ("vmovaps %%ymm15, %0" : : "m"(*dyn_buf1));
>   
>     /* We have a return statement to deal with
>        epilogue in different compilers.  */
> @@ -438,6 +438,7 @@ main ()
>     asm volatile ("vmovq %0, %%xmm0": : "m" (global_buf1));
>     asm volatile ("vmovq %0, %%xmm1": : "m" (global_buf1));
>     asm volatile ("vmovq %0, %%xmm2": : "m" (global_buf1));
> +  asm volatile ("vmovq %0, %%xmm3": : "m" (global_buf1));
>     asm volatile ("vmovq %0, %%xmm15": : "m" (global_buf1));
>   
>     vmov_test ();
> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
> index 2b2371d2cd1..f927960157e 100644
> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
> @@ -76,6 +76,8 @@ proc test_one_general_register {insn register value {prefix ""}} {
>   
>   # Shorthand to test reversing through one instruction and
>   # testing if a variable has the expected value.
> +# Value should always be the start of the stored values in the memory,
> +# not something found in the middle of it.
>   # Prefix, if used, should end with a colon and space.
>   
>   proc test_one_memory {insn mem value {dynamic false} {prefix ""}} {
> @@ -147,31 +149,32 @@ global decimal
>   if {[record_full_function "vmov"] == true} {
>       # Now execute backwards, checking all instructions.
>   
> +    # Explicitly test for the start of the array, since the value repeats.
>       test_one_memory "vmovaps" "dyn_buf1" \
> -	"0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> +	"\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>       test_one_memory "vmovapd" "dyn_buf1" \
> -	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> +	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>       test_one_register "vmovapd" "xmm15" ".*" "dynamic buffer: "
>       test_one_register "vmovaps" "xmm0" ".*" "dynamic buffer: "
>   
>       test_one_memory "vmovups" "dyn_buf1" \
> -	"0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> +	"\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>       test_one_memory "vmovupd" "dyn_buf1" \
> -	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> +	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>       test_one_register "vmovupd" "xmm15" ".*" "dynamic buffer: "
>       test_one_register "vmovups" "xmm0" ".*" "dynamic buffer: "
>   
>       test_one_memory "vmovups" "global_buf1" \
> -	"0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
> +	"\\\{0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
>       test_one_memory "vmovupd" "global_buf1" \
> -	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
> +	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
>       test_one_register "vmovupd" "xmm15" ".*" "global buffer: "
>       test_one_register "vmovups" "xmm0" ".*" "global buffer: "
>   
>       test_one_memory "vmovups" "buf1" \
> -	"0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
> +	"\\\{0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
>       test_one_memory "vmovupd" "buf1" \
> -	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
> +	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
>       test_one_register "vmovupd" "xmm15" "0xbff8000000000000" "local buffer: "
>       test_one_register "vmovups" "xmm0" "0xc004000000000000" "local buffer: "
>
  
Tom de Vries Jan. 16, 2025, 12:53 p.m. UTC | #2
On 1/16/25 13:49, Tom de Vries wrote:
> On 1/16/25 13:36, Guinevere Larsen wrote:
>> Tromey
> 
> Hi Guinevere,
> 
> I found some nits in the commit message.
> 
> Did you mean me, or did Tromey also report this?
> 
>> reported that some of the test for the vmov[u|a]p[s|d] was
> 
> grammar: some of the tests were failing / part of the test was failing.
> 
>> failing. In my machine xmm3 was consistently set to 0x54, but apparently
>> that is different depending on the system. This commit zeroes out xmm3
>> at the start of the test instead.
>>
>> While debugging the test failures, I also noticed an issue where the
>> recording wasn't saving all the required memory. That happened because
>> vmovs[s|d] shares its opcode with vmovap[s|d], meaning they seem to
>> share code paths, but the latter encodes memory modification size on
>> VEX.L whereas the former encodes in VEX.pp. So this commit fixed that,
>> and made the relevant tests more robust and complete
>>
> 
> Missing dot-at-end-of-line.
> 

I forgot to mention: with the patch applied, the test-case passes for me.

Thanks,
- Tom

>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32561
>> ---
>>   gdb/i386-tdep.c                               | 13 +++++++++----
>>   gdb/testsuite/gdb.reverse/i386-avx-reverse.c  | 17 +++++++++--------
>>   .../gdb.reverse/i386-avx-reverse.exp          | 19 +++++++++++--------
>>   3 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index 5f585b2ca7e..219de3a6c0f 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -4842,10 +4842,15 @@ i386_record_vex (struct i386_record_s *ir, 
>> uint8_t vex_w, uint8_t vex_r,
>>       }
>>         else
>>       {
>> -      /* VEX.pp stores whether we're moving a single or double precision
>> -         float.  It just happens that pp is exactly one less than
>> -         log2(size) so we can use that directly.  */
>> -      ir->ot = ir->pp;
>> +      /* Opcode 0x29 is trivial, the size of memory written is 
>> defined by
>> +         VEX.L.  Opcode 0x11 can refer to vmovs[s|d] or vmovup[s|d]; 
>> they
>> +         are differentiated by the most significant bit of VEX.pp, 
>> and the
>> +         latter works exactly like 0x29, but the former encodes the size
>> +         on VEX.pp itself.  */
>> +      if (opcode == 0x11 && (ir->pp & 2) != 0)
>> +        ir->ot = ir->pp;
>> +      else
>> +        ir->ot = 4 + ir->l;
>>         i386_record_lea_modrm (ir);
>>       }
>>         break;
>> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c b/gdb/ 
>> testsuite/gdb.reverse/i386-avx-reverse.c
>> index 0c26bcd8e85..815657594b0 100644
>> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
>> @@ -149,24 +149,24 @@ vmov_test ()
>>        opcodes, meaning they'll need to be tested separately.  */
>>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(buf0));
>> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(buf1));
>> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(buf1));
>>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(buf1));
>> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(buf1));
>> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(buf1));
>>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(global_buf0));
>> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(global_buf1));
>> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(global_buf1));
>>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(global_buf1));
>> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(global_buf1));
>> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(global_buf1));
>>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(*dyn_buf0));
>> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(*dyn_buf1));
>> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(*dyn_buf1));
>>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(*dyn_buf1));
>> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(*dyn_buf1));
>> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(*dyn_buf1));
>>     asm volatile ("vmovaps %0, %%xmm0"  : : "m"(*dyn_buf0));
>> -  asm volatile ("vmovapd %0, %%xmm15" : : "m"(*dyn_buf1));
>> +  asm volatile ("vmovapd %0, %%ymm15" : : "m"(*dyn_buf1));
>>     asm volatile ("vmovapd %%xmm0, %0"  : : "m"(*dyn_buf1));
>> -  asm volatile ("vmovaps %%xmm15, %0" : : "m"(*dyn_buf1));
>> +  asm volatile ("vmovaps %%ymm15, %0" : : "m"(*dyn_buf1));
>>     /* We have a return statement to deal with
>>        epilogue in different compilers.  */
>> @@ -438,6 +438,7 @@ main ()
>>     asm volatile ("vmovq %0, %%xmm0": : "m" (global_buf1));
>>     asm volatile ("vmovq %0, %%xmm1": : "m" (global_buf1));
>>     asm volatile ("vmovq %0, %%xmm2": : "m" (global_buf1));
>> +  asm volatile ("vmovq %0, %%xmm3": : "m" (global_buf1));
>>     asm volatile ("vmovq %0, %%xmm15": : "m" (global_buf1));
>>     vmov_test ();
>> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp b/gdb/ 
>> testsuite/gdb.reverse/i386-avx-reverse.exp
>> index 2b2371d2cd1..f927960157e 100644
>> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
>> @@ -76,6 +76,8 @@ proc test_one_general_register {insn register value 
>> {prefix ""}} {
>>   # Shorthand to test reversing through one instruction and
>>   # testing if a variable has the expected value.
>> +# Value should always be the start of the stored values in the memory,
>> +# not something found in the middle of it.
>>   # Prefix, if used, should end with a colon and space.
>>   proc test_one_memory {insn mem value {dynamic false} {prefix ""}} {
>> @@ -147,31 +149,32 @@ global decimal
>>   if {[record_full_function "vmov"] == true} {
>>       # Now execute backwards, checking all instructions.
>> +    # Explicitly test for the start of the array, since the value 
>> repeats.
>>       test_one_memory "vmovaps" "dyn_buf1" \
>> -    "0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>> +    "\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>>       test_one_memory "vmovapd" "dyn_buf1" \
>> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>>       test_one_register "vmovapd" "xmm15" ".*" "dynamic buffer: "
>>       test_one_register "vmovaps" "xmm0" ".*" "dynamic buffer: "
>>       test_one_memory "vmovups" "dyn_buf1" \
>> -    "0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>> +    "\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
>>       test_one_memory "vmovupd" "dyn_buf1" \
>> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
>>       test_one_register "vmovupd" "xmm15" ".*" "dynamic buffer: "
>>       test_one_register "vmovups" "xmm0" ".*" "dynamic buffer: "
>>       test_one_memory "vmovups" "global_buf1" \
>> -    "0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
>> +    "\\\{0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
>>       test_one_memory "vmovupd" "global_buf1" \
>> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
>> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
>>       test_one_register "vmovupd" "xmm15" ".*" "global buffer: "
>>       test_one_register "vmovups" "xmm0" ".*" "global buffer: "
>>       test_one_memory "vmovups" "buf1" \
>> -    "0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
>> +    "\\\{0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
>>       test_one_memory "vmovupd" "buf1" \
>> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
>> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
>>       test_one_register "vmovupd" "xmm15" "0xbff8000000000000" "local 
>> buffer: "
>>       test_one_register "vmovups" "xmm0" "0xc004000000000000" "local 
>> buffer: "
>
  
Guinevere Larsen Jan. 16, 2025, 12:58 p.m. UTC | #3
On 1/16/25 9:49 AM, Tom de Vries wrote:
> On 1/16/25 13:36, Guinevere Larsen wrote:
>> Tromey
>
> Hi Guinevere,
>
> I found some nits in the commit message.
>
> Did you mean me, or did Tromey also report this?
D'oh sorry, that's what I get for thinking "Tom" and writing the commit 
message in a rush late yesterday. I meant you and the first name 
collision screwed me up, sorry.
>
>> reported that some of the test for the vmov[u|a]p[s|d] was
>
> grammar: some of the tests were failing / part of the test was failing.
oops, fixed, thanks!
>
>> failing. In my machine xmm3 was consistently set to 0x54, but apparently
>> that is different depending on the system. This commit zeroes out xmm3
>> at the start of the test instead.
>>
>> While debugging the test failures, I also noticed an issue where the
>> recording wasn't saving all the required memory. That happened because
>> vmovs[s|d] shares its opcode with vmovap[s|d], meaning they seem to
>> share code paths, but the latter encodes memory modification size on
>> VEX.L whereas the former encodes in VEX.pp. So this commit fixed that,
>> and made the relevant tests more robust and complete
>>
>
> Missing dot-at-end-of-line.

Fixed!

I'll wait a bit before pushing, just in case anyone has any comments.
  
Schimpe, Christina Jan. 16, 2025, 1:57 p.m. UTC | #4
Hi Guinevere, 

I just wanted to share that I saw the same failures Tom reported:

FAIL: gdb.reverse/i386-avx-reverse.exp: verify dyn_buf1 before vmovapd
FAIL: gdb.reverse/i386-avx-reverse.exp: verify dyn_buf1 before vmovupd
FAIL: gdb.reverse/i386-avx-reverse.exp: verify global_buf1 before vmovupd
FAIL: gdb.reverse/i386-avx-reverse.exp: verify buf1 before vmovupd

and can confirm that they are fixed with this patch on my machine as well.

Christina

> -----Original Message-----
> From: Tom de Vries <tdevries@suse.de>
> Sent: Thursday, January 16, 2025 1:53 PM
> To: Guinevere Larsen <guinevere@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb/reverse: Fix recording vmov[u|a]p[s|d] instructions
> 
> On 1/16/25 13:49, Tom de Vries wrote:
> > On 1/16/25 13:36, Guinevere Larsen wrote:
> >> Tromey
> >
> > Hi Guinevere,
> >
> > I found some nits in the commit message.
> >
> > Did you mean me, or did Tromey also report this?
> >
> >> reported that some of the test for the vmov[u|a]p[s|d] was
> >
> > grammar: some of the tests were failing / part of the test was failing.
> >
> >> failing. In my machine xmm3 was consistently set to 0x54, but
> >> apparently that is different depending on the system. This commit
> >> zeroes out xmm3 at the start of the test instead.
> >>
> >> While debugging the test failures, I also noticed an issue where the
> >> recording wasn't saving all the required memory. That happened
> >> because vmovs[s|d] shares its opcode with vmovap[s|d], meaning they
> >> seem to share code paths, but the latter encodes memory modification
> >> size on VEX.L whereas the former encodes in VEX.pp. So this commit
> >> fixed that, and made the relevant tests more robust and complete
> >>
> >
> > Missing dot-at-end-of-line.
> >
> 
> I forgot to mention: with the patch applied, the test-case passes for me.
> 
> Thanks,
> - Tom
> 
> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32561
> >> ---
> >>   gdb/i386-tdep.c                               | 13 +++++++++----
> >>   gdb/testsuite/gdb.reverse/i386-avx-reverse.c  | 17
> >> +++++++++--------
> >>   .../gdb.reverse/i386-avx-reverse.exp          | 19
> >> +++++++++++--------
> >>   3 files changed, 29 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index
> >> 5f585b2ca7e..219de3a6c0f 100644
> >> --- a/gdb/i386-tdep.c
> >> +++ b/gdb/i386-tdep.c
> >> @@ -4842,10 +4842,15 @@ i386_record_vex (struct i386_record_s *ir,
> >> uint8_t vex_w, uint8_t vex_r,
> >>       }
> >>         else
> >>       {
> >> -      /* VEX.pp stores whether we're moving a single or double
> >> precision
> >> -         float.  It just happens that pp is exactly one less than
> >> -         log2(size) so we can use that directly.  */
> >> -      ir->ot = ir->pp;
> >> +      /* Opcode 0x29 is trivial, the size of memory written is
> >> defined by
> >> +         VEX.L.  Opcode 0x11 can refer to vmovs[s|d] or vmovup[s|d];
> >> they
> >> +         are differentiated by the most significant bit of VEX.pp,
> >> and the
> >> +         latter works exactly like 0x29, but the former encodes the
> >> +size
> >> +         on VEX.pp itself.  */
> >> +      if (opcode == 0x11 && (ir->pp & 2) != 0)
> >> +        ir->ot = ir->pp;
> >> +      else
> >> +        ir->ot = 4 + ir->l;
> >>         i386_record_lea_modrm (ir);
> >>       }
> >>         break;
> >> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c b/gdb/
> >> testsuite/gdb.reverse/i386-avx-reverse.c
> >> index 0c26bcd8e85..815657594b0 100644
> >> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
> >> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
> >> @@ -149,24 +149,24 @@ vmov_test ()
> >>        opcodes, meaning they'll need to be tested separately.  */
> >>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(buf0));
> >> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(buf1));
> >> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(buf1));
> >>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(buf1));
> >> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(buf1));
> >> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(buf1));
> >>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(global_buf0));
> >> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(global_buf1));
> >> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(global_buf1));
> >>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(global_buf1));
> >> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(global_buf1));
> >> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(global_buf1));
> >>     asm volatile ("vmovups %0, %%xmm0"  : : "m"(*dyn_buf0));
> >> -  asm volatile ("vmovupd %0, %%xmm15" : : "m"(*dyn_buf1));
> >> +  asm volatile ("vmovupd %0, %%ymm15" : : "m"(*dyn_buf1));
> >>     asm volatile ("vmovupd %%xmm0, %0"  : : "m"(*dyn_buf1));
> >> -  asm volatile ("vmovups %%xmm15, %0" : : "m"(*dyn_buf1));
> >> +  asm volatile ("vmovups %%ymm15, %0" : : "m"(*dyn_buf1));
> >>     asm volatile ("vmovaps %0, %%xmm0"  : : "m"(*dyn_buf0));
> >> -  asm volatile ("vmovapd %0, %%xmm15" : : "m"(*dyn_buf1));
> >> +  asm volatile ("vmovapd %0, %%ymm15" : : "m"(*dyn_buf1));
> >>     asm volatile ("vmovapd %%xmm0, %0"  : : "m"(*dyn_buf1));
> >> -  asm volatile ("vmovaps %%xmm15, %0" : : "m"(*dyn_buf1));
> >> +  asm volatile ("vmovaps %%ymm15, %0" : : "m"(*dyn_buf1));
> >>     /* We have a return statement to deal with
> >>        epilogue in different compilers.  */ @@ -438,6 +438,7 @@ main
> >> ()
> >>     asm volatile ("vmovq %0, %%xmm0": : "m" (global_buf1));
> >>     asm volatile ("vmovq %0, %%xmm1": : "m" (global_buf1));
> >>     asm volatile ("vmovq %0, %%xmm2": : "m" (global_buf1));
> >> +  asm volatile ("vmovq %0, %%xmm3": : "m" (global_buf1));
> >>     asm volatile ("vmovq %0, %%xmm15": : "m" (global_buf1));
> >>     vmov_test ();
> >> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp b/gdb/
> >> testsuite/gdb.reverse/i386-avx-reverse.exp
> >> index 2b2371d2cd1..f927960157e 100644
> >> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
> >> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
> >> @@ -76,6 +76,8 @@ proc test_one_general_register {insn register value
> >> {prefix ""}} {
> >>   # Shorthand to test reversing through one instruction and
> >>   # testing if a variable has the expected value.
> >> +# Value should always be the start of the stored values in the
> >> +memory, # not something found in the middle of it.
> >>   # Prefix, if used, should end with a colon and space.
> >>   proc test_one_memory {insn mem value {dynamic false} {prefix ""}} {
> >> @@ -147,31 +149,32 @@ global decimal
> >>   if {[record_full_function "vmov"] == true} {
> >>       # Now execute backwards, checking all instructions.
> >> +    # Explicitly test for the start of the array, since the value
> >> repeats.
> >>       test_one_memory "vmovaps" "dyn_buf1" \
> >> -    "0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> >> +    "\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> >>       test_one_memory "vmovapd" "dyn_buf1" \
> >> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> >> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> >>       test_one_register "vmovapd" "xmm15" ".*" "dynamic buffer: "
> >>       test_one_register "vmovaps" "xmm0" ".*" "dynamic buffer: "
> >>       test_one_memory "vmovups" "dyn_buf1" \
> >> -    "0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> >> +    "\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
> >>       test_one_memory "vmovupd" "dyn_buf1" \
> >> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> >> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
> >>       test_one_register "vmovupd" "xmm15" ".*" "dynamic buffer: "
> >>       test_one_register "vmovups" "xmm0" ".*" "dynamic buffer: "
> >>       test_one_memory "vmovups" "global_buf1" \
> >> -    "0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
> >> +    "\\\{0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
> >>       test_one_memory "vmovupd" "global_buf1" \
> >> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
> >> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
> >>       test_one_register "vmovupd" "xmm15" ".*" "global buffer: "
> >>       test_one_register "vmovups" "xmm0" ".*" "global buffer: "
> >>       test_one_memory "vmovups" "buf1" \
> >> -    "0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
> >> +    "\\\{0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
> >>       test_one_memory "vmovupd" "buf1" \
> >> -    "0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
> >> +    "\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
> >>       test_one_register "vmovupd" "xmm15" "0xbff8000000000000" "local
> >> buffer: "
> >>       test_one_register "vmovups" "xmm0" "0xc004000000000000" "local
> >> buffer: "
> >

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  
Guinevere Larsen Jan. 17, 2025, 6:46 p.m. UTC | #5
Thanks Christina and Tom,

I went ahead and pushed this patch.
  

Patch

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 5f585b2ca7e..219de3a6c0f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -4842,10 +4842,15 @@  i386_record_vex (struct i386_record_s *ir, uint8_t vex_w, uint8_t vex_r,
 	}
       else
 	{
-	  /* VEX.pp stores whether we're moving a single or double precision
-	     float.  It just happens that pp is exactly one less than
-	     log2(size) so we can use that directly.  */
-	  ir->ot = ir->pp;
+	  /* Opcode 0x29 is trivial, the size of memory written is defined by
+	     VEX.L.  Opcode 0x11 can refer to vmovs[s|d] or vmovup[s|d]; they
+	     are differentiated by the most significant bit of VEX.pp, and the
+	     latter works exactly like 0x29, but the former encodes the size
+	     on VEX.pp itself.  */
+	  if (opcode == 0x11 && (ir->pp & 2) != 0)
+	    ir->ot = ir->pp;
+	  else
+	    ir->ot = 4 + ir->l;
 	  i386_record_lea_modrm (ir);
 	}
       break;
diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
index 0c26bcd8e85..815657594b0 100644
--- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
+++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
@@ -149,24 +149,24 @@  vmov_test ()
      opcodes, meaning they'll need to be tested separately.  */
 
   asm volatile ("vmovups %0, %%xmm0"  : : "m"(buf0));
-  asm volatile ("vmovupd %0, %%xmm15" : : "m"(buf1));
+  asm volatile ("vmovupd %0, %%ymm15" : : "m"(buf1));
   asm volatile ("vmovupd %%xmm0, %0"  : : "m"(buf1));
-  asm volatile ("vmovups %%xmm15, %0" : : "m"(buf1));
+  asm volatile ("vmovups %%ymm15, %0" : : "m"(buf1));
 
   asm volatile ("vmovups %0, %%xmm0"  : : "m"(global_buf0));
-  asm volatile ("vmovupd %0, %%xmm15" : : "m"(global_buf1));
+  asm volatile ("vmovupd %0, %%ymm15" : : "m"(global_buf1));
   asm volatile ("vmovupd %%xmm0, %0"  : : "m"(global_buf1));
-  asm volatile ("vmovups %%xmm15, %0" : : "m"(global_buf1));
+  asm volatile ("vmovups %%ymm15, %0" : : "m"(global_buf1));
 
   asm volatile ("vmovups %0, %%xmm0"  : : "m"(*dyn_buf0));
-  asm volatile ("vmovupd %0, %%xmm15" : : "m"(*dyn_buf1));
+  asm volatile ("vmovupd %0, %%ymm15" : : "m"(*dyn_buf1));
   asm volatile ("vmovupd %%xmm0, %0"  : : "m"(*dyn_buf1));
-  asm volatile ("vmovups %%xmm15, %0" : : "m"(*dyn_buf1));
+  asm volatile ("vmovups %%ymm15, %0" : : "m"(*dyn_buf1));
 
   asm volatile ("vmovaps %0, %%xmm0"  : : "m"(*dyn_buf0));
-  asm volatile ("vmovapd %0, %%xmm15" : : "m"(*dyn_buf1));
+  asm volatile ("vmovapd %0, %%ymm15" : : "m"(*dyn_buf1));
   asm volatile ("vmovapd %%xmm0, %0"  : : "m"(*dyn_buf1));
-  asm volatile ("vmovaps %%xmm15, %0" : : "m"(*dyn_buf1));
+  asm volatile ("vmovaps %%ymm15, %0" : : "m"(*dyn_buf1));
 
   /* We have a return statement to deal with
      epilogue in different compilers.  */
@@ -438,6 +438,7 @@  main ()
   asm volatile ("vmovq %0, %%xmm0": : "m" (global_buf1));
   asm volatile ("vmovq %0, %%xmm1": : "m" (global_buf1));
   asm volatile ("vmovq %0, %%xmm2": : "m" (global_buf1));
+  asm volatile ("vmovq %0, %%xmm3": : "m" (global_buf1));
   asm volatile ("vmovq %0, %%xmm15": : "m" (global_buf1));
 
   vmov_test ();
diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
index 2b2371d2cd1..f927960157e 100644
--- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
@@ -76,6 +76,8 @@  proc test_one_general_register {insn register value {prefix ""}} {
 
 # Shorthand to test reversing through one instruction and
 # testing if a variable has the expected value.
+# Value should always be the start of the stored values in the memory,
+# not something found in the middle of it.
 # Prefix, if used, should end with a colon and space.
 
 proc test_one_memory {insn mem value {dynamic false} {prefix ""}} {
@@ -147,31 +149,32 @@  global decimal
 if {[record_full_function "vmov"] == true} {
     # Now execute backwards, checking all instructions.
 
+    # Explicitly test for the start of the array, since the value repeats.
     test_one_memory "vmovaps" "dyn_buf1" \
-	"0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
+	"\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
     test_one_memory "vmovapd" "dyn_buf1" \
-	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
+	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
     test_one_register "vmovapd" "xmm15" ".*" "dynamic buffer: "
     test_one_register "vmovaps" "xmm0" ".*" "dynamic buffer: "
 
     test_one_memory "vmovups" "dyn_buf1" \
-	"0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
+	"\\\{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28" true
     test_one_memory "vmovupd" "dyn_buf1" \
-	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
+	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x28" true
     test_one_register "vmovupd" "xmm15" ".*" "dynamic buffer: "
     test_one_register "vmovups" "xmm0" ".*" "dynamic buffer: "
 
     test_one_memory "vmovups" "global_buf1" \
-	"0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
+	"\\\{0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18"
     test_one_memory "vmovupd" "global_buf1" \
-	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
+	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x18"
     test_one_register "vmovupd" "xmm15" ".*" "global buffer: "
     test_one_register "vmovups" "xmm0" ".*" "global buffer: "
 
     test_one_memory "vmovups" "buf1" \
-	"0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
+	"\\\{0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38"
     test_one_memory "vmovupd" "buf1" \
-	"0x54, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
+	"\\\{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x38"
     test_one_register "vmovupd" "xmm15" "0xbff8000000000000" "local buffer: "
     test_one_register "vmovups" "xmm0" "0xc004000000000000" "local buffer: "