| Message ID | 1446658671-16238-1-git-send-email-yang.shi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: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
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 >
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
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(-)