Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31

[Thomas Preudhomme <thomas dot preudhomme at foss dot arm dot com>]

Ping?

Best regards,

Thomas

On 25/08/17 12:18, Thomas Preudhomme wrote:
> Hi,
>
> I've now also added a couple more changes:
>
> * size to_clear_bitmap according to maxregno to be consistent with its use
> * use directly TARGET_HARD_FLOAT instead of clear_vfpregs
>
>
> Original message below (ChangeLog unchanged):
>
> Function cmse_nonsecure_entry_clear_before_return has code to deal with
> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
> not support more than 16 double VFP registers (D0-D15). This makes this
> security-sensitive code harder to read for not much benefit since
> libcall for cmse_nonsecure_call functions do not deal with those high
> VFP registers anyway.
>
> This commit gets rid of this code for simplicity and fixes 2 issues in
> the same function:
>
> - stop the first loop when reaching maxregno to avoid dealing with VFP
>    registers if targetting Thumb-1 or using -mfloat-abi=soft
> - include maxregno in that loop
>
> ChangeLog entry is as follows:
>
> *** gcc/ChangeLog ***
>
> 2017-06-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>
>      * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>      Extensions with more than 16 double VFP registers.
>      (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>      to_clear_mask and all code related to it.  Replace the remaining
>      entry by a sbitmap and adapt code accordingly.
>
> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
> ARMv8-M Mainline.
>
> Is this ok for trunk?
>
> Best regards,
>
> Thomas
>
> On 23/08/17 11:56, Thomas Preudhomme wrote:
> > Ping?
> >
> > Best regards,
> >
> > Thomas
> >
> > On 17/07/17 17:25, Thomas Preudhomme wrote:
> > > My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed 
> > > patch in attachment.
> > >
> > > ChangeLog entry is unchanged:
> > >
> > > *** gcc/ChangeLog ***
> > >
> > > 2017-06-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> > >
> > >      * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> > >      Extensions with more than 16 double VFP registers.
> > >      (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> > >      to_clear_mask and all code related to it.  Replace the remaining
> > >      entry by a sbitmap and adapt code accordingly.
> > >
> > > Best regards,
> > >
> > > Thomas
> > >
> > > On 17/07/17 09:52, Thomas Preudhomme wrote:
> > > > Ping?
> > > >
> > > > Best regards,
> > > >
> > > > Thomas
> > > >
> > > > On 12/07/17 09:59, Thomas Preudhomme wrote:
> > > > > Hi Richard,
> > > > >
> > > > > On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
> > > > > > Hmm, I think that's because really this is a partial conversion.  It
> > > > > > looks like doing this properly would involve moving that existing code
> > > > > > to use sbitmaps as well.  I think doing that would be better for
> > > > > > long-term maintenance perspectives, but I'm not going to insist that you
> > > > > > do it now.
> > > > >
> > > > > There's also the assert later but I've found a way to improve it slightly. 
> > > > > While switching to auto_sbitmap I also changed the code slightly to 
> > > > > allocate directly bitmaps to the right size. Since the change is probably 
> > > > > bigger than what you had in mind I'd appreciate if you can give me an OK 
> > > > > again. See updated patch in attachment. ChangeLog entry is unchanged:
> > > > >
> > > > > 2017-06-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> > > > >
> > > > >      * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> > > > >      Extensions with more than 16 double VFP registers.
> > > > >      (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> > > > >      to_clear_mask and all code related to it.  Replace the remaining
> > > > >      entry by a sbitmap and adapt code accordingly.
> > > > >
> > > > > > As a result I'll let you take the call as to whether you keep this
> > > > > > version or go back to your earlier patch.  If you do decide to keep this
> > > > > > version, then see the comment below.
> > > > >
> > > > > Given the changes I'm more happy with how the patch looks now and making it 
> > > > > go in can be a nice incentive to change other ARMv8-M Security Extension 
> > > > > related code later on.
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Thomas
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 3d15a8185a74164743961d7d666cef4d60b8b11e..680a3c564bdad4ae7cacd57b61f099bdf42d3e73 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3658,6 +3658,11 @@ arm_option_override (void)
   if (use_cmse && !arm_arch_cmse)
     error ("target CPU does not support ARMv8-M Security Extensions");
 
+  /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+     and ARMv8-M Baseline and Mainline do not allow such configuration.  */
+  if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+    error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
   /* Disable scheduling fusion by default if it's not armv7 processor
      or doesn't prefer ldrd/strd.  */
   if (flag_schedule_fusion == 2
@@ -25038,42 +25043,37 @@ thumb1_expand_prologue (void)
 void
 cmse_nonsecure_entry_clear_before_return (void)
 {
-  uint64_t to_clear_mask[2];
+  int regno, maxregno = TARGET_HARD_FLOAT ? LAST_VFP_REGNUM : IP_REGNUM;
   uint32_t padding_bits_to_clear = 0;
   uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
-  int regno, maxregno = IP_REGNUM;
+  auto_sbitmap to_clear_bitmap (maxregno + 1);
   tree result_type;
   rtx result_rtl;
 
-  to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
-  to_clear_mask[0] |= (1ULL << IP_REGNUM);
+  bitmap_clear (to_clear_bitmap);
+  bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+  bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
 
   /* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
-     registers.  We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
-     to make sure the instructions used to clear them are present.  */
-  if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+     registers.  */
+  if (TARGET_HARD_FLOAT)
     {
-      uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
-      maxregno = LAST_VFP_REGNUM;
+      int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
 
-      float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
-      to_clear_mask[0] |= float_mask;
-
-      float_mask = (1ULL << (maxregno - 63)) - 1;
-      to_clear_mask[1] = float_mask;
+      bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
 
       /* Make sure we don't clear the two scratch registers used to clear the
 	 relevant FPSCR bits in output_return_instruction.  */
       emit_use (gen_rtx_REG (SImode, IP_REGNUM));
-      to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+      bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
       emit_use (gen_rtx_REG (SImode, 4));
-      to_clear_mask[0] &= ~(1ULL << 4);
+      bitmap_clear_bit (to_clear_bitmap, 4);
     }
 
   /* If the user has defined registers to be caller saved, these are no longer
      restored by the function before returning and must thus be cleared for
      security purposes.  */
-  for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+  for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
     {
       /* We do not touch registers that can be used to pass arguments as per
 	 the AAPCS, since these should never be made callee-saved by user
@@ -25083,29 +25083,45 @@ cmse_nonsecure_entry_clear_before_return (void)
       if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
 	continue;
       if (call_used_regs[regno])
-	to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+	bitmap_set_bit (to_clear_bitmap, regno);
     }
 
   /* Make sure we do not clear the registers used to return the result in.  */
   result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
   if (!VOID_TYPE_P (result_type))
     {
+      uint64_t to_clear_return_mask;
       result_rtl = arm_function_value (result_type, current_function_decl, 0);
 
       /* No need to check that we return in registers, because we don't
 	 support returning on stack yet.  */
-      to_clear_mask[0]
-	&= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
-				       padding_bits_to_clear_ptr);
+      gcc_assert (REG_P (result_rtl));
+      to_clear_return_mask
+	= compute_not_to_clear_mask (result_type, result_rtl, 0,
+				     padding_bits_to_clear_ptr);
+      if (to_clear_return_mask)
+	{
+	  gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+	  for (regno = R0_REGNUM; regno <= maxregno; regno++)
+	    {
+	      if (to_clear_return_mask & (1ULL << regno))
+		bitmap_clear_bit (to_clear_bitmap, regno);
+	    }
+	}
     }
 
   if (padding_bits_to_clear != 0)
     {
       rtx reg_rtx;
+      auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS);
+
       /* Padding bits to clear is not 0 so we know we are dealing with
 	 returning a composite type, which only uses r0.  Let's make sure that
 	 r1-r3 is cleared too, we will use r1 as a scratch register.  */
-      gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+      bitmap_clear (to_clear_arg_regs_bitmap);
+      bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+			NUM_ARG_REGS - 1);
+      gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
 
       reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
 
@@ -25127,7 +25143,7 @@ cmse_nonsecure_entry_clear_before_return (void)
 
   for (regno = R0_REGNUM; regno <= maxregno; regno++)
     {
-      if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+      if (!bitmap_bit_p (to_clear_bitmap, regno))
 	continue;
 
       if (IS_VFP_REGNUM (regno))
@@ -25136,7 +25152,7 @@ cmse_nonsecure_entry_clear_before_return (void)
 	     be cleared, use vmov.  */
 	  if (TARGET_VFP_DOUBLE
 	      && VFP_REGNO_OK_FOR_DOUBLE (regno)
-	      && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+	      && bitmap_bit_p (to_clear_bitmap, regno + 1))
 	    {
 	      emit_move_insn (gen_rtx_REG (DFmode, regno),
 			      CONST1_RTX (DFmode));