Re: [PATCH 10/15 V4] arm: Implement cortex-M return signing address codegen

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

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  <andrea.corallo@arm.com>

	* 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.