[PATCH] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[PATCH] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Nick Desaulniers]

Otherwise other compilers, like Clang, are prevented from compiling the
kernel. This flag was introduced in
3f135e57a4f76d24ae8d8a490314331f0ced40c5.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4430dd4..5a0ac84 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
 endif
 
 ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
-	KBUILD_CFLAGS += -maccumulate-outgoing-args
+	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif
 
 # Stackpointer is addressed different for 32 bit and 64 bit x86
-- 
2.9.3

Re: [PATCH] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Josh Poimboeuf]

On Wed, May 03, 2017 at 11:23:41PM -0700, Nick Desaulniers wrote:
> Otherwise other compilers, like Clang, are prevented from compiling the
> kernel. This flag was introduced in
> 3f135e57a4f76d24ae8d8a490314331f0ced40c5.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  arch/x86/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 4430dd4..5a0ac84 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
>  endif
>  
>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
> +	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
>  endif

I think this might silently cover up errors for older versions of gcc.
For example, what happens if '-maccumulate-outgoing-args' is needed, but
the version of gcc doesn't support it.  In that case I'd rather see gcc
report an error that the flag isn't supported, instead of it getting
silently disabled.

So how about this instead?

	KBUILD_CFLAGS += $(if $(filter gcc,$(cc-name)),-maccumulate-outgoing-args)

-- 
Josh

[Patch v2] x86/build: require only gcc use -maccumulate-outgoing-args

[Nick Desaulniers]

Other compilers, like clang, treat unknown compiler flags as errors.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4430dd489620..12757a252e6b 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
 endif
 
 ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
-	KBUILD_CFLAGS += -maccumulate-outgoing-args
+	KBUILD_CFLAGS += $(if $(filter gcc,$(cc-name)),-maccumulate-outgoing-args)
 endif
 
 # Stackpointer is addressed different for 32 bit and 64 bit x86
-- 
2.11.0

Re: [Patch v2] x86/build: require only gcc use -maccumulate-outgoing-args

[Ingo Molnar]

* Nick Desaulniers <nick.desaulniers@gmail.com> wrote:

> Other compilers, like clang, treat unknown compiler flags as errors.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  arch/x86/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 4430dd489620..12757a252e6b 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
>  endif
>  
>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
> +	KBUILD_CFLAGS += $(if $(filter gcc,$(cc-name)),-maccumulate-outgoing-args)
>  endif
>  
>  # Stackpointer is addressed different for 32 bit and 64 bit x86

The justification Josh gave for this pattern should be put into a comment and into 
the changelog as well.

Thanks,

	Ingo

Re: [Patch v2] x86/build: require only gcc use -maccumulate-outgoing-args

[hpa]

On May 4, 2017 11:23:33 PM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Nick Desaulniers <nick.desaulniers@gmail.com> wrote:
>
>> Other compilers, like clang, treat unknown compiler flags as errors.
>> 
>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>> ---
>>  arch/x86/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
>> index 4430dd489620..12757a252e6b 100644
>> --- a/arch/x86/Makefile
>> +++ b/arch/x86/Makefile
>> @@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
>>  endif
>>  
>>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
>> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
>> +	KBUILD_CFLAGS += $(if $(filter
>gcc,$(cc-name)),-maccumulate-outgoing-args)
>>  endif
>>  
>>  # Stackpointer is addressed different for 32 bit and 64 bit x86
>
>The justification Josh gave for this pattern should be put into a
>comment and into 
>the changelog as well.
>
>Thanks,
>
>	Ingo

However, I don't think Josh's explanation is correct.  I am pretty sure it is a performance issue, not a correctness issue, and besides, a version of gcc that old won't be able to compile the kernel for other reasons, as evidenced by the fact that noone has complained about this option being mandatory.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Patch v2] x86/build: require only gcc use -maccumulate-outgoing-args

[Josh Poimboeuf]

On Thu, May 04, 2017 at 11:38:57PM -0700, hpa@zytor.com wrote:
> On May 4, 2017 11:23:33 PM PDT, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >* Nick Desaulniers <nick.desaulniers@gmail.com> wrote:
> >
> >> Other compilers, like clang, treat unknown compiler flags as errors.
> >> 
> >> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> >> ---
> >>  arch/x86/Makefile | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> >> index 4430dd489620..12757a252e6b 100644
> >> --- a/arch/x86/Makefile
> >> +++ b/arch/x86/Makefile
> >> @@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
> >>  endif
> >>  
> >>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> >> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
> >> +	KBUILD_CFLAGS += $(if $(filter
> >gcc,$(cc-name)),-maccumulate-outgoing-args)
> >>  endif
> >>  
> >>  # Stackpointer is addressed different for 32 bit and 64 bit x86
> >
> >The justification Josh gave for this pattern should be put into a
> >comment and into 
> >the changelog as well.
> >
> >Thanks,
> >
> >	Ingo
> 
> However, I don't think Josh's explanation is correct.  I am pretty
> sure it is a performance issue, not a correctness issue

Why wouldn't it be a correctness issue?  The option is needed in a few
cases (involving older versions of gcc and certain configs) to avoid
some bugs (see the Makefile for more details).

> and besides, a version of gcc that old won't be able to compile the
> kernel for other reasons, as evidenced by the fact that noone has
> complained about this option being mandatory.

Yeah.  Looking at the gcc source, the option has actually been around
since 2000.  So, never mind!

I'd be ok with v1, plus a comment saying that clang doesn't support the
option.

-- 
Josh

[Patch v3] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Nick Desaulniers]

Clang does not support this machine dependent option.

Older versions of GCC (pre 3.0) may not support this option, added in
2000, but it's unlikely they can still compile the kernel.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4430dd489620..5a0ac8411792 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
 endif
 
 ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
-	KBUILD_CFLAGS += -maccumulate-outgoing-args
+	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif
 
 # Stackpointer is addressed different for 32 bit and 64 bit x86
-- 
2.11.0

Re: [Patch v3] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Josh Poimboeuf]

On Fri, May 05, 2017 at 08:05:09PM -0700, Nick Desaulniers wrote:
> Clang does not support this machine dependent option.
> 
> Older versions of GCC (pre 3.0) may not support this option, added in
> 2000, but it's unlikely they can still compile the kernel.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  arch/x86/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 4430dd489620..5a0ac8411792 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -179,7 +179,7 @@ ifdef CONFIG_JUMP_LABEL
>  endif
>  
>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
> +	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
>  endif
>  
>  # Stackpointer is addressed different for 32 bit and 64 bit x86

It would be useful to add a comment in the Makefile above the
KBUILD_CFLAGS line stating that clang doesn't support this flag.

-- 
Josh

[Patch v4] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Nick Desaulniers]

Clang does not support this machine dependent option.

Older versions of GCC (pre 3.0) may not support this option, added in
2000, but it's unlikely they can still compile the kernel.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4430dd489620..8c4dcba9a6a7 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -179,7 +179,8 @@ ifdef CONFIG_JUMP_LABEL
 endif
 
 ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
-	KBUILD_CFLAGS += -maccumulate-outgoing-args
+	# Unsupported by Clang.
+	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif
 
 # Stackpointer is addressed different for 32 bit and 64 bit x86
-- 
2.11.0

Re: [Patch v4] x86/build: don't add -maccumulate-outgoing-args w/o compiler support

[Josh Poimboeuf]

On Mon, May 08, 2017 at 08:29:46PM -0700, Nick Desaulniers wrote:
> Clang does not support this machine dependent option.
> 
> Older versions of GCC (pre 3.0) may not support this option, added in
> 2000, but it's unlikely they can still compile the kernel.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  arch/x86/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 4430dd489620..8c4dcba9a6a7 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -179,7 +179,8 @@ ifdef CONFIG_JUMP_LABEL
>  endif
>  
>  ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> -	KBUILD_CFLAGS += -maccumulate-outgoing-args
> +	# Unsupported by Clang.
> +	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
>  endif
>  
>  # Stackpointer is addressed different for 32 bit and 64 bit x86

Looks good to me, thanks for the iterations.

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[tip:x86/urgent] x86/build: Don't add -maccumulate-outgoing-args w/o compiler support

[tip-bot for Nick Desaulniers]

Commit-ID:  4a1bec4605838fd7872ec41677585e241e256785
Gitweb:     http://git.kernel.org/tip/4a1bec4605838fd7872ec41677585e241e256785
Author:     Nick Desaulniers <nick.desaulniers@gmail.com>
AuthorDate: Mon, 8 May 2017 20:29:46 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 9 May 2017 08:16:45 +0200

x86/build: Don't add -maccumulate-outgoing-args w/o compiler support

Clang does not support this machine dependent option.

Older versions of GCC (pre 3.0) may not support this option, added in
2000, but it's unlikely they can still compile a working kernel.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170509032946.20444-1-nick.desaulniers@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4430dd4..5851411 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -179,7 +179,8 @@ ifdef CONFIG_JUMP_LABEL
 endif
 
 ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
-	KBUILD_CFLAGS += -maccumulate-outgoing-args
+	# This compiler flag is not supported by Clang:
+	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif
 
 # Stackpointer is addressed different for 32 bit and 64 bit x86