[PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Yang Shi]

FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
it in arch/arm64/Kconfig.debug.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
 arch/arm64/Kconfig.debug | 4 ----
 1 file changed, 4 deletions(-)

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

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Will Deacon]

On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> it in arch/arm64/Kconfig.debug.

It might be worth noting that this adds a dependency on DEBUG_KERNEL
for building with frame pointers. I'm ok with that (it appears to be
enabled in defconfig and follows the vast majority of other archs) but
it is a change in behaviour.

With that:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

> Signed-off-by: Yang Shi <yang.shi@linaro.org>
> ---
>  arch/arm64/Kconfig.debug | 4 ----
>  1 file changed, 4 deletions(-)
> 
> 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
> 

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Mark Rutland]

On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > it in arch/arm64/Kconfig.debug.
> 
> It might be worth noting that this adds a dependency on DEBUG_KERNEL
> for building with frame pointers. I'm ok with that (it appears to be
> enabled in defconfig and follows the vast majority of other archs) but
> it is a change in behaviour.
> 
> With that:
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>

The code in arch/arm64/kernel/stacktrace.c assumes we have frame
pointers regardless of FRAME_POINTER. Depending on what the compiler
decides to use x29 for, we could get some weird fake unwinding and/or
dodgy memory accesses.

I think we should first audit the uses of frame pointers to ensure that
they are guarded for !FRAME_POINTER.

Thanks,
Mark.

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Will Deacon]

On Fri, Nov 06, 2015 at 12:50:02PM +0000, Mark Rutland wrote:
> On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> > On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > > it in arch/arm64/Kconfig.debug.
> > 
> > It might be worth noting that this adds a dependency on DEBUG_KERNEL
> > for building with frame pointers. I'm ok with that (it appears to be
> > enabled in defconfig and follows the vast majority of other archs) but
> > it is a change in behaviour.
> > 
> > With that:
> > 
> >   Acked-by: Will Deacon <will.deacon@arm.com>
> 
> The code in arch/arm64/kernel/stacktrace.c assumes we have frame
> pointers regardless of FRAME_POINTER. Depending on what the compiler
> decides to use x29 for, we could get some weird fake unwinding and/or
> dodgy memory accesses.
> 
> I think we should first audit the uses of frame pointers to ensure that
> they are guarded for !FRAME_POINTER.

Good point. The perf callchain code suffers from a similar issue.

Will

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Catalin Marinas]

On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > it in arch/arm64/Kconfig.debug.
> 
> It might be worth noting that this adds a dependency on DEBUG_KERNEL
> for building with frame pointers. I'm ok with that (it appears to be
> enabled in defconfig and follows the vast majority of other archs) but
> it is a change in behaviour.

We shouldn't have the dependency on DEBUG_KERNEL since we select
ARCH_WANT_FRAME_POINTERS on arm64. However, the patch would allow one to
disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
though).

-- 
Catalin

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Will Deacon]

On Fri, Nov 06, 2015 at 04:12:14PM +0000, Catalin Marinas wrote:
> On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> > On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > > it in arch/arm64/Kconfig.debug.
> > 
> > It might be worth noting that this adds a dependency on DEBUG_KERNEL
> > for building with frame pointers. I'm ok with that (it appears to be
> > enabled in defconfig and follows the vast majority of other archs) but
> > it is a change in behaviour.
> 
> We shouldn't have the dependency on DEBUG_KERNEL since we select
> ARCH_WANT_FRAME_POINTERS on arm64. However, the patch would allow one to
> disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
> though).

Ah yes, you're right about DEBUG_KERNEL. I completely misread the brackets
and the left-associativity of &&/|| works out.

I still think Rutland has a valid point wrt junk frame pointers, though.

Will

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Catalin Marinas]

On Fri, Nov 06, 2015 at 12:50:02PM +0000, Mark Rutland wrote:
> On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> > On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > > it in arch/arm64/Kconfig.debug.
> > 
> > It might be worth noting that this adds a dependency on DEBUG_KERNEL
> > for building with frame pointers. I'm ok with that (it appears to be
> > enabled in defconfig and follows the vast majority of other archs) but
> > it is a change in behaviour.
> > 
> > With that:
> > 
> >   Acked-by: Will Deacon <will.deacon@arm.com>
> 
> The code in arch/arm64/kernel/stacktrace.c assumes we have frame
> pointers regardless of FRAME_POINTER. Depending on what the compiler
> decides to use x29 for, we could get some weird fake unwinding and/or
> dodgy memory accesses.
> 
> I think we should first audit the uses of frame pointers to ensure that
> they are guarded for !FRAME_POINTER.

Or we just select FRAME_POINTER in the ARM64 Kconfig entry.

-- 
Catalin

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Will Deacon]

On Fri, Nov 06, 2015 at 04:21:09PM +0000, Catalin Marinas wrote:
> On Fri, Nov 06, 2015 at 12:50:02PM +0000, Mark Rutland wrote:
> > On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
> > > On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
> > > > FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
> > > > it in arch/arm64/Kconfig.debug.
> > > 
> > > It might be worth noting that this adds a dependency on DEBUG_KERNEL
> > > for building with frame pointers. I'm ok with that (it appears to be
> > > enabled in defconfig and follows the vast majority of other archs) but
> > > it is a change in behaviour.
> > > 
> > > With that:
> > > 
> > >   Acked-by: Will Deacon <will.deacon@arm.com>
> > 
> > The code in arch/arm64/kernel/stacktrace.c assumes we have frame
> > pointers regardless of FRAME_POINTER. Depending on what the compiler
> > decides to use x29 for, we could get some weird fake unwinding and/or
> > dodgy memory accesses.
> > 
> > I think we should first audit the uses of frame pointers to ensure that
> > they are guarded for !FRAME_POINTER.
> 
> Or we just select FRAME_POINTER in the ARM64 Kconfig entry.

Yang, did you see any benefit disabling frame pointers, or was this patch
purely based on you spotting a duplicate Kconfig entry?

Will

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Shi, Yang]

On 11/6/2015 8:25 AM, Will Deacon wrote:
> On Fri, Nov 06, 2015 at 04:21:09PM +0000, Catalin Marinas wrote:
>> On Fri, Nov 06, 2015 at 12:50:02PM +0000, Mark Rutland wrote:
>>> On Fri, Nov 06, 2015 at 12:30:09PM +0000, Will Deacon wrote:
>>>> On Wed, Nov 04, 2015 at 09:37:51AM -0800, Yang Shi wrote:
>>>>> FRAME_POINTER is defined in lib/Kconfig.debug, it is unnecessary to redefine
>>>>> it in arch/arm64/Kconfig.debug.
>>>>
>>>> It might be worth noting that this adds a dependency on DEBUG_KERNEL
>>>> for building with frame pointers. I'm ok with that (it appears to be
>>>> enabled in defconfig and follows the vast majority of other archs) but
>>>> it is a change in behaviour.
>>>>
>>>> With that:
>>>>
>>>>    Acked-by: Will Deacon <will.deacon@arm.com>
>>>
>>> The code in arch/arm64/kernel/stacktrace.c assumes we have frame
>>> pointers regardless of FRAME_POINTER. Depending on what the compiler
>>> decides to use x29 for, we could get some weird fake unwinding and/or
>>> dodgy memory accesses.
>>>
>>> I think we should first audit the uses of frame pointers to ensure that
>>> they are guarded for !FRAME_POINTER.
>>
>> Or we just select FRAME_POINTER in the ARM64 Kconfig entry.
>
> Yang, did you see any benefit disabling frame pointers, or was this patch
> purely based on you spotting a duplicate Kconfig entry?

It just spots a duplicate Kconfig entry.

FRAME_POINTER is defined in both lib/Kconfig.debug and 
arch/arm64/Kconfig.debug.

The lib/Kconfig.debug one looks like:

config FRAME_POINTER
         bool "Compile the kernel with frame pointers"
         depends on DEBUG_KERNEL && \
                 (CRIS || M68K || FRV || UML || \
                  AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || \
                 ARCH_WANT_FRAME_POINTERS
         default y if (DEBUG_INFO && UML) || ARCH_WANT_FRAME_POINTERS
         help
           If you say Y here the resulting kernel image will be slightly
           larger and slower, but it gives very useful debugging information
           in case of kernel bugs. (precise oopses/stacktraces/warnings)

The common one just depends on DEBUG_KERNEL && ARCH_WANT_FRAME_POINTERS.

ARCH_WANT_FRAME_POINTERS is selected by ARM64 kconfig entry.

To answer Catalin's question about:

> However, the patch would allow one to
> disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
> though).

No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of 
the patch.

Thanks,
Yang

>
> Will
>

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Catalin Marinas]

On Fri, Nov 06, 2015 at 09:23:38AM -0800, Shi, Yang wrote:
> On 11/6/2015 8:25 AM, Will Deacon wrote:
> >However, the patch would allow one to
> >disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
> >though).
> 
> No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of the
> patch.

In which case I suggest that we always select it just as a clearer
statement that the feature cannot be disabled (and you never know what
the compiler people decide to do in the future).

-- 
Catalin

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Shi, Yang]

On 11/6/2015 9:35 AM, Catalin Marinas wrote:
> On Fri, Nov 06, 2015 at 09:23:38AM -0800, Shi, Yang wrote:
>> On 11/6/2015 8:25 AM, Will Deacon wrote:
>>> However, the patch would allow one to
>>> disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
>>> though).
>>
>> No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of the
>> patch.
>
> In which case I suggest that we always select it just as a clearer
> statement that the feature cannot be disabled (and you never know what
> the compiler people decide to do in the future).

Do you mean select FRAME_POINTER in ARCH_WANT_FRAME_POINTERS?

Yes, we could, but this may cause other architectures which select 
ARCH_WANT_FRAME_POINTERS to have FRAME_POINTER selected too.

Or we could do:

select FRAME_POINTER is ARM64

Thanks,
Yang

>

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Catalin Marinas]

On Fri, Nov 06, 2015 at 09:39:07AM -0800, Shi, Yang wrote:
> On 11/6/2015 9:35 AM, Catalin Marinas wrote:
> >On Fri, Nov 06, 2015 at 09:23:38AM -0800, Shi, Yang wrote:
> >>On 11/6/2015 8:25 AM, Will Deacon wrote:
> >>>However, the patch would allow one to
> >>>disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
> >>>though).
> >>
> >>No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of the
> >>patch.
> >
> >In which case I suggest that we always select it just as a clearer
> >statement that the feature cannot be disabled (and you never know what
> >the compiler people decide to do in the future).
> 
> Do you mean select FRAME_POINTER in ARCH_WANT_FRAME_POINTERS?
> 
> Yes, we could, but this may cause other architectures which select
> ARCH_WANT_FRAME_POINTERS to have FRAME_POINTER selected too.

This would have been the ideal option, something like:

--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -322,7 +322,7 @@ config ARCH_WANT_FRAME_POINTERS
 	help
 
 config FRAME_POINTER
-	bool "Compile the kernel with frame pointers"
+	bool "Compile the kernel with frame pointers" if !ARCH_WANT_FRAME_POINTERS
 	depends on DEBUG_KERNEL && \
 		(CRIS || M68K || FRV || UML || \
 		 AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || \

But, as you said, we would need to check the other architectures
selecting ARCH_WANT_FRAME_POINTERS.

In the meantime:

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

-- 
Catalin

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Shi, Yang]

On 11/6/2015 9:51 AM, Catalin Marinas wrote:
> On Fri, Nov 06, 2015 at 09:39:07AM -0800, Shi, Yang wrote:
>> On 11/6/2015 9:35 AM, Catalin Marinas wrote:
>>> On Fri, Nov 06, 2015 at 09:23:38AM -0800, Shi, Yang wrote:
>>>> On 11/6/2015 8:25 AM, Will Deacon wrote:
>>>>> However, the patch would allow one to
>>>>> disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
>>>>> though).
>>>>
>>>> No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of the
>>>> patch.
>>>
>>> In which case I suggest that we always select it just as a clearer
>>> statement that the feature cannot be disabled (and you never know what
>>> the compiler people decide to do in the future).
>>
>> Do you mean select FRAME_POINTER in ARCH_WANT_FRAME_POINTERS?
>>
>> Yes, we could, but this may cause other architectures which select
>> ARCH_WANT_FRAME_POINTERS to have FRAME_POINTER selected too.
>
> This would have been the ideal option, something like:
>
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -322,7 +322,7 @@ config ARCH_WANT_FRAME_POINTERS
>   	help
>
>   config FRAME_POINTER
> -	bool "Compile the kernel with frame pointers"
> +	bool "Compile the kernel with frame pointers" if !ARCH_WANT_FRAME_POINTERS
>   	depends on DEBUG_KERNEL && \
>   		(CRIS || M68K || FRV || UML || \
>   		 AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || \
>
> But, as you said, we would need to check the other architectures
> selecting ARCH_WANT_FRAME_POINTERS.

How about:

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1d1521c..709255a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -319,6 +319,7 @@ config DEBUG_SECTION_MISMATCH
  #
  config ARCH_WANT_FRAME_POINTERS
         bool
+       select FRAME_POINTER if ARM64
         help

  config FRAME_POINTER

If other architectures want the same behavior, they could easily append 
to the is statement. If all arches which selects 
ARCH_WANT_FRAME_POINTERS, the if statement could be just removed.

Yang

>
> In the meantime:
>
> --- 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
>

Re: [PATCH] arm64: remove redundant FRAME_POINTER kconfig option

[Catalin Marinas]

On Fri, Nov 06, 2015 at 09:55:54AM -0800, Shi, Yang wrote:
> On 11/6/2015 9:51 AM, Catalin Marinas wrote:
> >On Fri, Nov 06, 2015 at 09:39:07AM -0800, Shi, Yang wrote:
> >>On 11/6/2015 9:35 AM, Catalin Marinas wrote:
> >>>On Fri, Nov 06, 2015 at 09:23:38AM -0800, Shi, Yang wrote:
> >>>>On 11/6/2015 8:25 AM, Will Deacon wrote:
> >>>>>However, the patch would allow one to
> >>>>>disable FRAME_POINTERS (not sure it has any effect on the aarch64 gcc
> >>>>>though).
> >>>>
> >>>>No, it doesn't. Actually, FRAME_POINTER could be disabled regardless of the
> >>>>patch.
> >>>
> >>>In which case I suggest that we always select it just as a clearer
> >>>statement that the feature cannot be disabled (and you never know what
> >>>the compiler people decide to do in the future).
> >>
> >>Do you mean select FRAME_POINTER in ARCH_WANT_FRAME_POINTERS?
> >>
> >>Yes, we could, but this may cause other architectures which select
> >>ARCH_WANT_FRAME_POINTERS to have FRAME_POINTER selected too.
> >
> >This would have been the ideal option, something like:
> >
> >--- a/lib/Kconfig.debug
> >+++ b/lib/Kconfig.debug
> >@@ -322,7 +322,7 @@ config ARCH_WANT_FRAME_POINTERS
> >  	help
> >
> >  config FRAME_POINTER
> >-	bool "Compile the kernel with frame pointers"
> >+	bool "Compile the kernel with frame pointers" if !ARCH_WANT_FRAME_POINTERS
> >  	depends on DEBUG_KERNEL && \
> >  		(CRIS || M68K || FRV || UML || \
> >  		 AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || \
> >
> >But, as you said, we would need to check the other architectures
> >selecting ARCH_WANT_FRAME_POINTERS.
> 
> How about:
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1d1521c..709255a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -319,6 +319,7 @@ config DEBUG_SECTION_MISMATCH
>  #
>  config ARCH_WANT_FRAME_POINTERS
>         bool
> +       select FRAME_POINTER if ARM64
>         help
> 
>  config FRAME_POINTER
> 
> If other architectures want the same behavior, they could easily append to
> the is statement. If all arches which selects ARCH_WANT_FRAME_POINTERS, the
> if statement could be just removed.

I prefer the select in the ARM64 Kconfig entry as below:

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

-- 
Catalin