[PATCH, GCC/ARM] Factor out CMSE register clearing code
Kyrill Tkachov
kyrylo.tkachov@foss.arm.com
Wed Nov 22 15:21:00 GMT 2017
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
More information about the Gcc-patches
mailing list