[PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[Alan Kao]

Enabling ftrace and module support at the same time fails the kernel
build process, because modules cannot find the _mcount symbol.  This
patch fixes this issue.

Signed-off-by: Alan Kao <alankao@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
Cc: Zong Li <zong@andestech.com>
---
 arch/riscv/kernel/mcount.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
index ce9bdc57a2a1..5721624886a1 100644
--- a/arch/riscv/kernel/mcount.S
+++ b/arch/riscv/kernel/mcount.S
@@ -126,5 +126,5 @@ do_trace:
 	RESTORE_ABI_STATE
 	ret
 ENDPROC(_mcount)
-EXPORT_SYMBOL(_mcount)
 #endif
+EXPORT_SYMBOL(_mcount)
-- 
2.17.0

Re: [PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[Steven Rostedt]

On Tue, 8 May 2018 11:21:57 +0800
Alan Kao <alankao@andestech.com> wrote:

> Enabling ftrace and module support at the same time fails the kernel
> build process, because modules cannot find the _mcount symbol.  This
> patch fixes this issue.

I think you have a much bigger issue.

> 
> Signed-off-by: Alan Kao <alankao@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> Cc: Zong Li <zong@andestech.com>
> ---
>  arch/riscv/kernel/mcount.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> index ce9bdc57a2a1..5721624886a1 100644
> --- a/arch/riscv/kernel/mcount.S
> +++ b/arch/riscv/kernel/mcount.S
> @@ -126,5 +126,5 @@ do_trace:
>  	RESTORE_ABI_STATE
>  	ret
>  ENDPROC(_mcount)
> -EXPORT_SYMBOL(_mcount)
>  #endif
> +EXPORT_SYMBOL(_mcount)

How does this work? How do you export _mcount if _mcount isn't even
defined?

-- Steve

Re: [PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[Alan Kao]

On Tue, May 08, 2018 at 09:11:42AM -0400, Steven Rostedt wrote:
> On Tue, 8 May 2018 11:21:57 +0800
> Alan Kao <alankao@andestech.com> wrote:
> 
> > Enabling ftrace and module support at the same time fails the kernel
> > build process, because modules cannot find the _mcount symbol.  This
> > patch fixes this issue.
> 
> I think you have a much bigger issue.
>

Yes, we do, but not this one.  Let me state more detail later.
 
> > 
> > Signed-off-by: Alan Kao <alankao@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > Cc: Zong Li <zong@andestech.com>
> > ---
> >  arch/riscv/kernel/mcount.S | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> > index ce9bdc57a2a1..5721624886a1 100644
> > --- a/arch/riscv/kernel/mcount.S
> > +++ b/arch/riscv/kernel/mcount.S
> > @@ -126,5 +126,5 @@ do_trace:
> >  	RESTORE_ABI_STATE
> >  	ret
> >  ENDPROC(_mcount)
> > -EXPORT_SYMBOL(_mcount)
> >  #endif
> > +EXPORT_SYMBOL(_mcount)
> 
> How does this work? How do you export _mcount if _mcount isn't even
> defined?

_mcount is defined.

This EXPORT_SYMBOL and the _mcount body was inside a
"#ifndef CONFIG_DYNAMIC_FTRACE" section, so if the config has dynamic ftrace
disabled, _mcount is visible.

For the dynamic ftrace case, there is a code snippet in this file:

> ENTRY(ftrace_stub)
> #ifdef CONFIG_DYNAMIC_FTRACE         
>       .global _mcount
>       .set    _mcount, ftrace_stub
> #endif
>        ret 
> ENDPROC(ftrace_stub)

so the _mcount symbol is visible in the kernel, but not for modules.
As a result, a module build always fails, because _mcount is neither exported,
nor aliased to a defined symbol.

That's the simple purpose of this patch:  we just want to eliminate the errors
during the kernel build process.

> 
> -- Steve

Talking about some bigger issues here, there will be twofold.

1. This patch lacks call-site collecting mechanism.

This feature requires a pattern check in recordmcount.pl.  With this,
section __mcount_loc is properly constructed, and the call-sites will be
registered during the loading stage.

However, the loading will then fail at the first try of make_nop, due to
the instruction check.  This is thus the second issue.


2. The instruction check for kernel texts is not applicable to module texts.

The check expects a call instruction to _mcount, but here it is a call to
the .plt section of the module.  After referencing the implementation of arm64,
I think it would need much more work to have make_nop's and make_call's behavior
right.


Quick summary: This patch ensures that a kernel build will not fail because of
the _mcount-missing problem.  But ftrace cannot trace loading modules for now
due to the stated reasons.

Thanks,
Alan Kao

Re: [PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[Steven Rostedt]

On Wed, 9 May 2018 10:36:05 +0800
Alan Kao <alankao@andestech.com> wrote:

> This EXPORT_SYMBOL and the _mcount body was inside a
> "#ifndef CONFIG_DYNAMIC_FTRACE" section, so if the config has dynamic ftrace
> disabled, _mcount is visible.
> 
> For the dynamic ftrace case, there is a code snippet in this file:
> 
> > ENTRY(ftrace_stub)
> > #ifdef CONFIG_DYNAMIC_FTRACE         
> >       .global _mcount
> >       .set    _mcount, ftrace_stub
> > #endif
> >        ret 
> > ENDPROC(ftrace_stub)  

I missed that, thanks.

> 
> Talking about some bigger issues here, there will be twofold.
> 
> 1. This patch lacks call-site collecting mechanism.
> 
> This feature requires a pattern check in recordmcount.pl.  With this,
> section __mcount_loc is properly constructed, and the call-sites will be
> registered during the loading stage.
> 
> However, the loading will then fail at the first try of make_nop, due to
> the instruction check.  This is thus the second issue.
> 
> 
> 2. The instruction check for kernel texts is not applicable to module texts.
> 
> The check expects a call instruction to _mcount, but here it is a call to
> the .plt section of the module.  After referencing the implementation of arm64,
> I think it would need much more work to have make_nop's and make_call's behavior
> right.

And powerpc64 I believe does something similar (which I think arm64
based it's work from).

> 
> 
> Quick summary: This patch ensures that a kernel build will not fail because of
> the _mcount-missing problem.  But ftrace cannot trace loading modules for now
> due to the stated reasons.

Probably why they had modules break to begin with ;-)

-- Steve

Re: [PATCH] riscv/ftrace: Fix the problem modules cannot find _mcount

[Palmer Dabbelt]

I've rewritten the commit message to match what this patch actually
does.  Assuming it's OK, I'll include it as part of the commits for this
merge window.

Thanks!

[PATCH] riscv/ftrace: Export _mcount when FUNCTION_GRAPH_TRACER isn't set

[Palmer Dabbelt]

From: Alan Kao <alankao@andestech.com>

The EXPORT_SYMBOL(_mcount) for RISC-V ended up inside a
CONFIG_FUNCTION_GRAPH_TRACER ifdef.  If you enable modules without
enabling CONFIG_FUNCTION_GRAPH_TRACER then you'll get a build error
without this patch because the modules won't be able to find _mcount.

The new behavior is to export _mcount whenever CONFIG_FUNCTION_TRACER is
defined.  This matches what every other architecture is doing.

Signed-off-by: Alan Kao <alankao@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
Cc: Zong Li <zong@andestech.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/mcount.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
index ce9bdc57a2a1..5721624886a1 100644
--- a/arch/riscv/kernel/mcount.S
+++ b/arch/riscv/kernel/mcount.S
@@ -126,5 +126,5 @@ do_trace:
 	RESTORE_ABI_STATE
 	ret
 ENDPROC(_mcount)
-EXPORT_SYMBOL(_mcount)
 #endif
+EXPORT_SYMBOL(_mcount)
-- 
2.16.4

Re: [PATCH] riscv/ftrace: Export _mcount when FUNCTION_GRAPH_TRACER isn't set

[Alan Kao]

Hi Palmer,

Thanks for the refining work! But a small correction here:

On Mon, Jun 04, 2018 at 01:30:28PM -0700, Palmer Dabbelt wrote:
> From: Alan Kao <alankao@andestech.com>
> 
> The EXPORT_SYMBOL(_mcount) for RISC-V ended up inside a
> CONFIG_FUNCTION_GRAPH_TRACER ifdef.  If you enable modules without
> enabling CONFIG_FUNCTION_GRAPH_TRACER then you'll get a build error
> without this patch because the modules won't be able to find _mcount.

the export was inside a CONFIG_DYNAMIC_FTRACE section instead of
CONFIG_FUNCTION_GRAPH_TRACER.

> The new behavior is to export _mcount whenever CONFIG_FUNCTION_TRACER is
> defined.  This matches what every other architecture is doing.

Also, this patch is just a workaround and there are still issues in bringing
up ftrace for modules.  For details, please check previous emails between
Steve and me.

Thanks again for this better commit message.
Alan Kao

> 
> Signed-off-by: Alan Kao <alankao@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> Cc: Zong Li <zong@andestech.com>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  arch/riscv/kernel/mcount.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> index ce9bdc57a2a1..5721624886a1 100644
> --- a/arch/riscv/kernel/mcount.S
> +++ b/arch/riscv/kernel/mcount.S
> @@ -126,5 +126,5 @@ do_trace:
>  	RESTORE_ABI_STATE
>  	ret
>  ENDPROC(_mcount)
> -EXPORT_SYMBOL(_mcount)
>  #endif
> +EXPORT_SYMBOL(_mcount)
> -- 
> 2.16.4
>