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
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
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.
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
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.
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
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
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).
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
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
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
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 helpconfig 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
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).
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
linaro-kernel@lists.linaro.org
-
Catalin Marinas -
Mark Rutland -
Shi, Yang -
Will Deacon -
Yang Shi