[PATCH, GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and -mword-relocations
Thomas Preudhomme
thomas.preudhomme@linaro.org
Mon Oct 15 15:21:00 GMT 2018
Ping?
Best regards,
Thomas
On Fri, 5 Oct 2018 at 17:50, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
>
> Hi Ramana and Kyrill,
>
> I've reworked the patch to add some documentation of the option
> conflict and reworked the -mword-relocation logic slightly to set the
> variable explicitely in PIC mode rather than test for PIC and word
> relocation everywhere.
>
> ChangeLog entries are now as follows:
>
> *** gcc/ChangeLog ***
>
> 2018-10-02 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.
> (arm_option_override): Enable -mword-relocations if -fpic or -fPIC.
> * config/arm/arm.md (SYMBOL_REF MOVT splitter): Stop checking for
> flag_pic.
> * doc/invoke.texi (-mword-relocations): Mention conflict with
> -mslow-flash-data.
> (-mslow-flash-data): Reciprocally.
>
> *** 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.
>
> Is this ok for trunk?
>
> Best regards,
>
> Thomas
>
> On Tue, 2 Oct 2018 at 13:39, Ramana Radhakrishnan
> <ramana.radhakrishnan@foss.arm.com> wrote:
> >
> > On 02/10/2018 11:42, Thomas Preudhomme wrote:
> > > Hi Ramana,
> > >
> > > On Thu, 27 Sep 2018 at 11:14, Ramana Radhakrishnan
> > > <ramana.radhakrishnan@arm.com> wrote:
> > >>
> > >> 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.
> > >>
> > >> -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.
> > >
> > > Technically, -mslow-flash-data does not forbid literal pool, it just
> > > discourages it because it's slower than many instructions. -mpure-code
> > > on the other hand reuse the same logic and does forbid literal pools.
> > > We could treat -mslow-flash-data differently but the question is
> > > whether it is worth the trouble.
> >
> > Well, yeah I don't see the need for it. You could argue that
> > -mslow-flash-data can be porous but realistically if you want this as an
> > effective performance option , such porosity should be discouraged very
> > strongly ;)
> >
> > >
> > > By the way, I've noticed that the documentation for -mword-relocations
> > > says it defaults to on for -fpic and -fPIC but when looking through
> > > the code I saw that target_word_relocation is not set in these case,
> > > rather the initial commit checks that introduced -mword-relocation
> > > also checks for flag_pic when checking target_word_relocation. However
> > > a later commit added one more check for target_word_relocations but
> > > nothing for flag_pic. I'm now consolidating this so that flag_pic sets
> > > target_word_relocations. I'll do a regression testing with -fPIC and
> > > then post an updated patch.
> >
> > I'm reasonably sure that's *not* going to have *any* effect on code
> > generation as in the -fpic / -fPIC case we always produce the funny GOT
> > unspecs and have never used movw / movt instructions in those sequences
> > for addressing. If that had happened most of the world's dynamic
> > libraries would have faulted by now because I don't think they can
> > process absolute movw / movt relocations.
> >
> >
> > It is automatically implied by the fact that we never produced PC
> > relative versions of the immediates that get put into movw / movt
> > instructions. I don't even remember us having effective relocations to
> > implement this but this is going back a few years now.
> >
> >
> > Sure that probably needs a comment rather than being implicit from the
> > source or from my own head :)
> >
> > Ramana
More information about the Gcc-patches
mailing list