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 78DF83940CE6 for ; Mon, 5 Dec 2022 16:38:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 78DF83940CE6 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 CA2AB23A; Mon, 5 Dec 2022 08:38:42 -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 6AFA63F73D; Mon, 5 Dec 2022 08:38:35 -0800 (PST) Message-ID: <2d22c659-1452-6302-0dd0-270763510950@foss.arm.com> Date: Mon, 5 Dec 2022 16:38:34 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 From: Richard Earnshaw Subject: Re: [PATCH 10/15 V4] arm: Implement cortex-M return signing address codegen To: Andrea Corallo , Andrea Corallo via Gcc-patches Cc: Richard Earnshaw , nd References: Content-Language: en-GB In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.9 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 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? 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. +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. + 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. @@ -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 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. 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? R.