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

[Ramana Radhakrishnan <ramana dot radhakrishnan at foss dot arm dot com>]

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