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