[PATCH, GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and -mword-relocations

[Ramana Radhakrishnan]

On 27/09/2018 09:26, Kyrill Tkachov wrote:
> Hi Thomas,
> 
> On 26/09/18 18:39, Thomas Preudhomme wrote:
>> Hi,
>>
>> GCC ICEs under -mslow-flash-data and -mword-relocations because there
>> is no way to load an address, both literal pools and MOVW/MOVT being
>> forbidden. This patch gives an error message when both options are
>> specified by the user and adds the according dg-skip-if directives for
>> tests that use either of these options.
>>
>> ChangeLog entries are as follows:
>>
>> *** gcc/ChangeLog ***
>>
>> 2018-09-25  Thomas Preud'homme  <thomas.preudhomme@linaro.org>
>>
>>       PR target/87374
>>       * config/arm/arm.c (arm_option_check_internal): Disable the combined
>>       use of -mslow-flash-data and -mword-relocations.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2018-09-25  Thomas Preud'homme  <thomas.preudhomme@linaro.org>
>>
>>       PR target/87374
>>       * gcc.target/arm/movdi_movt.c: Skip if both -mslow-flash-data and
>>       -mword-relocations would be passed when compiling the test.
>>       * gcc.target/arm/movsi_movt.c: Likewise.
>>       * gcc.target/arm/pr81863.c: Likewise.
>>       * gcc.target/arm/thumb2-slow-flash-data-1.c: Likewise.
>>       * gcc.target/arm/thumb2-slow-flash-data-2.c: Likewise.
>>       * gcc.target/arm/thumb2-slow-flash-data-3.c: Likewise.
>>       * gcc.target/arm/thumb2-slow-flash-data-4.c: Likewise.
>>       * gcc.target/arm/thumb2-slow-flash-data-5.c: Likewise.
>>       * gcc.target/arm/tls-disable-literal-pool.c: Likewise.
>>
>>
>> Testing: Bootstrapped in Thumb-2 mode. No testsuite regression when
>> targeting arm-none-eabi. Modified tests get skipped as expected when
>> running the testsuite with -mslow-flash-data (pr81863.c) or
>> -mword-relocations (all the others).
>>
>>
>> Is this ok for trunk? I'd also appreciate guidance on whether this is
>> worth a backport. It's a simple patch but on the other hand it only
>> prevents some option combination, it does not fix anything so I have
>> mixed feelings.
> 
> In my opinion -mslow-flash-data is more of a tuning option rather than a security/ABI feature
> and therefore erroring out on its combination with -mword-relocations feels odd.
> I'm leaning more towards making -mword-relocations or any other option that really requires constant pools
> to bypass/disable the effects of -mslow-flash-data instead.
> For me, as a user, it would give a more friendly experience.
> That said, you have more familiarity with M-profile users. Would such users prefer a hard error when the -mslow-flash-data option
> has its effects bypassed? Maybe we could emit a warning rather than an error when this happens?


-mslow-flash-data and -mword-relocations are contradictory in their 
expectations. mslow-flash-data is for not putting anything in the 
literal pool whereas mword-relocations is purely around the use of movw 
/ movt instructions for word sized values. I wish we had called 
-mslow-flash-data something else (probably -mno-literal-pools). 
-mslow-flash-data is used primarily by M-profile users and 
-mword-relocations IIUC was a point fix for use in the Linux kernel for 
module loads at a time when not all module loaders in the linux kernel 
were fixed for the movw / movt relocations and armv7-a / thumb2 was in 
it's infancy . Thus they are used by different constituencies in general 
and I wouldn't see them used together by actual users.

Considering the above, I would prefer a hard error rather than a warning 
as they are contradictory and I'd prefer that we error'd out. Further 
this bugzilla entry is probably created with fuzzing with a variety of 
options rather than from any real use case.

Oh and yes, lets update invoke.texi while here.


regards
Ramana



> 
> Thanks,
> Kyrill
> 
>> Best regards,
>>
>> Thomas
> 
>