From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 129F83858D1E for ; Mon, 9 Jan 2023 17:22:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 129F83858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 916CE1042; Mon, 9 Jan 2023 09:23:14 -0800 (PST) Received: from [10.2.78.76] (unknown [10.2.78.76]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E66963F587; Mon, 9 Jan 2023 09:22:31 -0800 (PST) Message-ID: <35e22f98-8c00-e591-112c-64cc6612a7ad@foss.arm.com> Date: Mon, 9 Jan 2023 17:22:30 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH 9/15] arm: Set again stack pointer as CFA reg when popping if necessary Content-Language: en-GB To: Andrea Corallo , Andrea Corallo via Gcc-patches Cc: nd , Richard Earnshaw References: <280ceb21-4ea7-33ae-0d87-c3f01db33a9e@foss.arm.com> From: Richard Earnshaw In-Reply-To: <280ceb21-4ea7-33ae-0d87-c3f01db33a9e@foss.arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3488.2 required=5.0 tests=BAYES_00,BODY_8BITS,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 writes: >> >>> Richard Earnshaw writes: >>> >>>> On 27/09/2022 16:24, Kyrylo Tkachov via Gcc-patches wrote: >>>>> >>>>>> -----Original Message----- >>>>>> From: Andrea Corallo >>>>>> Sent: Tuesday, September 27, 2022 11:06 AM >>>>>> To: Kyrylo Tkachov >>>>>> Cc: Andrea Corallo via Gcc-patches ; Richard >>>>>> Earnshaw ; nd >>>>>> Subject: Re: [PATCH 9/15] arm: Set again stack pointer as CFA reg >>>>>> when >>>>>> popping if necessary >>>>>> >>>>>> Kyrylo Tkachov writes: >>>>>> >>>>>>> Hi Andrea, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: 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 >>>>>>>> Cc: Richard Earnshaw ; nd >>>>>>>> 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.