Hi Ramana, On Thu, 27 Sep 2018 at 11:14, Ramana Radhakrishnan <ramana.radhakrish...@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.preudho...@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.preudho...@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
