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

[Richard Earnshaw via Gcc-patches]

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.earns...@foss.arm.com> writes:

On 27/09/2022 16:24, Kyrylo Tkachov via Gcc-patches wrote:

-----Original Message-----
From: Andrea Corallo <andrea.cora...@arm.com>
Sent: Tuesday, September 27, 2022 11:06 AM
To: Kyrylo Tkachov <kyrylo.tkac...@arm.com>
Cc: Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org>; Richard
Earnshaw <richard.earns...@arm.com>; nd <n...@arm.com>
Subject: Re: [PATCH 9/15] arm: Set again stack pointer as CFA reg 
when
popping if necessary

Kyrylo Tkachov <kyrylo.tkac...@arm.com> writes:

Hi Andrea,

-----Original Message-----
From: Gcc-patches <gcc-patches-
bounces+kyrylo.tkachov=arm....@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.earns...@arm.com>; nd <n...@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.