[PATCH] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[PATCH] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Anton Vorontsov]

According to this discussion:

http://lkml.org/lkml/2008/7/25/338
http://lkml.org/lkml/2008/7/26/72

Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. 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

On PPC we can safely use __builtin_return_address(1..6) w/o frame
pointers, and with this patch the trace output looks OK:

[...]
  <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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

Btw, on PowerPC tracing is also broken w/o "ring-buffer: fix alignment
problem" patch (currently collecting dust in the -tip tree, commit
082605de5f82eb692cc90f7fda071cc01bb5ac34). Any chance the fix go into
Linus' tree, to not waste other people's time bisecting and debugging
the problem? ;-)

For google: tracing, regression, "ring-buffer: move some metadata
into buffer page", commit abc9b56d66fbd4d93302ef4bf6fa726e1b8255f9,
answer is here.

 include/linux/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9c5bc6b..13eba02 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -146,7 +146,7 @@ static inline void __ftrace_enabled_restore(int enabled)
 #endif
 }
 
-#ifdef CONFIG_FRAME_POINTER
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_PPC)
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
 # define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))
-- 
1.5.6.5

Re: [PATCH] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Benjamin Herrenschmidt]

On Sat, 2009-01-31 at 22:06 +0300, Anton Vorontsov wrote:
> According to this discussion:
> 
> http://lkml.org/lkml/2008/7/25/338
> http://lkml.org/lkml/2008/7/26/72
> 
> Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
> makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
> requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. Therefore
> tracing is completely useless on PowerPC:

But we need them for -pg which ftrace uses no ? (ie, gcc forces you to
have -fno-omit-frame-pointers with -fpg iirc).

Now, regardless, I agree that on PPC, __builtin_return_address() should
always work with our without that cruft, so we may as well apply that
patch...

> [...]
>   <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
> 
> On PPC we can safely use __builtin_return_address(1..6) w/o frame
> pointers, and with this patch the trace output looks OK:
> 
> [...]
>   <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
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> Btw, on PowerPC tracing is also broken w/o "ring-buffer: fix alignment
> problem" patch (currently collecting dust in the -tip tree, commit
> 082605de5f82eb692cc90f7fda071cc01bb5ac34). Any chance the fix go into
> Linus' tree, to not waste other people's time bisecting and debugging
> the problem? ;-)
> 
> For google: tracing, regression, "ring-buffer: move some metadata
> into buffer page", commit abc9b56d66fbd4d93302ef4bf6fa726e1b8255f9,
> answer is here.
> 
>  include/linux/ftrace.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 9c5bc6b..13eba02 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -146,7 +146,7 @@ static inline void __ftrace_enabled_restore(int enabled)
>  #endif
>  }
>  
> -#ifdef CONFIG_FRAME_POINTER
> +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_PPC)
>  /* TODO: need to fix this for ARM */
>  # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
>  # define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))

Re: [PATCH] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Steven Rostedt]

On Mon, 2009-02-02 at 11:35 +1100, Benjamin Herrenschmidt wrote:
> On Sat, 2009-01-31 at 22:06 +0300, Anton Vorontsov wrote:
> > According to this discussion:
> > 
> > http://lkml.org/lkml/2008/7/25/338
> > http://lkml.org/lkml/2008/7/26/72
> > 
> > Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
> > makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
> > requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. Therefore
> > tracing is completely useless on PowerPC:
> 
> But we need them for -pg which ftrace uses no ? (ie, gcc forces you to
> have -fno-omit-frame-pointers with -fpg iirc).

Yes, ftrace function tracing requires -pg which requires FRAME_POINTER
turned on.

> 
> Now, regardless, I agree that on PPC, __builtin_return_address() should
> always work with our without that cruft, so we may as well apply that
> patch...
> 
> > [...]
> >   <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
> > 
> > On PPC we can safely use __builtin_return_address(1..6) w/o frame
> > pointers, and with this patch the trace output looks OK:
> > 
> > [...]
> >   <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
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> > 
> > Btw, on PowerPC tracing is also broken w/o "ring-buffer: fix alignment
> > problem" patch (currently collecting dust in the -tip tree, commit
> > 082605de5f82eb692cc90f7fda071cc01bb5ac34). Any chance the fix go into
> > Linus' tree, to not waste other people's time bisecting and debugging
> > the problem? ;-)
> > 
> > For google: tracing, regression, "ring-buffer: move some metadata
> > into buffer page", commit abc9b56d66fbd4d93302ef4bf6fa726e1b8255f9,
> > answer is here.
> > 
> >  include/linux/ftrace.h |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 9c5bc6b..13eba02 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -146,7 +146,7 @@ static inline void __ftrace_enabled_restore(int enabled)
> >  #endif
> >  }
> >  
> > -#ifdef CONFIG_FRAME_POINTER
> > +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_PPC)

Perhaps we should add a HAVE_NORMAL_FRAME_POINTERS in 
arch/powerpc/Kconfig under PPC and then we can change the above line to:

#if defined(CONFIG_FRAME_POINTERS) || \
		defined(CONFIG_HAVE_NORMAL_FRAME_POINTERS)

This way when another arch wants to belong to this, we do not need to
have a list of archs here.

-- Steve

> >  /* TODO: need to fix this for ARM */
> >  # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
> >  # define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))
> 

[PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Anton Vorontsov]

According to this discussion:

http://lkml.org/lkml/2008/7/25/338
http://lkml.org/lkml/2008/7/26/72

Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. 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

This patch introduces a ARCH_HAS_NORMAL_FRAME_POINTERS Kconfig symbol,
when selected the CALLER_ADDR macros are available without the
FRAME_POINTER Kconfig symbol.

With this patch the trace output turns into:

[...]
  <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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Mon, Feb 02, 2009 at 09:04:15AM -0500, Steven Rostedt wrote:
[...]
> > > -#ifdef CONFIG_FRAME_POINTER
> > > +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_PPC)
> 
> Perhaps we should add a HAVE_NORMAL_FRAME_POINTERS in 
> arch/powerpc/Kconfig under PPC and then we can change the above line to:
> 
> #if defined(CONFIG_FRAME_POINTERS) || \
> 		defined(CONFIG_HAVE_NORMAL_FRAME_POINTERS)
> 
> This way when another arch wants to belong to this, we do not need to
> have a list of archs here.

Would it be better if we introduce ARCH_HAS_NORMAL_FRAME_POINTERS
in lib/Kconfig.debug, along with ARCH_WANT_FRAME_POINTERS?

Note that we can't use ARCH_WANT_FRAME_POINTERS for our needs since
that symbol is used for other (mostly cosmetic) purposes: whether we
we want CONFIG_FRAME_POINTER depend on CONFIG_DEBUG_KERNEL, and
whether frame pointers should be default =y (see commit
da4276b8299a6544dc41ac2485d3ffca5811b3fb).

 arch/powerpc/Kconfig   |    1 +
 include/linux/ftrace.h |    3 ++-
 lib/Kconfig.debug      |    6 ++++++
 3 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 74cc312..d1c67bd 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -111,6 +111,7 @@ config PPC
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FUNCTION_TRACER
+	select ARCH_HAS_NORMAL_FRAME_POINTERS
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select HAVE_IDE
 	select HAVE_IOREMAP_PROT
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7840e71..ede3fe2 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -237,7 +237,8 @@ static inline void __ftrace_enabled_restore(int enabled)
 #endif
 }
 
-#ifdef CONFIG_FRAME_POINTER
+#if defined(CONFIG_FRAME_POINTER) || \
+		defined(CONFIG_ARCH_HAS_NORMAL_FRAME_POINTERS)
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
 # define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 29044f5..808f4e2 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -579,6 +579,12 @@ config ARCH_WANT_FRAME_POINTERS
 	bool
 	help
 
+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 && \
-- 
1.5.6.5

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Ingo Molnar]

* Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> According to this discussion:
> 
> http://lkml.org/lkml/2008/7/25/338
> http://lkml.org/lkml/2008/7/26/72
> 
> Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
> makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
> requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. [...]

hm, why not add PPC to FRAME_POINTERS list of architectures, and select it 
from the powerpc arch Kconfig? Does that cause complications somewhere?

	Ingo

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Anton Vorontsov]

On Tue, Feb 03, 2009 at 05:06:45PM +0100, Ingo Molnar wrote:
> 
> * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > According to this discussion:
> > 
> > http://lkml.org/lkml/2008/7/25/338
> > http://lkml.org/lkml/2008/7/26/72
> > 
> > Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
> > makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
> > requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. [...]
> 
> hm, why not add PPC to FRAME_POINTERS list of architectures, and select it 
> from the powerpc arch Kconfig? Does that cause complications somewhere?

-fno-omit-frame-pointers makes the code worse w/o any actual
benefit that we would use. Plus, there is a long standing bug in
gcc that makes -fno-omit-frame-pointer generate wrong code for PPC
targets:

http://lkml.org/lkml/2008/9/2/25

That is, the only tracer that needs[1] -fno-omit-frame-pointer is
"FUNCTION_TRCER", but we workaround the issue via -mno-sched-epilog,
quoting arch/powerpc/Makefile:

# Work around a gcc code-gen bug with -fno-omit-frame-pointer.
ifeq ($(CONFIG_FUNCTION_TRACER),y)
KBUILD_CFLAGS           += -mno-sched-epilog
endif

[1] Btw, why exactly do we need the -fno-omit-frame-pointer for
"FUNCTION_TRCER" tracer? Why just -pg isn't sufficient?..

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

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Steven Rostedt]

On Tue, 2009-02-03 at 19:19 +0300, Anton Vorontsov wrote:
> On Tue, Feb 03, 2009 at 05:06:45PM +0100, Ingo Molnar wrote:

> [1] Btw, why exactly do we need the -fno-omit-frame-pointer for
> "FUNCTION_TRCER" tracer? Why just -pg isn't sufficient?..
> 

The problem is this that is in the toplevel Makefile:


ifdef CONFIG_FRAME_POINTER
KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
else
KBUILD_CFLAGS	+= -fomit-frame-pointer
endif


-pg is incompatible with -fomit-frame-pointer

-- Steve

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Anton Vorontsov]

On Tue, Feb 03, 2009 at 11:32:18AM -0500, Steven Rostedt wrote:
> 
> On Tue, 2009-02-03 at 19:19 +0300, Anton Vorontsov wrote:
> > On Tue, Feb 03, 2009 at 05:06:45PM +0100, Ingo Molnar wrote:
> 
> > [1] Btw, why exactly do we need the -fno-omit-frame-pointer for
> > "FUNCTION_TRCER" tracer? Why just -pg isn't sufficient?..
> > 
> 
> The problem is this that is in the toplevel Makefile:
> 
> 
> ifdef CONFIG_FRAME_POINTER
> KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> else
> KBUILD_CFLAGS	+= -fomit-frame-pointer
> endif
> 
> 
> -pg is incompatible with -fomit-frame-pointer

Ah...

$ gcc -pg -fomit-frame-pointer -S c.c
gcc: -pg and -fomit-frame-pointer are incompatible

It's hard-coded in gcc, in the code that don't know about
architecture details.

But on PowerPC -O1 implies -fomit-frame-pointer, that is

gcc -pg -O1 -fno-omit-frame-pointer
and
gcc -pg -O1

produce different outputs. Thus -pg -O should be the same
as "-pg -O -fomit-framepointer".

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

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Benjamin Herrenschmidt]

On Tue, 2009-02-03 at 21:59 +0300, Anton Vorontsov wrote:
> On Tue, Feb 03, 2009 at 11:32:18AM -0500, Steven Rostedt wrote:
> > 
> > On Tue, 2009-02-03 at 19:19 +0300, Anton Vorontsov wrote:
> > > On Tue, Feb 03, 2009 at 05:06:45PM +0100, Ingo Molnar wrote:
> > 
> > > [1] Btw, why exactly do we need the -fno-omit-frame-pointer for
> > > "FUNCTION_TRCER" tracer? Why just -pg isn't sufficient?..
> > > 
> > 
> > The problem is this that is in the toplevel Makefile:
> > 
> > 
> > ifdef CONFIG_FRAME_POINTER
> > KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
> > else
> > KBUILD_CFLAGS	+= -fomit-frame-pointer
> > endif
> > 

And don't forget the gcc bug that miscompiles function epilogues on ppc
with -fno-omit-frame-pointer (iirc), which we need to work around using
-mno-sched-epilog.

Currently, we set that only if CONFIG_FUNCTION_TRACER is set, ie, we
assume that we only set -fno-omit-frame-pointer when
CONFIG_FUNCTION_TRACER is set.

Ben.

Re: [PATCH v2] ftrace: On PowerPC we don't need frame pointers for CALLER_ADDRs

[Anton Vorontsov]

On Tue, Feb 03, 2009 at 07:19:55PM +0300, Anton Vorontsov wrote:
> On Tue, Feb 03, 2009 at 05:06:45PM +0100, Ingo Molnar wrote:
> > 
> > * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > According to this discussion:
> > > 
> > > http://lkml.org/lkml/2008/7/25/338
> > > http://lkml.org/lkml/2008/7/26/72
> > > 
> > > Frame pointers do nothing useful on PowerPC, so lib/Kconfig.debug
> > > makes CONFIG_FRAME_POINTER unselectable on PPC targets. But ftrace.h
> > > requires CONFIG_FRAME_POINTER for CALLER_ADDR macros. [...]
> > 
> > hm, why not add PPC to FRAME_POINTERS list of architectures, and select it 
> > from the powerpc arch Kconfig? Does that cause complications somewhere?
> 
> -fno-omit-frame-pointers makes the code worse w/o any actual
> benefit that we would use. Plus, there is a long standing bug in
> gcc that makes -fno-omit-frame-pointer generate wrong code for PPC
> targets:
> 
> http://lkml.org/lkml/2008/9/2/25
> 
> That is, the only tracer that needs[1] -fno-omit-frame-pointer is
> "FUNCTION_TRCER", but we workaround the issue via -mno-sched-epilog,
> quoting arch/powerpc/Makefile:
> 
> # Work around a gcc code-gen bug with -fno-omit-frame-pointer.
> ifeq ($(CONFIG_FUNCTION_TRACER),y)
> KBUILD_CFLAGS           += -mno-sched-epilog
> endif

Thinking about it more... we can workaround the bug the other way,
and then permit CONFIG_FRAME_POINTER on PowerPC.

Patches are coming...

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

[PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Anton Vorontsov]

This patch gives arches more freedom on overwriting CFLAGS, specifically
on PowerPC we want to remove -fno-omit-frame-pointer flag.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 7715b2c..d1ba93f 100644
--- a/Makefile
+++ b/Makefile
@@ -525,8 +525,6 @@ else
 KBUILD_CFLAGS	+= -O2
 endif
 
-include $(srctree)/arch/$(SRCARCH)/Makefile
-
 ifneq (CONFIG_FRAME_WARN,0)
 KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
 endif
@@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
 KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
 endif
 
+include $(srctree)/arch/$(SRCARCH)/Makefile
+
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 CHECKFLAGS     += $(NOSTDINC_FLAGS)
-- 
1.5.6.5

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Ingo Molnar]

* Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch gives arches more freedom on overwriting CFLAGS, specifically
> on PowerPC we want to remove -fno-omit-frame-pointer flag.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Makefile |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7715b2c..d1ba93f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -525,8 +525,6 @@ else
>  KBUILD_CFLAGS	+= -O2
>  endif
>  
> -include $(srctree)/arch/$(SRCARCH)/Makefile
> -
>  ifneq (CONFIG_FRAME_WARN,0)
>  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
>  endif
> @@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
>  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
>  endif
>  
> +include $(srctree)/arch/$(SRCARCH)/Makefile
> +
>  # arch Makefile may override CC so keep this after arch Makefile is included
>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> -- 

this patch is really for Sam to judge - Cc:-ed him.

	Ingo

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 22:26 +0100, Ingo Molnar wrote:
> > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > +
> >  # arch Makefile may override CC so keep this after arch Makefile is
> included
> >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> -print-file-name=include)
> >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > -- 
> 
> this patch is really for Sam to judge - Cc:-ed him.

Sam ? I have a few more powerpc patches depending on that one... so I'd
like to know if it's totally doomed or not :-)

Cheers,
Ben.

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2009-02-04 at 22:26 +0100, Ingo Molnar wrote:
> > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > +
> > >  # arch Makefile may override CC so keep this after arch Makefile is
> > included
> > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> > -print-file-name=include)
> > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > -- 
> > 
> > this patch is really for Sam to judge - Cc:-ed him.
> 
> Sam ? I have a few more powerpc patches depending on that one... so I'd
> like to know if it's totally doomed or not :-)

Well, toplevel Makefile changes are not to be taken lightly :-)

Looks good to me but it's handy to have Sam's Ack on it too.

	Ingo

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Steven Rostedt]

On Wed, 2009-02-11 at 14:23 +0100, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > On Wed, 2009-02-04 at 22:26 +0100, Ingo Molnar wrote:
> > > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > > +
> > > >  # arch Makefile may override CC so keep this after arch Makefile is
> > > included
> > > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> > > -print-file-name=include)
> > > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > > -- 
> > > 
> > > this patch is really for Sam to judge - Cc:-ed him.
> > 
> > Sam ? I have a few more powerpc patches depending on that one... so I'd
> > like to know if it's totally doomed or not :-)
> 
> Well, toplevel Makefile changes are not to be taken lightly :-)
> 
> Looks good to me but it's handy to have Sam's Ack on it too.
> 

Someone on IRC said that Sam is on vacation. This could take a while for
an ACK.


-- Steve

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Sam Ravnborg]

On Wed, Feb 11, 2009 at 02:51:34PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-02-04 at 22:26 +0100, Ingo Molnar wrote:
> > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > +
> > >  # arch Makefile may override CC so keep this after arch Makefile is
> > included
> > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> > -print-file-name=include)
> > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > -- 
> > 
> > this patch is really for Sam to judge - Cc:-ed him.
> 
> Sam ? I have a few more powerpc patches depending on that one... so I'd
> like to know if it's totally doomed or not :-)

See my answer to the mail from Ingo.

We really need to do this on Kconfig level it at all possible.

	Sam

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Sam Ravnborg]

On Wed, Feb 04, 2009 at 10:26:12PM +0100, Ingo Molnar wrote:
> 
> * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > This patch gives arches more freedom on overwriting CFLAGS, specifically
> > on PowerPC we want to remove -fno-omit-frame-pointer flag.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  Makefile |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 7715b2c..d1ba93f 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -525,8 +525,6 @@ else
> >  KBUILD_CFLAGS	+= -O2
> >  endif
> >  
> > -include $(srctree)/arch/$(SRCARCH)/Makefile
> > -
> >  ifneq (CONFIG_FRAME_WARN,0)
> >  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
> >  endif
> > @@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
> >  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
> >  endif
> >  
> > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > +
> >  # arch Makefile may override CC so keep this after arch Makefile is included
> >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > -- 
> 
> this patch is really for Sam to judge - Cc:-ed him.

If we move the include further down then the following:

    # Force gcc to behave correct even for buggy distributions
    # Arch Makefiles may override this setting
    KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)

will most likely fail.

If popwerpc needs to get rid of "-fno-omit-frame-pointer" then
we need a way to express this at KConfig level and NOT by doing
some tricks with CFLAGS.

	Sam

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> On Wed, Feb 04, 2009 at 10:26:12PM +0100, Ingo Molnar wrote:
> > 
> > * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > This patch gives arches more freedom on overwriting CFLAGS, specifically
> > > on PowerPC we want to remove -fno-omit-frame-pointer flag.
> > > 
> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > ---
> > >  Makefile |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 7715b2c..d1ba93f 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -525,8 +525,6 @@ else
> > >  KBUILD_CFLAGS	+= -O2
> > >  endif
> > >  
> > > -include $(srctree)/arch/$(SRCARCH)/Makefile
> > > -
> > >  ifneq (CONFIG_FRAME_WARN,0)
> > >  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
> > >  endif
> > > @@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
> > >  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
> > >  endif
> > >  
> > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > +
> > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > -- 
> > 
> > this patch is really for Sam to judge - Cc:-ed him.
> 
> If we move the include further down then the following:
> 
>     # Force gcc to behave correct even for buggy distributions
>     # Arch Makefiles may override this setting
>     KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> 
> will most likely fail.

ah, ok. (I long ago made the mental note of "dont change the toplevel Makefile
if you can avoid it" - this reinforces that.)

> If popwerpc needs to get rid of "-fno-omit-frame-pointer" then
> we need a way to express this at KConfig level and NOT by doing
> some tricks with CFLAGS.

Here is what we have in the toplevel Makefile at the moment:

 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS   += -fno-omit-frame-pointer -fno-optimize-sibling-calls
 else
 KBUILD_CFLAGS   += -fomit-frame-pointer
 endif

My original suggestion (more than a week ago) was to make PPC always
select FRAME_POINTERS.

It was pointed out that -fno-omit-frame-pointers (i.e.: generate frame
pointers) not only makes the code less optimal on PPC, but it can also be 
miscompiled.

But instrumentation really needs to know whether __builtin_return_address(1)
[etc] is reliable, whether stack tracing is fast - and other details -  and PPC
is the odd one out.

So the question is: even with FRAME_POINTERS disabled on PPC, is 
__builtin_return_address(1)/(2) reliable, and is save_stack_trace() fast? (i.e.
can it walk down the stack frame efficiently, or does it have to scan the full
kernel stack) I.e. does PPC have all the material advantages of frame pointers?

	Ingo

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Benjamin Herrenschmidt]

On Sat, 2009-02-14 at 23:03 +0100, Ingo Molnar wrote:
> 
> So the question is: even with FRAME_POINTERS disabled on PPC, is 
> __builtin_return_address(1)/(2) reliable, and is save_stack_trace() fast? (i.e.
> can it walk down the stack frame efficiently, or does it have to scan the full
> kernel stack) I.e. does PPC have all the material advantages of frame pointers?

Yes, we do. We effectively have frame pointers in fact, they may only be
omitted in leaf functions but then gcc __builtin_return_address() knows
how to handle that afaik.

Cheers,
Ben.

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Sat, 2009-02-14 at 23:03 +0100, Ingo Molnar wrote:
> > 
> > So the question is: even with FRAME_POINTERS disabled on PPC, is 
> > __builtin_return_address(1)/(2) reliable, and is save_stack_trace() fast? (i.e.
> > can it walk down the stack frame efficiently, or does it have to scan the full
> > kernel stack) I.e. does PPC have all the material advantages of frame pointers?
> 
> Yes, we do. We effectively have frame pointers in fact, they may only be
> omitted in leaf functions but then gcc __builtin_return_address() knows
> how to handle that afaik.

So basically we want to define FRAME_POINTERS on PPC, but do not want
the -fno-omit-frame-pointers flag.

Originally (many moons ago) FRAME_POINTER _was_ just the toplevel Makefile
detail, but these days we've got a handful of secondary uses as well,
expressing the reliability of backtraces in essence.

We could split the whole option (affecting lots of files), or we could zap
that compiler flag in the PPC case - it is only PPC that worries about this
anyway.

	Ingo

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Anton Vorontsov]

On Sat, Feb 14, 2009 at 08:57:02PM +0100, Sam Ravnborg wrote:
[...]
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -525,8 +525,6 @@ else
> > >  KBUILD_CFLAGS	+= -O2
> > >  endif
> > >  
> > > -include $(srctree)/arch/$(SRCARCH)/Makefile
> > > -
> > >  ifneq (CONFIG_FRAME_WARN,0)
> > >  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
> > >  endif
> > > @@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
> > >  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
> > >  endif
> > >  
> > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > +
> > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > -- 
> > 
> > this patch is really for Sam to judge - Cc:-ed him.
> 
> If we move the include further down then the following:
> 
>     # Force gcc to behave correct even for buggy distributions
>     # Arch Makefiles may override this setting
>     KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> 
> will most likely fail.

Hm... I don't get it, the comment says that the arch "may override
this setting", but currently it may not, since arch makefile is
included before this line. So my patch also fixes this "bug", no?

Or arches supposed to overwrite "cc-option" behaviour? Hm..

There is an interesting commit:

commit e06b8b98da071f7dd78fb7822991694288047df0
Author: Sam Ravnborg <sam@ravnborg.org>
Date:   Wed Feb 13 22:43:28 2008 +0100

    kbuild: allow -fstack-protector to take effect
[...]
--- a/Makefile
+++ b/Makefile
@@ -507,6 +507,10 @@ else
 KBUILD_CFLAGS  += -O2
 endif

+# Force gcc to behave correct even for buggy distributions
+# Arch Makefiles may override this setting
+KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
+
 include $(srctree)/arch/$(SRCARCH)/Makefile
 
 ifdef CONFIG_FRAME_POINTER
@@ -525,9 +529,6 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
 KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
 endif
 
-# Force gcc to behave correct even for buggy distributions
-KBUILD_CFLAGS         += $(call cc-option, -fno-stack-protector)
-
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 CHECKFLAGS     += $(NOSTDINC_FLAGS)


^^ Which does the same (wrt fno-stack-protector) as the patch I
sent, i.e. includes arch Makefile later.

But then, this commit reverted things back (w/o your Ack):

commit bef5b54bd7bf8117c75cb943d64549134c6d9a1f
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Wed Jul 16 13:02:24 2008 +0100

    Fix MIPS cross-compile problem
    [...]
    Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[...]
--- a/Makefile
+++ b/Makefile
@@ -508,6 +508,8 @@ else
 KBUILD_CFLAGS  += -O2
 endif
 
+include $(srctree)/arch/$(SRCARCH)/Makefile
+
 ifneq (CONFIG_FRAME_WARN,0)
 KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
 endif
@@ -516,8 +518,6 @@ endif
 # Arch Makefiles may override this setting
 KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
 
-include $(srctree)/arch/$(SRCARCH)/Makefile
-
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS  += -fno-omit-frame-pointer -fno-optimize-sibling-calls
 else


:-?

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

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Anton Vorontsov]

On Mon, Feb 16, 2009 at 05:20:01PM +0300, Anton Vorontsov wrote:
> On Sat, Feb 14, 2009 at 08:57:02PM +0100, Sam Ravnborg wrote:
> [...]
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -525,8 +525,6 @@ else
> > > >  KBUILD_CFLAGS	+= -O2
> > > >  endif
> > > >  
> > > > -include $(srctree)/arch/$(SRCARCH)/Makefile
> > > > -
> > > >  ifneq (CONFIG_FRAME_WARN,0)
> > > >  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
> > > >  endif
> > > > @@ -555,6 +553,8 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
> > > >  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
> > > >  endif
> > > >  
> > > > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > > > +
> > > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > > >  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> > > > -- 
> > > 
> > > this patch is really for Sam to judge - Cc:-ed him.
> > 
> > If we move the include further down then the following:
> > 
> >     # Force gcc to behave correct even for buggy distributions
> >     # Arch Makefiles may override this setting
> >     KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> > 
> > will most likely fail.
> 
> Hm... I don't get it, the comment says that the arch "may override
> this setting", but currently it may not, since arch makefile is
> included before this line. So my patch also fixes this "bug", no?
> 
> Or arches supposed to overwrite "cc-option" behaviour? Hm..
> 
> There is an interesting commit:
> 
> commit e06b8b98da071f7dd78fb7822991694288047df0
> Author: Sam Ravnborg <sam@ravnborg.org>
> Date:   Wed Feb 13 22:43:28 2008 +0100
> 
>     kbuild: allow -fstack-protector to take effect
> [...]
> --- a/Makefile
> +++ b/Makefile
> @@ -507,6 +507,10 @@ else
>  KBUILD_CFLAGS  += -O2
>  endif
> 
> +# Force gcc to behave correct even for buggy distributions
> +# Arch Makefiles may override this setting
> +KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> +
>  include $(srctree)/arch/$(SRCARCH)/Makefile
>  
>  ifdef CONFIG_FRAME_POINTER
> @@ -525,9 +529,6 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
>  KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
>  endif
>  
> -# Force gcc to behave correct even for buggy distributions
> -KBUILD_CFLAGS         += $(call cc-option, -fno-stack-protector)
> -
>  # arch Makefile may override CC so keep this after arch Makefile is included
>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
> 
> 
> ^^ Which does the same (wrt fno-stack-protector) as the patch I
> sent, i.e. includes arch Makefile later.
> 
> But then, this commit reverted things back (w/o your Ack):
> 
> commit bef5b54bd7bf8117c75cb943d64549134c6d9a1f
> Author: Ralf Baechle <ralf@linux-mips.org>
> Date:   Wed Jul 16 13:02:24 2008 +0100
> 
>     Fix MIPS cross-compile problem
>     [...]
>     Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Ok, I think I got it. It seems that Ralf was using this feature
of arch/mips/Makefile:

ifneq ($(SUBARCH),$(ARCH))
  ifeq ($(CROSS_COMPILE),)
    CROSS_COMPILE := $(call cc-cross-prefix, $(tool-archpref)-linux-  $(tool-archpref)-linux-gnu-  $(tool-archpref)-unknown-linux-gnu-)
  endif
endif

There are few arches that have "auto detection" of CROSS_COMPILE
variable: mips, m68k, parisc, xtensa, h8300 and blackfin.

So `normal' arches don't have any chance to overwrite the flags
because of the few offenders above. ;-)

> [...]
> --- a/Makefile
> +++ b/Makefile
> @@ -508,6 +508,8 @@ else
>  KBUILD_CFLAGS  += -O2
>  endif
>  
> +include $(srctree)/arch/$(SRCARCH)/Makefile
> +
>  ifneq (CONFIG_FRAME_WARN,0)
>  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
>  endif
> @@ -516,8 +518,6 @@ endif
>  # Arch Makefiles may override this setting
>  KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
>  
> -include $(srctree)/arch/$(SRCARCH)/Makefile
> -
>  ifdef CONFIG_FRAME_POINTER
>  KBUILD_CFLAGS  += -fno-omit-frame-pointer -fno-optimize-sibling-calls
>  else

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

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Sam Ravnborg]

> 
> There are few arches that have "auto detection" of CROSS_COMPILE
> variable: mips, m68k, parisc, xtensa, h8300 and blackfin.
> 
> So `normal' arches don't have any chance to overwrite the flags
> because of the few offenders above. ;-)

We also have archs where we add options to gcc that may
impact for example -fno-stack-protector.
We had a bug in x86 in this area for a while.

So the only sane thing is to only add the most generic options to
KBUILD_CFLAGS before we include the arch specific part.
And we can then add the conditional options later - like
the KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
in this case.

We learned a few lessons in this area over time as your
history digging also showed.

	Sam

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Anton Vorontsov]

On Mon, Feb 16, 2009 at 05:20:01PM +0300, Anton Vorontsov wrote:
[...]
> But then, this commit reverted things back (w/o your Ack):
> 
> commit bef5b54bd7bf8117c75cb943d64549134c6d9a1f
> Author: Ralf Baechle <ralf@linux-mips.org>
> Date:   Wed Jul 16 13:02:24 2008 +0100
> 
>     Fix MIPS cross-compile problem
>     [...]
>     Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> [...]
> --- a/Makefile
> +++ b/Makefile
> @@ -508,6 +508,8 @@ else
>  KBUILD_CFLAGS  += -O2
>  endif
>  
> +include $(srctree)/arch/$(SRCARCH)/Makefile
> +
>  ifneq (CONFIG_FRAME_WARN,0)
>  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
>  endif
> @@ -516,8 +518,6 @@ endif
>  # Arch Makefiles may override this setting
>  KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
>  
> -include $(srctree)/arch/$(SRCARCH)/Makefile
> -
>  ifdef CONFIG_FRAME_POINTER
>  KBUILD_CFLAGS  += -fno-omit-frame-pointer -fno-optimize-sibling-calls
>  else

Btw, I wonder if x86's CONFIG_CC_STACKPROTECTOR is a no-op since
v2.6.27-rc1? Seems like it is.

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

Re: [PATCH 1/3] Makefile: Include arch Makefiles as late as possible

[Ingo Molnar]

* Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Mon, Feb 16, 2009 at 05:20:01PM +0300, Anton Vorontsov wrote:
> [...]
> > But then, this commit reverted things back (w/o your Ack):
> > 
> > commit bef5b54bd7bf8117c75cb943d64549134c6d9a1f
> > Author: Ralf Baechle <ralf@linux-mips.org>
> > Date:   Wed Jul 16 13:02:24 2008 +0100
> > 
> >     Fix MIPS cross-compile problem
> >     [...]
> >     Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > [...]
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -508,6 +508,8 @@ else
> >  KBUILD_CFLAGS  += -O2
> >  endif
> >  
> > +include $(srctree)/arch/$(SRCARCH)/Makefile
> > +
> >  ifneq (CONFIG_FRAME_WARN,0)
> >  KBUILD_CFLAGS += $(call cc-option,-Wframe-larger-than=${CONFIG_FRAME_WARN})
> >  endif
> > @@ -516,8 +518,6 @@ endif
> >  # Arch Makefiles may override this setting
> >  KBUILD_CFLAGS += $(call cc-option, -fno-stack-protector)
> >  
> > -include $(srctree)/arch/$(SRCARCH)/Makefile
> > -
> >  ifdef CONFIG_FRAME_POINTER
> >  KBUILD_CFLAGS  += -fno-omit-frame-pointer -fno-optimize-sibling-calls
> >  else
> 
> Btw, I wonder if x86's CONFIG_CC_STACKPROTECTOR is a no-op since
> v2.6.27-rc1? Seems like it is.

that's fixed and merged up for 2.6.30.

	Ingo

[PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

Remove -fno-omit-frame-pointer flag from CFLAGS.
Remove -fno-omit-frame-pointer workarounds.

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

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 72d17f5..627aa7f 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -120,10 +120,12 @@ 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
+# Remove -fno-omit-frame-pointer flag. There are two reasons:
+# 1. On powerpc we always have frame pointers.
+# 2. Some gcc versions might generate wrong code with -fno-omit-frame-pointer
+#    flag. So, if for some reason you're going to remove this line, then add
+#    -mno-sched-epilog to workaround the gcc bug.
+KBUILD_CFLAGS := $(subst -fno-omit-frame-pointer,,$(KBUILD_CFLAGS))
 
 cpu-as-$(CONFIG_4xx)		+= -Wa,-m405
 cpu-as-$(CONFIG_6xx)		+= -Wa,-maltivec
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 8d1a419..a97f215 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
 
 ifdef CONFIG_DYNAMIC_FTRACE
 # dynamic ftrace setup.
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg
 endif
 
 endif
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 29044f5..657126c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -402,7 +402,7 @@ config LOCKDEP
 	bool
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
 	select STACKTRACE
-	select FRAME_POINTER if !X86 && !MIPS && !PPC
+	select FRAME_POINTER if !X86 && !MIPS
 	select KALLSYMS
 	select KALLSYMS_ALL
 
@@ -583,7 +583,7 @@ 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
@@ -771,13 +771,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

Re: [PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 18:08 +0300, Anton Vorontsov wrote:
> Remove -fno-omit-frame-pointer flag from CFLAGS.
> Remove -fno-omit-frame-pointer workarounds.

But what about -pg -requires -fno-omit-frame-pointer ?

Ben.

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  arch/powerpc/Makefile                    |   10 ++++++----
>  arch/powerpc/kernel/Makefile             |   10 +++++-----
>  arch/powerpc/platforms/powermac/Makefile |    2 +-
>  lib/Kconfig.debug                        |    8 ++++----
>  4 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 72d17f5..627aa7f 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -120,10 +120,12 @@ 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
> +# Remove -fno-omit-frame-pointer flag. There are two reasons:
> +# 1. On powerpc we always have frame pointers.
> +# 2. Some gcc versions might generate wrong code with -fno-omit-frame-pointer
> +#    flag. So, if for some reason you're going to remove this line, then add
> +#    -mno-sched-epilog to workaround the gcc bug.
> +KBUILD_CFLAGS := $(subst -fno-omit-frame-pointer,,$(KBUILD_CFLAGS))
>  
>  cpu-as-$(CONFIG_4xx)		+= -Wa,-m405
>  cpu-as-$(CONFIG_6xx)		+= -Wa,-maltivec
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 8d1a419..a97f215 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
>  
>  ifdef CONFIG_DYNAMIC_FTRACE
>  # dynamic ftrace setup.
> -CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_ftrace.o = -pg
>  endif
>  
>  endif
> 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 29044f5..657126c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -402,7 +402,7 @@ config LOCKDEP
>  	bool
>  	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>  	select STACKTRACE
> -	select FRAME_POINTER if !X86 && !MIPS && !PPC
> +	select FRAME_POINTER if !X86 && !MIPS
>  	select KALLSYMS
>  	select KALLSYMS_ALL
>  
> @@ -583,7 +583,7 @@ 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
> @@ -771,13 +771,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 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Steven Rostedt]

On Thu, 2009-02-05 at 11:31 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-02-04 at 18:08 +0300, Anton Vorontsov wrote:
> > Remove -fno-omit-frame-pointer flag from CFLAGS.
> > Remove -fno-omit-frame-pointer workarounds.
> 
> But what about -pg -requires -fno-omit-frame-pointer ?

I don't think it requires -fno-omit-frame-pointer. It is just
incompatible with -fomit-frame-pointer.

-- Steve

Re: [PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

On Wed, Feb 04, 2009 at 07:35:54PM -0500, Steven Rostedt wrote:
> 
> On Thu, 2009-02-05 at 11:31 +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2009-02-04 at 18:08 +0300, Anton Vorontsov wrote:
> > > Remove -fno-omit-frame-pointer flag from CFLAGS.
> > > Remove -fno-omit-frame-pointer workarounds.
> > 
> > But what about -pg -requires -fno-omit-frame-pointer ?
> 
> I don't think it requires -fno-omit-frame-pointer. It is just
> incompatible with -fomit-frame-pointer.

Yep, -fno-omit-frame-poiner isn't needed for -pg.

I explained this in http://lkml.org/lkml/2009/2/3/342 :

| Ah...
|
| $ gcc -pg -fomit-frame-pointer -S c.c
| gcc: -pg and -fomit-frame-pointer are incompatible
|
| It's hard-coded in gcc, in the code that don't know about
| architecture details.
|
| But on PowerPC -O1 implies -fomit-frame-pointer, that is
|
| gcc -pg -O1 -fno-omit-frame-pointer
| and
| gcc -pg -O1
|
| produce different outputs. Thus -pg -O should be the same
| as "-pg -O -fomit-framepointer".

That is,

`gcc -O -pg -fno-omit-frame-pointer -S` output:

main:
        mflr 0
        stw 0,4(1)
        bl _mcount
        stwu 1,-16(1)
        mflr 0
        stw 0,20(1)
        stw 31,12(1)
        mr 31,1
        li 3,0
        lwz 11,0(1)
        lwz 0,4(11)
        mtlr 0
        lwz 31,-4(11)
        mr 1,11
        blr

^^ -pg works (_mcount call), but r31 wasted.

Now `gcc -O -pg -S` output:

main:
        mflr 0
        stw 0,4(1)
        bl _mcount
        stwu 1,-16(1)
        mflr 0
        stw 0,20(1)
        li 3,0
        lwz 0,20(1)
        mtlr 0
        addi 1,1,16
        blr

^^ _mcount is still there, and r31 isn't used.

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

Re: [PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 19:35 -0500, Steven Rostedt wrote:
> On Thu, 2009-02-05 at 11:31 +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2009-02-04 at 18:08 +0300, Anton Vorontsov wrote:
> > > Remove -fno-omit-frame-pointer flag from CFLAGS.
> > > Remove -fno-omit-frame-pointer workarounds.
> > 
> > But what about -pg -requires -fno-omit-frame-pointer ?
> 
> I don't think it requires -fno-omit-frame-pointer. It is just
> incompatible with -fomit-frame-pointer.

>From a quick test, you seem to be right.

Now the question of course is whether -pg will internally trigger
the bug for which we need the workaround tho...

Ben.

Re: [PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

On Thu, Feb 05, 2009 at 12:15:31PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-02-04 at 19:35 -0500, Steven Rostedt wrote:
> > On Thu, 2009-02-05 at 11:31 +1100, Benjamin Herrenschmidt wrote:
> > > On Wed, 2009-02-04 at 18:08 +0300, Anton Vorontsov wrote:
> > > > Remove -fno-omit-frame-pointer flag from CFLAGS.
> > > > Remove -fno-omit-frame-pointer workarounds.
> > > 
> > > But what about -pg -requires -fno-omit-frame-pointer ?
> > 
> > I don't think it requires -fno-omit-frame-pointer. It is just
> > incompatible with -fomit-frame-pointer.
> 
> From a quick test, you seem to be right.
> 
> Now the question of course is whether -pg will internally trigger
> the bug for which we need the workaround tho...

According to http://marc.info/?l=linux-kernel&m=121958694215835&w=4
just removing -fno-omit-frame-pointer solved the problem.

But I'll double check that on the actual hardware.

Thanks,

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

Re: [PATCH 2/3] powerpc: Make it possible to safely select CONFIG_FRAME_POINTER

[Anton Vorontsov]

On Thu, Feb 05, 2009 at 04:30:23AM +0300, Anton Vorontsov wrote:
[...]
> > > > > Remove -fno-omit-frame-pointer flag from CFLAGS.
> > > > > Remove -fno-omit-frame-pointer workarounds.
> > > > 
> > > > But what about -pg -requires -fno-omit-frame-pointer ?
> > > 
> > > I don't think it requires -fno-omit-frame-pointer. It is just
> > > incompatible with -fomit-frame-pointer.
> > 
> > From a quick test, you seem to be right.
> > 
> > Now the question of course is whether -pg will internally trigger
> > the bug for which we need the workaround tho...
> 
> According to http://marc.info/?l=linux-kernel&m=121958694215835&w=4
> just removing -fno-omit-frame-pointer solved the problem.
> 
> But I'll double check that on the actual hardware.

Confirmed. Just -pg doesn't cause any ill effects (for the test
I built kernel on a ppc32 board).

While simply adding -fno-omit-frame-pointer caused this during
bootup:

...
Waiting for /dev to be fully populated...
Unable to handle kernel paging request for data at address 0x00000901
Faulting instruction address: 0xc00d9a4c
Oops: Kernel access of bad area, sig: 11 [#1]
MPC837x RDB
Modules linked in:
NIP: c00d9a4c LR: c00d9a34 CTR: c015ac7c
REGS: cf197cd0 TRAP: 0300   Not tainted  (2.6.29-rc3-45527-g727839a-dirty)
MSR: 00009032 <EE,ME,IR,DR>  CR: 44084228  XER: 20000000
DAR: 00000901, DSISR: 22000000
TASK = cfa42a80[1140] 'udevd' THREAD: cf196000
GPR00: fffff000 cf197d80 cfa42a80 cf4213f0 c058e678 00000003 cf42145a 7f7f7f7f
GPR08: 0000012c 00000000 c0580000 cf197d80 84084228 1002414c cf197e9c c058e7b4
GPR16: c058e678 fffffff4 c05b0000 cf197dc0 cf197db8 cf197dc8 00000000 00000000
GPR24: c00e4e30 00000000 24084222 00000001 00000901 cf197e98 cf4213f0 cf197d80
NIP [c00d9a4c] do_lookup+0x7c/0xf8
LR [c00d9a34] do_lookup+0x64/0xf8
Call Trace:
[cf197d80] [c00d9a34] do_lookup+0x64/0xf8 (unreliable)
[cf197db0] [c00db73c] __link_path_walk+0x180/0xdc4
[cf197e30] [c00dc3d8] path_walk+0x58/0xe0
[cf197e60] [c00dc55c] do_path_lookup+0x8c/0x164
[cf197e90] [c00dd260] user_path_at+0x64/0xac
[cf197f00] [c00d3760] sys_readlinkat+0x40/0xb4
[cf197f30] [c00d380c] sys_readlink+0x38/0x50
[cf197f40] [c0018040] ret_from_syscall+0x0/0x38
--- Exception: c01 at 0xfed4754
    LR = 0x10006280
Instruction dump:
80090000 2f800000 419e0028 7fa4eb78 7c0903a6 4e800421 7c7b1b79 40810040
3800f000 7fc3f378 7f9e0040 419d0018 <935c0000> 7f83e378 93dc0004 4bfffcd1
---[ end trace 4815c47675962320 ]---

FWIW, gcc version 4.2.0.

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

[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 e2a4ff6..48f7a37 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -86,6 +86,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.
@@ -108,6 +109,7 @@ config PREEMPT_TRACER
 	depends on DEBUG_KERNEL
 	select TRACING
 	select TRACER_MAX_TRACE
+	select FRAME_POINTER
 	help
 	  This option measures the time spent in preemption off critical
 	  sections, with microsecond accuracy.
@@ -136,6 +138,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 3/3] tracing: Tracers that use CALLER_ADDR macros should select FRAME_POINTER

[Frédéric Weisbecker]

2009/2/4 Anton Vorontsov <avorontsov@ru.mvista.com>:
> 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 e2a4ff6..48f7a37 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -86,6 +86,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.
> @@ -108,6 +109,7 @@ config PREEMPT_TRACER
>        depends on DEBUG_KERNEL
>        select TRACING
>        select TRACER_MAX_TRACE
> +       select FRAME_POINTER
>        help
>          This option measures the time spent in preemption off critical
>          sections, with microsecond accuracy.
> @@ -136,6 +138,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.
> --


Looks right.

BTW, how behaves builtin_return_address in case of !FRAME_POINTERS ?
I guess it would only work with the first caller builtin_return_address(0)

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

[Steven Rostedt]

On Wed, 2009-02-04 at 16:26 +0100, Frédéric Weisbecker wrote:
> 2009/2/4 Anton Vorontsov <avorontsov@ru.mvista.com>:
> > 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 e2a4ff6..48f7a37 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -86,6 +86,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.
> > @@ -108,6 +109,7 @@ config PREEMPT_TRACER
> >        depends on DEBUG_KERNEL
> >        select TRACING
> >        select TRACER_MAX_TRACE
> > +       select FRAME_POINTER
> >        help
> >          This option measures the time spent in preemption off critical
> >          sections, with microsecond accuracy.
> > @@ -136,6 +138,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.
> > --
> 
> 
> Looks right.
> 
> BTW, how behaves builtin_return_address in case of !FRAME_POINTERS ?
> I guess it would only work with the first caller builtin_return_address(0)


>From ftrace.h:

#ifdef CONFIG_FRAME_POINTER
/* TODO: need to fix this for ARM */
# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
# define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))
# define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2))
# define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3))
# define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4))
# define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5))
# define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6))
#else
# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
# define CALLER_ADDR1 0UL
# define CALLER_ADDR2 0UL
# define CALLER_ADDR3 0UL
# define CALLER_ADDR4 0UL
# define CALLER_ADDR5 0UL
# define CALLER_ADDR6 0UL
#endif

Yep!

-- Steve

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

[Anton Vorontsov]

On Wed, Feb 04, 2009 at 04:26:15PM +0100, Frédéric Weisbecker wrote:
[...]
> > @@ -136,6 +138,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.
> > --
> 
> 
> Looks right.
> 
> BTW, how behaves builtin_return_address in case of !FRAME_POINTERS ?
> I guess it would only work with the first caller builtin_return_address(0)

It depends on the architecture. On PowerPC we always have frame pointers,
thus __builtin_return_address(1..) will always work. On x86 it won't work
that way.

Thanks,

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

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

[Frédéric Weisbecker]

2009/2/4 Steven Rostedt <srostedt@redhat.com>:
>
> >From ftrace.h:
>
> #ifdef CONFIG_FRAME_POINTER
> /* TODO: need to fix this for ARM */
> # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
> # define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1))
> # define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2))
> # define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3))
> # define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4))
> # define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5))
> # define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6))
> #else
> # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
> # define CALLER_ADDR1 0UL
> # define CALLER_ADDR2 0UL
> # define CALLER_ADDR3 0UL
> # define CALLER_ADDR4 0UL
> # define CALLER_ADDR5 0UL
> # define CALLER_ADDR6 0UL
> #endif
>
> Yep!
>

Ok.

2009/2/4 Anton Vorontsov <avorontsov@ru.mvista.com>:
> It depends on the architecture. On PowerPC we always have frame pointers,
> thus __builtin_return_address(1..) will always work. On x86 it won't work
> that way.
>
> Thanks,
>
> --
> Anton Vorontsov

Thanks for your answers!

RE: [PATCH v2] ftrace: On PowerPC we don't need frame pointers forCALLER_ADDRs

[Usha Rani Konudula]

unsubscribe linux-kernel

(no subject)

[Usha Rani Konudula]

unsubscribe linux-kernel

[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 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

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

[Anton Vorontsov]

Hi all,

In v5 there are few fixes suggested by Sam Ravnborg:

- Use 8 spaces for indentation in Makefile;
- Rename ARCH_HAS_NORMAL_FRAME_POINTERS to HAVE_NORMAL_FRAME_POINTER;

Thanks,

--- 

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.

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.

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

[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 450d3c2..ad44791 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -122,6 +122,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.
@@ -143,6 +144,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.
@@ -171,6 +173,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.6.2.2