lkml.org 
[lkml]   [2010]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 7/7] jump label: Add work around to i386 gcc asm goto bug
* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> On i386 (not x86_64) early implementations of gcc would have a bug
> with asm goto causing it to produce code like the following:
>
> (This was noticed by Peter Zijlstra)
>
> 56 pushl 0
> 67 nopl jmp 0x6f
> popl
> jmp 0x8c
>
> 6f mov
> test
> je 0x8c
>
> 8c mov
> call *(%esp)
>
> The jump added in the asm goto skipped over the popl that matched
> the pushl 0, which lead up to a quick crash of the system when
> the jump was enabled. The nopl is defined in the asm goto () statement
> and when tracepoints are enabled, the nop changes to a jump to the label
> that was specified by the asm goto. asm goto is suppose to tell gcc that
> the code in the asm might jump to an external label. Here gcc obviously
> fails to make that work.
>
> The bug report for gcc is here:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
>
> The bug only appears on x86 when not compiled with
> -maccumulate-outgoing-args. This option is always set on x86_64 and it
> is also the work around for a function graph tracer i386 bug.
> (See commit: 746357d6a526d6da9d89a2ec645b28406e959c2e)
> This explains why the bug only showed up on i386 when function graph
> tracer was not enabled.
>
> This patch now adds a CONFIG_JUMP_LABEL option that is default
> off instead of using jump labels by default. When jump labels are
> enabled, the -maccumulate-outgoing-args will be used (causing a
> slightly larger kernel image on i386). This option will exist
> until we have a way to detect if the gcc compiler in use is safe
> to use on all configurations without the work around.
>
> Note, there exists such a test, but for now we will keep the enabling
> of jump label as a manual option.
>
> Archs that know the compiler is safe with asm goto, may choose to
> select JUMP_LABEL and enable it by default.

Looks good. You can add my

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

if you feel like it.

Thanks,

Mathieu

>
> Reported-by: Ingo Molnar <mingo@elte.hu>
> Cause-discovered-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Baron <jbaron@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: David Daney <ddaney@caviumnetworks.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: David Miller <davem@davemloft.net>
> Cc: Richard Henderson <rth@redhat.com>
> LKML-Reference: <1288028746.3673.11.camel@laptop>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> arch/Kconfig | 14 ++++++++++++++
> arch/x86/Makefile_32.cpu | 13 ++++++++++++-
> include/linux/jump_label.h | 2 +-
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 53d7f61..8bf0fa6 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -42,6 +42,20 @@ config KPROBES
> for kernel debugging, non-intrusive instrumentation and testing.
> If in doubt, say "N".
>
> +config JUMP_LABEL
> + bool "Optimize trace point call sites"
> + depends on HAVE_ARCH_JUMP_LABEL
> + help
> + If it is detected that the compiler has support for "asm goto",
> + the kernel will compile trace point locations with just a
> + nop instruction. When trace points are enabled, the nop will
> + be converted to a jump to the trace function. This technique
> + lowers overhead and stress on the branch prediction of the
> + processor.
> +
> + On i386, options added to the compiler flags may increase
> + the size of the kernel slightly.
> +
> config OPTPROBES
> def_bool y
> depends on KPROBES && HAVE_OPTPROBES
> diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
> index 1255d95..f2ee1ab 100644
> --- a/arch/x86/Makefile_32.cpu
> +++ b/arch/x86/Makefile_32.cpu
> @@ -51,7 +51,18 @@ cflags-$(CONFIG_X86_GENERIC) += $(call tune,generic,$(call tune,i686))
> # prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
> # tracer assumptions. For i686, generic, core2 this is set by the
> # compiler anyway
> -cflags-$(CONFIG_FUNCTION_GRAPH_TRACER) += $(call cc-option,-maccumulate-outgoing-args)
> +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.
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 1947a12..7880f18 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -1,7 +1,7 @@
> #ifndef _LINUX_JUMP_LABEL_H
> #define _LINUX_JUMP_LABEL_H
>
> -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
> # include <asm/jump_label.h>
> # define HAVE_JUMP_LABEL
> #endif
> --
> 1.7.1
>
>

--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com


\
 
 \ /
  Last update: 2010-10-29 22:05    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site