Re: [PATCH 9/15] arm: Set again stack pointer as CFA reg when popping if necessary

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 09/01/2023 16:48, Richard Earnshaw via Gcc-patches wrote:
> 
> 
> On 09/01/2023 14:58, Andrea Corallo via Gcc-patches wrote:
>> Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>
>>> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
>>>
>>>> On 27/09/2022 16:24, Kyrylo Tkachov via Gcc-patches wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrea Corallo <andrea.corallo@arm.com>
>>>>>> Sent: Tuesday, September 27, 2022 11:06 AM
>>>>>> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>> Cc: Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org>; Richard
>>>>>> Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>
>>>>>> Subject: Re: [PATCH 9/15] arm: Set again stack pointer as CFA reg 
>>>>>> when
>>>>>> popping if necessary
>>>>>>
>>>>>> Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:
>>>>>>
>>>>>>> Hi Andrea,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Gcc-patches <gcc-patches-
>>>>>>>> bounces+kyrylo.tkachov=arm.com@gcc.gnu.org> On Behalf Of Andrea
>>>>>>>> Corallo via Gcc-patches
>>>>>>>> Sent: Friday, August 12, 2022 4:34 PM
>>>>>>>> To: Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org>
>>>>>>>> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>
>>>>>>>> Subject: [PATCH 9/15] arm: Set again stack pointer as CFA reg when
>>>>>> popping
>>>>>>>> if necessary
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> this patch enables 'arm_emit_multi_reg_pop' to set again the stack
>>>>>>>> pointer as CFA reg when popping if this is necessary.
>>>>>>>>
>>>>>>>
>>>>>>>   From what I can tell from similar functions this is correct, 
>>>>>>> but could you
>>>>>> elaborate on why this change is needed for my understanding please?
>>>>>>> Thanks,
>>>>>>> Kyrill
>>>>>>
>>>>>> Hi Kyrill,
>>>>>>
>>>>>> sure, if the frame pointer was set, than it is the current CFA 
>>>>>> register.
>>>>>> If we request to adjust the current CFA register offset indicating it
>>>>>> being SP (while it's actually FP) that is indeed not correct and the
>>>>>> incoherence we will be detected by an assertion in the dwarf emission
>>>>>> machinery.
>>>>> Thanks,  the patch is ok
>>>>> Kyrill
>>>>>
>>>>>>
>>>>>> Best Regards
>>>>>>
>>>>>>     Andrea
>>>>
>>>> Hmm, wait.  Why would a multi-reg pop be updating the stack pointer?
>>>
>>> Hi Richard,
>>>
>>> not sure I understand, isn't any pop updating SP by definition?
>>
>>
>> Back on this,
>>
>> compiling:
>>
>> =======
>> int i;
>>
>> void foo (int);
>>
>> int bar()
>> {
>>    foo (i);
>>    return 0;
>> }
>> =======
>>
>> With -march=armv8.1-m.main+fp -mbranch-protection=pac-ret+leaf -mthumb 
>> -O0 -g
>>
>> Produces the following asm for bar.
>>
>> bar:
>>     @ args = 0, pretend = 0, frame = 0
>>     @ frame_needed = 1, uses_anonymous_args = 0
>>     pac    ip, lr, sp
>>     push    {r3, r7, ip, lr}
>>     add    r7, sp, #0
>>     ldr    r3, .L3
>>     ldr    r3, [r3]
>>     mov    r0, r3
>>     bl    foo
>>     movs    r3, #0
>>     mov    r0, r3
>>     pop    {r3, r7, ip, lr}
>>     aut    ip, lr, sp
>>     bx    lr
>>
>> The offending instruction causing the ICE (without this patch) when
>> emitting dwarf is "pop {r3, r7, ip, lr}".
>>
>> The current CFA reg when emitting the multipop is R7 (the frame
>> pointer).  If is not the multipop that has the duty to restore SP as
>> current CFA here which other instruction should do it?
>>
> 
> Digging a bit deeper, I'm now even more confused.  arm_expand_epilogue 
> contains (parphrasing the code):
> 
>   if frame_pointer_needed
>     {
>       if arm
>         {}
>       else
>         {
>           if adjust
>             r7 += adjust
>           mov sp, r7    // Reset CFA to SP
>         }
>      }
> 
> so there should always be a move of r7 into SP, even if this is strictly 
> redundant.  I don't understand why this doesn't happen for your 
> testcase.  Can you dig a bit deeper?  I wonder if we've (probably 
> incorrectly) assumed that this function doesn't need an epilogue but can 
> use a simple return?  I don't think we should do that when 
> authentication is needed: a simple return should really be one instruction.
> 

So I strongly suspect the real problem here is that use_return_insn () 
in arm.cc needs to be updated to return false when using pointer 
authentication.  The specification for this function says that a return 
can be done in one instruction; and clearly when we need authentication 
more than one is needed.

R.

>> Best Regards
>>
>>    Andrea
> 
> R.