Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE

["Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>]

On 08/07/2020 09:04, Andre Simoes Dias Vieira wrote:
>
> On 07/07/2020 13:43, Christophe Lyon wrote:
>> Hi,
>>
>>
>> On Mon, 6 Jul 2020 at 16:31, Andre Vieira (lists)
>> <andre.simoesdiasvieira@arm.com> wrote:
>>>
>>> On 30/06/2020 14:50, Andre Vieira (lists) wrote:
>>>> On 29/06/2020 11:15, Christophe Lyon wrote:
>>>>> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
>>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>>> On 23/06/2020 21:52, Christophe Lyon wrote:
>>>>>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
>>>>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>>>>>>>> Sent: 22 June 2020 09:52
>>>>>>>>>> To: gcc-patches@gcc.gnu.org
>>>>>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
>>>>>>>>>> callee saved
>>>>>>>>>> registers with CMSE
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> As reported in bugzilla when the -mcmse option is used while
>>>>>>>>>> compiling
>>>>>>>>>> for size (-Os) with a thumb-1 target the generated code will
>>>>>>>>>> clear the
>>>>>>>>>> registers r7-r10. These however are callee saved and should be
>>>>>>>>>> preserved
>>>>>>>>>> accross ABI boundaries. The reason this happens is because these
>>>>>>>>>> registers are made "fixed" when optimising for size with Thumb-1
>>>>>>>>>> in a
>>>>>>>>>> way to make sure they are not used, as pushing and popping
>>>>>>>>>> hi-registers
>>>>>>>>>> requires extra moves to and from LO_REGS.
>>>>>>>>>>
>>>>>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
>>>>>>>>>> accounts for
>>>>>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
>>>>>>>>>> aware of
>>>>>>>>>> 'callee_saved_reg_p''s definition, as it does still take call 
>>>>>>>>>> used
>>>>>>>>>> registers into account, which aren't callee_saved in my opinion,
>>>>>>>>>> so it
>>>>>>>>>> is a rather misnoemer, works in our advantage here though as it
>>>>>>>>>> does
>>>>>>>>>> exactly what we need.
>>>>>>>>>>
>>>>>>>>>> Regression tested on arm-none-eabi.
>>>>>>>>>>
>>>>>>>>>> Is this OK for trunk? (Will eventually backport to previous
>>>>>>>>>> versions if
>>>>>>>>>> stable.)
>>>>>>>>> Ok.
>>>>>>>>> Thanks,
>>>>>>>>> Kyrill
>>>>>>>> As I was getting ready to push this I noticed I didn't add any
>>>>>>>> skip-ifs
>>>>>>>> to prevent this failing with specific target options. So here's 
>>>>>>>> a new
>>>>>>>> version with those.
>>>>>>>>
>>>>>>>> Still OK?
>>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> This is not sufficient to skip arm-linux-gnueabi* configs built 
>>>>>>> with
>>>>>>> non-default cpu/fpu.
>>>>>>>
>>>>>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
>>>>>>> --with-fpu=neon-fp16 --with-float=hard
>>>>>>> I see:
>>>>>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
>>>>>>> Excess errors:
>>>>>>> cc1: error: ARMv8-M Security Extensions incompatible with 
>>>>>>> selected FPU
>>>>>>> cc1: error: target CPU does not support ARM mode
>>>>>>>
>>>>>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
>>>>>> Resending as I don't think my earlier one made it to the lists
>>>>>> (sorry if
>>>>>> you are receiving this double!)
>>>>>>
>>>>>> I'm not following this, before I go off and try to reproduce it,
>>>>>> what do
>>>>>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
>>>>>> -Os'?
>>>>>> These are the options you are seeing in the log file? Surely they
>>>>>> should
>>>>>> override the default options? Only thing I can think of is this 
>>>>>> might
>>>>>> need an extra -mfloat-abi=soft to make sure it overrides the default
>>>>>> float-abi.  Could you give that a try?
>>>>> No it doesn't make a difference alone.
>>>>>
>>>>> I also had to add:
>>>>> -mfpu=auto (that clears the above warning)
>>>>> -mthumb otherwise we now get cc1: error: target CPU does not support
>>>>> ARM mode
>>>>>
>>>>> Looks like some effective-target machinery is needed
>>>> So I had a look at this,  I was pretty sure that -mfloat-abi=soft
>>>> overwrote -mfpu=<>, which in large it does, as in no FP instructions
>>>> will be generated but the error you see only checks for the right
>>>> number of FP registers. Which doesn't check whether
>>>> 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
>>>> check-effective-target for armv8-m.base for this test as it is indeed
>>>> a better approach than my bag of skip-ifs. I'm testing it locally to
>>>> make sure my changes don't break anything.
>>>>
>>>> Cheers,
>>>> Andre
>>> Hi,
>>>
>>> Sorry for the delay. So I changed the test to use the effective-target
>>> machinery as you suggested and I also made sure that you don't get the
>>> "ARMv8-M Security Extensions incompatible with selected FPU" when
>>> -mfloat-abi=soft.
>>> Further changed 'asm' to '__asm__' to avoid failures with '-std=' 
>>> options.
>>>
>>> Regression tested on arm-none-eabi.
>>> @Christophe: could you test this for your configuration, shouldn't fail
>>> anymore!
>>>
>> Indeed with your patch I don't see any failure with pr95646.c
>>
>> Note that it is still unsupported with arm-eabi when running the tests
>> with -mcpu=cortex-mXX
>> because the compiler complains that -mcpu=cortex-mXX conflicts with
>> -march=armv8-m.base,
>> thus the effective-target test fails.
>>
>> BTW, is that warning useful/practical? Wouldn't it be more convenient
>> if the last -mcpu/-march
>> on the command line was the only one taken into account? (I had a
>> similar issue when
>> running tests (libstdc++) getting -march=armv8-m.main+fp from their
>> multilib environment
>> and forcing -mcpu=cortex-m33 because it also means '+dsp' and produces
>> a warning;
>> I had to use -mcpu=cortex-m33 -march=armv8-m.main+fp+dsp to 
>> workaround this)
> Yeah I've been annoyed by that before, also in the context of testing 
> multilibs.
>
> Even though I can see how it can be a useful warning though, if you 
> are using these in build-systems and you accidentally introduce a new 
> (incompatible) -mcpu/-march alongside the old one. Though it seems 
> arbitrary, as we do not warn against multiple -mcpu's or -march's...
>
> I would be in favor of introducing a flag to turn off the warning, 
> such that we don't have to worry about it in the testsuite. 
> Unfortunately, it seems -march always wins, regardless of order .... 
> Meaning that if we do add and use such a warning-off option, we would 
> also have to always use '-march' as additional-options when creating 
> testcases, to ensure it overrides any  RUNTESTFLAG incompatible -mcpu 
> or different -march options.
>
> Either way, its a bit of a mess that could probably do with some 
> cleaning up. One thing at a time though.
Ping.

Still need an OK to commit this patch.
>>
>> It passes with the config I test without overriding runtestflags flags
>> though, so it's covered.
>>
>> Thanks
>>
>> Christophe
>>
>>> Is this OK for trunk?
>>>
>>> Cheers,
>>> Andre
>>>
>>> gcc/ChangeLog:
>>> 2020-07-06  Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>
>>>           * config/arm/arm.c 
>>> (arm_options_perform_arch_sanity_checks): Do not
>>>           check +D32 for CMSE if -mfloat-abi=soft
>>>
>>> gcc/testsuite/ChangeLog:
>>> 2020-07-06  Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>
>>>           * gcc.target/arm/pr95646.c: Fix testism.
>>>>> Christophe
>>>>>
>>>>>
>>>>>> Cheers,
>>>>>> Andre
>>>>>>> Christophe
>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Andre
>>>>>>>>>> Cheers,
>>>>>>>>>> Andre
>>>>>>>>>>
>>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>> 2020-06-22  Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>>
>>>>>>>>>>              PR target/95646
>>>>>>>>>>              * config/arm/arm.c:
>>>>>>>>>> (cmse_nonsecure_entry_clear_before_return):
>>>>>>>>>> Use 'callee_saved_reg_p' instead of
>>>>>>>>>>              'calL_used_or_fixed_reg_p'.
>>>>>>>>>>
>>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>> 2020-06-22  Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>>
>>>>>>>>>>              PR target/95646
>>>>>>>>>>              * gcc.target/arm/pr95646.c: New test.