diff mbox series

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

Message ID gkrh72hbhmp.fsf@arm.com
State New, archived
Headers
Series arm: Enables return address verification and branch target identification on Cortex-M |

Commit Message

Andrea Corallo Aug. 12, 2022, 3:34 p.m. UTC 
  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.

/gcc/

	* config/arm/arm.cc (arm_emit_multi_reg_pop): If the frame pointer
	was set define again the stack pointer as CFA reg when popping.

Comments

Andrea Corallo Sept. 5, 2022, 4:52 p.m. UTC | #1 
Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:

> 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.
>
> /gcc/
>
> 	* config/arm/arm.cc (arm_emit_multi_reg_pop): If the frame pointer
> 	was set define again the stack pointer as CFA reg when popping.

Ping

  Andrea
Kyrylo Tkachov Sept. 27, 2022, 9:03 a.m. UTC | #2 
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

> /gcc/
> 
> 	* config/arm/arm.cc (arm_emit_multi_reg_pop): If the frame pointer
> 	was set define again the stack pointer as CFA reg when popping.
Andrea Corallo Sept. 27, 2022, 10:05 a.m. UTC | #3 
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.

Best Regards

  Andrea
Kyrylo Tkachov Sept. 27, 2022, 3:24 p.m. UTC | #4 
> -----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
Richard Earnshaw Oct. 21, 2022, 12:30 p.m. UTC | #5 
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? 
Please can you show a code sequence where this is needed.

R.
Andrea Corallo Oct. 26, 2022, 8:49 a.m. UTC | #6 
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?

BR

  Andrea
Richard Earnshaw Nov. 8, 2022, 2:57 p.m. UTC | #7 
On 26/10/2022 09:49, Andrea Corallo via Gcc-patches wrote:
> 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?

Yes, but the SP must already be the CFA before this instruction, since 
SP must be the base of the pop. So the reg note changing the CFA to SP 
can't be right.  I'm thinking there must be some earlier restore of SP 
that's missing a frame-related note.

R.

> 
> BR
> 
>    Andrea
Andrea Corallo Jan. 9, 2023, 2:58 p.m. UTC | #8 
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?

Best Regards

  Andrea
Richard Earnshaw Jan. 9, 2023, 3:57 p.m. UTC | #9 
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?

Ah, OK.  I think this is a special case, though, because in this 
specific case the frame pointer (r7) and the stack pointer point to the 
same place.  This means that in the epilogue we don't start by restoring 
SP from FP (at which point we tell the dwarf code that the frame is back 
in SP again).

For example, if I have:


int i;

void foo (int, int*);

int bar()
{
   int j[10];
   foo (i,j);
   return 0;
}


then the epilogue sequence starts with:

         adds    r7, r7, #40
         .cfi_def_cfa_offset 8
         mov     sp, r7
         .cfi_def_cfa_register 13

And then the pop works correctly as-is.

But I'm not convinced that this is enough anyway, you cause the compiler 
to output a directive that changes the CFA pointer back to r13, but you 
don't output anything that changes the CFA offset.  So I think this 
means that the CFA state machine ends up pointing to the wrong location, 
but it's hard to tell as you haven't shown the CFA directives in your 
example above.

> 
> Best Regards
> 
>    Andrea

R.
Richard Earnshaw Jan. 9, 2023, 4:48 p.m. UTC | #10 
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.

> Best Regards
> 
>    Andrea

R.
Richard Earnshaw Jan. 9, 2023, 5:22 p.m. UTC | #11 
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.
Andrea Corallo Jan. 11, 2023, 9:55 a.m. UTC | #12 
Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> 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.

So yes I agree with your analysis.  I'm respinning 10/15 to include your
suggestion and I believe we can just drop this patch.

Thanks

  Andrea
diff mbox series

Patch

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index ceec14f84b6..a5cf4225aa2 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -22303,8 +22303,18 @@  arm_emit_multi_reg_pop (unsigned long saved_regs_mask)
 
   REG_NOTES (par) = dwarf;
   if (!return_in_pc)
-    arm_add_cfa_adjust_cfa_note (par, UNITS_PER_WORD * num_regs,
-				 stack_pointer_rtx, stack_pointer_rtx);
+    {
+      /* If the frame pointer was set define again the stack pointer
+         as CFA reg.  */
+      if (frame_pointer_needed)
+        {
+          RTX_FRAME_RELATED_P (par) = 1;
+          add_reg_note (par, REG_CFA_DEF_CFA, stack_pointer_rtx);
+        }
+      else
+        arm_add_cfa_adjust_cfa_note (par, UNITS_PER_WORD * num_regs,
+                                     stack_pointer_rtx, stack_pointer_rtx);
+    }
 }
 
 /* Generate and emit an insn pattern that we will recognize as a pop_multi