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

[Kyrill Tkachov]

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.preudho...@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.preudho...@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