diff mbox

[2/5] toolchain/external: fix wrapper by not passing conflicting flags

Message ID 5d0fbb9b1f4439750dde9bb7788b273ec94a3bd5.1389134731.git.yann.morin.1998@free.fr
State Accepted
Commit 2c1dc32647eb308126b0ae80a91988059d39aa7b
Headers show

Commit Message

Yann E. MORIN Jan. 7, 2014, 10:46 p.m. UTC 
From: "Yann E. MORIN" <yann.morin.1998@free.fr>

In our wrapper, we forcibly add the -march=, -mcpu= and-mtune= flags
to the actual copmpiler, this in an attempt to always generate correct
and optimised code for the target.

But in some cases, the caller knows better than we do, and passes its
own set, or subset of those flags. In this case, some may conflict with
the ones we pass. The most prominent offender being the Linux kernel.

For example, on the ARM Rapsberry Pi, the Linux kernel will set the
-march=armv6 flag and no -mcpu= flag, but we pass -mcpu=arm1176jzf-s,
which conflicts:

    drivers/scsi/scsi_trace.c:1:0: warning: switch -mcpu=arm1176jzf-s
    conflicts with -march=armv6 switch

(and so for all the files the kernel compiles, pretty messy)
(note: arm1176jzf-s is not an armv6, it is an armv6zk. Yeah...)

To avoid this situation, we scan our commandline for any occurence of
the possibly conflicting flags. If none is found, then we add our owns.
If any is found, then we don't add any of our owns.

The idea behind this is that we trust the caller to know better than
we do what it is doing. Since the biggest, and sole so far, offender
is the Linux kernel, then this is a rather safe bet.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Peter Korsgaard <jacmet@uclibc.org>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Maxime Hadjinlian <maxime.hadjinlian@gmail.com>
---
 .../toolchain-external/ext-toolchain-wrapper.c     | 41 ++++++++++++++++------
 1 file changed, 31 insertions(+), 10 deletions(-)

Comments

Maxime Hadjinlian Jan. 8, 2014, 12:45 p.m. UTC | #1 
Hi Yann, all

On Tue, Jan 7, 2014 at 11:46 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> In our wrapper, we forcibly add the -march=, -mcpu= and-mtune= flags
> to the actual copmpiler, this in an attempt to always generate correct
> and optimised code for the target.
>
> But in some cases, the caller knows better than we do, and passes its
> own set, or subset of those flags. In this case, some may conflict with
> the ones we pass. The most prominent offender being the Linux kernel.
>
> For example, on the ARM Rapsberry Pi, the Linux kernel will set the
> -march=armv6 flag and no -mcpu= flag, but we pass -mcpu=arm1176jzf-s,
> which conflicts:
>
>     drivers/scsi/scsi_trace.c:1:0: warning: switch -mcpu=arm1176jzf-s
>     conflicts with -march=armv6 switch
>
> (and so for all the files the kernel compiles, pretty messy)
> (note: arm1176jzf-s is not an armv6, it is an armv6zk. Yeah...)
>
> To avoid this situation, we scan our commandline for any occurence of
> the possibly conflicting flags. If none is found, then we add our owns.
> If any is found, then we don't add any of our owns.
>
> The idea behind this is that we trust the caller to know better than
> we do what it is doing. Since the biggest, and sole so far, offender
> is the Linux kernel, then this is a rather safe bet.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <jacmet@uclibc.org>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Maxime Hadjinlian <maxime.hadjinlian@gmail.com>
> ---
>  .../toolchain-external/ext-toolchain-wrapper.c     | 41 ++++++++++++++++------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> index d54f1f5..81c6ed1 100644
> --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
> +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> @@ -30,21 +30,17 @@ static char sysroot[PATH_MAX];
>   * that we only pass the predefined one to the real compiler if the inverse
>   * option isn't in the argument list.
>   * This specifies the worst case number of extra arguments we might pass
> + * Currently, we have:
> + *     -mfloat-abi=
> + *     -march=
> + *     -mtune=
> + *     -mcpu=
>   */
> -#define EXCLUSIVE_ARGS 1
> +#define EXCLUSIVE_ARGS 4
>
>  static char *predef_args[] = {
>         path,
>         "--sysroot", sysroot,
> -#ifdef BR_ARCH
> -       "-march=" BR_ARCH,
> -#endif /* BR_ARCH */
> -#ifdef BR_TUNE
> -       "-mtune=" BR_TUNE,
> -#endif /* BR_TUNE */
> -#ifdef BR_CPU
> -       "-mcpu=" BR_CPU,
> -#endif
>  #ifdef BR_ABI
>         "-mabi=" BR_ABI,
>  #endif
> @@ -157,6 +153,31 @@ int main(int argc, char **argv)
>                 *cur++ = "-mfloat-abi=" BR_FLOAT_ABI;
>  #endif
>
> +#if defined(BR_ARCH) || \
> +    defined(BR_TUNE) || \
> +    defined(BR_CPU)
> +       /* Add our -march/cpu/tune/abi flags, but only if none are
> +        * already specified on the commandline
> +        */
> +       for (i = 1; i < argc; i++) {
> +               if (!strncmp(argv[i], "-march=", strlen("-march=")) ||
> +                   !strncmp(argv[i], "-mtune=", strlen("-mtune=")) ||
> +                   !strncmp(argv[i], "-mcpu=",  strlen("-mcpu=" )))
> +                       break;
> +       }
> +       if (i == argc) {
> +#ifdef BR_ARCH
> +               *cur++ = "-march=" BR_ARCH;
> +#endif
> +#ifdef BR_TUNE
> +               *cur++ = "-mtune=" BR_TUNE;
> +#endif
> +#ifdef BR_CPU
> +               *cur++ = "-mcpu=" BR_CPU;
> +#endif
> +       }
> +#endif /* ARCH || TUNE || CPU */
> +
>         /* append forward args */
>         memcpy(cur, &argv[1], sizeof(char *) * (argc - 1));
>         cur += argc - 1;
> --
> 1.8.1.2
>
Seems good to me.
Shouldn't we change the different defconfig that exists ? At least the
RaspberryPi so it would sets an example (and maybe add something about
this in the manual ?)
Thomas Petazzoni Jan. 8, 2014, 3:51 p.m. UTC | #2 
Dear Maxime Hadjinlian,

On Wed, 8 Jan 2014 13:45:59 +0100, Maxime Hadjinlian wrote:

> Shouldn't we change the different defconfig that exists ? At least the
> RaspberryPi so it would sets an example (and maybe add something about
> this in the manual ?)

Why would the defconfig need some changes following this patch?

Thomas
Maxime Hadjinlian Jan. 8, 2014, 3:57 p.m. UTC | #3 
On Wed, Jan 8, 2014 at 4:51 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Maxime Hadjinlian,
Hi Thomas,
>
> On Wed, 8 Jan 2014 13:45:59 +0100, Maxime Hadjinlian wrote:
>
>> Shouldn't we change the different defconfig that exists ? At least the
>> RaspberryPi so it would sets an example (and maybe add something about
>> this in the manual ?)
>
> Why would the defconfig need some changes following this patch?
To set a visible example of settings the correct -march/mcpu/mtune options ?
Also, this would eliminate all the warnings that Yann saw while
building a kernel for the rpi, so it seems like a good candidate.
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
Yann E. MORIN Jan. 8, 2014, 6:01 p.m. UTC | #4 
Maxime, All,

On 2014-01-08 16:57 +0100, Maxime Hadjinlian spake thusly:
> On Wed, Jan 8, 2014 at 4:51 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Dear Maxime Hadjinlian,
> Hi Thomas,
> >
> > On Wed, 8 Jan 2014 13:45:59 +0100, Maxime Hadjinlian wrote:
> >
> >> Shouldn't we change the different defconfig that exists ? At least the
> >> RaspberryPi so it would sets an example (and maybe add something about
> >> this in the manual ?)
> >
> > Why would the defconfig need some changes following this patch?
> To set a visible example of settings the correct -march/mcpu/mtune options ?
> Also, this would eliminate all the warnings that Yann saw while
> building a kernel for the rpi, so it seems like a good candidate.

There is nothing in that patch that impacts the configuration.
There is no reason to change any defconfig to 'activate' this change.

Rather, the config options BR2_GCC_TARGET_CPU, BR2_GCC_TARGET_ARCH, and
BR2_GCC_TARGET_MTUNE, if any, are pased at the time we build the wrapper
itself.

Those options are set automatically when you choose, in the menuconfig,
the type of CPU your target has. And since that will automatically set
the BR2_GCC_TARGET_{ARCH,CPU,MTUNE} options, they are forcibly set.

And since the raspberrypi_defconfig (as you use that as an example)
already sets BR2_arm1176jzf_s=y, this will set BR2_GCC_TARGET_ARCH and
BR2_GCC_TARGET_CPU.

And this is exactly this situation this patch fixes: our options forcing
the compiler flags, when some are already provided on the command line.

So, there is no need (and no reason) to change any of our defconfigs to
take advantage of this change.

Regards,
Yann E. MORIN.
Maxime Hadjinlian Jan. 8, 2014, 6:46 p.m. UTC | #5 
Hi all,

On Wed, Jan 8, 2014 at 7:01 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Maxime, All,
>
> On 2014-01-08 16:57 +0100, Maxime Hadjinlian spake thusly:
>> On Wed, Jan 8, 2014 at 4:51 PM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Dear Maxime Hadjinlian,
>> Hi Thomas,
>> >
>> > On Wed, 8 Jan 2014 13:45:59 +0100, Maxime Hadjinlian wrote:
>> >
>> >> Shouldn't we change the different defconfig that exists ? At least the
>> >> RaspberryPi so it would sets an example (and maybe add something about
>> >> this in the manual ?)
>> >
>> > Why would the defconfig need some changes following this patch?
>> To set a visible example of settings the correct -march/mcpu/mtune options ?
>> Also, this would eliminate all the warnings that Yann saw while
>> building a kernel for the rpi, so it seems like a good candidate.
>
> There is nothing in that patch that impacts the configuration.
> There is no reason to change any defconfig to 'activate' this change.
>
> Rather, the config options BR2_GCC_TARGET_CPU, BR2_GCC_TARGET_ARCH, and
> BR2_GCC_TARGET_MTUNE, if any, are pased at the time we build the wrapper
> itself.
>
> Those options are set automatically when you choose, in the menuconfig,
> the type of CPU your target has. And since that will automatically set
> the BR2_GCC_TARGET_{ARCH,CPU,MTUNE} options, they are forcibly set.
>
> And since the raspberrypi_defconfig (as you use that as an example)
> already sets BR2_arm1176jzf_s=y, this will set BR2_GCC_TARGET_ARCH and
> BR2_GCC_TARGET_CPU.
>
> And this is exactly this situation this patch fixes: our options forcing
> the compiler flags, when some are already provided on the command line.
>
> So, there is no need (and no reason) to change any of our defconfigs to
> take advantage of this change.
>
> Regards,
> Yann E. MORIN.
My  bad,
I spoke too quickly and I misunderstood some information.

Thanks for your clarifications.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
Peter Korsgaard Jan. 9, 2014, 8:28 p.m. UTC | #6 
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > In our wrapper, we forcibly add the -march=, -mcpu= and-mtune= flags
 > to the actual copmpiler, this in an attempt to always generate correct
 > and optimised code for the target.

 > But in some cases, the caller knows better than we do, and passes its
 > own set, or subset of those flags. In this case, some may conflict with
 > the ones we pass. The most prominent offender being the Linux kernel.

 > For example, on the ARM Rapsberry Pi, the Linux kernel will set the
 > -march=armv6 flag and no -mcpu= flag, but we pass -mcpu=arm1176jzf-s,
 > which conflicts:

 >     drivers/scsi/scsi_trace.c:1:0: warning: switch -mcpu=arm1176jzf-s
 >     conflicts with -march=armv6 switch

 > (and so for all the files the kernel compiles, pretty messy)
 > (note: arm1176jzf-s is not an armv6, it is an armv6zk. Yeah...)

 > To avoid this situation, we scan our commandline for any occurence of
 > the possibly conflicting flags. If none is found, then we add our owns.
 > If any is found, then we don't add any of our owns.

 > The idea behind this is that we trust the caller to know better than
 > we do what it is doing. Since the biggest, and sole so far, offender
 > is the Linux kernel, then this is a rather safe bet.

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
 > Cc: Peter Korsgaard <jacmet@uclibc.org>
 > Cc: Arnout Vandecappelle <arnout@mind.be>
 > Cc: Maxime Hadjinlian <maxime.hadjinlian@gmail.com>

Committed, thanks.
diff mbox

Patch

diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
index d54f1f5..81c6ed1 100644
--- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
+++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
@@ -30,21 +30,17 @@  static char sysroot[PATH_MAX];
  * that we only pass the predefined one to the real compiler if the inverse
  * option isn't in the argument list.
  * This specifies the worst case number of extra arguments we might pass
+ * Currently, we have:
+ * 	-mfloat-abi=
+ * 	-march=
+ * 	-mtune=
+ * 	-mcpu=
  */
-#define EXCLUSIVE_ARGS	1
+#define EXCLUSIVE_ARGS	4
 
 static char *predef_args[] = {
 	path,
 	"--sysroot", sysroot,
-#ifdef BR_ARCH
-	"-march=" BR_ARCH,
-#endif /* BR_ARCH */
-#ifdef BR_TUNE
-	"-mtune=" BR_TUNE,
-#endif /* BR_TUNE */
-#ifdef BR_CPU
-	"-mcpu=" BR_CPU,
-#endif
 #ifdef BR_ABI
 	"-mabi=" BR_ABI,
 #endif
@@ -157,6 +153,31 @@  int main(int argc, char **argv)
 		*cur++ = "-mfloat-abi=" BR_FLOAT_ABI;
 #endif
 
+#if defined(BR_ARCH) || \
+    defined(BR_TUNE) || \
+    defined(BR_CPU)
+	/* Add our -march/cpu/tune/abi flags, but only if none are
+	 * already specified on the commandline
+	 */
+	for (i = 1; i < argc; i++) {
+		if (!strncmp(argv[i], "-march=", strlen("-march=")) ||
+		    !strncmp(argv[i], "-mtune=", strlen("-mtune=")) ||
+		    !strncmp(argv[i], "-mcpu=",  strlen("-mcpu=" )))
+			break;
+	}
+	if (i == argc) {
+#ifdef BR_ARCH
+		*cur++ = "-march=" BR_ARCH;
+#endif
+#ifdef BR_TUNE
+		*cur++ = "-mtune=" BR_TUNE;
+#endif
+#ifdef BR_CPU
+		*cur++ = "-mcpu=" BR_CPU;
+#endif
+	}
+#endif /* ARCH || TUNE || CPU */
+
 	/* append forward args */
 	memcpy(cur, &argv[1], sizeof(char *) * (argc - 1));
 	cur += argc - 1;