Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602586 > unrolled thread

[PATCH] x86: mostly disable '-maccumulate-outgoing-args'

Started byJosh Poimboeuf <jpoimboe@redhat.com>
First post2017-03-16 17:00 +0100
Last post2017-03-22 17:00 +0100
Articles 10 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 17:00 +0100
    Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 18:40 +0100
      Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 19:40 +0100
        Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 20:00 +0100
          Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:10 +0100
          Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 20:20 +0100
        Re: [PATCH] x86: mostly disable '-maccumulate-outgoing-args' Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:10 +0100
    [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 20:40 +0100
      Re: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args' Ingo Molnar <mingo@kernel.org> - 2017-03-22 09:00 +0100
        Re: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args' Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-22 17:00 +0100

#1602586 — [PATCH] x86: mostly disable '-maccumulate-outgoing-args'

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-16 17:00 +0100
Subject[PATCH] x86: mostly disable '-maccumulate-outgoing-args'
Message-ID<tlFYJ-td-3@gated-at.bofh.it>
Subject: [PATCH] x86: mostly disable '-maccumulate-outgoing-args'

The gcc '-maccumulate-outgoing-args' flag is enabled for most configs,
mostly because of issues which are no longer relevant.  For most
configs, with most recent versions of gcc, it's no longer needed.

Clarify which cases need it, and only enable it for those cases.  Also
produce a compile-time error for the ftrace graph + mcount + '-Os' case,
which will otherwise cause runtime failures.

The main benefit of '-maccumulate-outgoing-args' is that it prevents an
ugly prologue for functions which have aligned stacks.  But removing the
option also has some benefits: more readable argument saves, smaller
text size, and (presumably) slightly improved performance.

Here are the object size savings for 32-bit and 64-bit defconfig
kernels:

      text	   data	    bss	     dec	    hex	filename
  10006710	3543328	1773568	15323606	 e9d1d6	vmlinux.x86-32.before
   9706358	3547424	1773568	15027350	 e54c96	vmlinux.x86-32.after

      text	   data	    bss	     dec	    hex	filename
  10652105	4537576	 843776	16033457	 f4a6b1	vmlinux.x86-64.before
  10639629	4537576	 843776	16020981	 f475f5	vmlinux.x86-64.after

That comes out to a 3% text size improvement on x86-32 and a 0.1% text
size improvement on x86-64.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/Makefile        | 29 +++++++++++++++++++++++++----
 arch/x86/Makefile_32.cpu | 18 ------------------
 arch/x86/kernel/ftrace.c |  6 ++++++
 scripts/Kbuild.include   |  4 ++++
 4 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 2d44933..fa45989b 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -120,10 +120,6 @@ else
         # -funit-at-a-time shrinks the kernel .text considerably
         # unfortunately it makes reading oopses harder.
         KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
-
-        # this works around some issues with generating unwind tables in older gccs
-        # newer gccs do it by default
-        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
 endif
 
 ifdef CONFIG_X86_X32
@@ -147,6 +143,31 @@ ifeq ($(CONFIG_KMEMCHECK),y)
 	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
 endif
 
+# If the function graph tracer is used with mcount instead of fentry,
+# '-maccumulate-outgoing-args' is needed to prevent gcc bug
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
+ifdef CONFIG_FUNCTION_GRAPH_TRACER
+  ifndef CONFIG_HAVE_FENTRY
+	ACCUMULATE_OUTGOING_ARGS := 1
+  else
+    ifeq ($(call cc-option, -mfentry),)
+	ACCUMULATE_OUTGOING_ARGS := 1
+    endif
+  endif
+endif
+
+# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent
+# gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
+ifdef CONFIG_JUMP_LABEL
+  ifneq ($(ACCUMULATE_OUTGOING_ARGS), 1)
+	ACCUMULATE_OUTGOING_ARGS = $(call cc-if-fullversion, -lt, 040502, 1)
+  endif
+endif
+
+ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
+	KBUILD_CFLAGS += -maccumulate-outgoing-args
+endif
+
 # Stackpointer is addressed different for 32 bit and 64 bit x86
 sp-$(CONFIG_X86_32) := esp
 sp-$(CONFIG_X86_64) := rsp
diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
index 6647ed4..a45eb15 100644
--- a/arch/x86/Makefile_32.cpu
+++ b/arch/x86/Makefile_32.cpu
@@ -45,24 +45,6 @@ cflags-$(CONFIG_MGEODE_LX)	+= $(call cc-option,-march=geode,-march=pentium-mmx)
 # cpu entries
 cflags-$(CONFIG_X86_GENERIC) 	+= $(call tune,generic,$(call tune,i686))
 
-# Work around the pentium-mmx code generator madness of gcc4.4.x which
-# does stack alignment by generating horrible code _before_ the mcount
-# prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
-# tracer assumptions. For i686, generic, core2 this is set by the
-# compiler anyway
-ifeq ($(CONFIG_FUNCTION_GRAPH_TRACER), y)
-ADD_ACCUMULATE_OUTGOING_ARGS := y
-endif
-
-# Work around to a bug with asm goto with first implementations of it
-# in gcc causing gcc to mess up the push and pop of the stack in some
-# uses of asm goto.
-ifeq ($(CONFIG_JUMP_LABEL), y)
-ADD_ACCUMULATE_OUTGOING_ARGS := y
-endif
-
-cflags-$(ADD_ACCUMULATE_OUTGOING_ARGS) += $(call cc-option,-maccumulate-outgoing-args)
-
 # Bug fix for binutils: this option is required in order to keep
 # binutils from generating NOPL instructions against our will.
 ifneq ($(CONFIG_X86_P6_NOP),y)
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8f3d9cf..59f9b46 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -29,6 +29,12 @@
 #include <asm/ftrace.h>
 #include <asm/nops.h>
 
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && \
+	!defined(CC_USING_FENTRY) && \
+	!defined(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE)
+# error Your compiler does not support function graph tracing
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index d6ca649..afe3fd3 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -148,6 +148,10 @@ cc-fullversion = $(shell $(CONFIG_SHELL) \
 # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
 cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
 
+# cc-if-fullversion
+# Usage:  EXTRA_CFLAGS += $(call cc-if-fullversion, -lt, 040502, -O1)
+cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo $(4))
+
 # cc-ldoption
 # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
 cc-ldoption = $(call try-run,\
-- 
2.7.4

[toc] | [next] | [standalone]

#1602701

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-03-16 18:40 +0100
Message-ID<tlHxv-1ET-17@gated-at.bofh.it>
In reply to#1602586
On Thu, 16 Mar 2017 10:42:08 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/Makefile        | 29 +++++++++++++++++++++++++----
>  arch/x86/Makefile_32.cpu | 18 ------------------
>  arch/x86/kernel/ftrace.c |  6 ++++++
>  scripts/Kbuild.include   |  4 ++++
>  4 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 2d44933..fa45989b 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -120,10 +120,6 @@ else
>          # -funit-at-a-time shrinks the kernel .text considerably
>          # unfortunately it makes reading oopses harder.
>          KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
> -
> -        # this works around some issues with generating unwind tables in older gccs
> -        # newer gccs do it by default
> -        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
>  endif
>  
>  ifdef CONFIG_X86_X32
> @@ -147,6 +143,31 @@ ifeq ($(CONFIG_KMEMCHECK),y)
>  	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
>  endif
>  
> +# If the function graph tracer is used with mcount instead of fentry,
> +# '-maccumulate-outgoing-args' is needed to prevent gcc bug

			"to prevent a gcc bug"

> +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
> +ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +  ifndef CONFIG_HAVE_FENTRY
> +	ACCUMULATE_OUTGOING_ARGS := 1
> +  else
> +    ifeq ($(call cc-option, -mfentry),)

Hmm, the blank entry makes me nervous. I wonder if it would be better
if we had ifneq ($(call cc-option-yn, -mfentry),y)

Unfortunately, there's one of each in the existing kernel, so there is
really no precedence.

> +	ACCUMULATE_OUTGOING_ARGS := 1
> +    endif
> +  endif
> +endif
> +
> +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent

Can we make a test instead? I hate testing versions, and things get
backported all the time. We usually like to have a test case instead of
relying on versions. Not to mention, a newer gcc may one day break.

-- Steve

> +# gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
> +ifdef CONFIG_JUMP_LABEL
> +  ifneq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> +	ACCUMULATE_OUTGOING_ARGS = $(call cc-if-fullversion, -lt, 040502, 1)
> +  endif
> +endif
> +
> +ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
> +	KBUILD_CFLAGS += -maccumulate-outgoing-args
> +endif
> +
>  # Stackpointer is addressed different for 32 bit and 64 bit x86
>  sp-$(CONFIG_X86_32) := esp
>  sp-$(CONFIG_X86_64) := rsp
> diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
> index 6647ed4..a45eb15 100644
> --- a/arch/x86/Makefile_32.cpu
> +++ b/arch/x86/Makefile_32.cpu
> @@ -45,24 +45,6 @@ cflags-$(CONFIG_MGEODE_LX)	+= $(call cc-option,-march=geode,-march=pentium-mmx)
>  # cpu entries
>  cflags-$(CONFIG_X86_GENERIC) 	+= $(call tune,generic,$(call tune,i686))
>  
> -# Work around the pentium-mmx code generator madness of gcc4.4.x which
> -# does stack alignment by generating horrible code _before_ the mcount
> -# prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
> -# tracer assumptions. For i686, generic, core2 this is set by the
> -# compiler anyway
> -ifeq ($(CONFIG_FUNCTION_GRAPH_TRACER), y)
> -ADD_ACCUMULATE_OUTGOING_ARGS := y
> -endif
> -
> -# Work around to a bug with asm goto with first implementations of it
> -# in gcc causing gcc to mess up the push and pop of the stack in some
> -# uses of asm goto.
> -ifeq ($(CONFIG_JUMP_LABEL), y)
> -ADD_ACCUMULATE_OUTGOING_ARGS := y
> -endif
> -
> -cflags-$(ADD_ACCUMULATE_OUTGOING_ARGS) += $(call cc-option,-maccumulate-outgoing-args)
> -
>  # Bug fix for binutils: this option is required in order to keep
>  # binutils from generating NOPL instructions against our will.
>  ifneq ($(CONFIG_X86_P6_NOP),y)
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8f3d9cf..59f9b46 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -29,6 +29,12 @@
>  #include <asm/ftrace.h>
>  #include <asm/nops.h>
>  
> +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && \
> +	!defined(CC_USING_FENTRY) && \
> +	!defined(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE)
> +# error Your compiler does not support function graph tracing
> +#endif
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
>  int ftrace_arch_code_modify_prepare(void)
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index d6ca649..afe3fd3 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -148,6 +148,10 @@ cc-fullversion = $(shell $(CONFIG_SHELL) \
>  # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
>  cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
>  
> +# cc-if-fullversion
> +# Usage:  EXTRA_CFLAGS += $(call cc-if-fullversion, -lt, 040502, -O1)
> +cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo $(4))
> +
>  # cc-ldoption
>  # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
>  cc-ldoption = $(call try-run,\

[toc] | [prev] | [next] | [standalone]

#1602768

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-16 19:40 +0100
Message-ID<tlItB-2iT-37@gated-at.bofh.it>
In reply to#1602701
On Thu, Mar 16, 2017 at 01:32:01PM -0400, Steven Rostedt wrote:
> On Thu, 16 Mar 2017 10:42:08 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/Makefile        | 29 +++++++++++++++++++++++++----
> >  arch/x86/Makefile_32.cpu | 18 ------------------
> >  arch/x86/kernel/ftrace.c |  6 ++++++
> >  scripts/Kbuild.include   |  4 ++++
> >  4 files changed, 35 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > index 2d44933..fa45989b 100644
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -120,10 +120,6 @@ else
> >          # -funit-at-a-time shrinks the kernel .text considerably
> >          # unfortunately it makes reading oopses harder.
> >          KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
> > -
> > -        # this works around some issues with generating unwind tables in older gccs
> > -        # newer gccs do it by default
> > -        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
> >  endif
> >  
> >  ifdef CONFIG_X86_X32
> > @@ -147,6 +143,31 @@ ifeq ($(CONFIG_KMEMCHECK),y)
> >  	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
> >  endif
> >  
> > +# If the function graph tracer is used with mcount instead of fentry,
> > +# '-maccumulate-outgoing-args' is needed to prevent gcc bug
> 
> 			"to prevent a gcc bug"

It was

  "to prevent gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109"

where "gcc bug" was an adjective and the URL was a noun.  But yeah,
that's kind of confusing, and the line wrap made it more so.  Maybe I'll
change it to

  "to prevent a gcc bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109)"

and a similar change for the jump label bug comment.

> > +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
> > +ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +  ifndef CONFIG_HAVE_FENTRY
> > +	ACCUMULATE_OUTGOING_ARGS := 1
> > +  else
> > +    ifeq ($(call cc-option, -mfentry),)
> 
> Hmm, the blank entry makes me nervous. I wonder if it would be better
> if we had ifneq ($(call cc-option-yn, -mfentry),y)
> 
> Unfortunately, there's one of each in the existing kernel, so there is
> really no precedence.

Either way seems fine.  I'll go with your suggested change.

> > +	ACCUMULATE_OUTGOING_ARGS := 1
> > +    endif
> > +  endif
> > +endif
> > +
> > +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent
> 
> Can we make a test instead? I hate testing versions, and things get
> backported all the time. We usually like to have a test case instead of
> relying on versions. Not to mention, a newer gcc may one day break.

Tests are generally better, but I'm not sure how to test for this
cleanly.  The test is rather big for embedding in a makefile:

  https://gcc.gnu.org/bugzilla/attachment.cgi?id=22199

Any ideas?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1602776

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-16 20:00 +0100
Message-ID<tlIMV-2t1-1@gated-at.bofh.it>
In reply to#1602768
On Thu, Mar 16, 2017 at 01:36:35PM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 16, 2017 at 01:32:01PM -0400, Steven Rostedt wrote:
> > On Thu, 16 Mar 2017 10:42:08 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > +	ACCUMULATE_OUTGOING_ARGS := 1
> > > +    endif
> > > +  endif
> > > +endif
> > > +
> > > +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent
> > 
> > Can we make a test instead? I hate testing versions, and things get
> > backported all the time. We usually like to have a test case instead of
> > relying on versions. Not to mention, a newer gcc may one day break.
> 
> Tests are generally better, but I'm not sure how to test for this
> cleanly.  The test is rather big for embedding in a makefile:
> 
>   https://gcc.gnu.org/bugzilla/attachment.cgi?id=22199
> 
> Any ideas?

After some snooping I discovered there's some precedent for doing this
in the scripts/gcc-*.sh files.  So maybe I'll add a test there and call
it from the Makefile.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1602783

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-03-16 20:10 +0100
Message-ID<tlIWC-2Na-5@gated-at.bofh.it>
In reply to#1602776
On Thu, 16 Mar 2017 14:04:01 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > After some snooping I discovered there's some precedent for doing this
> > in the scripts/gcc-*.sh files.  So maybe I'll add a test there and call
> > it from the Makefile.  
> 
> But now I realize that those other tests are just build tests, whereas
> this one needs to be executed.  That's a no-go for cross compilers.  So
> I think we need to do the version check after all.
> 

Fine, but please add a comment saying such.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602789

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-16 20:20 +0100
Message-ID<tlIWC-2Na-7@gated-at.bofh.it>
In reply to#1602776
On Thu, Mar 16, 2017 at 01:53:05PM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 16, 2017 at 01:36:35PM -0500, Josh Poimboeuf wrote:
> > On Thu, Mar 16, 2017 at 01:32:01PM -0400, Steven Rostedt wrote:
> > > On Thu, 16 Mar 2017 10:42:08 -0500
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > +	ACCUMULATE_OUTGOING_ARGS := 1
> > > > +    endif
> > > > +  endif
> > > > +endif
> > > > +
> > > > +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent
> > > 
> > > Can we make a test instead? I hate testing versions, and things get
> > > backported all the time. We usually like to have a test case instead of
> > > relying on versions. Not to mention, a newer gcc may one day break.
> > 
> > Tests are generally better, but I'm not sure how to test for this
> > cleanly.  The test is rather big for embedding in a makefile:
> > 
> >   https://gcc.gnu.org/bugzilla/attachment.cgi?id=22199
> > 
> > Any ideas?
> 
> After some snooping I discovered there's some precedent for doing this
> in the scripts/gcc-*.sh files.  So maybe I'll add a test there and call
> it from the Makefile.

But now I realize that those other tests are just build tests, whereas
this one needs to be executed.  That's a no-go for cross compilers.  So
I think we need to do the version check after all.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1602785

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-03-16 20:10 +0100
Message-ID<tlIWC-2Na-13@gated-at.bofh.it>
In reply to#1602768
On Thu, 16 Mar 2017 13:36:35 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Thu, Mar 16, 2017 at 01:32:01PM -0400, Steven Rostedt wrote:
> > On Thu, 16 Mar 2017 10:42:08 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >   
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > ---
> > >  arch/x86/Makefile        | 29 +++++++++++++++++++++++++----
> > >  arch/x86/Makefile_32.cpu | 18 ------------------
> > >  arch/x86/kernel/ftrace.c |  6 ++++++
> > >  scripts/Kbuild.include   |  4 ++++
> > >  4 files changed, 35 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > > index 2d44933..fa45989b 100644
> > > --- a/arch/x86/Makefile
> > > +++ b/arch/x86/Makefile
> > > @@ -120,10 +120,6 @@ else
> > >          # -funit-at-a-time shrinks the kernel .text considerably
> > >          # unfortunately it makes reading oopses harder.
> > >          KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
> > > -
> > > -        # this works around some issues with generating unwind tables in older gccs
> > > -        # newer gccs do it by default
> > > -        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
> > >  endif
> > >  
> > >  ifdef CONFIG_X86_X32
> > > @@ -147,6 +143,31 @@ ifeq ($(CONFIG_KMEMCHECK),y)
> > >  	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
> > >  endif
> > >  
> > > +# If the function graph tracer is used with mcount instead of fentry,
> > > +# '-maccumulate-outgoing-args' is needed to prevent gcc bug  
> > 
> > 			"to prevent a gcc bug"  
> 
> It was
> 
>   "to prevent gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109"
> 
> where "gcc bug" was an adjective and the URL was a noun.  But yeah,
> that's kind of confusing, and the line wrap made it more so.  Maybe I'll
> change it to
> 
>   "to prevent a gcc bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109)"

Hmm, "the" would have made it work too.

> 
> and a similar change for the jump label bug comment.
> 
> > > +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
> > > +ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +  ifndef CONFIG_HAVE_FENTRY
> > > +	ACCUMULATE_OUTGOING_ARGS := 1
> > > +  else
> > > +    ifeq ($(call cc-option, -mfentry),)  
> > 
> > Hmm, the blank entry makes me nervous. I wonder if it would be better
> > if we had ifneq ($(call cc-option-yn, -mfentry),y)
> > 
> > Unfortunately, there's one of each in the existing kernel, so there is
> > really no precedence.  
> 
> Either way seems fine.  I'll go with your suggested change.
> 
> > > +	ACCUMULATE_OUTGOING_ARGS := 1
> > > +    endif
> > > +  endif
> > > +endif
> > > +
> > > +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent  
> > 
> > Can we make a test instead? I hate testing versions, and things get
> > backported all the time. We usually like to have a test case instead of
> > relying on versions. Not to mention, a newer gcc may one day break.  
> 
> Tests are generally better, but I'm not sure how to test for this
> cleanly.  The test is rather big for embedding in a makefile:
> 
>   https://gcc.gnu.org/bugzilla/attachment.cgi?id=22199
> 
> Any ideas?
> 

I'd reply but I see you figured it out yourself.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1602807 — [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-16 20:40 +0100
Subject[PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'
Message-ID<tlJpE-2Zo-13@gated-at.bofh.it>
In reply to#1602586
The gcc '-maccumulate-outgoing-args' flag is enabled for most configs,
mostly because of issues which are no longer relevant.  For most
configs, and with most recent versions of gcc, it's no longer needed.

Clarify which cases need it, and only enable it for those cases.  Also
produce a compile-time error for the ftrace graph + mcount + '-Os' case,
which will otherwise cause runtime failures.

The main benefit of '-maccumulate-outgoing-args' is that it prevents an
ugly prologue for functions which have aligned stacks.  But removing the
option also has some benefits: more readable argument saves, smaller
text size, and (presumably) slightly improved performance.

Here are the object size savings for 32-bit and 64-bit defconfig
kernels:

      text	   data	    bss	     dec	    hex	filename
  10006710	3543328	1773568	15323606	 e9d1d6	vmlinux.x86-32.before
   9706358	3547424	1773568	15027350	 e54c96	vmlinux.x86-32.after

      text	   data	    bss	     dec	    hex	filename
  10652105	4537576	 843776	16033457	 f4a6b1	vmlinux.x86-64.before
  10639629	4537576	 843776	16020981	 f475f5	vmlinux.x86-64.after

That comes out to a 3% text size improvement on x86-32 and a 0.1% text
size improvement on x86-64.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
v2:
- improve readability of the comments
- add comment about why gcc version needs to be checked
- use cc-option-yn instead of cc-option

 arch/x86/Makefile        | 31 +++++++++++++++++++++++++++----
 arch/x86/Makefile_32.cpu | 18 ------------------
 arch/x86/kernel/ftrace.c |  6 ++++++
 scripts/Kbuild.include   |  4 ++++
 4 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 2d44933..04c87be 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -120,10 +120,6 @@ else
         # -funit-at-a-time shrinks the kernel .text considerably
         # unfortunately it makes reading oopses harder.
         KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
-
-        # this works around some issues with generating unwind tables in older gccs
-        # newer gccs do it by default
-        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
 endif
 
 ifdef CONFIG_X86_X32
@@ -147,6 +143,33 @@ ifeq ($(CONFIG_KMEMCHECK),y)
 	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
 endif
 
+# If the function graph tracer is used with mcount instead of fentry,
+# '-maccumulate-outgoing-args' is needed to prevent a gcc bug
+# (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109)
+ifdef CONFIG_FUNCTION_GRAPH_TRACER
+  ifndef CONFIG_HAVE_FENTRY
+	ACCUMULATE_OUTGOING_ARGS := 1
+  else
+    ifeq ($(call cc-option-yn, -mfentry), n)
+	ACCUMULATE_OUTGOING_ARGS := 1
+    endif
+  endif
+endif
+
+# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent a
+# gcc bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226).  There's no way
+# to test for this bug at compile-time because the test case needs to execute,
+# which is a no-go for cross compilers.  So check the gcc version instead.
+ifdef CONFIG_JUMP_LABEL
+  ifneq ($(ACCUMULATE_OUTGOING_ARGS), 1)
+	ACCUMULATE_OUTGOING_ARGS = $(call cc-if-fullversion, -lt, 040502, 1)
+  endif
+endif
+
+ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
+	KBUILD_CFLAGS += -maccumulate-outgoing-args
+endif
+
 # Stackpointer is addressed different for 32 bit and 64 bit x86
 sp-$(CONFIG_X86_32) := esp
 sp-$(CONFIG_X86_64) := rsp
diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
index 6647ed4..a45eb15 100644
--- a/arch/x86/Makefile_32.cpu
+++ b/arch/x86/Makefile_32.cpu
@@ -45,24 +45,6 @@ cflags-$(CONFIG_MGEODE_LX)	+= $(call cc-option,-march=geode,-march=pentium-mmx)
 # cpu entries
 cflags-$(CONFIG_X86_GENERIC) 	+= $(call tune,generic,$(call tune,i686))
 
-# Work around the pentium-mmx code generator madness of gcc4.4.x which
-# does stack alignment by generating horrible code _before_ the mcount
-# prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
-# tracer assumptions. For i686, generic, core2 this is set by the
-# compiler anyway
-ifeq ($(CONFIG_FUNCTION_GRAPH_TRACER), y)
-ADD_ACCUMULATE_OUTGOING_ARGS := y
-endif
-
-# Work around to a bug with asm goto with first implementations of it
-# in gcc causing gcc to mess up the push and pop of the stack in some
-# uses of asm goto.
-ifeq ($(CONFIG_JUMP_LABEL), y)
-ADD_ACCUMULATE_OUTGOING_ARGS := y
-endif
-
-cflags-$(ADD_ACCUMULATE_OUTGOING_ARGS) += $(call cc-option,-maccumulate-outgoing-args)
-
 # Bug fix for binutils: this option is required in order to keep
 # binutils from generating NOPL instructions against our will.
 ifneq ($(CONFIG_X86_P6_NOP),y)
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8f3d9cf..59f9b46 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -29,6 +29,12 @@
 #include <asm/ftrace.h>
 #include <asm/nops.h>
 
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && \
+	!defined(CC_USING_FENTRY) && \
+	!defined(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE)
+# error Your compiler does not support function graph tracing
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index d6ca649..afe3fd3 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -148,6 +148,10 @@ cc-fullversion = $(shell $(CONFIG_SHELL) \
 # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
 cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
 
+# cc-if-fullversion
+# Usage:  EXTRA_CFLAGS += $(call cc-if-fullversion, -lt, 040502, -O1)
+cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo $(4))
+
 # cc-ldoption
 # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
 cc-ldoption = $(call try-run,\
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1606267 — Re: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'

FromIngo Molnar <mingo@kernel.org>
Date2017-03-22 09:00 +0100
SubjectRe: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'
Message-ID<tnJlw-7r8-11@gated-at.bofh.it>
In reply to#1602807
* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> The gcc '-maccumulate-outgoing-args' flag is enabled for most configs,
> mostly because of issues which are no longer relevant.  For most
> configs, and with most recent versions of gcc, it's no longer needed.
> 
> Clarify which cases need it, and only enable it for those cases.  Also
> produce a compile-time error for the ftrace graph + mcount + '-Os' case,
> which will otherwise cause runtime failures.
> 
> The main benefit of '-maccumulate-outgoing-args' is that it prevents an
> ugly prologue for functions which have aligned stacks.  But removing the
> option also has some benefits: more readable argument saves, smaller
> text size, and (presumably) slightly improved performance.
> 
> Here are the object size savings for 32-bit and 64-bit defconfig
> kernels:
> 
>       text	   data	    bss	     dec	    hex	filename
>   10006710	3543328	1773568	15323606	 e9d1d6	vmlinux.x86-32.before
>    9706358	3547424	1773568	15027350	 e54c96	vmlinux.x86-32.after
> 
>       text	   data	    bss	     dec	    hex	filename
>   10652105	4537576	 843776	16033457	 f4a6b1	vmlinux.x86-64.before
>   10639629	4537576	 843776	16020981	 f475f5	vmlinux.x86-64.after
> 
> That comes out to a 3% text size improvement on x86-32 and a 0.1% text
> size improvement on x86-64.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> v2:
> - improve readability of the comments
> - add comment about why gcc version needs to be checked
> - use cc-option-yn instead of cc-option
> 
>  arch/x86/Makefile        | 31 +++++++++++++++++++++++++++----
>  arch/x86/Makefile_32.cpu | 18 ------------------
>  arch/x86/kernel/ftrace.c |  6 ++++++
>  scripts/Kbuild.include   |  4 ++++
>  4 files changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 2d44933..04c87be 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -120,10 +120,6 @@ else
>          # -funit-at-a-time shrinks the kernel .text considerably
>          # unfortunately it makes reading oopses harder.
>          KBUILD_CFLAGS += $(call cc-option,-funit-at-a-time)
> -
> -        # this works around some issues with generating unwind tables in older gccs
> -        # newer gccs do it by default
> -        KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)
>  endif
>  
>  ifdef CONFIG_X86_X32
> @@ -147,6 +143,33 @@ ifeq ($(CONFIG_KMEMCHECK),y)
>  	KBUILD_CFLAGS += $(call cc-option,-fno-builtin-memcpy)
>  endif
>  
> +# If the function graph tracer is used with mcount instead of fentry,
> +# '-maccumulate-outgoing-args' is needed to prevent a gcc bug
> +# (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109)
> +ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +  ifndef CONFIG_HAVE_FENTRY
> +	ACCUMULATE_OUTGOING_ARGS := 1
> +  else
> +    ifeq ($(call cc-option-yn, -mfentry), n)
> +	ACCUMULATE_OUTGOING_ARGS := 1
> +    endif
> +  endif
> +endif
> +
> +# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent a
> +# gcc bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226).  There's no way
> +# to test for this bug at compile-time because the test case needs to execute,
> +# which is a no-go for cross compilers.  So check the gcc version instead.

In documentation please refer to GCC the compiler as 'GCC' (upper case), while gcc 
the command as 'gcc'.

Also, even in Kbuild try to follow the kernel style - which for comments would be 
something like:

#
# Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent a
# GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226).  There's no way
# to test for this bug at compile-time because the test case needs to execute,
# which is a no-go for cross compilers.  So check the GCC version instead.
#

... even though there's plenty of bad example in the Makefile you are changing.

> +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && \
> +	!defined(CC_USING_FENTRY) && \
> +	!defined(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE)
> +# error Your compiler does not support function graph tracing
> +#endif

Might make sense to add the compiler option that is missing, i.e. something like:

  # error Your compiler does not support function graph tracing (-mfentry)

(or whatever compiler feature is missing.)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1606679 — Re: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-03-22 17:00 +0100
SubjectRe: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'
Message-ID<tnQQ1-4OP-9@gated-at.bofh.it>
In reply to#1606267
On Wed, Mar 22, 2017 at 08:51:53AM +0100, Ingo Molnar wrote:
> > +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && \
> > +	!defined(CC_USING_FENTRY) && \
> > +	!defined(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE)
> > +# error Your compiler does not support function graph tracing
> > +#endif
> 
> Might make sense to add the compiler option that is missing, i.e. something like:
> 
>   # error Your compiler does not support function graph tracing (-mfentry)
> 
> (or whatever compiler feature is missing.)

I left it vague because otherwise it would need a paragraph :-)

After Steven's latest patches which port fentry to x86-32, I think the
precise version would be:

  # error The following combination is not supported: ((compiler missing -mfentry) || (CONFIG_X86_32 and !CONFIG_DYNAMIC_FTRACE)) && CONFIG_FUNCTION_GRAPH_TRACER && CONFIG_CC_OPTIMIZE_FOR_SIZE.

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web