[BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Wei Yongjun]

After the following kernel commit, sparse warning lot of
EXPORT_SYMBOL()'d symbol non-static like this:

  CHECK   net/sctp/socket.c
net/sctp/socket.c:4292:1: warning:
 symbol '__ksymtab_sctp_do_peeloff' was not declared. Should it be static?


commit e0f244c63fc9d192dfd399cc2677bbdca61994b1
Author: Andi Kleen <ak@linux.intel.com>
Date:   Wed Oct 23 10:57:58 2013 +1030

    asmlinkage, module: Make ksymtab and kcrctab symbols and __this_module __visible
    
    Make the ksymtab symbols for EXPORT_SYMBOL visible.
    This prevents the LTO compiler from adding a .NUMBER prefix,
    which avoids various problems in later export processing.
    
    Cc: rusty@rustcorp.com.au
    Signed-off-by: Andi Kleen <ak@linux.intel.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

    Cc: rusty@rustcorp.com.au
    Signed-off-by: Andi Kleen <ak@linux.intel.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/export.h b/include/linux/export.h
index 412cd50..3f2793d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -43,7 +43,7 @@ extern struct module __this_module;
 /* Mark the CRC weak since genksyms apparently decides not to
  * generate a checksums for some symbols */
 #define __CRC_SYMBOL(sym, sec)                                 \
-       extern void *__crc_##sym __attribute__((weak));         \
+       extern __visible void *__crc_##sym __attribute__((weak));               \
        static const unsigned long __kcrctab_##sym              \
        __used                                                  \
        __attribute__((section("___kcrctab" sec "+" #sym), unused))     \
@@ -59,7 +59,7 @@ extern struct module __this_module;
        static const char __kstrtab_##sym[]                     \
        __attribute__((section("__ksymtab_strings"), aligned(1))) \
        = VMLINUX_SYMBOL_STR(sym);                              \
-       static const struct kernel_symbol __ksymtab_##sym       \
+       __visible const struct kernel_symbol __ksymtab_##sym    \
        __used                                                  \
        __attribute__((section("___ksymtab" sec "+" #sym), unused))     \
        = { (unsigned long)&sym, __kstrtab_##sym }


Any idea?

Regards,
Yongjun Wei

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Johannes Berg]

On Fri, 2013-11-15 at 14:46 +0800, Wei Yongjun wrote:
> After the following kernel commit, sparse warning lot of
> EXPORT_SYMBOL()'d symbol non-static like this:
> 
>   CHECK   net/sctp/socket.c
> net/sctp/socket.c:4292:1: warning:
>  symbol '__ksymtab_sctp_do_peeloff' was not declared. Should it be static?

> Author: Andi Kleen <ak@linux.intel.com>
> Date:   Wed Oct 23 10:57:58 2013 +1030
> 
>     asmlinkage, module: Make ksymtab and kcrctab symbols and __this_module __visible
>     
>     Make the ksymtab symbols for EXPORT_SYMBOL visible.
>     This prevents the LTO compiler from adding a .NUMBER prefix,
>     which avoids various problems in later export processing.

> Any idea?

Well, sparse is clearly "right", for all it cares it might very well be
static, but it seems this is necessary for something in the kernel and
we clearly can't forward-declare it in a header file. Perhaps we can add
some annotation to say
"__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
suppress this warning? This is getting annoying to me as well :-)

johannes

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Andi Kleen]

> Well, sparse is clearly "right", for all it cares it might very well be
> static, but it seems this is necessary for something in the kernel and
> we clearly can't forward-declare it in a header file. Perhaps we can add
> some annotation to say
> "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> suppress this warning? This is getting annoying to me as well :-)

We could do something like

typeof(foo); 

in the macro. Not sure if that would make sparse happy.

Also this is really working around a problem upto gcc 4.8. that was fixed
in gcc 4.9 (adding numerical postfixes to all symbols)  If it's ok to let LTO only support 4.9+ the patches could be reverted.

-Andi

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Johannes Berg]

On Sun, 2013-11-24 at 12:28 -0800, Andi Kleen wrote:
> > Well, sparse is clearly "right", for all it cares it might very well be
> > static, but it seems this is necessary for something in the kernel and
> > we clearly can't forward-declare it in a header file. Perhaps we can add
> > some annotation to say
> > "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> > suppress this warning? This is getting annoying to me as well :-)
> 
> We could do something like
> 
> typeof(foo); 
> 
> in the macro. Not sure if that would make sparse happy.

I wouldn't think so, after all that's just using the symbol, not
declaring it, and using it clearly happens all the time. I only see an
annotation as a solution, which is ugly but this crops up everywhere and
makes sparse output pretty much unreadable.

> Also this is really working around a problem upto gcc 4.8. that was fixed
> in gcc 4.9 (adding numerical postfixes to all symbols)  If it's ok to
> let LTO only support 4.9+ the patches could be reverted.

That I can't comment on. :)

johannes

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Josh Triplett]

On Sun, Nov 24, 2013 at 12:28:51PM -0800, Andi Kleen wrote:
> > Well, sparse is clearly "right", for all it cares it might very well be
> > static, but it seems this is necessary for something in the kernel and
> > we clearly can't forward-declare it in a header file. Perhaps we can add
> > some annotation to say
> > "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> > suppress this warning? This is getting annoying to me as well :-)
> 
> We could do something like
> 
> typeof(foo); 
> 
> in the macro. Not sure if that would make sparse happy.

If you're just looking to mollify sparse, the easiest way is to
put a prototype of the symbol right before the symbol itself.

> Also this is really working around a problem upto gcc 4.8. that was fixed
> in gcc 4.9 (adding numerical postfixes to all symbols)  If it's ok to let LTO only support 4.9+ the patches could be reverted.

That seems like the best solution, assuming the later scripts can't be
fixed to cope with the numeric suffixes from 4.8.

- Josh Triplett

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Johannes Berg]

On Sun, 2013-11-24 at 12:45 -0800, Josh Triplett wrote:
> On Sun, Nov 24, 2013 at 12:28:51PM -0800, Andi Kleen wrote:
> > > Well, sparse is clearly "right", for all it cares it might very well be
> > > static, but it seems this is necessary for something in the kernel and
> > > we clearly can't forward-declare it in a header file. Perhaps we can add
> > > some annotation to say
> > > "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> > > suppress this warning? This is getting annoying to me as well :-)
> > 
> > We could do something like
> > 
> > typeof(foo); 
> > 
> > in the macro. Not sure if that would make sparse happy.
> 
> If you're just looking to mollify sparse,

I am, pretty much, since now I get a ton of output and have to either
grep it or dig through it... :)

> the easiest way is to
> put a prototype of the symbol right before the symbol itself.

Oh, right, good point, thanks Josh. Andi, do you want to do that?
Otherwise I can post a patch.

johannes

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Johannes Berg]

On Sun, 2013-11-24 at 21:48 +0100, Johannes Berg wrote:
> On Sun, 2013-11-24 at 12:45 -0800, Josh Triplett wrote:
> > On Sun, Nov 24, 2013 at 12:28:51PM -0800, Andi Kleen wrote:
> > > > Well, sparse is clearly "right", for all it cares it might very well be
> > > > static, but it seems this is necessary for something in the kernel and
> > > > we clearly can't forward-declare it in a header file. Perhaps we can add
> > > > some annotation to say
> > > > "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> > > > suppress this warning? This is getting annoying to me as well :-)
> > > 
> > > We could do something like
> > > 
> > > typeof(foo); 
> > > 
> > > in the macro. Not sure if that would make sparse happy.
> > 
> > If you're just looking to mollify sparse,
> 
> I am, pretty much, since now I get a ton of output and have to either
> grep it or dig through it... :)
> 
> > the easiest way is to
> > put a prototype of the symbol right before the symbol itself.
> 
> Oh, right, good point, thanks Josh. Andi, do you want to do that?
> Otherwise I can post a patch.

The below seems to mollify sparse, but all the attributes make my head
spin so I'm not sure it's really what we need.

--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -59,6 +59,7 @@ extern struct module __this_module;
 	static const char __kstrtab_##sym[]			\
 	__attribute__((section("__ksymtab_strings"), aligned(1))) \
 	= VMLINUX_SYMBOL_STR(sym);				\
+	extern const struct kernel_symbol __ksymtab_##sym;	\
 	__visible const struct kernel_symbol __ksymtab_##sym	\
 	__used							\
 	__attribute__((section("___ksymtab" sec "+" #sym), unused))	\


johannes

Re: [BUG] sparse warning EXPORT_SYMBOL()'d symbol non-static

[Josh Triplett]

On Sun, Nov 24, 2013 at 09:52:50PM +0100, Johannes Berg wrote:
> On Sun, 2013-11-24 at 21:48 +0100, Johannes Berg wrote:
> > On Sun, 2013-11-24 at 12:45 -0800, Josh Triplett wrote:
> > > On Sun, Nov 24, 2013 at 12:28:51PM -0800, Andi Kleen wrote:
> > > > > Well, sparse is clearly "right", for all it cares it might very well be
> > > > > static, but it seems this is necessary for something in the kernel and
> > > > > we clearly can't forward-declare it in a header file. Perhaps we can add
> > > > > some annotation to say
> > > > > "__attribute__((yes_I_know_but_really_dont_want_this_to_be_static))" to
> > > > > suppress this warning? This is getting annoying to me as well :-)
> > > > 
> > > > We could do something like
> > > > 
> > > > typeof(foo); 
> > > > 
> > > > in the macro. Not sure if that would make sparse happy.
> > > 
> > > If you're just looking to mollify sparse,
> > 
> > I am, pretty much, since now I get a ton of output and have to either
> > grep it or dig through it... :)
> > 
> > > the easiest way is to
> > > put a prototype of the symbol right before the symbol itself.
> > 
> > Oh, right, good point, thanks Josh. Andi, do you want to do that?
> > Otherwise I can post a patch.
> 
> The below seems to mollify sparse, but all the attributes make my head
> spin so I'm not sure it's really what we need.
> 
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -59,6 +59,7 @@ extern struct module __this_module;
>  	static const char __kstrtab_##sym[]			\
>  	__attribute__((section("__ksymtab_strings"), aligned(1))) \
>  	= VMLINUX_SYMBOL_STR(sym);				\
> +	extern const struct kernel_symbol __ksymtab_##sym;	\
>  	__visible const struct kernel_symbol __ksymtab_##sym	\
>  	__used							\
>  	__attribute__((section("___ksymtab" sec "+" #sym), unused))	\

Yes, that should work as a workaround, though I still think making it
possible for the symbol to be static would be preferable.

- Josh Triplett