Re: [PATCH 00/24] Fix reading and writing pseudo registers in non-current frames

[Luis Machado <luis.machado@arm.com>]

On 11/13/23 13:10, Luis Machado wrote:
> Simon,
> 
> On 11/9/23 19:04, Simon Marchi wrote:
>> On 11/8/23 14:34, Simon Marchi wrote:
>>> Ah, damn, probably because I switched to byte_vector,  which doesn't do
>>> the zero-initialization we want to do.  Here's a new patch (that applies
>>> on the series directly) that doesn't use byte_vector.
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 1815d78dec4..200e740e013 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -3300,7 +3300,7 @@ aarch64_pseudo_write_1 (gdbarch *gdbarch, frame_info_ptr next_frame,
>>>                         int regnum_offset,
>>>                         gdb::array_view<const gdb_byte> buf)
>>>  {
>>> -  unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>>> +  unsigned raw_regnum = AARCH64_V0_REGNUM + regnum_offset;
>>>    gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>>>
>>>    /* Enough space for a full vector register.
>>> @@ -3309,11 +3309,11 @@ aarch64_pseudo_write_1 (gdbarch *gdbarch, frame_info_ptr next_frame,
>>>       various 'scalar' pseudo registers to behavior like architectural
>>>       writes, register width bytes are written the remainder are set to
>>>       zero.  */
>>> -  constexpr int raw_reg_size = 16;
>>> +  int raw_reg_size = register_size (gdbarch, raw_regnum);
>>>    gdb_byte raw_buf[raw_reg_size] {};
>>> -  gdb::array_view<gdb_byte> raw_view (raw_buf);
>>> +  gdb::array_view<gdb_byte> raw_view (raw_buf, raw_reg_size);
>>>    copy (buf, raw_view.slice (0, buf.size ()));
>>> -  put_frame_register (next_frame, v_regnum, raw_view);
>>> +  put_frame_register (next_frame, raw_regnum, raw_view);
>>>  }
>>>
>>>  /* Given REGNUM, a SME pseudo-register number, store the bytes from DATA to the
>>>
>>> Simon
>>
>> I managed to run a Debian AArch64 image in qemu, with SVE support, so I
>> was able to reproduce the failures you mentioned.  In the end, here's a
>> version of aarch64_pseudo_write_1 that works for me (written as to
>> minimize the number of unnecessary changes, since that seems to
>> introduce unexpected bugs...).
>>
>> static void
>> aarch64_pseudo_write_1 (gdbarch *gdbarch, frame_info_ptr next_frame,
>> 			int regnum_offset,
>> 			gdb::array_view<const gdb_byte> buf)
>> {
>>   unsigned raw_regnum = AARCH64_V0_REGNUM + regnum_offset;
>>
>>   /* Enough space for a full vector register.  */
>>   int raw_reg_size = register_size (gdbarch, raw_regnum);
>>   gdb_byte raw_buf[raw_reg_size];
>>   gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>>
>>   /* Ensure the register buffer is zero, we want gdb writes of the
>>      various 'scalar' pseudo registers to behavior like architectural
>>      writes, register width bytes are written the remainder are set to
>>      zero.  */
>>   memset (raw_buf, 0, register_size (gdbarch, AARCH64_V0_REGNUM));
>>
>>   gdb::array_view<gdb_byte> raw_view (raw_buf, raw_reg_size);
>>   copy (buf, raw_view.slice (0, buf.size ()));
>>   put_frame_register (next_frame, raw_regnum, raw_view);
>> }
>>
>> Simon
> 
> Sorry for the late reply. I was out a couple days last week.
> 
> The above seems to make gdb.arch/aarch64-fp.exp happy, but gdb.arch/aarch64-pseudo-unwind.exp is still slightly unhappy:
> 
> FAIL: gdb.arch/aarch64-pseudo-unwind.exp: caller, after change: p/x $v8.q.u
> FAIL: gdb.arch/aarch64-pseudo-unwind.exp: caller, after change: p/x $s8.u
> FAIL: gdb.arch/aarch64-pseudo-unwind.exp: continue to breakpoint: continue to break_here_c
> FAIL: gdb.arch/aarch64-pseudo-unwind.exp: p/x value
> 
> This was tested on hardware (AWS's Graviton 3). Let me play with it a bit to understand what's up. I also see a
> SIGSEGV in the test that shouldn't be there. I'm guessing some sort of raw register corruption, as it leads to
> pc == 0x0.

I think I understand what's going on here. In the callee, we have CFI stating we're saving the following registers:

.cfi_offset 29, -16  // x29 (SP)
.cfi_offset 30, -8   // x30 (LR)
.cfi_offset 72, -32  // v8 -> 128-bit in size

So we're reserving a slot of 16 bytes to save v8, which is fine. But later, when we go up a frame and try to put a v8 pseudo-register
value to the frame there are two distinct scenarios.

The first one is on a sytem that doesn't support SVE or that does support SVE but the vector length is 128-bit, which
means z8 (the raw register) is the same size as v8 (the pseudo-register). This works fine, because we use the size of
the raw register to put the pseudo-register value to the frame.

Now, if we have SVE support and the vector length is bigger than 128-bit (in my case it is 256-bit), then using the
size of the raw register (z8) will put 32 bytes into a slot of 16 bytes, corrupting (IIUC) both SP and LR, and leading
to a segfault.

Technically we would be fine with using the correct size of the slot when putting a pseudo-register to a frame, but
I think there are further complications.

Take, for instance, the following text from the aadwarf64 (https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst),
note 5 from DWARF registers 64-95:

"In a similar manner to the general register file the size of an FP/Advanced SIMD register is taken from some external context to the register
number. If no context is available then only the least significant 64 bits of the register are referenced. In particular this means that the most
significant part of a SIMD register is unrecoverable by frame unwinding."

So this makes me think we might have situations where we don't know exactly what the size of the slot is. We may have saved all 128 bits of
a v register, but it may also be the case we only saved 64 bits. How we distinguish what size to use for putting a v pseudo-register to a frame
isn't completely clear to me at this point.

I'm wondering if we should instead use x/w registers for testing this functionality for aarch64.