Mailing List Archive: Linux: Kernel

[PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

Index | Next | Previous | View Threaded

jaswinder at kernel

Apr 9, 2009, 9:37 AM

Post #1 of 6 (513 views)
Permalink

[PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

Impact: fix sparse warning:
arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?

Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
---
arch/x86/include/asm/ftrace.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bd2c651..49616d4 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
#endif /* __ASSEMBLY__ */
#endif /* CONFIG_FUNCTION_TRACER */

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef __ASSEMBLY__
+extern void prepare_ftrace_return(unsigned long *, unsigned long);
+#endif
+#endif
+
#endif /* _ASM_X86_FTRACE_H */
--
1.6.0.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

fweisbec at gmail

Apr 9, 2009, 10:20 AM

Post #2 of 6 (463 views)
Permalink

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used [In reply to]

On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> Impact: fix sparse warning:
> arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
>
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> ---
> arch/x86/include/asm/ftrace.h | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index bd2c651..49616d4 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
> #endif /* __ASSEMBLY__ */
> #endif /* CONFIG_FUNCTION_TRACER */
>
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +#ifndef __ASSEMBLY__
> +extern void prepare_ftrace_return(unsigned long *, unsigned long);

But it is only used from asm code so there is no need to make
its prototype public.

I don't think this is necessary but if you really think it is, then I would prefer
you use the already existing #ifndef __ASSEMBLY__ block.

Thanks,
Frederic.

> +#endif
> +#endif
> +
> #endif /* _ASM_X86_FTRACE_H */
> --
> 1.6.0.6
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo [at] vger
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jaswinder at kernel

Apr 9, 2009, 10:41 AM

Post #3 of 6 (464 views)
Permalink

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used [In reply to]

On Thu, 2009-04-09 at 19:20 +0200, Frederic Weisbecker wrote:
> On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> > Impact: fix sparse warning:
> > arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> >
> > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> > ---
> > arch/x86/include/asm/ftrace.h | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index bd2c651..49616d4 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
> > #endif /* __ASSEMBLY__ */
> > #endif /* CONFIG_FUNCTION_TRACER */
> >
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +#ifndef __ASSEMBLY__
> > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
>
>
> But it is only used from asm code so there is no need to make
> its prototype public.
>
> I don't think this is necessary but if you really think it is, then I would prefer
> you use the already existing #ifndef __ASSEMBLY__ block.
>

Like this:

Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used

Impact: fix sparse warning:
arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?

Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
---
arch/x86/include/asm/ftrace.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bd2c651..ddc9236 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
};

#endif /* CONFIG_DYNAMIC_FTRACE */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+extern void prepare_ftrace_return(unsigned long *, unsigned long);
+#endif
+
#endif /* __ASSEMBLY__ */
#endif /* CONFIG_FUNCTION_TRACER */

--
1.6.0.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

rostedt at goodmis

Apr 9, 2009, 11:55 AM

Post #4 of 6 (465 views)
Permalink

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used [In reply to]

On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:

> On Thu, 2009-04-09 at 19:20 +0200, Frederic Weisbecker wrote:
> > On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> > > Impact: fix sparse warning:
> > > arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > >
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> > > ---
> > > arch/x86/include/asm/ftrace.h | 6 ++++++
> > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index bd2c651..49616d4 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
> > > #endif /* __ASSEMBLY__ */
> > > #endif /* CONFIG_FUNCTION_TRACER */
> > >
> > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +#ifndef __ASSEMBLY__
> > > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> >
> >
> > But it is only used from asm code so there is no need to make
> > its prototype public.
> >
> > I don't think this is necessary but if you really think it is, then I would prefer
> > you use the already existing #ifndef __ASSEMBLY__ block.
> >

It is not necessary.

>
> Like this:
>
> Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
>
> Impact: fix sparse warning:
> arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
>
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> ---
> arch/x86/include/asm/ftrace.h | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index bd2c651..ddc9236 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
> };
>
> #endif /* CONFIG_DYNAMIC_FTRACE */
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> +#endif
> +

The only caller is from assembly, so this patch is unnecessary. Unless we
have some rule that all functions must have a prototype, even when it is
only called from assembly.

-- Steve

> #endif /* __ASSEMBLY__ */
> #endif /* CONFIG_FUNCTION_TRACER */
>
> --
> 1.6.0.6
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jaswinder at kernel

Apr 9, 2009, 9:29 PM

Post #5 of 6 (456 views)
Permalink

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used [In reply to]

On Thu, 2009-04-09 at 14:55 -0400, Steven Rostedt wrote:
>
> On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:
>
> >
> > Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
> >
> > Impact: fix sparse warning:
> > arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> >
> > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> > ---
> > arch/x86/include/asm/ftrace.h | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index bd2c651..ddc9236 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
> > };
> >
> > #endif /* CONFIG_DYNAMIC_FTRACE */
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> > +#endif
> > +
>
> The only caller is from assembly, so this patch is unnecessary. Unless we
> have some rule that all functions must have a prototype, even when it is
> only called from assembly.
>

Then:

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 18dfa30..75c0682 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -408,6 +408,7 @@ int ftrace_disable_ftrace_graph_caller(void)
* Hook the return address and push it in the stack of return addrs
* in current thread info.
*/
+asmlinkage
void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
{
unsigned long old;

Josh: Do you think sparse should not warn like for this case.
What will be the best solution for this case.

--
JSR

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

fweisbec at gmail

Apr 10, 2009, 5:58 AM

Post #6 of 6 (453 views)
Permalink

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used [In reply to]

On Fri, Apr 10, 2009 at 09:59:55AM +0530, Jaswinder Singh Rajput wrote:
> On Thu, 2009-04-09 at 14:55 -0400, Steven Rostedt wrote:
> >
> > On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:
> >
> > >
> > > Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
> > >
> > > Impact: fix sparse warning:
> > > arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > >
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput [at] gmail>
> > > ---
> > > arch/x86/include/asm/ftrace.h | 5 +++++
> > > 1 files changed, 5 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index bd2c651..ddc9236 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
> > > };
> > >
> > > #endif /* CONFIG_DYNAMIC_FTRACE */
> > > +
> > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> > > +#endif
> > > +
> >
> > The only caller is from assembly, so this patch is unnecessary. Unless we
> > have some rule that all functions must have a prototype, even when it is
> > only called from assembly.
> >
>
> Then:
>
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 18dfa30..75c0682 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -408,6 +408,7 @@ int ftrace_disable_ftrace_graph_caller(void)
> * Hook the return address and push it in the stack of return addrs
> * in current thread info.
> */
> +asmlinkage

No. This is how prepare_ftrace_return was called at first.
But note that prepare_ftrace_return may be called for every
kernel function while tracing, hence every pico optimization
is important. asmlinkage will push the arguments to the stack
while we want to keep the fastcall here by passing them to the
registers.

Sparse is a useful tool, but sometime there are exceptions when its
warnings should be ignored IMO.

Frederic.

> void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> {
> unsigned long old;
>
>
> Josh: Do you think sparse should not warn like for this case.
> What will be the best solution for this case.
>
> --
> JSR
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads