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 9148E3986656 for ; Mon, 12 Dec 2022 10:53:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9148E3986656 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 B49B02F4; Mon, 12 Dec 2022 02:53:50 -0800 (PST) Received: from [10.57.8.228] (unknown [10.57.8.228]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 71FE53F5A1; Mon, 12 Dec 2022 02:53:09 -0800 (PST) Message-ID: <9643aa03-b0d0-6c8b-c668-a1a6e5814c6e@foss.arm.com> Date: Mon, 12 Dec 2022 10:53:08 +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 10/15 V5] arm: Implement cortex-M return signing address codegen Content-Language: en-GB To: Andrea Corallo Cc: Andrea Corallo via Gcc-patches , Richard Earnshaw , nd References: <2d22c659-1452-6302-0dd0-270763510950@foss.arm.com> From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.6 required=5.0 tests=BAYES_00,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/12/2022 14:16, Andrea Corallo via Gcc-patches wrote: > Hi Richard, > > thanks for reviewing. > > Richard Earnshaw writes: > >> On 07/11/2022 08:57, Andrea Corallo via Gcc-patches wrote: >>> Hi all, >>> please find attached the lastest version of this patch incorporating >>> some >>> more improvents. Feel free to ignore V3. >>> Best Regards >>> Andrea >>> >> >>> As part of previous upstream suggestions a test for varargs has been >>> added and '-mtpcs-frame' is deemed being incompatible with this return >>> signing address feature being introduced. >> >> I don't see any check for the tpcs-frame incompatibility? What >> happens if a user does combine the options? > > Check added. > >> gcc/Changelog >> >> 2021-11-03 Andrea Corallo >> >> * config/arm/arm.h (arm_arch8m_main): Declare it. >> * config/arm/arm.cc (arm_arch8m_main): Define it. >> (arm_option_reconfigure_globals): Set arm_arch8m_main. >> (arm_compute_frame_layout, arm_expand_prologue) >> (thumb2_expand_return, arm_expand_epilogue) >> (arm_conditional_register_usage): Update for pac codegen. >> (arm_current_function_pac_enabled_p): New function. >> * config/arm/arm.md (pac_ip_lr_sp, pacbti_ip_lr_sp, aut_ip_lr_sp): >> Add new patterns. >> * config/arm/unspecs.md (UNSPEC_PAC_IP_LR_SP) >> (UNSPEC_PACBTI_IP_LR_SP, UNSPEC_AUT_IP_LR_SP): Add unspecs. >> >> You're missing an entry for aarch_bti_enabled () - yes I realize >> that's just a placeholder at present and will be fully defined in >> patch 12. > > Fixed > >> +static bool >> +aarch_bti_enabled () >> +{ >> + return false; >> +} >> + >> >> No comment on this function (and in patch 12 it moves to a different >> location). It would be best to have it in the right place at this >> point in time. >> >> + clobber_ip = (IS_NESTED (func_type) >> + && (((TARGET_APCS_FRAME && frame_pointer_needed && >> TARGET_ARM) >> + || ((flag_stack_check == STATIC_BUILTIN_STACK_CHECK >> + || flag_stack_clash_protection) >> + && !df_regs_ever_live_p (LR_REGNUM) >> + && arm_r3_live_at_start_p ())) >> + || (arm_current_function_pac_enabled_p ()))); >> >> Redundant parenthesis around arm_current_function_pac_enabled_p () call. > > Fixed > >> + gcc_assert(arm_compute_static_chain_stack_bytes() == 4 >> + || arm_current_function_pac_enabled_p ()); >> >> I wonder if this assert is now really serving a useful purpose. I'd >> consider removing it. > > Removed > >> @@ -27309,7 +27340,7 @@ thumb2_expand_return (bool simple_return) >> to assert it for now to ensure that future code changes do not silently >> change this behavior. */ >> gcc_assert (!IS_CMSE_ENTRY (arm_current_func_type ())); >> - if (num_regs == 1) >> + if (num_regs == 1 && !arm_current_function_pac_enabled_p ()) >> { >> rtx par = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (2)); >> rtx reg = gen_rtx_REG (SImode, PC_REGNUM); >> @@ -27324,10 +27355,20 @@ thumb2_expand_return (bool simple_return) >> } >> else >> { >> - saved_regs_mask &= ~ (1 << LR_REGNUM); >> - saved_regs_mask |= (1 << PC_REGNUM); >> - arm_emit_multi_reg_pop (saved_regs_mask); >> - } >> + if (arm_current_function_pac_enabled_p ()) >> + { >> + gcc_assert (!(saved_regs_mask & (1 << PC_REGNUM))); >> + arm_emit_multi_reg_pop (saved_regs_mask); >> + emit_insn (gen_aut_nop ()); >> + emit_jump_insn (simple_return_rtx); >> + } >> + else >> + { >> + saved_regs_mask &= ~ (1 << LR_REGNUM); >> + saved_regs_mask |= (1 << PC_REGNUM); >> + arm_emit_multi_reg_pop (saved_regs_mask); >> + } >> + } >> } >> else >> >> The logic for these blocks would, I think, be better expressed as >> >> if (pac_enabled) >> ... >> else if (num_regs == 1) >> ... // existing code >> else >> ... // existing code > > Done > >> Also, I think (out of an abundance of caution) we really need a >> scheduling barrier placed before calls to gen_aut_nop() pattern is >> emitted, to ensure that the scheduler never tries to move this >> instruction away from the position we place it. Use gen_blockage() >> for that (see TARGET_SCHED_PROLOG). Alternatively, we could make the >> UNSPEC_PAC_NOP an unspec_volatile, which has the same effect (IIRC) >> without needing an additional insn - if you use this approach, then >> please make sure this is explained in a comment. >> >> +(define_insn "pacbti_nop" >> + [(set (reg:SI IP_REGNUM) >> + (unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)] >> + UNSPEC_PACBTI_NOP))] >> + "arm_arch8m_main" >> + "pacbti\t%|ip, %|lr, %|sp" >> + [(set_attr "conds" "unconditional")]) >> >> The additional side-effect of this being a BTI landing pad means that >> we mustn't move any other instruction before it. So I think this >> needs to be an unspec_volatile as well. > > Done > >> On the tests, they are OK as they stand, but we lack anything that >> will be tested when suitable hardware is unavailable (all tests are >> "dg-do run"). Can we please have some compile-only tests as well? > > Added three compile only tests. > > Please find attached the latest version of the patch. > > BR > > Andrea > + if (TARGET_TPCS_FRAME) + error ("Return address signing and %<-mtpcs-frame%> are incompatible."); So really this is 'not implemented' rather than not compatible - I don't see why we couldn't implement this if we really wanted to. It's not worth implementing it because tpcs-frames are very much legacy these days. So the message should use sorry() and say 'is not supported' rather than 'are incompatible'. +(define_insn "pacbti_nop" + [(set (reg:SI IP_REGNUM) + (unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)] + VUNSPEC_PACBTI_NOP))] No, this needs to be unspec_volatile, not unspec. +(define_insn "aut_nop" + [(unspec:SI [(reg:SI IP_REGNUM) (reg:SI SP_REGNUM) (reg:SI LR_REGNUM)] + VUNSPEC_AUT_NOP)] Similarly. R.