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

[Steven Rostedt]

On Thu, 16 Mar 2017 10:42:08 -0500
Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:

> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  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,\