[PATCH] ARM: use choice for kernel unwinders

[PATCH] ARM: use choice for kernel unwinders

[Stefan Agner]

While in theory multiple unwinders could be compiled in, it does
not make sense in practise. Use a choice to make the unwinder
selection mutually exclusive and mandatory.

Already before this commit it has not been possible to deselect
FRAME_POINTER. Remove the obsolete comment.

Furthermore, to produce a meaningful backtrace with FRAME_POINTER
enabled the kernel needs a specific function prologue:
    mov    ip, sp
    stmfd    sp!, {fp, ip, lr, pc}
    sub    fp, ip, #4

To get to the required prologue gcc uses apcs and no-sched-prolog.
This compiler options are not available on clang, and clang is not
able to generate the required prologue. Make the FRAME_POINTER
config symbol depending on !clang.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/Kconfig.debug | 43 ++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 693f84392f1b..ba09e744384a 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -48,30 +48,41 @@ config DEBUG_WX
 
 		If in doubt, say "Y".
 
-# RMK wants arm kernels compiled with frame pointers or stack unwinding.
-# If you know what you are doing and are willing to live without stack
-# traces, you can get a slightly smaller kernel by setting this option to
-# n, but then RMK will have to kill you ;).
-config FRAME_POINTER
-	bool
-	depends on !THUMB2_KERNEL
-	default y if !ARM_UNWIND || FUNCTION_GRAPH_TRACER
+choice
+	prompt "Choose kernel unwinder"
+	default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
+	default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
+	help
+	  This determines which method will be used for unwinding kernel stack
+	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
+	  livepatch, lockdep, and more.
+
+config UNWINDER_FRAME_POINTER
+	bool "Frame pointer unwinder"
+	depends on !THUMB2_KERNEL && !CC_IS_CLANG
+	select FRAME_POINTER
 	help
-	  If you say N here, the resulting kernel will be slightly smaller and
-	  faster. However, if neither FRAME_POINTER nor ARM_UNWIND are enabled,
-	  when a problem occurs with the kernel, the information that is
-	  reported is severely limited.
+	  This option enables the frame pointer unwinder for unwinding
+	  kernel stack traces.
 
-config ARM_UNWIND
-	bool "Enable stack unwinding support (EXPERIMENTAL)"
+config UNWINDER_ARM
+	bool "ARM EABI stack unwinder"
 	depends on AEABI
-	default y
+	select ARM_UNWIND
 	help
 	  This option enables stack unwinding support in the kernel
 	  using the information automatically generated by the
 	  compiler. The resulting kernel image is slightly bigger but
 	  the performance is not affected. Currently, this feature
-	  only works with EABI compilers. If unsure say Y.
+	  only works with EABI compilers.
+
+endchoice
+
+config ARM_UNWIND
+	bool
+
+config FRAME_POINTER
+	bool
 
 config OLD_MCOUNT
 	bool
-- 
2.18.0

Re: [PATCH] ARM: use choice for kernel unwinders

[Arnd Bergmann]

On Wed, Aug 22, 2018 at 12:24 AM Stefan Agner <stefan@agner.ch> wrote:
>
> While in theory multiple unwinders could be compiled in, it does
> not make sense in practise. Use a choice to make the unwinder
> selection mutually exclusive and mandatory.
>
> Already before this commit it has not been possible to deselect
> FRAME_POINTER. Remove the obsolete comment.
>
> Furthermore, to produce a meaningful backtrace with FRAME_POINTER
> enabled the kernel needs a specific function prologue:
>     mov    ip, sp
>     stmfd    sp!, {fp, ip, lr, pc}
>     sub    fp, ip, #4
>
> To get to the required prologue gcc uses apcs and no-sched-prolog.
> This compiler options are not available on clang, and clang is not
> able to generate the required prologue. Make the FRAME_POINTER
> config symbol depending on !clang.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Looks ok to me. I've added it to my randconfig test environment, you
will hear from me within a day if I run into build regressions.

We may still want to clean up these three lines:

lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC &&
!ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !X86
lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
!MICROBLAZE && !ARM_UNWIND && !ARC && !X86
lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
!MICROBLAZE && !ARM_UNWIND && !ARC && !X86

in which ARM is the odd case that currently depends on an architecture
specific rather than the architecture itself.
We could introduce a 'config ARCH_HAS_UNWINDER' symbol that gets
selected by mips, ppc, s390, microblaze, arm and x86 unconditionally,
and then simplify the 'select' statements here.

       Arnd

Re: [PATCH] ARM: use choice for kernel unwinders

[Arnd Bergmann]

On Wed, Aug 22, 2018 at 12:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Aug 22, 2018 at 12:24 AM Stefan Agner <stefan@agner.ch> wrote:
> >
> > While in theory multiple unwinders could be compiled in, it does
> > not make sense in practise. Use a choice to make the unwinder
> > selection mutually exclusive and mandatory.
> >
> > Already before this commit it has not been possible to deselect
> > FRAME_POINTER. Remove the obsolete comment.
> >
> > Furthermore, to produce a meaningful backtrace with FRAME_POINTER
> > enabled the kernel needs a specific function prologue:
> >     mov    ip, sp
> >     stmfd    sp!, {fp, ip, lr, pc}
> >     sub    fp, ip, #4
> >
> > To get to the required prologue gcc uses apcs and no-sched-prolog.
> > This compiler options are not available on clang, and clang is not
> > able to generate the required prologue. Make the FRAME_POINTER
> > config symbol depending on !clang.
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>
> Looks ok to me. I've added it to my randconfig test environment, you
> will hear from me within a day if I run into build regressions.

I needed a small hunk to avoid a warning:

WARNING: unmet direct dependencies detected for FRAME_POINTER
  Depends on [n]: DEBUG_KERNEL [=y] && (M68K || UML || SUPERH) ||
ARCH_WANT_FRAME_POINTERS [=n]
  Selected by [y]:
  - LOCKDEP [=y] && DEBUG_KERNEL [=y] && LOCK_DEBUGGING_SUPPORT [=y]
&& !MIPS && !PPC && !ARM_UNWIND [=n] && !S390 && !MICROBLAZE && !ARC
&& !X86
  - UNWINDER_FRAME_POINTER [=y] && <choice> && !THUMB2_KERNEL [=n] &&
!CC_IS_CLANG [=n]

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 6e27419b90c0..08f914be6248 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -57,6 +57,7 @@ choice
 config UNWINDER_FRAME_POINTER
        bool "Frame pointer unwinder"
        depends on !THUMB2_KERNEL && !CC_IS_CLANG
+       select ARCH_WANT_FRAME_POINTERS
        select FRAME_POINTER
        help
          This option enables the frame pointer unwinder for unwinding

Re: [PATCH] ARM: use choice for kernel unwinders

[Stefan Agner]

On 22.08.2018 12:02, Arnd Bergmann wrote:
> On Wed, Aug 22, 2018 at 12:24 AM Stefan Agner <stefan@agner.ch> wrote:
>>
>> While in theory multiple unwinders could be compiled in, it does
>> not make sense in practise. Use a choice to make the unwinder
>> selection mutually exclusive and mandatory.
>>
>> Already before this commit it has not been possible to deselect
>> FRAME_POINTER. Remove the obsolete comment.
>>
>> Furthermore, to produce a meaningful backtrace with FRAME_POINTER
>> enabled the kernel needs a specific function prologue:
>>     mov    ip, sp
>>     stmfd    sp!, {fp, ip, lr, pc}
>>     sub    fp, ip, #4
>>
>> To get to the required prologue gcc uses apcs and no-sched-prolog.
>> This compiler options are not available on clang, and clang is not
>> able to generate the required prologue. Make the FRAME_POINTER
>> config symbol depending on !clang.
>>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> 
> Looks ok to me. I've added it to my randconfig test environment, you
> will hear from me within a day if I run into build regressions.
> 
> We may still want to clean up these three lines:
> 
> lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC &&
> !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !X86
> lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
> !MICROBLAZE && !ARM_UNWIND && !ARC && !X86
> lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
> !MICROBLAZE && !ARM_UNWIND && !ARC && !X86
> 
> in which ARM is the odd case that currently depends on an architecture
> specific rather than the architecture itself.

I guess we would just follow X86 lead by saying ARM is guaranteed to
have unwinding support, and hence we can add !ARM.

> We could introduce a 'config ARCH_HAS_UNWINDER' symbol that gets
> selected by mips, ppc, s390, microblaze, arm and x86 unconditionally,
> and then simplify the 'select' statements here.

Yeah I was thinking about something like that too.

It seems to be a bit weird to me that lib/Kconfig.debug selects a
specific stack unwinding technique...

Ideally other config symbol should just ask arch to make sure a
unwinding technique is available (NEED_STACK_UNWINDING?) and arch then
makes sure to provide a reasonable default.

This then also would make it possible to select no stack unwinding in
case arch supports that and all the users of stack unwinding are
disabled too. Not sure how that exactly would look like in Kconfig, I
was thinking like:

choice
    prompt "Choose kernel unwinder"
    optional if !NEED_STACK_UNWINDING
    default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
    default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER

But "optional if" does not exist yet :-)

Reading the comments in arch/arm/Kconfig.debug seems to suggest that
once upon a time it was possible to disable stack unwinding on ARM.

But then, maybe we don't really want to go there? Might be interesting
for tinification efforts.

--
Stefan

Re: [PATCH] ARM: use choice for kernel unwinders

[Arnd Bergmann]

On Wed, Aug 22, 2018 at 4:38 PM Stefan Agner <stefan@agner.ch> wrote:
> On 22.08.2018 12:02, Arnd Bergmann wrote:
> > On Wed, Aug 22, 2018 at 12:24 AM Stefan Agner <stefan@agner.ch> wrote:
> >
> > Looks ok to me. I've added it to my randconfig test environment, you
> > will hear from me within a day if I run into build regressions.
> >
> > We may still want to clean up these three lines:
> >
> > lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC &&
> > !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !X86
> > lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
> > !MICROBLAZE && !ARM_UNWIND && !ARC && !X86
> > lib/Kconfig.debug:  select FRAME_POINTER if !MIPS && !PPC && !S390 &&
> > !MICROBLAZE && !ARM_UNWIND && !ARC && !X86
> >
> > in which ARM is the odd case that currently depends on an architecture
> > specific rather than the architecture itself.
>
> I guess we would just follow X86 lead by saying ARM is guaranteed to
> have unwinding support, and hence we can add !ARM.

Right, that was the idea.

> > We could introduce a 'config ARCH_HAS_UNWINDER' symbol that gets
> > selected by mips, ppc, s390, microblaze, arm and x86 unconditionally,
> > and then simplify the 'select' statements here.
>
> Yeah I was thinking about something like that too.
>
> It seems to be a bit weird to me that lib/Kconfig.debug selects a
> specific stack unwinding technique...

This must be a historic artifact from the time when FRAME_POINTER
was the only unwinding method that existed. We may also have some
architectures that don't support any unwinding.

> Ideally other config symbol should just ask arch to make sure a
> unwinding technique is available (NEED_STACK_UNWINDING?) and arch then
> makes sure to provide a reasonable default.
>
> This then also would make it possible to select no stack unwinding in
> case arch supports that and all the users of stack unwinding are
> disabled too. Not sure how that exactly would look like in Kconfig, I
> was thinking like:
>
> choice
>     prompt "Choose kernel unwinder"
>     optional if !NEED_STACK_UNWINDING
>     default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
>     default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
>
> But "optional if" does not exist yet :-)

You can write that as

choice
      prompt "Choose kernel unwinder" if NEED_STACK_UNWINDING

This will hide the prompt when NEED_STACK_UNWINDING is disabled,
making it impossible to pick one of the two unwinders.

> Reading the comments in arch/arm/Kconfig.debug seems to suggest that
> once upon a time it was possible to disable stack unwinding on ARM.
>
> But then, maybe we don't really want to go there? Might be interesting
> for tinification efforts.

I'd leave that for another day ;-)

      Arnd