2015-11-09 18:28:25

by Shi, Yang

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

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 <[email protected]>
---
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


2015-11-10 10:37:57

by Catalin Marinas

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

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 <[email protected]>

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

--
Catalin

2015-11-10 11:09:09

by yalin wang

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


> On Nov 10, 2015, at 18:37, Catalin Marinas <[email protected]> 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 <[email protected]>
>
> 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

2015-11-10 11:35:11

by Catalin Marinas

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

On Tue, Nov 10, 2015 at 07:09:00PM +0800, yalin wang wrote:
> > On Nov 10, 2015, at 18:37, Catalin Marinas <[email protected]> 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 <[email protected]>
> >
> > 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

2015-11-10 11:43:44

by yalin wang

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


> On Nov 10, 2015, at 19:35, Catalin Marinas <[email protected]> wrote:
>
> On Tue, Nov 10, 2015 at 07:09:00PM +0800, yalin wang wrote:
>>> On Nov 10, 2015, at 18:37, Catalin Marinas <[email protected]> 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 <[email protected]>
>>>
>>> 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







2015-11-10 11:53:59

by Will Deacon

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

On Tue, Nov 10, 2015 at 07:43:35PM +0800, yalin wang wrote:
> > On Nov 10, 2015, at 19:35, Catalin Marinas <[email protected]> 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

2015-11-10 16:51:11

by Shi, Yang

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

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 <[email protected]>
>
> Patch applied but I changed the commit log slightly. Thanks.
>