[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[Yang Shi]

FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
is never used.
This adds a dependency on DEBUG_KERNEL for building with frame pointers.

ARM64 depends on frame pointer to get correct stack backtrace and need
FRAME_POINTER kconfig option enabled all the time.
However, currect implementation makes it could be disabled, so force it
to be selected by ARM64.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
change v3 --> v2:
squash two patches into one.

 arch/arm64/Kconfig       | 1 +
 arch/arm64/Kconfig.debug | 4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 440d906..b554da2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -27,6 +27,7 @@ config ARM64
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS
 	select EDAC_SUPPORT
+	select FRAME_POINTER
 	select GENERIC_ALLOCATOR
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_CLOCKEVENTS_BROADCAST
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index d6285ef..915dea7 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -2,10 +2,6 @@ menu "Kernel hacking"
 
 source "lib/Kconfig.debug"
 
-config FRAME_POINTER
-	bool
-	default y
-
 config ARM64_PTDUMP
 	bool "Export kernel pagetable layout to userspace via debugfs"
 	depends on DEBUG_KERNEL
-- 
2.0.2

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[Catalin Marinas]

On Mon, Nov 09, 2015 at 10:09:55AM -0800, Yang Shi wrote:
> FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
> is never used.

That's not true since the arm64 definition seems to take precedence.

> This adds a dependency on DEBUG_KERNEL for building with frame pointers.

It doesn't because arm64 selects ARCH_WANT_FRAME_POINTERS.

> ARM64 depends on frame pointer to get correct stack backtrace and need
> FRAME_POINTER kconfig option enabled all the time.
> However, currect implementation makes it could be disabled, so force it
> to be selected by ARM64.
> 
> Signed-off-by: Yang Shi <yang.shi@linaro.org>

Patch applied but I changed the commit log slightly. Thanks.

-- 
Catalin

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[yalin wang]

> On Nov 10, 2015, at 18:37, Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Mon, Nov 09, 2015 at 10:09:55AM -0800, Yang Shi wrote:
>> FRAME_POINTER  is defined in lib/Kconfig.debug, it is unnecessary to redefine
>> it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
>> is never used.
> 
> That's not true since the arm64 definition seems to take precedence.
> 
>> This adds a dependency on DEBUG_KERNEL for building with frame pointers.
> 
> It doesn't because arm64 selects ARCH_WANT_FRAME_POINTERS.
> 
>> ARM64 depends on frame pointer to get correct stack backtrace and need
>> FRAME_POINTER kconfig option enabled all the time.
>> However, currect implementation makes it could be disabled, so force it
>> to be selected by ARM64.
>> 
>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
> 
> Patch applied but I changed the commit log slightly. Thanks.
i have a question,
why FRAME_POINTER  config must be enabled ?
and i see ARM arch can  disable this config .
if i don?t need stack trace dump and the software release is for 
final product , don?t need debug stack trace log .
is it possible to disable it for performance reason ?

Thanks

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[Catalin Marinas]

On Tue, Nov 10, 2015 at 07:09:00PM +0800, yalin wang wrote:
> > On Nov 10, 2015, at 18:37, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > On Mon, Nov 09, 2015 at 10:09:55AM -0800, Yang Shi wrote:
> >> FRAME_POINTER  is defined in lib/Kconfig.debug, it is unnecessary to redefine
> >> it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
> >> is never used.
> > 
> > That's not true since the arm64 definition seems to take precedence.
> > 
> >> This adds a dependency on DEBUG_KERNEL for building with frame pointers.
> > 
> > It doesn't because arm64 selects ARCH_WANT_FRAME_POINTERS.
> > 
> >> ARM64 depends on frame pointer to get correct stack backtrace and need
> >> FRAME_POINTER kconfig option enabled all the time.
> >> However, currect implementation makes it could be disabled, so force it
> >> to be selected by ARM64.
> >> 
> >> Signed-off-by: Yang Shi <yang.shi@linaro.org>
> > 
> > Patch applied but I changed the commit log slightly. Thanks.
> i have a question,
> why FRAME_POINTER  config must be enabled ?
> and i see ARM arch can  disable this config .
> if i don?t need stack trace dump and the software release is for 
> final product , don?t need debug stack trace log .
> is it possible to disable it for performance reason ?

If you don't need any stack trace, perf etc., in theory you can disable
the option. However, the aarch64 gcc compiler always generates it (I'm
not sure whether the AAPCS mandates it). Anyway, the performance impact
is very small since there are more general purpose registers available
in AArch64 already.

-- 
Catalin

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[yalin wang]

> On Nov 10, 2015, at 19:35, Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Tue, Nov 10, 2015 at 07:09:00PM +0800, yalin wang wrote:
>>> On Nov 10, 2015, at 18:37, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>> 
>>> On Mon, Nov 09, 2015 at 10:09:55AM -0800, Yang Shi wrote:
>>>> FRAME_POINTER  is defined in lib/Kconfig.debug, it is unnecessary to redefine
>>>> it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
>>>> is never used.
>>> 
>>> That's not true since the arm64 definition seems to take precedence.
>>> 
>>>> This adds a dependency on DEBUG_KERNEL for building with frame pointers.
>>> 
>>> It doesn't because arm64 selects ARCH_WANT_FRAME_POINTERS.
>>> 
>>>> ARM64 depends on frame pointer to get correct stack backtrace and need
>>>> FRAME_POINTER kconfig option enabled all the time.
>>>> However, currect implementation makes it could be disabled, so force it
>>>> to be selected by ARM64.
>>>> 
>>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>>> 
>>> Patch applied but I changed the commit log slightly. Thanks.
>> i have a question,
>> why FRAME_POINTER  config must be enabled ?
>> and i see ARM arch can  disable this config .
>> if i don?t need stack trace dump and the software release is for 
>> final product , don?t need debug stack trace log .
>> is it possible to disable it for performance reason ?
> 
> If you don't need any stack trace, perf etc., in theory you can disable
> the option. However, the aarch64 gcc compiler always generates it (I'm
> not sure whether the AAPCS mandates it). Anyway, the performance impact
> is very small since there are more general purpose registers available
> in AArch64 already.
> 
i just make a test with -fomit-frame-pointer,  seems gcc can generate code without frame pointer,
like ARM arch.
version:
aarch64-linux-gnu-gcc
gcc version 4.9.2 20140904 (prerelease) (crosstool-NG linaro-1.13.1-4.9-2014.09 - Linaro GCC 4.9-2014.09)

why AARCH64 don?t have frame unwind info just like ARM arch?

Thanks

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[Will Deacon]

On Tue, Nov 10, 2015 at 07:43:35PM +0800, yalin wang wrote:
> > On Nov 10, 2015, at 19:35, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Tue, Nov 10, 2015 at 07:09:00PM +0800, yalin wang wrote:
> >> i have a question,
> >> why FRAME_POINTER  config must be enabled ?
> >> and i see ARM arch can  disable this config .
> >> if i don?t need stack trace dump and the software release is for 
> >> final product , don?t need debug stack trace log .
> >> is it possible to disable it for performance reason ?
> > 
> > If you don't need any stack trace, perf etc., in theory you can disable
> > the option. However, the aarch64 gcc compiler always generates it (I'm
> > not sure whether the AAPCS mandates it). Anyway, the performance impact
> > is very small since there are more general purpose registers available
> > in AArch64 already.
> > 
> i just make a test with -fomit-frame-pointer,  seems gcc can generate code
> without frame pointer,

Building without frame-pointers *severely* limits our ability (as
open-source developers) to debug problems that we are unable to reproduce
locally. Given the lack of widely available hardware compared to the
number of platforms in development, I'm strongly opposed to offering this
as a supported option for mainline kernels without a compelling performance
argument.

We have a significant number of general-purpose registers available on
arm64, so I would expect the omission of a framepointer to have a
somewhat limited impact on performance.

Will

[V3 PATCH] arm64: remove redundant FRAME_POINTER kconfig option and force to select it

[Shi, Yang]

On 11/10/2015 2:37 AM, Catalin Marinas wrote:
> On Mon, Nov 09, 2015 at 10:09:55AM -0800, Yang Shi wrote:
>> FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
>> it in arch/arm64/Kconfig.debug. Actually, the one defined in arm64 directory
>> is never used.
>
> That's not true since the arm64 definition seems to take precedence.
>
>> This adds a dependency on DEBUG_KERNEL for building with frame pointers.
>
> It doesn't because arm64 selects ARCH_WANT_FRAME_POINTERS.

Thanks for pointing it out. I misread the order of precedence, still 
think of C syntax at the first place.

Yang

>
>> ARM64 depends on frame pointer to get correct stack backtrace and need
>> FRAME_POINTER kconfig option enabled all the time.
>> However, currect implementation makes it could be disabled, so force it
>> to be selected by ARM64.
>>
>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>
> Patch applied but I changed the commit log slightly. Thanks.
>