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