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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170322075153.GB10809@gmail.com>
Date:   Wed, 22 Mar 2017 08:51:53 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     x86@...nel.org, Andy Lutomirski <luto@...capital.net>,
        Pavel Machek <pavel@....cz>,
        kernel list <linux-kernel@...r.kernel.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        Peter Anvin <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2] x86: mostly disable '-maccumulate-outgoing-args'


* Josh Poimboeuf <jpoimboe@...hat.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@...hat.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