| Message ID | CAKnkMGtgViqG9J3kF_MtKmOU9r2rfLrcu_Vw8GE8n71QbROVtQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and -mword-relocations | expand |
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? Thanks, Kyrill > Best regards, > > Thomas
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 > >
On 27/09/18 11:13, Ramana Radhakrishnan 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. > Ah, thank you for the background Ramana. The naming of mslow-flash-data is confusing indeed. It sounds more like a tuning request rather than a hard requirement. > 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. > Yes, in that case erroring out is easier. > Oh and yes, lets update invoke.texi while here. > Yes. This deserves an entry in the documentation. Thanks, Kyrill > > regards > Ramana > > >> 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? >> >> Thanks, >> Kyrill >> >>> Best regards, >>> >>> Thomas >> >> >
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. 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. > > 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. Done. Will be part of the updated patch. Best regards, Thomas
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
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
From d21e1e0c343f60e4e7de293b4c3cb0e87bf22f2f Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme@linaro.org>
Date: Tue, 25 Sep 2018 15:55:10 +0100
Subject: [PATCH] [PATCH, GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and
-mword-relocations
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. It also explicitely set the
option when in PIC mode as per documentation rather than always check
for target_word_relocation together with flag_pic.
ChangeLog entries are 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
---
gcc/config/arm/arm.c | 22 +++++++++++++------
gcc/config/arm/arm.md | 2 +-
gcc/doc/invoke.texi | 4 ++--
gcc/testsuite/gcc.target/arm/movdi_movt.c | 1 +
gcc/testsuite/gcc.target/arm/movsi_movt.c | 1 +
gcc/testsuite/gcc.target/arm/pr81863.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-1.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-2.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-3.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-4.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-5.c | 1 +
.../gcc.target/arm/tls-disable-literal-pool.c | 1 +
12 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 6332e68df05..043bbe534a2 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -2893,17 +2893,22 @@ arm_option_check_internal (struct gcc_options *opts)
flag_pic = 0;
}
- /* We only support -mpure-code and -mslow-flash-data on M-profile targets
- with MOVT. */
- if ((target_pure_code || target_slow_flash_data)
- && (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON))
+ if (target_pure_code || target_slow_flash_data)
{
const char *flag = (target_pure_code ? "-mpure-code" :
"-mslow-flash-data");
- error ("%s only supports non-pic code on M-profile targets with the "
- "MOVT instruction", flag);
- }
+ /* We only support -mpure-code and -mslow-flash-data on M-profile targets
+ with MOVT. */
+ if (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON)
+ error ("%s only supports non-pic code on M-profile targets with the "
+ "MOVT instruction", flag);
+
+ /* Cannot load addresses: -mslow-flash-data forbids literal pool and
+ -mword-relocations forbids relocation of MOVT/MOVW. */
+ if (target_word_relocations)
+ error ("%s incompatible with -mword-relocations", flag);
+ }
}
/* Recompute the global settings depending on target attribute options. */
@@ -3489,6 +3494,9 @@ arm_option_override (void)
arm_pic_register = pic_register;
}
+ if (flag_pic)
+ target_word_relocations = 1;
+
/* Enable -mfix-cortex-m3-ldrd by default for Cortex-M3 cores. */
if (fix_cm3_ldrd == 2)
{
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 270b8e454b3..a773518cefa 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6128,7 +6128,7 @@
[(set (match_operand:SI 0 "arm_general_register_operand" "")
(match_operand:SI 1 "general_operand" ""))]
"TARGET_USE_MOVT && GET_CODE (operands[1]) == SYMBOL_REF
- && !flag_pic && !target_word_relocations
+ && !target_word_relocations
&& !arm_tls_referenced_p (operands[1])"
[(clobber (const_int 0))]
{
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 7ef4e7a449b..8030b911cc4 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -16964,7 +16964,7 @@ this option and always use the original scheme.
Only generate absolute relocations on word-sized values (i.e. R_ARM_ABS32).
This is enabled by default on targets (uClinux, SymbianOS) where the runtime
loader imposes this restriction, and when @option{-fpic} or @option{-fPIC}
-is specified.
+is specified. This option conflicts with @option{-mslow-flash-data}.
@item -mfix-cortex-m3-ldrd
@opindex mfix-cortex-m3-ldrd
@@ -17001,7 +17001,7 @@ to Neon is high.
Assume loading data from flash is slower than fetching instruction.
Therefore literal load is minimized for better performance.
This option is only supported when compiling for ARMv7 M-profile and
-off by default.
+off by default. It conflicts with @option{-mword-relocations}.
@item -masm-syntax-unified
@opindex masm-syntax-unified
diff --git a/gcc/testsuite/gcc.target/arm/movdi_movt.c b/gcc/testsuite/gcc.target/arm/movdi_movt.c
index e2a28ccbd99..a01ffa0dc93 100644
--- a/gcc/testsuite/gcc.target/arm/movdi_movt.c
+++ b/gcc/testsuite/gcc.target/arm/movdi_movt.c
@@ -1,4 +1,5 @@
/* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mslow-flash-data" } */
unsigned long long
diff --git a/gcc/testsuite/gcc.target/arm/movsi_movt.c b/gcc/testsuite/gcc.target/arm/movsi_movt.c
index 3cf46e2fd17..19d202ecd33 100644
--- a/gcc/testsuite/gcc.target/arm/movsi_movt.c
+++ b/gcc/testsuite/gcc.target/arm/movsi_movt.c
@@ -1,4 +1,5 @@
/* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mslow-flash-data" } */
unsigned
diff --git a/gcc/testsuite/gcc.target/arm/pr81863.c b/gcc/testsuite/gcc.target/arm/pr81863.c
index 63b1ed66b2c..225a0c5cc2b 100644
--- a/gcc/testsuite/gcc.target/arm/pr81863.c
+++ b/gcc/testsuite/gcc.target/arm/pr81863.c
@@ -1,5 +1,6 @@
/* testsuite/gcc.target/arm/pr48183.c */
/* { dg-do compile } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mslow-flash-data" } } */
/* { dg-options "-O2 -mword-relocations -march=armv7-a -marm" } */
/* { dg-final { scan-assembler-not "\[\\t \]+movw" } } */
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
index 089a72b67f3..d10391a69ac 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
@@ -6,6 +6,7 @@
/* { dg-do compile } */
/* { dg-require-effective-target arm_cortex_m } */
/* { dg-require-effective-target arm_thumb2_ok } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mthumb -mslow-flash-data" } */
float sf;
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
index c87e050639d..90bd44e27e5 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
float f (float);
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
index 8c6210ee6c9..5d9cd9c4df2 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -mthumb -mslow-flash-data" } */
/* From PR71607 */
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
index 1bcb6924ed2..0eeddd5e6ec 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
double __attribute__ ((target ("fpu=fpv5-d16")))
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
index 808fff05faa..7d52f3801b6 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
double __attribute__ ((target ("fpu=fpv5-sp-d16")))
diff --git a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
index 283201fdd97..834eaf6db92 100644
--- a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
+++ b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
@@ -2,6 +2,7 @@
/* { dg-require-effective-target tls_native } */
/* { dg-require-effective-target arm_cortex_m } */
/* { dg-require-effective-target arm_thumb2_ok } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-mslow-flash-data" } */
__thread int x = 0;
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
Ping? Best regards, Thomas On Mon, 15 Oct 2018 at 16:01, Thomas Preudhomme <thomas.preudhomme@linaro.org> wrote: > > 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 From d21e1e0c343f60e4e7de293b4c3cb0e87bf22f2f Mon Sep 17 00:00:00 2001 From: Thomas Preud'homme <thomas.preudhomme@linaro.org> Date: Tue, 25 Sep 2018 15:55:10 +0100 Subject: [PATCH] [PATCH, GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and -mword-relocations 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. It also explicitely set the option when in PIC mode as per documentation rather than always check for target_word_relocation together with flag_pic. ChangeLog entries are 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 --- gcc/config/arm/arm.c | 22 +++++++++++++------ gcc/config/arm/arm.md | 2 +- gcc/doc/invoke.texi | 4 ++-- gcc/testsuite/gcc.target/arm/movdi_movt.c | 1 + gcc/testsuite/gcc.target/arm/movsi_movt.c | 1 + gcc/testsuite/gcc.target/arm/pr81863.c | 1 + .../gcc.target/arm/thumb2-slow-flash-data-1.c | 1 + .../gcc.target/arm/thumb2-slow-flash-data-2.c | 1 + .../gcc.target/arm/thumb2-slow-flash-data-3.c | 1 + .../gcc.target/arm/thumb2-slow-flash-data-4.c | 1 + .../gcc.target/arm/thumb2-slow-flash-data-5.c | 1 + .../gcc.target/arm/tls-disable-literal-pool.c | 1 + 12 files changed, 27 insertions(+), 10 deletions(-) diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 6332e68df05..043bbe534a2 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -2893,17 +2893,22 @@ arm_option_check_internal (struct gcc_options *opts) flag_pic = 0; } - /* We only support -mpure-code and -mslow-flash-data on M-profile targets - with MOVT. */ - if ((target_pure_code || target_slow_flash_data) - && (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON)) + if (target_pure_code || target_slow_flash_data) { const char *flag = (target_pure_code ? "-mpure-code" : "-mslow-flash-data"); - error ("%s only supports non-pic code on M-profile targets with the " - "MOVT instruction", flag); - } + /* We only support -mpure-code and -mslow-flash-data on M-profile targets + with MOVT. */ + if (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON) + error ("%s only supports non-pic code on M-profile targets with the " + "MOVT instruction", flag); + + /* Cannot load addresses: -mslow-flash-data forbids literal pool and + -mword-relocations forbids relocation of MOVT/MOVW. */ + if (target_word_relocations) + error ("%s incompatible with -mword-relocations", flag); + } } /* Recompute the global settings depending on target attribute options. */ @@ -3489,6 +3494,9 @@ arm_option_override (void) arm_pic_register = pic_register; } + if (flag_pic) + target_word_relocations = 1; + /* Enable -mfix-cortex-m3-ldrd by default for Cortex-M3 cores. */ if (fix_cm3_ldrd == 2) { diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index 270b8e454b3..a773518cefa 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -6128,7 +6128,7 @@ [(set (match_operand:SI 0 "arm_general_register_operand" "") (match_operand:SI 1 "general_operand" ""))] "TARGET_USE_MOVT && GET_CODE (operands[1]) == SYMBOL_REF - && !flag_pic && !target_word_relocations + && !target_word_relocations && !arm_tls_referenced_p (operands[1])" [(clobber (const_int 0))] { diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 7ef4e7a449b..8030b911cc4 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -16964,7 +16964,7 @@ this option and always use the original scheme. Only generate absolute relocations on word-sized values (i.e. R_ARM_ABS32). This is enabled by default on targets (uClinux, SymbianOS) where the runtime loader imposes this restriction, and when @option{-fpic} or @option{-fPIC} -is specified. +is specified. This option conflicts with @option{-mslow-flash-data}. @item -mfix-cortex-m3-ldrd @opindex mfix-cortex-m3-ldrd @@ -17001,7 +17001,7 @@ to Neon is high. Assume loading data from flash is slower than fetching instruction. Therefore literal load is minimized for better performance. This option is only supported when compiling for ARMv7 M-profile and -off by default. +off by default. It conflicts with @option{-mword-relocations}. @item -masm-syntax-unified @opindex masm-syntax-unified diff --git a/gcc/testsuite/gcc.target/arm/movdi_movt.c b/gcc/testsuite/gcc.target/arm/movdi_movt.c index e2a28ccbd99..a01ffa0dc93 100644 --- a/gcc/testsuite/gcc.target/arm/movdi_movt.c +++ b/gcc/testsuite/gcc.target/arm/movdi_movt.c @@ -1,4 +1,5 @@ /* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mslow-flash-data" } */ unsigned long long diff --git a/gcc/testsuite/gcc.target/arm/movsi_movt.c b/gcc/testsuite/gcc.target/arm/movsi_movt.c index 3cf46e2fd17..19d202ecd33 100644 --- a/gcc/testsuite/gcc.target/arm/movsi_movt.c +++ b/gcc/testsuite/gcc.target/arm/movsi_movt.c @@ -1,4 +1,5 @@ /* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mslow-flash-data" } */ unsigned diff --git a/gcc/testsuite/gcc.target/arm/pr81863.c b/gcc/testsuite/gcc.target/arm/pr81863.c index 63b1ed66b2c..225a0c5cc2b 100644 --- a/gcc/testsuite/gcc.target/arm/pr81863.c +++ b/gcc/testsuite/gcc.target/arm/pr81863.c @@ -1,5 +1,6 @@ /* testsuite/gcc.target/arm/pr48183.c */ /* { dg-do compile } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mslow-flash-data" } } */ /* { dg-options "-O2 -mword-relocations -march=armv7-a -marm" } */ /* { dg-final { scan-assembler-not "\[\\t \]+movw" } } */ diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c index 089a72b67f3..d10391a69ac 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c @@ -6,6 +6,7 @@ /* { dg-do compile } */ /* { dg-require-effective-target arm_cortex_m } */ /* { dg-require-effective-target arm_thumb2_ok } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mthumb -mslow-flash-data" } */ float sf; diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c index c87e050639d..90bd44e27e5 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ float f (float); diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c index 8c6210ee6c9..5d9cd9c4df2 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -mthumb -mslow-flash-data" } */ /* From PR71607 */ diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c index 1bcb6924ed2..0eeddd5e6ec 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ double __attribute__ ((target ("fpu=fpv5-d16"))) diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c index 808fff05faa..7d52f3801b6 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ double __attribute__ ((target ("fpu=fpv5-sp-d16"))) diff --git a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c index 283201fdd97..834eaf6db92 100644 --- a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c +++ b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c @@ -2,6 +2,7 @@ /* { dg-require-effective-target tls_native } */ /* { dg-require-effective-target arm_cortex_m } */ /* { dg-require-effective-target arm_thumb2_ok } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-mslow-flash-data" } */ __thread int x = 0;
Ping? Best regards, Thomas On Tue, 23 Oct 2018 at 10:10, Thomas Preudhomme <thomas.preudhomme@linaro.org> wrote: > > Ping? > > Best regards, > > Thomas > > On Mon, 15 Oct 2018 at 16:01, Thomas Preudhomme > <thomas.preudhomme@linaro.org> wrote: > > > > 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
On Fri, Oct 5, 2018 at 5:50 PM 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. Ok. Thanks, Ramana > > 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
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 6332e68df05..5beffc875c1 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -2893,17 +2893,22 @@ arm_option_check_internal (struct gcc_options *opts) flag_pic = 0; } - /* We only support -mpure-code and -mslow-flash-data on M-profile targets - with MOVT. */ - if ((target_pure_code || target_slow_flash_data) - && (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON)) + if (target_pure_code || target_slow_flash_data) { const char *flag = (target_pure_code ? "-mpure-code" : "-mslow-flash-data"); - error ("%s only supports non-pic code on M-profile targets with the " - "MOVT instruction", flag); - } + /* We only support -mpure-code and -mslow-flash-data on M-profile targets + with MOVT. */ + if (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON) + error ("%s only supports non-pic code on M-profile targets with the " + "MOVT instruction", flag); + + /* Cannot load addresses: -mslow-flash-data forbids literal pool and + -mword-relocations forbids relocation of MOVT/MOVW. */ + if (target_word_relocations) + error ("%s incompatible with -mword-relocations", flag); + } } /* Recompute the global settings depending on target attribute options. */ diff --git a/gcc/testsuite/gcc.target/arm/movdi_movt.c b/gcc/testsuite/gcc.target/arm/movdi_movt.c index e2a28ccbd99..a01ffa0dc93 100644 --- a/gcc/testsuite/gcc.target/arm/movdi_movt.c +++ b/gcc/testsuite/gcc.target/arm/movdi_movt.c @@ -1,4 +1,5 @@ /* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mslow-flash-data" } */ unsigned long long diff --git a/gcc/testsuite/gcc.target/arm/movsi_movt.c b/gcc/testsuite/gcc.target/arm/movsi_movt.c index 3cf46e2fd17..19d202ecd33 100644 --- a/gcc/testsuite/gcc.target/arm/movsi_movt.c +++ b/gcc/testsuite/gcc.target/arm/movsi_movt.c @@ -1,4 +1,5 @@ /* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mslow-flash-data" } */ unsigned diff --git a/gcc/testsuite/gcc.target/arm/pr81863.c b/gcc/testsuite/gcc.target/arm/pr81863.c index 63b1ed66b2c..225a0c5cc2b 100644 --- a/gcc/testsuite/gcc.target/arm/pr81863.c +++ b/gcc/testsuite/gcc.target/arm/pr81863.c @@ -1,5 +1,6 @@ /* testsuite/gcc.target/arm/pr48183.c */ /* { dg-do compile } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mslow-flash-data" } } */ /* { dg-options "-O2 -mword-relocations -march=armv7-a -marm" } */ /* { dg-final { scan-assembler-not "\[\\t \]+movw" } } */ diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c index 089a72b67f3..d10391a69ac 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c @@ -6,6 +6,7 @@ /* { dg-do compile } */ /* { dg-require-effective-target arm_cortex_m } */ /* { dg-require-effective-target arm_thumb2_ok } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-O2 -mthumb -mslow-flash-data" } */ float sf; diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c index c87e050639d..90bd44e27e5 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ float f (float); diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c index 8c6210ee6c9..5d9cd9c4df2 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -mthumb -mslow-flash-data" } */ /* From PR71607 */ diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c index 1bcb6924ed2..0eeddd5e6ec 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ double __attribute__ ((target ("fpu=fpv5-d16"))) diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c index 808fff05faa..7d52f3801b6 100644 --- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c +++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c @@ -3,6 +3,7 @@ /* { dg-require-effective-target arm_thumb2_ok } */ /* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */ /* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */ double __attribute__ ((target ("fpu=fpv5-sp-d16"))) diff --git a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c index 283201fdd97..834eaf6db92 100644 --- a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c +++ b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c @@ -2,6 +2,7 @@ /* { dg-require-effective-target tls_native } */ /* { dg-require-effective-target arm_cortex_m } */ /* { dg-require-effective-target arm_thumb2_ok } */ +/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */ /* { dg-options "-mslow-flash-data" } */ __thread int x = 0;
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. Best regards, Thomas