Re: [PATCH] Aarch64 SVE: Fix stack smashing when calling functions

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 17 Sep 2018, at 19:42, Joel Brobecker <brobecker@adacore.com> wrote:
> 
> Hi Alan,
> 
>> Using "call" on a function that passes arguments via float registers can cause
>> gdb to overflow buffers.
> 
>> Ensure enough memory is reserved to hold a full FP register.
>> 
>> 2018-09-17  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* aarch64-tdep.c (pass_in_v): Use register size.
>> 	(aarch64_extract_return_value): Likewise.
>> 	(aarch64_store_return_value): Likewise.
> 
> Do we have a testcase already that demonstrates the problem?
> Otherwise, it would be nice to add one.

I should have mentioned that in the description. I can add:
“This fixes gdb.base/callfuncs.exp for Aarch64 SVE."

> 
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 6993e9061e..516eb138dc 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -1358,7 +1358,10 @@ pass_in_v (struct gdbarch *gdbarch,
>>   if (info->nsrn < 8)
>>     {
>>       int regnum = AARCH64_V0_REGNUM + info->nsrn;
>> -      gdb_byte reg[V_REGISTER_SIZE];
>> +      /* Enough space for a full vector register.  */
>> +      gdb_byte reg[register_size (gdbarch, regnum)];
>> +      gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>> +      gdb_assert (len <= sizeof (reg));
> 
> Could you explain the relationship between making the buffer large
> enough, which is the purpose of this patch, and the assertion that
> AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM?
> 
> I don't see a problem with that assertion, but for archeology
> purposes, it is better to decorelate changes that are independent.
> It helps better document why we introduced changes.

Thinking about it, that assert should be removed.
I was reusing three lines from aarch64_pseudo_read_value_1, which
passed AARCH64_V0_REGNUM into register_size. There the assert made
sense. When I switched to use regnum I didn’t rethink the assert.

I’ll remove the three new instances of that assert from this patch.

Are you happy with those those changes?


Thanks,
Alan.