This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, GCC/ARM] Factor out CMSE register clearing code


On 22/11/17 15:07, Thomas Preudhomme wrote:



On 22/11/17 14:45, Kyrill Tkachov wrote:

Hi Thomas,

On 15/11/17 17:12, Thomas Preudhomme wrote:

Hi,

Functions cmse_nonsecure_call_clear_caller_saved and
cmse_nonsecure_entry_clear_before_return both contain very similar code
to clear registers. What's worse, they differ slightly at times so if a
bug is found in one careful thoughts is needed to decide whether the
other function needs fixing too.

This commit addresses the situation by factoring the two pieces of code
into a new function. In doing so the code generated to clear VFP
registers in cmse_nonsecure_call now uses the same sequence as
cmse_nonsecure_entry functions. Tests expectation are thus updated
accordingly.

ChangeLog entry are as follow:

*** gcc/ChangeLog ***

2017-10-24  Thomas Preud'homme <thomas.preudhomme@arm.com>

        * config/arm/arm.c (cmse_clear_registers): New function.
(cmse_nonsecure_call_clear_caller_saved): Replace register clearing 
        code by call to cmse_clear_registers.
        (cmse_nonsecure_entry_clear_before_return): Likewise.

*** gcc/ChangeLog ***

2017-10-24  Thomas Preud'homme <thomas.preudhomme@arm.com>
* gcc.target/arm/cmse/mainline/hard-sp/cmse-13.c: Adapt expectations 
        to vmov instructions now generated.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-7.c: Likewise.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-8.c: Likewise.
        * gcc.target/arm/cmse/mainline/hard/cmse-13.c: Likewise.
        * gcc.target/arm/cmse/mainline/hard/cmse-7.c: Likewise.
        * gcc.target/arm/cmse/mainline/hard/cmse-8.c: Likewise.

Testing: bootstrapped on arm-linux-gnueabihf and no regression in the
testsuite.

Is this ok for trunk?
This looks mostly ok, but I have a concern from reading the code that I'd like some help with... 

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 9b494e9529a4470c18192a4561e03d2f80e90797..22c9add0722974902b2a89b2b0a75759ff8ba37c 100644 
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -16991,6 +16991,128 @@ compute_not_to_clear_mask (tree arg_type, rtx arg_rtx, int regno, 
    return not_to_clear_mask;
  }
+/* Clear registers secret before doing a cmse_nonsecure_call or returning from + a cmse_nonsecure_entry function. TO_CLEAR_BITMAP indicates which registers + are to be fully cleared, using the value in register CLEARING_REG if more + efficient. The PADDING_BITS_LEN entries array PADDING_BITS_TO_CLEAR gives + the bits that needs to be cleared in caller-saved core registers, with 
+   SCRATCH_REG used as a scratch register for that clearing.
+
+   NOTE: one of three following assertions must hold:
+   - SCRATCH_REG is a low register
+ - CLEARING_REG is in the set of registers fully cleared (ie. its bit is set 
+     in TO_CLEAR_BITMAP)
+   - CLEARING_REG is a low register.  */
+
+static void
+cmse_clear_registers (sbitmap to_clear_bitmap, uint32_t *padding_bits_to_clear, 
+              int padding_bits_len, rtx scratch_reg, rtx clearing_reg)
+{
+  bool saved_clearing = false;
+  rtx saved_clearing_reg = NULL_RTX;
+ int i, regno, clearing_regno, minregno = R0_REGNUM, maxregno = minregno - 1; 
+

Here minregno becomes 0 and maxregno becomes -1...

+  gcc_assert (arm_arch_cmse);
+
+  if (!bitmap_empty_p (to_clear_bitmap))
+    {
+      minregno = bitmap_first_set_bit (to_clear_bitmap);
+      maxregno = bitmap_last_set_bit (to_clear_bitmap);
+    }
...and here is a path on maxregno may not be set to a proper register number... 
<snip>
If bitmap is empty yes, ie. if no bit is set and no register should be cleared. 



+
+  for (regno = minregno; regno <= maxregno; regno++)
+    {
+      if (!bitmap_bit_p (to_clear_bitmap, regno))
+    continue;
+
...and here we iterate from minregno (potentially 0) to maxregno (potentially -1) which will lead to trouble. 
Are there any guarantees that this case will not occur?
It absolutely does occur and that's on purpose. If maxregno is -1 it means there is no bit to clear and so it is fine to do nothing.
Err, of course. If maxregno is -1 then the for loop does nothing. My brain was reading this as "until regno <= maxregno" for some reason. 

This is ok for trunk.
Sorry for the noise,
Kyrill


Best regards,

Thomas
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]