Re: [PATCH 1/1] [gdb/reverse] Fix failing test: i386-avx-reverse

[Guinevere Larsen <guinevere@redhat.com>]

Hi! Thank you for this patch!

I have a comments, slightly expanding Tom's reply. A v2 with updated 
commit wording should be ready to push :)
On 7/19/25 11:48 PM, Tom de Vries wrote:
> On 7/19/25 22:00, Shiven Kashyap wrote:
>> Running the standalone test `gdb.reverse` leads to the following 
>> failure:
>> '''
>> FAIL: gdb.reverse/i386-avx-reverse.exp: verify ymm15 before vbroadcastsd
>> '''
>>
>
> Hi,
>
> thanks for the patch.
>
> I first couldn't reproduce this, but then I managed using target board 
> unix/-fPIE/-pie.
>
>> This happens because the AVX broadcast instructions expect an actual 
>> value, but instead
>> it cast memory address of a buffer like so (see 'vpbroadcast_test' in 
>> 'i386-avx-reverse.c'):
>> '''
>> asm volatile ("vbroadcastss %0, %%xmm15": : "m" (dyn_buf0));
>> '''
>>
>
> This is a bit misleading, because vbroadcastss can handle memory 
> operands, not just "actual values".

I second this, the instruction doesn't expect anything, but the test 
itself expects to have the contents in the address, not the address. I 
think the issue is better described as the test expecting the stored 
value, as those are fully under our control.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> After your fix (which looks correct to me), the generated instruction 
> still accesses a memory address:
> ...
>   400abd:       48 8b 05 04 26 00 00    mov    0x2604(%rip),%rax
>                                           # 4030c8 <dyn_buf0>
>   400ac4:       c4 62 79 18 38          vbroadcastss (%rax),%xmm15
> ...
> but this time, the correct one.
>
> Thanks,
> - Tom
>
>> which lead to the test failing for the next instruction 
>> (`vbroadcastsd`), which depended on the correct value being broadcast 
>> to the register.
>> Also, updated the corresponding expected output 
>> (gdb.reverse/i386-avx-reverse.exp) to match.
>>
>> Tested on x86-64 Linux.
>>
>> Signed-off-by: Shiven Kashyap <shivenkashyap24@gmail.com>
>> ---
>>   gdb/testsuite/gdb.reverse/i386-avx-reverse.c   | 4 ++--
>>   gdb/testsuite/gdb.reverse/i386-avx-reverse.exp | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c 
>> b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
>> index a3d64272f24..bf6cb77f313 100644
>> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.c
>> @@ -299,10 +299,10 @@ vpbroadcast_test ()
>>     asm volatile ("vbroadcastss %xmm1, %xmm0");
>>     asm volatile ("vbroadcastss %xmm1, %ymm15");
>>     asm volatile ("vbroadcastss %0, %%ymm0" : : "m" (global_buf0));
>> -  asm volatile ("vbroadcastss %0, %%xmm15": : "m" (dyn_buf0));
>> +  asm volatile ("vbroadcastss %0, %%xmm15": : "m" (*dyn_buf0));
>>     asm volatile ("vbroadcastsd %xmm1, %ymm0");
>>     asm volatile ("vbroadcastsd %0, %%ymm15": : "m" (global_buf0));
>> -  asm volatile ("vbroadcastf128 %0, %%ymm0" : : "m" (dyn_buf0));
>> +  asm volatile ("vbroadcastf128 %0, %%ymm0" : : "m" (*dyn_buf0));
>>       /* We have a return statement to deal with
>>        epilogue in different compilers.  */
>> diff --git a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp 
>> b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
>> index 7e75542720c..fb04260766e 100644
>> --- a/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/i386-avx-reverse.exp
>> @@ -395,7 +395,7 @@ if {[record_full_function "vpbroadcast"] == true} {
>>       test_one_register "vbroadcastf128" "ymm0" \
>>       "0x17161514131211101716151413121110, 
>> 0x17161514131211101716151413121110"
>>       test_one_register "vbroadcastsd" "ymm15" \
>> -    "0x404060004040600040406000404060, 0x0"
>> +    "0x23222120232221202322212023222120, 0x0"
>>       test_one_register "vbroadcastsd" "ymm0" \
>>       "0x13121110131211101312111013121110, 
>> 0x13121110131211101312111013121110"
>