| Message ID | 740157ba-104b-af44-7c94-28d7172b65f1@foss.arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Thomas, Thanks for working on this, On 12 December 2016 at 18:52, Thomas Preudhomme <thomas.preudhomme@foss.arm.com> wrote: > Hi, > > The logic to make -mthumb optional for Thumb-only devices is only executed > when no -marm or -mthumb is given on the command-line. This includes > configuring GCC with --with-mode= because this makes the option to be passed > before any others. The optional_mthumb-* testcases are skipped when -marm or > -mthumb is passed on the command line but not when GCC was configured with > --with-mode. Not only are the tests meaningless in these configurations, > they also spuriously FAIL if --with-mode=arm was used since the test are > built for Thumb-only devices, as reported by Christophe Lyon in [1]. > > [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html > > This patch adds logic to target-support.exp to check how was GCC configured > and changes the optional_mthumb testcases to be skipped if there is a > default mode (--with-mode=). It also fixes a couple of typo on the > selectors. > How hard would it be to skip these tests only if --with-mode=arm, such that they would still pass in configurations --with-mode=thumb? It seems easy to extend what you propose here, doesn't it? Christophe > ChangeLog entry is as follows: > > > *** gcc/testsuite/ChangeLog *** > > 2016-12-09 Thomas Preud'homme <thomas.preudhomme@arm.com> > > * lib/target-supports.exp (check_configured_with): New procedure. > (check_effective_target_default_mode): new effective target. > * gcc.target/arm/optional_thumb-1.c: Skip if GCC was configured with > a > default mode. Fix dg-skip-if target selector syntax. > * gcc.target/arm/optional_thumb-2.c: Likewise. > * gcc.target/arm/optional_thumb-3.c: Fix dg-skip-if target selector > syntax. > > > Is this ok for stage3? > > Best regards, > > Thomas
On 12/12/16 21:17, Christophe Lyon wrote: > Hi Thomas, > > Thanks for working on this, > > > On 12 December 2016 at 18:52, Thomas Preudhomme > <thomas.preudhomme@foss.arm.com> wrote: >> Hi, >> >> The logic to make -mthumb optional for Thumb-only devices is only executed >> when no -marm or -mthumb is given on the command-line. This includes >> configuring GCC with --with-mode= because this makes the option to be passed >> before any others. The optional_mthumb-* testcases are skipped when -marm or >> -mthumb is passed on the command line but not when GCC was configured with >> --with-mode. Not only are the tests meaningless in these configurations, >> they also spuriously FAIL if --with-mode=arm was used since the test are >> built for Thumb-only devices, as reported by Christophe Lyon in [1]. >> >> [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html >> >> This patch adds logic to target-support.exp to check how was GCC configured >> and changes the optional_mthumb testcases to be skipped if there is a >> default mode (--with-mode=). It also fixes a couple of typo on the >> selectors. >> > > How hard would it be to skip these tests only if --with-mode=arm, > such that they would still pass in configurations --with-mode=thumb? > > It seems easy to extend what you propose here, doesn't it? It is but IMO it gives a false sense of quality since it does not test the optional -mthumb logic. What is the motivation to make it run with --with-mode=thumb? Best regards, Thomas
On 13 December 2016 at 10:54, Thomas Preudhomme <thomas.preudhomme@foss.arm.com> wrote: > On 12/12/16 21:17, Christophe Lyon wrote: >> >> Hi Thomas, >> >> Thanks for working on this, >> >> >> On 12 December 2016 at 18:52, Thomas Preudhomme >> <thomas.preudhomme@foss.arm.com> wrote: >>> >>> Hi, >>> >>> The logic to make -mthumb optional for Thumb-only devices is only >>> executed >>> when no -marm or -mthumb is given on the command-line. This includes >>> configuring GCC with --with-mode= because this makes the option to be >>> passed >>> before any others. The optional_mthumb-* testcases are skipped when -marm >>> or >>> -mthumb is passed on the command line but not when GCC was configured >>> with >>> --with-mode. Not only are the tests meaningless in these configurations, >>> they also spuriously FAIL if --with-mode=arm was used since the test are >>> built for Thumb-only devices, as reported by Christophe Lyon in [1]. >>> >>> [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html >>> >>> This patch adds logic to target-support.exp to check how was GCC >>> configured >>> and changes the optional_mthumb testcases to be skipped if there is a >>> default mode (--with-mode=). It also fixes a couple of typo on the >>> selectors. >>> >> >> How hard would it be to skip these tests only if --with-mode=arm, >> such that they would still pass in configurations --with-mode=thumb? >> >> It seems easy to extend what you propose here, doesn't it? > > > It is but IMO it gives a false sense of quality since it does not test the > optional -mthumb logic. > > What is the motivation to make it run with --with-mode=thumb? > With your patch, they will appear as unsupported, right? It's not a big deal. Do you know how most people configure their toolchains? Linaro and Ubuntu (I think) use --with-mode=thumb. Do most people use no --with-mode configure flag? In the validations I run against trunk, I always have --with-mode, except for one arm-none-eabi configuration where I also use default cpu/fpu. Maybe I should remove --with-mode=thumb from my config --with-cpu=cortex-m3? (but I don't remember if the new logic was backported to gcc-5/gcc-6 ?) Christophe > Best regards, > > Thomas
On 13/12/16 10:11, Christophe Lyon wrote: > On 13 December 2016 at 10:54, Thomas Preudhomme > <thomas.preudhomme@foss.arm.com> wrote: >> On 12/12/16 21:17, Christophe Lyon wrote: >>> >>> Hi Thomas, >>> >>> Thanks for working on this, >>> >>> >>> On 12 December 2016 at 18:52, Thomas Preudhomme >>> <thomas.preudhomme@foss.arm.com> wrote: >>>> >>>> Hi, >>>> >>>> The logic to make -mthumb optional for Thumb-only devices is only >>>> executed >>>> when no -marm or -mthumb is given on the command-line. This includes >>>> configuring GCC with --with-mode= because this makes the option to be >>>> passed >>>> before any others. The optional_mthumb-* testcases are skipped when -marm >>>> or >>>> -mthumb is passed on the command line but not when GCC was configured >>>> with >>>> --with-mode. Not only are the tests meaningless in these configurations, >>>> they also spuriously FAIL if --with-mode=arm was used since the test are >>>> built for Thumb-only devices, as reported by Christophe Lyon in [1]. >>>> >>>> [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html >>>> >>>> This patch adds logic to target-support.exp to check how was GCC >>>> configured >>>> and changes the optional_mthumb testcases to be skipped if there is a >>>> default mode (--with-mode=). It also fixes a couple of typo on the >>>> selectors. >>>> >>> >>> How hard would it be to skip these tests only if --with-mode=arm, >>> such that they would still pass in configurations --with-mode=thumb? >>> >>> It seems easy to extend what you propose here, doesn't it? >> >> >> It is but IMO it gives a false sense of quality since it does not test the >> optional -mthumb logic. >> >> What is the motivation to make it run with --with-mode=thumb? >> > > With your patch, they will appear as unsupported, right? > It's not a big deal. yes indeed. > > Do you know how most people configure their toolchains? > Linaro and Ubuntu (I think) use --with-mode=thumb. > Do most people use no --with-mode configure flag? I cannot answer for people in general but in our case we use --with-multilib-list which will build the toolchain for several -march, -mfpu and -mfloat combinations. But it's good to have a mix, it finds more issues in the testsuite as this very examples shows. > > In the validations I run against trunk, I always have --with-mode, > except for one arm-none-eabi configuration where I also use > default cpu/fpu. For good reason because without those it will compile target libraries without any option and so the default would be used (ie ARMv4T in default mode). Either you need to specify some --with-* or you built with multilib. > > Maybe I should remove --with-mode=thumb from my config > --with-cpu=cortex-m3? (but I don't remember if the new logic > was backported to gcc-5/gcc-6 ?) No it wasn't backported. Best regards, Thomas
On 13 December 2016 at 12:18, Thomas Preudhomme <thomas.preudhomme@foss.arm.com> wrote: > On 13/12/16 10:11, Christophe Lyon wrote: >> >> On 13 December 2016 at 10:54, Thomas Preudhomme >> <thomas.preudhomme@foss.arm.com> wrote: >>> >>> On 12/12/16 21:17, Christophe Lyon wrote: >>>> >>>> >>>> Hi Thomas, >>>> >>>> Thanks for working on this, >>>> >>>> >>>> On 12 December 2016 at 18:52, Thomas Preudhomme >>>> <thomas.preudhomme@foss.arm.com> wrote: >>>>> >>>>> >>>>> Hi, >>>>> >>>>> The logic to make -mthumb optional for Thumb-only devices is only >>>>> executed >>>>> when no -marm or -mthumb is given on the command-line. This includes >>>>> configuring GCC with --with-mode= because this makes the option to be >>>>> passed >>>>> before any others. The optional_mthumb-* testcases are skipped when >>>>> -marm >>>>> or >>>>> -mthumb is passed on the command line but not when GCC was configured >>>>> with >>>>> --with-mode. Not only are the tests meaningless in these >>>>> configurations, >>>>> they also spuriously FAIL if --with-mode=arm was used since the test >>>>> are >>>>> built for Thumb-only devices, as reported by Christophe Lyon in [1]. >>>>> >>>>> [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html >>>>> >>>>> This patch adds logic to target-support.exp to check how was GCC >>>>> configured >>>>> and changes the optional_mthumb testcases to be skipped if there is a >>>>> default mode (--with-mode=). It also fixes a couple of typo on the >>>>> selectors. >>>>> >>>> >>>> How hard would it be to skip these tests only if --with-mode=arm, >>>> such that they would still pass in configurations --with-mode=thumb? >>>> >>>> It seems easy to extend what you propose here, doesn't it? >>> >>> >>> >>> It is but IMO it gives a false sense of quality since it does not test >>> the >>> optional -mthumb logic. >>> >>> What is the motivation to make it run with --with-mode=thumb? >>> >> >> With your patch, they will appear as unsupported, right? >> It's not a big deal. > > > yes indeed. > OK, that's fine if that's your intent. >> >> Do you know how most people configure their toolchains? >> Linaro and Ubuntu (I think) use --with-mode=thumb. >> Do most people use no --with-mode configure flag? > > > I cannot answer for people in general but in our case we use > --with-multilib-list which will build the toolchain for several -march, > -mfpu and -mfloat combinations. But it's good to have a mix, it finds more > issues in the testsuite as this very examples shows. > >> >> In the validations I run against trunk, I always have --with-mode, >> except for one arm-none-eabi configuration where I also use >> default cpu/fpu. > > > For good reason because without those it will compile target libraries > without any option and so the default would be used (ie ARMv4T in default > mode). Either you need to specify some --with-* or you built with multilib. > I keep this configuration with all defaults to make sure everything still works for the default, armv4t indeed. And there are often problems in the testsuite as you know, because people test with more modern configurations. >> >> Maybe I should remove --with-mode=thumb from my config >> --with-cpu=cortex-m3? (but I don't remember if the new logic >> was backported to gcc-5/gcc-6 ?) > > > No it wasn't backported. > > Best regards, > > Thomas
diff --git a/gcc/testsuite/gcc.target/arm/optional_thumb-1.c b/gcc/testsuite/gcc.target/arm/optional_thumb-1.c index 23df62887ba4aaa1d8717a34ecda9a40246f0552..99cb0c3f33b601fff4493feef72765f7590e18f6 100644 --- a/gcc/testsuite/gcc.target/arm/optional_thumb-1.c +++ b/gcc/testsuite/gcc.target/arm/optional_thumb-1.c @@ -1,5 +1,5 @@ -/* { dg-do compile } */ -/* { dg-skip-if "-marm/-mthumb/-march/-mcpu given" { *-*-*} { "-marm" "-mthumb" "-march=*" "-mcpu=*" } } */ +/* { dg-do compile { target { ! default_mode } } } */ +/* { dg-skip-if "-marm/-mthumb/-march/-mcpu given" { *-*-* } { "-marm" "-mthumb" "-march=*" "-mcpu=*" } } */ /* { dg-options "-march=armv6-m" } */ /* Check that -mthumb is not needed when compiling for a Thumb-only target. */ diff --git a/gcc/testsuite/gcc.target/arm/optional_thumb-2.c b/gcc/testsuite/gcc.target/arm/optional_thumb-2.c index 4bd53a45eca97e62dd3b86d5a1a66c5ca21e7aad..280dfb3fec55570b6cfe934303c9bd3d50322b86 100644 --- a/gcc/testsuite/gcc.target/arm/optional_thumb-2.c +++ b/gcc/testsuite/gcc.target/arm/optional_thumb-2.c @@ -1,5 +1,5 @@ -/* { dg-do compile } */ -/* { dg-skip-if "-marm/-mthumb/-march/-mcpu given" { *-*-*} { "-marm" "-mthumb" "-march=*" "-mcpu=*" } } */ +/* { dg-do compile { target { ! default_mode } } } */ +/* { dg-skip-if "-marm/-mthumb/-march/-mcpu given" { *-*-* } { "-marm" "-mthumb" "-march=*" "-mcpu=*" } } */ /* { dg-options "-mcpu=cortex-m4" } */ /* Check that -mthumb is not needed when compiling for a Thumb-only target. */ diff --git a/gcc/testsuite/gcc.target/arm/optional_thumb-3.c b/gcc/testsuite/gcc.target/arm/optional_thumb-3.c index f1fd5c8840b191e600c20a7817c611bb9bb645df..d9150e09e475dfbeb7b0b0c153c913b1ad6f0777 100644 --- a/gcc/testsuite/gcc.target/arm/optional_thumb-3.c +++ b/gcc/testsuite/gcc.target/arm/optional_thumb-3.c @@ -1,8 +1,8 @@ /* { dg-do compile } */ /* { dg-require-effective-target arm_cortex_m } */ -/* { dg-skip-if "-mthumb given" { *-*-*} { "-mthumb" } } */ +/* { dg-skip-if "-mthumb given" { *-*-* } { "-mthumb" } } */ /* { dg-options "-marm" } */ -/* { dg-error "target CPU does not support ARM mode" "missing error with -marm on Thumb-only targets" { target *-*-*} 0 } */ +/* { dg-error "target CPU does not support ARM mode" "missing error with -marm on Thumb-only targets" { target *-*-* } 0 } */ /* Check that -marm gives an error when compiling for a Thumb-only target. */ diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 0fc0bafa67d8d34ec74ce2d1d7a2323a375615cc..f7511ef3aebca72c798496fb95ce43fcbbc08ed1 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -252,6 +252,20 @@ proc check_runtime {prop args} { }] } +# Return 1 if GCC was configured with $pattern. +proc check_configured_with { pattern } { + global tool + + set gcc_output [${tool}_target_compile "-v" "" "none" ""] + if { [ regexp "Configured with: \[^\n\]*$pattern" $gcc_output ] } { + verbose "Matched: $pattern" 2 + return 1 + } + + verbose "Failed to match: $pattern" 2 + return 0 +} + ############################### # proc check_weak_available { } ############################### @@ -3797,6 +3811,12 @@ proc add_options_for_arm_arch_v7ve { flags } { return "$flags -march=armv7ve" } +# Return 1 if GCC was configured with --with-mode= +proc check_effective_target_default_mode { } { + + return [check_configured_with "with-mode="] +} + # Return 1 if this is an ARM target where -marm causes ARM to be # used (not Thumb)
Hi, The logic to make -mthumb optional for Thumb-only devices is only executed when no -marm or -mthumb is given on the command-line. This includes configuring GCC with --with-mode= because this makes the option to be passed before any others. The optional_mthumb-* testcases are skipped when -marm or -mthumb is passed on the command line but not when GCC was configured with --with-mode. Not only are the tests meaningless in these configurations, they also spuriously FAIL if --with-mode=arm was used since the test are built for Thumb-only devices, as reported by Christophe Lyon in [1]. [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02082.html This patch adds logic to target-support.exp to check how was GCC configured and changes the optional_mthumb testcases to be skipped if there is a default mode (--with-mode=). It also fixes a couple of typo on the selectors. ChangeLog entry is as follows: *** gcc/testsuite/ChangeLog *** 2016-12-09 Thomas Preud'homme <thomas.preudhomme@arm.com> * lib/target-supports.exp (check_configured_with): New procedure. (check_effective_target_default_mode): new effective target. * gcc.target/arm/optional_thumb-1.c: Skip if GCC was configured with a default mode. Fix dg-skip-if target selector syntax. * gcc.target/arm/optional_thumb-2.c: Likewise. * gcc.target/arm/optional_thumb-3.c: Fix dg-skip-if target selector syntax. Is this ok for stage3? Best regards, Thomas