[PATCH v4 0/3] Tracers vs. CALLER_ADDR on PowerPC

[PATCH v4 0/3] Tracers vs. CALLER_ADDR on PowerPC

[Anton Vorontsov]

Hi all,

Here is another approach to fixing tracers vs. CALLER_ADDR problem
on PowerPC.

Preface for those who don't know or forgot what the problem is:

Gcc frame pointers do nothing useful on PowerPC (they're harmful,
actually), and thus lib/Kconfig.debug makes CONFIG_FRAME_POINTER
unselectable on PPC targets, but CALLER_ADDR macros are available
only with CONFIG_FRAME_POINTER, therefore tracing is completely
useless on PowerPC:

[...]
  <idle>-0       0X.h3    2us+:      0:140:R   + [000]  1733:120:S mvtsd
  <idle>-0       0X.h3    9us+: 0 (0)
  <idle>-0       0X..3   72us : 0 (0)
  <idle>-0       0X..3   73us :      0:140:R ==> [000]  1733:120:R mvtsd

While it should look like this:

[...]
  <idle>-0       0X.h3    2us+:      0:140:R   + [000]  1740:120:S mvtsd
  <idle>-0       0X.h3    9us+: hrtimer_wakeup (__run_hrtimer)
  <idle>-0       0X..3   87us : cpu_idle (__got2_end)
  <idle>-0       0X..3   89us :      0:140:R ==> [000]  1740:120:R mvtsd

I've tried to fix the issue via expanding the #ifdef in the ftrace.h:
http://lkml.org/lkml/2009/1/31/141

Then Steven Rostedt suggested to implement something more generic,
i.e. HAVE_NORMAL_FRAME_POINTERS Kconfig symbol.

I found a way to solve the problem w/o additional symbols, but
with some Makefile magic (http://lkml.org/lkml/2009/2/4/273).
But because of top-level Makefile issues on other arches
(http://lkml.org/lkml/2009/2/14/89) I had to abandon the approach.


So, this patch set combines Steven Rostedt's idea and a small
Makefile change, so that now only top-level Makefile has to know
about the new symbol, and the rest of the kernel can stay with
using CONFIG_FRAME_POINTER.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
When defined, the top level Makefile won't add -fno-omit-frame-pointer
cflag (the flag is useless in PowerPC kernels, and also makes gcc
generate wrong code).

Also move ARCH_WANT_FRAME_POINTERS's help text.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Makefile             |    7 +++++--
 arch/powerpc/Kconfig |    1 +
 lib/Kconfig.debug    |   16 ++++++++++------
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 46c04c5..bf41b05 100644
--- a/Makefile
+++ b/Makefile
@@ -538,9 +538,12 @@ KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
 endif
 
 ifdef CONFIG_FRAME_POINTER
-KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
+  KBUILD_CFLAGS	+= -fno-optimize-sibling-calls
+  ifndef ARCH_HAS_NORMAL_FRAME_POINTERS
+    KBUILD_CFLAGS	+= -fno-omit-frame-pointer
+  endif
 else
-KBUILD_CFLAGS	+= -fomit-frame-pointer
+  KBUILD_CFLAGS	+= -fomit-frame-pointer
 endif
 
 ifdef CONFIG_DEBUG_INFO
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 97f9a64..4587e66 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -113,6 +113,7 @@ config PPC
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select ARCH_WANT_OPTIONAL_GPIOLIB
+	select ARCH_HAS_NORMAL_FRAME_POINTERS
 	select HAVE_IDE
 	select HAVE_IOREMAP_PROT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4b63b6b..fc8cd1f 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -661,20 +661,24 @@ config DEBUG_NOTIFIERS
 	  This is a relatively cheap check but if you care about maximum
 	  performance, say N.
 
-#
-# Select this config option from the architecture Kconfig, if it
-# it is preferred to always offer frame pointers as a config
-# option on the architecture (regardless of KERNEL_DEBUG):
-#
 config ARCH_WANT_FRAME_POINTERS
 	bool
 	help
+	  Select this config option from the architecture Kconfig, if it
+	  it is preferred to always offer frame pointers as a config
+	  option on the architecture (regardless of KERNEL_DEBUG).
+
+config ARCH_HAS_NORMAL_FRAME_POINTERS
+	bool
+	help
+	  Architectures should select this symbol if their ABI implies
+	  having a frame pointer.
 
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	depends on DEBUG_KERNEL && \
 		(CRIS || M68K || M68KNOMMU || FRV || UML || S390 || \
-		 AVR32 || SUPERH || BLACKFIN || MN10300) || \
+		 AVR32 || SUPERH || BLACKFIN || MN10300 || PPC) || \
 		ARCH_WANT_FRAME_POINTERS
 	default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
 	help
-- 
1.5.6.5

[PATCH 2/3] powerpc: Remove -fno-omit-frame-pointer workarounds

[Anton Vorontsov]

The workarounds aren't needed any longer since the top level Makefile
doesn't pass -fno-omit-frame-pointer cflag for PowerPC.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/Makefile                    |    5 -----
 arch/powerpc/kernel/Makefile             |   12 ++++++------
 arch/powerpc/platforms/powermac/Makefile |    2 +-
 lib/Kconfig.debug                        |    6 +++---
 4 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 551fc58..1dd7748 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -120,11 +120,6 @@ ifeq ($(CONFIG_6xx),y)
 KBUILD_CFLAGS		+= -mcpu=powerpc
 endif
 
-# Work around a gcc code-gen bug with -fno-omit-frame-pointer.
-ifeq ($(CONFIG_FUNCTION_TRACER),y)
-KBUILD_CFLAGS		+= -mno-sched-epilog
-endif
-
 cpu-as-$(CONFIG_4xx)		+= -Wa,-m405
 cpu-as-$(CONFIG_6xx)		+= -Wa,-maltivec
 cpu-as-$(CONFIG_POWER4)		+= -Wa,-maltivec
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index dfec3d2..f86caeb 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -14,14 +14,14 @@ endif
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_cputable.o = -pg
+CFLAGS_REMOVE_prom_init.o = -pg
+CFLAGS_REMOVE_btext.o = -pg
+CFLAGS_REMOVE_prom.o = -pg
 # do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg
 # timers used by tracing
-CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_time.o = -pg
 endif
 
 obj-y				:= cputable.o ptrace.o syscalls.o \
diff --git a/arch/powerpc/platforms/powermac/Makefile b/arch/powerpc/platforms/powermac/Makefile
index 50f1693..0eb8781 100644
--- a/arch/powerpc/platforms/powermac/Makefile
+++ b/arch/powerpc/platforms/powermac/Makefile
@@ -2,7 +2,7 @@ CFLAGS_bootx_init.o  		+= -fPIC
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_bootx_init.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_bootx_init.o = -pg
 endif
 
 obj-y				+= pic.o setup.o time.o feature.o pci.o \
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fc8cd1f..713620d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -493,7 +493,7 @@ config LOCKDEP
 	bool
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
 	select STACKTRACE
-	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND
+	select FRAME_POINTER if !MIPS && !ARM_UNWIND
 	select KALLSYMS
 	select KALLSYMS_ALL
 
@@ -866,13 +866,13 @@ config FAULT_INJECTION_STACKTRACE_FILTER
 	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
 	depends on !X86_64
 	select STACKTRACE
-	select FRAME_POINTER if !PPC
+	select FRAME_POINTER
 	help
 	  Provide stacktrace filter for fault-injection capabilities
 
 config LATENCYTOP
 	bool "Latency measuring infrastructure"
-	select FRAME_POINTER if !MIPS && !PPC
+	select FRAME_POINTER if !MIPS
 	select KALLSYMS
 	select KALLSYMS_ALL
 	select STACKTRACE
-- 
1.5.6.5

[PATCH 3/3] tracing: Tracers that use CALLER_ADDR macros should select FRAME_POINTER

[Anton Vorontsov]

Irqsoff, switch and preempt tracers use CALLER_ADDR macros, so they
should select FRAME_POINTER. Otherwise traces are meaningless.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 kernel/trace/Kconfig |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 774aba7..9fc98a7 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -107,6 +107,7 @@ config IRQSOFF_TRACER
 	select TRACE_IRQFLAGS
 	select TRACING
 	select TRACER_MAX_TRACE
+	select FRAME_POINTER
 	help
 	  This option measures the time spent in irqs-off critical
 	  sections, with microsecond accuracy.
@@ -128,6 +129,7 @@ config PREEMPT_TRACER
 	depends on PREEMPT
 	select TRACING
 	select TRACER_MAX_TRACE
+	select FRAME_POINTER
 	help
 	  This option measures the time spent in preemption off critical
 	  sections, with microsecond accuracy.
@@ -156,6 +158,7 @@ config SCHED_TRACER
 	select TRACING
 	select CONTEXT_SWITCH_TRACER
 	select TRACER_MAX_TRACE
+	select FRAME_POINTER
 	help
 	  This tracer tracks the latency of the highest priority task
 	  to be scheduled in, starting from the point it has woken up.
-- 
1.5.6.5

Re: [PATCH v4 0/3] Tracers vs. CALLER_ADDR on PowerPC

[Anton Vorontsov]

On Fri, Mar 20, 2009 at 07:44:04PM +0300, Anton Vorontsov wrote:
> Hi all,
> 
> Here is another approach to fixing tracers vs. CALLER_ADDR problem
> on PowerPC.
> 
> Preface for those who don't know or forgot what the problem is:
> 
> Gcc frame pointers do nothing useful on PowerPC (they're harmful,
> actually), and thus lib/Kconfig.debug makes CONFIG_FRAME_POINTER
> unselectable on PPC targets, but CALLER_ADDR macros are available
> only with CONFIG_FRAME_POINTER, therefore tracing is completely
> useless on PowerPC:
> 
> [...]
>   <idle>-0       0X.h3    2us+:      0:140:R   + [000]  1733:120:S mvtsd
>   <idle>-0       0X.h3    9us+: 0 (0)
>   <idle>-0       0X..3   72us : 0 (0)
>   <idle>-0       0X..3   73us :      0:140:R ==> [000]  1733:120:R mvtsd
> 
> While it should look like this:
> 
> [...]
>   <idle>-0       0X.h3    2us+:      0:140:R   + [000]  1740:120:S mvtsd
>   <idle>-0       0X.h3    9us+: hrtimer_wakeup (__run_hrtimer)
>   <idle>-0       0X..3   87us : cpu_idle (__got2_end)
>   <idle>-0       0X..3   89us :      0:140:R ==> [000]  1740:120:R mvtsd
> 
> I've tried to fix the issue via expanding the #ifdef in the ftrace.h:
> http://lkml.org/lkml/2009/1/31/141
> 
> Then Steven Rostedt suggested to implement something more generic,
> i.e. HAVE_NORMAL_FRAME_POINTERS Kconfig symbol.
> 
> I found a way to solve the problem w/o additional symbols, but
> with some Makefile magic (http://lkml.org/lkml/2009/2/4/273).
> But because of top-level Makefile issues on other arches
> (http://lkml.org/lkml/2009/2/14/89) I had to abandon the approach.

Oh, and btw, I'm aware of

  commit c79a61f55773d2519fd0525bf58385f7d20752d3
  Author: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
  Date:   Fri Feb 27 21:30:03 2009 +0100

      tracing: make CALLER_ADDRx overwriteable

But I think the patch set is still applicable, considering that
it removes gcc bug workaround in a nice way, and makes
CONFIG_FRAME_POINTER available on PowerPC, thus other code
can rely on that.

If not, I can just fill-in the asm/ftrace.h for PowerPC.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Steven Rostedt]

Ben,

Can you ACK this patch? Or even take it in your tree?

Thanks,

-- Steve

On Fri, 2009-03-20 at 19:44 +0300, Anton Vorontsov wrote:
> This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
> When defined, the top level Makefile won't add -fno-omit-frame-pointer
> cflag (the flag is useless in PowerPC kernels, and also makes gcc
> generate wrong code).
> 
> Also move ARCH_WANT_FRAME_POINTERS's help text.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Makefile             |    7 +++++--
>  arch/powerpc/Kconfig |    1 +
>  lib/Kconfig.debug    |   16 ++++++++++------
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 46c04c5..bf41b05 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -538,9 +538,12 @@ KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
>  endif
>  
>  ifdef CONFIG_FRAME_POINTER
> -KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> +  KBUILD_CFLAGS	+= -fno-optimize-sibling-calls
> +  ifndef ARCH_HAS_NORMAL_FRAME_POINTERS
> +    KBUILD_CFLAGS	+= -fno-omit-frame-pointer
> +  endif
>  else
> -KBUILD_CFLAGS	+= -fomit-frame-pointer
> +  KBUILD_CFLAGS	+= -fomit-frame-pointer
>  endif
>  
>  ifdef CONFIG_DEBUG_INFO
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 97f9a64..4587e66 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -113,6 +113,7 @@ config PPC
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select ARCH_WANT_OPTIONAL_GPIOLIB
> +	select ARCH_HAS_NORMAL_FRAME_POINTERS
>  	select HAVE_IDE
>  	select HAVE_IOREMAP_PROT
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 4b63b6b..fc8cd1f 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -661,20 +661,24 @@ config DEBUG_NOTIFIERS
>  	  This is a relatively cheap check but if you care about maximum
>  	  performance, say N.
>  
> -#
> -# Select this config option from the architecture Kconfig, if it
> -# it is preferred to always offer frame pointers as a config
> -# option on the architecture (regardless of KERNEL_DEBUG):
> -#
>  config ARCH_WANT_FRAME_POINTERS
>  	bool
>  	help
> +	  Select this config option from the architecture Kconfig, if it
> +	  it is preferred to always offer frame pointers as a config
> +	  option on the architecture (regardless of KERNEL_DEBUG).
> +
> +config ARCH_HAS_NORMAL_FRAME_POINTERS
> +	bool
> +	help
> +	  Architectures should select this symbol if their ABI implies
> +	  having a frame pointer.
>  
>  config FRAME_POINTER
>  	bool "Compile the kernel with frame pointers"
>  	depends on DEBUG_KERNEL && \
>  		(CRIS || M68K || M68KNOMMU || FRV || UML || S390 || \
> -		 AVR32 || SUPERH || BLACKFIN || MN10300) || \
> +		 AVR32 || SUPERH || BLACKFIN || MN10300 || PPC) || \
>  		ARCH_WANT_FRAME_POINTERS
>  	default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
>  	help

Re: [PATCH 2/3] powerpc: Remove -fno-omit-frame-pointer workarounds

[Steven Rostedt]

Ben,

Can you ACK or take this patch too.

Thanks,

-- Steve


On Fri, 2009-03-20 at 19:44 +0300, Anton Vorontsov wrote:
> The workarounds aren't needed any longer since the top level Makefile
> doesn't pass -fno-omit-frame-pointer cflag for PowerPC.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  arch/powerpc/Makefile                    |    5 -----
>  arch/powerpc/kernel/Makefile             |   12 ++++++------
>  arch/powerpc/platforms/powermac/Makefile |    2 +-
>  lib/Kconfig.debug                        |    6 +++---
>  4 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 551fc58..1dd7748 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -120,11 +120,6 @@ ifeq ($(CONFIG_6xx),y)
>  KBUILD_CFLAGS		+= -mcpu=powerpc
>  endif
>  
> -# Work around a gcc code-gen bug with -fno-omit-frame-pointer.
> -ifeq ($(CONFIG_FUNCTION_TRACER),y)
> -KBUILD_CFLAGS		+= -mno-sched-epilog
> -endif
> -
>  cpu-as-$(CONFIG_4xx)		+= -Wa,-m405
>  cpu-as-$(CONFIG_6xx)		+= -Wa,-maltivec
>  cpu-as-$(CONFIG_POWER4)		+= -Wa,-maltivec
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index dfec3d2..f86caeb 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -14,14 +14,14 @@ endif
>  
>  ifdef CONFIG_FUNCTION_TRACER
>  # Do not trace early boot code
> -CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_cputable.o = -pg
> +CFLAGS_REMOVE_prom_init.o = -pg
> +CFLAGS_REMOVE_btext.o = -pg
> +CFLAGS_REMOVE_prom.o = -pg
>  # do not trace tracer code
> -CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_ftrace.o = -pg
>  # timers used by tracing
> -CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_time.o = -pg
>  endif
>  
>  obj-y				:= cputable.o ptrace.o syscalls.o \
> diff --git a/arch/powerpc/platforms/powermac/Makefile b/arch/powerpc/platforms/powermac/Makefile
> index 50f1693..0eb8781 100644
> --- a/arch/powerpc/platforms/powermac/Makefile
> +++ b/arch/powerpc/platforms/powermac/Makefile
> @@ -2,7 +2,7 @@ CFLAGS_bootx_init.o  		+= -fPIC
>  
>  ifdef CONFIG_FUNCTION_TRACER
>  # Do not trace early boot code
> -CFLAGS_REMOVE_bootx_init.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_bootx_init.o = -pg
>  endif
>  
>  obj-y				+= pic.o setup.o time.o feature.o pci.o \
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index fc8cd1f..713620d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -493,7 +493,7 @@ config LOCKDEP
>  	bool
>  	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>  	select STACKTRACE
> -	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND
> +	select FRAME_POINTER if !MIPS && !ARM_UNWIND
>  	select KALLSYMS
>  	select KALLSYMS_ALL
>  
> @@ -866,13 +866,13 @@ config FAULT_INJECTION_STACKTRACE_FILTER
>  	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
>  	depends on !X86_64
>  	select STACKTRACE
> -	select FRAME_POINTER if !PPC
> +	select FRAME_POINTER
>  	help
>  	  Provide stacktrace filter for fault-injection capabilities
>  
>  config LATENCYTOP
>  	bool "Latency measuring infrastructure"
> -	select FRAME_POINTER if !MIPS && !PPC
> +	select FRAME_POINTER if !MIPS
>  	select KALLSYMS
>  	select KALLSYMS_ALL
>  	select STACKTRACE

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

On Fri, Mar 20, 2009 at 11:49:29PM -0400, Steven Rostedt wrote:
> Ben,
> 
> Can you ACK this patch? Or even take it in your tree?

Benjamin, have you had a chance to look into this? Sam, could
you also take a look?

Thanks!

> On Fri, 2009-03-20 at 19:44 +0300, Anton Vorontsov wrote:
> > This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
> > When defined, the top level Makefile won't add -fno-omit-frame-pointer
> > cflag (the flag is useless in PowerPC kernels, and also makes gcc
> > generate wrong code).
> > 
> > Also move ARCH_WANT_FRAME_POINTERS's help text.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  Makefile             |    7 +++++--
> >  arch/powerpc/Kconfig |    1 +
> >  lib/Kconfig.debug    |   16 ++++++++++------
> >  3 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 46c04c5..bf41b05 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -538,9 +538,12 @@ KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> >  endif
> >  
> >  ifdef CONFIG_FRAME_POINTER
> > -KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> > +  KBUILD_CFLAGS	+= -fno-optimize-sibling-calls
> > +  ifndef ARCH_HAS_NORMAL_FRAME_POINTERS
> > +    KBUILD_CFLAGS	+= -fno-omit-frame-pointer
> > +  endif
> >  else
> > -KBUILD_CFLAGS	+= -fomit-frame-pointer
> > +  KBUILD_CFLAGS	+= -fomit-frame-pointer
> >  endif
> >  
> >  ifdef CONFIG_DEBUG_INFO
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 97f9a64..4587e66 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -113,6 +113,7 @@ config PPC
> >  	select HAVE_FUNCTION_TRACER
> >  	select HAVE_FUNCTION_GRAPH_TRACER
> >  	select ARCH_WANT_OPTIONAL_GPIOLIB
> > +	select ARCH_HAS_NORMAL_FRAME_POINTERS
> >  	select HAVE_IDE
> >  	select HAVE_IOREMAP_PROT
> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 4b63b6b..fc8cd1f 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -661,20 +661,24 @@ config DEBUG_NOTIFIERS
> >  	  This is a relatively cheap check but if you care about maximum
> >  	  performance, say N.
> >  
> > -#
> > -# Select this config option from the architecture Kconfig, if it
> > -# it is preferred to always offer frame pointers as a config
> > -# option on the architecture (regardless of KERNEL_DEBUG):
> > -#
> >  config ARCH_WANT_FRAME_POINTERS
> >  	bool
> >  	help
> > +	  Select this config option from the architecture Kconfig, if it
> > +	  it is preferred to always offer frame pointers as a config
> > +	  option on the architecture (regardless of KERNEL_DEBUG).
> > +
> > +config ARCH_HAS_NORMAL_FRAME_POINTERS
> > +	bool
> > +	help
> > +	  Architectures should select this symbol if their ABI implies
> > +	  having a frame pointer.
> >  
> >  config FRAME_POINTER
> >  	bool "Compile the kernel with frame pointers"
> >  	depends on DEBUG_KERNEL && \
> >  		(CRIS || M68K || M68KNOMMU || FRV || UML || S390 || \
> > -		 AVR32 || SUPERH || BLACKFIN || MN10300) || \
> > +		 AVR32 || SUPERH || BLACKFIN || MN10300 || PPC) || \
> >  		ARCH_WANT_FRAME_POINTERS
> >  	default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
> >  	help
> 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Benjamin Herrenschmidt]

On Sat, 2009-03-28 at 13:48 +0300, Anton Vorontsov wrote:
> On Fri, Mar 20, 2009 at 11:49:29PM -0400, Steven Rostedt wrote:
> > Ben,
> > 
> > Can you ACK this patch? Or even take it in your tree?
> 
> Benjamin, have you had a chance to look into this? Sam, could
> you also take a look?

Those patches look ok. I'm a little bit surprised that -mno-sched-epilog
isn't needed, gcc weirdness never ceases to amaze me, but the
Makefile/Kconfig churning isn't for me to judge. If Sam is happy, then
let's merge it.

Ben.

> Thanks!
> 
> > On Fri, 2009-03-20 at 19:44 +0300, Anton Vorontsov wrote:
> > > This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
> > > When defined, the top level Makefile won't add -fno-omit-frame-pointer
> > > cflag (the flag is useless in PowerPC kernels, and also makes gcc
> > > generate wrong code).
> > > 
> > > Also move ARCH_WANT_FRAME_POINTERS's help text.
> > > 
> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > ---
> > >  Makefile             |    7 +++++--
> > >  arch/powerpc/Kconfig |    1 +
> > >  lib/Kconfig.debug    |   16 ++++++++++------
> > >  3 files changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 46c04c5..bf41b05 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -538,9 +538,12 @@ KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> > >  endif
> > >  
> > >  ifdef CONFIG_FRAME_POINTER
> > > -KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> > > +  KBUILD_CFLAGS	+= -fno-optimize-sibling-calls
> > > +  ifndef ARCH_HAS_NORMAL_FRAME_POINTERS
> > > +    KBUILD_CFLAGS	+= -fno-omit-frame-pointer
> > > +  endif
> > >  else
> > > -KBUILD_CFLAGS	+= -fomit-frame-pointer
> > > +  KBUILD_CFLAGS	+= -fomit-frame-pointer
> > >  endif
> > >  
> > >  ifdef CONFIG_DEBUG_INFO
> > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > index 97f9a64..4587e66 100644
> > > --- a/arch/powerpc/Kconfig
> > > +++ b/arch/powerpc/Kconfig
> > > @@ -113,6 +113,7 @@ config PPC
> > >  	select HAVE_FUNCTION_TRACER
> > >  	select HAVE_FUNCTION_GRAPH_TRACER
> > >  	select ARCH_WANT_OPTIONAL_GPIOLIB
> > > +	select ARCH_HAS_NORMAL_FRAME_POINTERS
> > >  	select HAVE_IDE
> > >  	select HAVE_IOREMAP_PROT
> > >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index 4b63b6b..fc8cd1f 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -661,20 +661,24 @@ config DEBUG_NOTIFIERS
> > >  	  This is a relatively cheap check but if you care about maximum
> > >  	  performance, say N.
> > >  
> > > -#
> > > -# Select this config option from the architecture Kconfig, if it
> > > -# it is preferred to always offer frame pointers as a config
> > > -# option on the architecture (regardless of KERNEL_DEBUG):
> > > -#
> > >  config ARCH_WANT_FRAME_POINTERS
> > >  	bool
> > >  	help
> > > +	  Select this config option from the architecture Kconfig, if it
> > > +	  it is preferred to always offer frame pointers as a config
> > > +	  option on the architecture (regardless of KERNEL_DEBUG).
> > > +
> > > +config ARCH_HAS_NORMAL_FRAME_POINTERS
> > > +	bool
> > > +	help
> > > +	  Architectures should select this symbol if their ABI implies
> > > +	  having a frame pointer.
> > >  
> > >  config FRAME_POINTER
> > >  	bool "Compile the kernel with frame pointers"
> > >  	depends on DEBUG_KERNEL && \
> > >  		(CRIS || M68K || M68KNOMMU || FRV || UML || S390 || \
> > > -		 AVR32 || SUPERH || BLACKFIN || MN10300) || \
> > > +		 AVR32 || SUPERH || BLACKFIN || MN10300 || PPC) || \
> > >  		ARCH_WANT_FRAME_POINTERS
> > >  	default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
> > >  	help
> > 
> 

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Sam Ravnborg]

On Mon, Mar 30, 2009 at 09:54:50AM +1100, Benjamin Herrenschmidt wrote:
> On Sat, 2009-03-28 at 13:48 +0300, Anton Vorontsov wrote:
> > On Fri, Mar 20, 2009 at 11:49:29PM -0400, Steven Rostedt wrote:
> > > Ben,
> > > 
> > > Can you ACK this patch? Or even take it in your tree?
> > 
> > Benjamin, have you had a chance to look into this? Sam, could
> > you also take a look?
> 
> Those patches look ok. I'm a little bit surprised that -mno-sched-epilog
> isn't needed, gcc weirdness never ceases to amaze me, but the
> Makefile/Kconfig churning isn't for me to judge. If Sam is happy, then
> let's merge it.

I will take a closer look tonight or tomorrow - as day-time job permits.

	Sam

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Sam Ravnborg]

On Fri, Mar 20, 2009 at 07:44:29PM +0300, Anton Vorontsov wrote:
> This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
> When defined, the top level Makefile won't add -fno-omit-frame-pointer
> cflag (the flag is useless in PowerPC kernels, and also makes gcc
> generate wrong code).
> 
> Also move ARCH_WANT_FRAME_POINTERS's help text.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Hi Anton - sorry for the late feedback.

1) The preferred naming for variables that are supposed to be selcted
   are "HAVE_xxxx".
   So in this case it would be:
   HAVE_NORMAL_FRAME_POINTER

2) I do not really understand the interpretation of normal in this case,
   can it be more specific?

   Looking at your patch then if specified we use "-fomit-frame-pointer"
   in the case where it is set.
   So I read it like:

      If arch has normal framepointer then we omit them - strange?

3) Indent in top-level MAkefile for new stuff is generally 8 spaces.
   Do NOT use tabs as this would confuse make and rener assignmnet non-functional.

4) The individual "HAVE_* members should be sorted alphabetically in the arch
   Kconfig file (which they seldomly are :-( )

	Sam

> ---
>  Makefile             |    7 +++++--
>  arch/powerpc/Kconfig |    1 +
>  lib/Kconfig.debug    |   16 ++++++++++------
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 46c04c5..bf41b05 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -538,9 +538,12 @@ KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
>  endif
>  
>  ifdef CONFIG_FRAME_POINTER
> -KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> +  KBUILD_CFLAGS	+= -fno-optimize-sibling-calls
> +  ifndef ARCH_HAS_NORMAL_FRAME_POINTERS
> +    KBUILD_CFLAGS	+= -fno-omit-frame-pointer
> +  endif
>  else
> -KBUILD_CFLAGS	+= -fomit-frame-pointer
> +  KBUILD_CFLAGS	+= -fomit-frame-pointer
>  endif
>  
>  ifdef CONFIG_DEBUG_INFO
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 97f9a64..4587e66 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -113,6 +113,7 @@ config PPC
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select ARCH_WANT_OPTIONAL_GPIOLIB
> +	select ARCH_HAS_NORMAL_FRAME_POINTERS
>  	select HAVE_IDE
>  	select HAVE_IOREMAP_PROT
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 4b63b6b..fc8cd1f 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -661,20 +661,24 @@ config DEBUG_NOTIFIERS
>  	  This is a relatively cheap check but if you care about maximum
>  	  performance, say N.
>  
> -#
> -# Select this config option from the architecture Kconfig, if it
> -# it is preferred to always offer frame pointers as a config
> -# option on the architecture (regardless of KERNEL_DEBUG):
> -#
>  config ARCH_WANT_FRAME_POINTERS
>  	bool
>  	help
> +	  Select this config option from the architecture Kconfig, if it
> +	  it is preferred to always offer frame pointers as a config
> +	  option on the architecture (regardless of KERNEL_DEBUG).
> +
> +config ARCH_HAS_NORMAL_FRAME_POINTERS
> +	bool
> +	help
> +	  Architectures should select this symbol if their ABI implies
> +	  having a frame pointer.
>  
>  config FRAME_POINTER
>  	bool "Compile the kernel with frame pointers"
>  	depends on DEBUG_KERNEL && \
>  		(CRIS || M68K || M68KNOMMU || FRV || UML || S390 || \
> -		 AVR32 || SUPERH || BLACKFIN || MN10300) || \
> +		 AVR32 || SUPERH || BLACKFIN || MN10300 || PPC) || \
>  		ARCH_WANT_FRAME_POINTERS
>  	default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
>  	help
> -- 
> 1.5.6.5
> 

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Steven Rostedt]

On Sun, 2009-04-05 at 23:07 +0200, Sam Ravnborg wrote:

> 4) The individual "HAVE_* members should be sorted alphabetically in the arch
>    Kconfig file (which they seldomly are :-( )

Note, my first port of Ftrace to PPC sorted the Kconfig HAVE_* macros,
which ended up being reverted :-/

http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg21222.html

As stated, I did the sort without reporting the change in the
change-log, which I should have done.

-- Steve

Re: [PATCH 1/3] powerpc, Makefile: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

On Sun, Apr 05, 2009 at 11:07:56PM +0200, Sam Ravnborg wrote:
> On Fri, Mar 20, 2009 at 07:44:29PM +0300, Anton Vorontsov wrote:
> > This patch introduces ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol.
> > When defined, the top level Makefile won't add -fno-omit-frame-pointer
> > cflag (the flag is useless in PowerPC kernels, and also makes gcc
> > generate wrong code).
> > 
> > Also move ARCH_WANT_FRAME_POINTERS's help text.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> Hi Anton - sorry for the late feedback.
> 
> 1) The preferred naming for variables that are supposed to be selcted
>    are "HAVE_xxxx".
>    So in this case it would be:
>    HAVE_NORMAL_FRAME_POINTER

OK, will rename.

> 2) I do not really understand the interpretation of normal in this case,
>    can it be more specific?

Normal means that architecture's ABI implies having a framepointer,
we don't need any gcc flags.

I'm not sure if we can name it any better, that is what was
suggested by Steven and I don't have any better variant. ;-)

>    Looking at your patch then if specified we use "-fomit-frame-pointer"
>    in the case where it is set.

No. We specify neither -fomit-frame-pointer, nor we specify
-fno-omit-frame-pointer (because that flag causes gcc to generate
wrong code).

>    So I read it like:
> 
>       If arch has normal framepointer then we omit them - strange?
> 
> 3) Indent in top-level MAkefile for new stuff is generally 8 spaces.
>    Do NOT use tabs as this would confuse make and rener assignmnet non-functional.
> 
> 4) The individual "HAVE_* members should be sorted alphabetically in the arch
>    Kconfig file (which they seldomly are :-( )
> 
> 	Sam

Well, yes. So looking into the Kconfig, I'm quite unsure where
to put it to make it alphabetically correct. Ok, let's put
it after K. ;-)

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2