Kconfig dependency issue on function-graph tracer and frame pointer on arm

Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Masami Hiramatsu]

Hello,

Recently, Naresh reported that the function-graph tracer on the latest
kernel crashes on arm. I could reproduce it and bisected. I finally found
the commit f9b58e8c7d03 ("ARM: 8800/1: use choice for kernel unwinders")
was the first bad commit.

Actually, this commit is just changing Kconfig for making kernel unwinder
choosable. However, as a side effect, this makes CONFIG_FRAME_POINTER=n
by default even if CONFIG_FUNCTION_GRAPH_TRACER=y.
(Note that function-graph tracer implementation on arm depends on
 FRAME_POINTER.)

This seems odd, because the commit introduced below code

+choice
+       prompt "Choose kernel unwinder"
+       default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
+       default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
+       help

So, it seems UNWINDER_FRAME_POINTER is used if FUNCTION_GRAPH_TRACER=y.
However, it seems not working. (I guess this is a wrong syntax for Kconfig)

Moreover, since this just setting default value, there seems no direct
dependency to FRAME_POINTER from FUNCTION_GRAPH_TRACER. I guess user can
change it...

I made a fix below (which I tested). This is ugly (arch specific dependency
in generic part) but works.
It enables both of FRAME_POINTER and UNWINDER_ARM.

However I still have some questions.

- Is above choice+default a correct syntax for Kconfig? (Masahiro?)
- Can UNWINDER_ARM work with FRAME_POINTER? (Arnd?)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 5e3de28c7677..f79c54680f3b 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -160,6 +160,7 @@ config FUNCTION_GRAPH_TRACER
        depends on HAVE_FUNCTION_GRAPH_TRACER
        depends on FUNCTION_TRACER
        depends on !X86_32 || !CC_OPTIMIZE_FOR_SIZE
+       select ARCH_WANT_FRAME_POINTERS if ARM
        default y
        help
          Enable the kernel to trace a function at both its return

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Russell King - ARM Linux admin]

On Sun, Apr 14, 2019 at 07:47:05PM +0900, Masami Hiramatsu wrote:
> Hello,
> 
> Recently, Naresh reported that the function-graph tracer on the latest
> kernel crashes on arm. I could reproduce it and bisected. I finally found
> the commit f9b58e8c7d03 ("ARM: 8800/1: use choice for kernel unwinders")
> was the first bad commit.

I don't think that littering the rest of the kernel Kconfig with ARM
specific stuff is really a viable solution to this.

If we examine the current situation, we have:

- THUMB2_KERNEL selecting ARM_UNWIND when enabled.
- UNWINDER_FRAME_POINTER disabled if THUMB2_KERNEL is enabled, provided
  we're not using Clang.  This leaves UNWINDER_ARM as the only choice,
  which also selects ARM_UNWIND.
- The default choice is dependent on the settings of AEABI and
  FUNCTION_GRAPH_TRACER.
- HAVE_FUNCTION_GRAPH_TRACER is disabled if THUMB2_KERNEL is enabled.

which seems to be _way_ too messy.

Looking back before this commit, the function graph tracer never had a
strong dependence on frame pointers being enabled in the kernel, but it
seems the code relies upon them (see ftrace_return_to_handler() in
kernel/trace/ and return_to_handler in arch/arm/kernel/entry-frace.S).
There is also the __ftrace_graph_caller macro which seems to rely on it.

Since Clang does not support frame pointers, we shouldn't even offer
the function graph tracer for Clang compilers, so let's do that with
the first hunk of the patch below.

The subsequent hunks remove the defaulting of the choice according to
the function graph tracer - this is not a "hint" where the user can
still choose either option irrespective of the state of the function
graph tracer.  They should only be able to select the frame pointer
option in that case.

Another way forward would be for someone to put the work in to making
the function graph tracer work without frame pointers.

So, how about this:

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 850b4805e2d1..9aed25a6019b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -73,7 +73,7 @@ config ARM
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
-	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
+	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
 	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
 	select HAVE_GCC_PLUGINS
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 6d6e0330930b..e388af4594a6 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -47,8 +47,8 @@ config DEBUG_WX
 
 choice
 	prompt "Choose kernel unwinder"
-	default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
-	default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
+	default UNWINDER_ARM if AEABI
+	default UNWINDER_FRAME_POINTER if !AEABI
 	help
 	  This determines which method will be used for unwinding kernel stack
 	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
@@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
 
 config UNWINDER_ARM
 	bool "ARM EABI stack unwinder"
-	depends on AEABI
+	depends on AEABI && !FUNCTION_GRAPH_TRACER
 	select ARM_UNWIND
 	help
 	  This option enables stack unwinding support in the kernel

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Masami Hiramatsu]

On Sun, 14 Apr 2019 14:34:58 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Sun, Apr 14, 2019 at 07:47:05PM +0900, Masami Hiramatsu wrote:
> > Hello,
> > 
> > Recently, Naresh reported that the function-graph tracer on the latest
> > kernel crashes on arm. I could reproduce it and bisected. I finally found
> > the commit f9b58e8c7d03 ("ARM: 8800/1: use choice for kernel unwinders")
> > was the first bad commit.
> 
> I don't think that littering the rest of the kernel Kconfig with ARM
> specific stuff is really a viable solution to this.
> 
> If we examine the current situation, we have:
> 
> - THUMB2_KERNEL selecting ARM_UNWIND when enabled.
> - UNWINDER_FRAME_POINTER disabled if THUMB2_KERNEL is enabled, provided
>   we're not using Clang.  This leaves UNWINDER_ARM as the only choice,
>   which also selects ARM_UNWIND.
> - The default choice is dependent on the settings of AEABI and
>   FUNCTION_GRAPH_TRACER.
> - HAVE_FUNCTION_GRAPH_TRACER is disabled if THUMB2_KERNEL is enabled.
> 
> which seems to be _way_ too messy.
> 
> Looking back before this commit, the function graph tracer never had a
> strong dependence on frame pointers being enabled in the kernel, but it
> seems the code relies upon them (see ftrace_return_to_handler() in
> kernel/trace/ and return_to_handler in arch/arm/kernel/entry-frace.S).
> There is also the __ftrace_graph_caller macro which seems to rely on it.

Yes, so I think similar bug is hiding in other LTS kernels. It should
have a dependency to FRAME_POINTER on arm.

> Since Clang does not support frame pointers, we shouldn't even offer
> the function graph tracer for Clang compilers, so let's do that with
> the first hunk of the patch below.
> 
> The subsequent hunks remove the defaulting of the choice according to
> the function graph tracer - this is not a "hint" where the user can
> still choose either option irrespective of the state of the function
> graph tracer.  They should only be able to select the frame pointer
> option in that case.

Agreed. Using default for making dependency is wrong.

> 
> Another way forward would be for someone to put the work in to making
> the function graph tracer work without frame pointers.

Yes, we eventually need that. But for fixing current released kernel
(this bug is in v5.0 series), I think Kconfig fix is needed.

> 
> So, how about this:
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 850b4805e2d1..9aed25a6019b 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -73,7 +73,7 @@ config ARM
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>  	select HAVE_EXIT_THREAD
>  	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> -	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
> +	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
>  	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
>  	select HAVE_GCC_PLUGINS
>  	select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 6d6e0330930b..e388af4594a6 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -47,8 +47,8 @@ config DEBUG_WX
>  
>  choice
>  	prompt "Choose kernel unwinder"
> -	default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
> -	default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
> +	default UNWINDER_ARM if AEABI
> +	default UNWINDER_FRAME_POINTER if !AEABI

If UNWINDER_ARM depends on ARM EABI, would we really need this "if !AEABI"?
(I doubt we need these default...)

>  	help
>  	  This determines which method will be used for unwinding kernel stack
>  	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
> @@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
>  
>  config UNWINDER_ARM
>  	bool "ARM EABI stack unwinder"
> -	depends on AEABI
> +	depends on AEABI && !FUNCTION_GRAPH_TRACER

Hmm, AFAIK, FUNCTION_GRAPH_TRACER only depends on FRAME_POINTER, but not
UNWINDER_FRAME_POINTER. So can user still choose UNWINDER_ARM even if
FUNCTION_GRAPH_TRACER=y ? (Of course that may not be a meaningful option)

Thank you,

>  	select ARM_UNWIND
>  	help
>  	  This option enables stack unwinding support in the kernel
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Russell King - ARM Linux admin]

On Sun, Apr 14, 2019 at 11:52:38PM +0900, Masami Hiramatsu wrote:
> On Sun, 14 Apr 2019 14:34:58 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Sun, Apr 14, 2019 at 07:47:05PM +0900, Masami Hiramatsu wrote:
> > > Hello,
> > > 
> > > Recently, Naresh reported that the function-graph tracer on the latest
> > > kernel crashes on arm. I could reproduce it and bisected. I finally found
> > > the commit f9b58e8c7d03 ("ARM: 8800/1: use choice for kernel unwinders")
> > > was the first bad commit.
> > 
> > I don't think that littering the rest of the kernel Kconfig with ARM
> > specific stuff is really a viable solution to this.
> > 
> > If we examine the current situation, we have:
> > 
> > - THUMB2_KERNEL selecting ARM_UNWIND when enabled.
> > - UNWINDER_FRAME_POINTER disabled if THUMB2_KERNEL is enabled, provided
> >   we're not using Clang.  This leaves UNWINDER_ARM as the only choice,
> >   which also selects ARM_UNWIND.
> > - The default choice is dependent on the settings of AEABI and
> >   FUNCTION_GRAPH_TRACER.
> > - HAVE_FUNCTION_GRAPH_TRACER is disabled if THUMB2_KERNEL is enabled.
> > 
> > which seems to be _way_ too messy.
> > 
> > Looking back before this commit, the function graph tracer never had a
> > strong dependence on frame pointers being enabled in the kernel, but it
> > seems the code relies upon them (see ftrace_return_to_handler() in
> > kernel/trace/ and return_to_handler in arch/arm/kernel/entry-frace.S).
> > There is also the __ftrace_graph_caller macro which seems to rely on it.
> 
> Yes, so I think similar bug is hiding in other LTS kernels. It should
> have a dependency to FRAME_POINTER on arm.
> 
> > Since Clang does not support frame pointers, we shouldn't even offer
> > the function graph tracer for Clang compilers, so let's do that with
> > the first hunk of the patch below.
> > 
> > The subsequent hunks remove the defaulting of the choice according to
> > the function graph tracer - this is not a "hint" where the user can
> > still choose either option irrespective of the state of the function
> > graph tracer.  They should only be able to select the frame pointer
> > option in that case.
> 
> Agreed. Using default for making dependency is wrong.
> 
> > 
> > Another way forward would be for someone to put the work in to making
> > the function graph tracer work without frame pointers.
> 
> Yes, we eventually need that. But for fixing current released kernel
> (this bug is in v5.0 series), I think Kconfig fix is needed.
> 
> > 
> > So, how about this:
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 850b4805e2d1..9aed25a6019b 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -73,7 +73,7 @@ config ARM
> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
> >  	select HAVE_EXIT_THREAD
> >  	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > -	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
> > +	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
> >  	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> >  	select HAVE_GCC_PLUGINS
> >  	select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
> > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> > index 6d6e0330930b..e388af4594a6 100644
> > --- a/arch/arm/Kconfig.debug
> > +++ b/arch/arm/Kconfig.debug
> > @@ -47,8 +47,8 @@ config DEBUG_WX
> >  
> >  choice
> >  	prompt "Choose kernel unwinder"
> > -	default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
> > -	default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
> > +	default UNWINDER_ARM if AEABI
> > +	default UNWINDER_FRAME_POINTER if !AEABI
> 
> If UNWINDER_ARM depends on ARM EABI, would we really need this "if !AEABI"?
> (I doubt we need these default...)
> 
> >  	help
> >  	  This determines which method will be used for unwinding kernel stack
> >  	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
> > @@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
> >  
> >  config UNWINDER_ARM
> >  	bool "ARM EABI stack unwinder"
> > -	depends on AEABI
> > +	depends on AEABI && !FUNCTION_GRAPH_TRACER
> 
> Hmm, AFAIK, FUNCTION_GRAPH_TRACER only depends on FRAME_POINTER, but not
> UNWINDER_FRAME_POINTER. So can user still choose UNWINDER_ARM even if
> FUNCTION_GRAPH_TRACER=y ? (Of course that may not be a meaningful option)

The UNWINDER_* options do not control anything except whether
FRAME_POINTER is enabled or not.  FRAME_POINTER controls not only
whether we build with frame pointers, but also how we unwind.
If both ARM_UNWIND and FRAME_POINTER are set, the kernel will
fail to link due to a multiple definition of unwind_frame().

The UNWINDER_* symbols were added in the commit you referenced
merely to select which of ARM_UNWIND or FRAME_POINTER are
enabled.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Masami Hiramatsu]

On Sun, 14 Apr 2019 16:37:04 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Sun, Apr 14, 2019 at 11:52:38PM +0900, Masami Hiramatsu wrote:
> > On Sun, 14 Apr 2019 14:34:58 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > 
> > > On Sun, Apr 14, 2019 at 07:47:05PM +0900, Masami Hiramatsu wrote:
> > > > Hello,
> > > > 
> > > > Recently, Naresh reported that the function-graph tracer on the latest
> > > > kernel crashes on arm. I could reproduce it and bisected. I finally found
> > > > the commit f9b58e8c7d03 ("ARM: 8800/1: use choice for kernel unwinders")
> > > > was the first bad commit.
> > > 
> > > I don't think that littering the rest of the kernel Kconfig with ARM
> > > specific stuff is really a viable solution to this.
> > > 
> > > If we examine the current situation, we have:
> > > 
> > > - THUMB2_KERNEL selecting ARM_UNWIND when enabled.
> > > - UNWINDER_FRAME_POINTER disabled if THUMB2_KERNEL is enabled, provided
> > >   we're not using Clang.  This leaves UNWINDER_ARM as the only choice,
> > >   which also selects ARM_UNWIND.
> > > - The default choice is dependent on the settings of AEABI and
> > >   FUNCTION_GRAPH_TRACER.
> > > - HAVE_FUNCTION_GRAPH_TRACER is disabled if THUMB2_KERNEL is enabled.
> > > 
> > > which seems to be _way_ too messy.
> > > 
> > > Looking back before this commit, the function graph tracer never had a
> > > strong dependence on frame pointers being enabled in the kernel, but it
> > > seems the code relies upon them (see ftrace_return_to_handler() in
> > > kernel/trace/ and return_to_handler in arch/arm/kernel/entry-frace.S).
> > > There is also the __ftrace_graph_caller macro which seems to rely on it.
> > 
> > Yes, so I think similar bug is hiding in other LTS kernels. It should
> > have a dependency to FRAME_POINTER on arm.
> > 
> > > Since Clang does not support frame pointers, we shouldn't even offer
> > > the function graph tracer for Clang compilers, so let's do that with
> > > the first hunk of the patch below.
> > > 
> > > The subsequent hunks remove the defaulting of the choice according to
> > > the function graph tracer - this is not a "hint" where the user can
> > > still choose either option irrespective of the state of the function
> > > graph tracer.  They should only be able to select the frame pointer
> > > option in that case.
> > 
> > Agreed. Using default for making dependency is wrong.
> > 
> > > 
> > > Another way forward would be for someone to put the work in to making
> > > the function graph tracer work without frame pointers.
> > 
> > Yes, we eventually need that. But for fixing current released kernel
> > (this bug is in v5.0 series), I think Kconfig fix is needed.
> > 
> > > 
> > > So, how about this:
> > > 
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 850b4805e2d1..9aed25a6019b 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -73,7 +73,7 @@ config ARM
> > >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
> > >  	select HAVE_EXIT_THREAD
> > >  	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > > -	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
> > > +	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
> > >  	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> > >  	select HAVE_GCC_PLUGINS
> > >  	select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
> > > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> > > index 6d6e0330930b..e388af4594a6 100644
> > > --- a/arch/arm/Kconfig.debug
> > > +++ b/arch/arm/Kconfig.debug
> > > @@ -47,8 +47,8 @@ config DEBUG_WX
> > >  
> > >  choice
> > >  	prompt "Choose kernel unwinder"
> > > -	default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
> > > -	default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
> > > +	default UNWINDER_ARM if AEABI
> > > +	default UNWINDER_FRAME_POINTER if !AEABI
> > 
> > If UNWINDER_ARM depends on ARM EABI, would we really need this "if !AEABI"?
> > (I doubt we need these default...)
> > 
> > >  	help
> > >  	  This determines which method will be used for unwinding kernel stack
> > >  	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
> > > @@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
> > >  
> > >  config UNWINDER_ARM
> > >  	bool "ARM EABI stack unwinder"
> > > -	depends on AEABI
> > > +	depends on AEABI && !FUNCTION_GRAPH_TRACER
> > 
> > Hmm, AFAIK, FUNCTION_GRAPH_TRACER only depends on FRAME_POINTER, but not
> > UNWINDER_FRAME_POINTER. So can user still choose UNWINDER_ARM even if
> > FUNCTION_GRAPH_TRACER=y ? (Of course that may not be a meaningful option)
> 
> The UNWINDER_* options do not control anything except whether
> FRAME_POINTER is enabled or not.  FRAME_POINTER controls not only
> whether we build with frame pointers, but also how we unwind.
> If both ARM_UNWIND and FRAME_POINTER are set, the kernel will
> fail to link due to a multiple definition of unwind_frame().

Thank you for the explanation :) got it.

> 
> The UNWINDER_* symbols were added in the commit you referenced
> merely to select which of ARM_UNWIND or FRAME_POINTER are
> enabled.

OK, this looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Arnd Bergmann]

On Sun, Apr 14, 2019 at 3:35 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:

> The subsequent hunks remove the defaulting of the choice according to
> the function graph tracer - this is not a "hint" where the user can
> still choose either option irrespective of the state of the function
> graph tracer.  They should only be able to select the frame pointer
> option in that case.
>
> Another way forward would be for someone to put the work in to making
> the function graph tracer work without frame pointers.

I think Stefan was already looking into making CONFIG_FUNCTION_TRACER
work with clang. I don't know what the status of that work is, but I
think getting
FUNCTION_GRAPH_TRACER working at the same time would be best.

I never noticed the Kconfig issue here, because I was using a patch to
turn off FUNCTION_TRACER on ARM with clang to make it build, and that
turns off FUNCTION_GRAPH_TRACER in the process.

> So, how about this:
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 850b4805e2d1..9aed25a6019b 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -73,7 +73,7 @@ config ARM
>         select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>         select HAVE_EXIT_THREAD
>         select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> -       select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
> +       select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
>         select HAVE_FUNCTION_TRACER if !XIP_KERNEL
>         select HAVE_GCC_PLUGINS
>         select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 6d6e0330930b..e388af4594a6 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -47,8 +47,8 @@ config DEBUG_WX
>
>  choice
>         prompt "Choose kernel unwinder"
> -       default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
> -       default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
> +       default UNWINDER_ARM if AEABI
> +       default UNWINDER_FRAME_POINTER if !AEABI
>         help
>           This determines which method will be used for unwinding kernel stack
>           traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
> @@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
>
>  config UNWINDER_ARM
>         bool "ARM EABI stack unwinder"
> -       depends on AEABI
> +       depends on AEABI && !FUNCTION_GRAPH_TRACER
>         select ARM_UNWIND
>         help
>           This option enables stack unwinding support in the kernel

This looks good to me in the meantime, at least if there is any
way to get the non-graph FUNCTION_TRACER to build with clang.

       Arnd

Re: Kconfig dependency issue on function-graph tracer and frame pointer on arm

[Stefan Agner]

On 15.04.2019 14:28, Arnd Bergmann wrote:
> On Sun, Apr 14, 2019 at 3:35 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> 
>> The subsequent hunks remove the defaulting of the choice according to
>> the function graph tracer - this is not a "hint" where the user can
>> still choose either option irrespective of the state of the function
>> graph tracer.  They should only be able to select the frame pointer
>> option in that case.
>>
>> Another way forward would be for someone to put the work in to making
>> the function graph tracer work without frame pointers.
> 
> I think Stefan was already looking into making CONFIG_FUNCTION_TRACER
> work with clang. I don't know what the status of that work is, but I
> think getting
> FUNCTION_GRAPH_TRACER working at the same time would be best.

Function Tracer is currently blocked by buggy mcount implemention on
Clang side. I do have a hacked up version which works with the buggy
Clang implementation, but not something we want to merge IMHO. see also:
https://bugs.llvm.org/show_bug.cgi?id=33845

> 
> I never noticed the Kconfig issue here, because I was using a patch to
> turn off FUNCTION_TRACER on ARM with clang to make it build, and that
> turns off FUNCTION_GRAPH_TRACER in the process.
> 
>> So, how about this:
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 850b4805e2d1..9aed25a6019b 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -73,7 +73,7 @@ config ARM
>>         select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>>         select HAVE_EXIT_THREAD
>>         select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
>> -       select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL
>> +       select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
>>         select HAVE_FUNCTION_TRACER if !XIP_KERNEL

I think due to the fact above, we should add  && !CC_IS_CLANG here too.

>>         select HAVE_GCC_PLUGINS
>>         select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index 6d6e0330930b..e388af4594a6 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -47,8 +47,8 @@ config DEBUG_WX
>>
>>  choice
>>         prompt "Choose kernel unwinder"
>> -       default UNWINDER_ARM if AEABI && !FUNCTION_GRAPH_TRACER
>> -       default UNWINDER_FRAME_POINTER if !AEABI || FUNCTION_GRAPH_TRACER
>> +       default UNWINDER_ARM if AEABI
>> +       default UNWINDER_FRAME_POINTER if !AEABI
>>         help
>>           This determines which method will be used for unwinding kernel stack
>>           traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
>> @@ -65,7 +65,7 @@ config UNWINDER_FRAME_POINTER
>>
>>  config UNWINDER_ARM
>>         bool "ARM EABI stack unwinder"
>> -       depends on AEABI
>> +       depends on AEABI && !FUNCTION_GRAPH_TRACER
>>         select ARM_UNWIND
>>         help
>>           This option enables stack unwinding support in the kernel
> 
> This looks good to me in the meantime, at least if there is any
> way to get the non-graph FUNCTION_TRACER to build with clang.

Looks sensible to me too.

Note that a similar issue came up a while ago on the mailing list:
https://marc.info/?l=linux-arm-kernel&m=154739414703313&w=2
[added Jeremy]

Unfortunately this never really materialized in a mergeable patch.

--
Stefan