[BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Wanlong Gao]

Hi Folks,


If kernel config CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y, then the sparse will report non-static
warnings like following:

> /git/linux/fs/inode.c:74:8: warning: symbol 'nr_inodes' was not declared. Should it be static?
> /git/linux/fs/inode.c:75:8: warning: symbol 'nr_unused' was not declared. Should it be static?
>
> $ vi +74 fs/inode.c
>   73 
>   74 static DEFINE_PER_CPU(unsigned long, nr_inodes);
>   75 static DEFINE_PER_CPU(unsigned long, nr_unused);
>   76 


Any thoughts?


Thanks,
Wanlong Gao

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Tejun Heo]

Hello,

On Tue, Nov 26, 2013 at 11:37:03AM +0800, Wanlong Gao wrote:
> If kernel config CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y, then the sparse will report non-static
> warnings like following:
> 
> > /git/linux/fs/inode.c:74:8: warning: symbol 'nr_inodes' was not declared. Should it be static?
> > /git/linux/fs/inode.c:75:8: warning: symbol 'nr_unused' was not declared. Should it be static?
> >
> > $ vi +74 fs/inode.c
> >   73 
> >   74 static DEFINE_PER_CPU(unsigned long, nr_inodes);
> >   75 static DEFINE_PER_CPU(unsigned long, nr_unused);
> >   76 

Hmmm, so, if FORCE_WEAK_PER_CPU is set or the arch needs WEAK_PER_CPU,
DEFINE_PER_CPU() explodes into mind-bending series of definitions to
ensure that the symbol is globally unique to avoid breaking weak decl
requirements on a few archs.  In the process static is dropped from
the actual declaration and it's apparently missing extern decl in
front of it.

Does the following patch make it go away?

Thanks.

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index 57e890a..8c490cc 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -69,6 +69,7 @@
 	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
 	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
 	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
+	extern __typeof__(type) name;					\
 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
 	__typeof__(type) name
 #else

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Josh Triplett]

On Tue, Dec 03, 2013 at 05:25:43PM -0500, Tejun Heo wrote:
> Hello,
> 
> On Tue, Nov 26, 2013 at 11:37:03AM +0800, Wanlong Gao wrote:
> > If kernel config CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y, then the sparse will report non-static
> > warnings like following:
> > 
> > > /git/linux/fs/inode.c:74:8: warning: symbol 'nr_inodes' was not declared. Should it be static?
> > > /git/linux/fs/inode.c:75:8: warning: symbol 'nr_unused' was not declared. Should it be static?
> > >
> > > $ vi +74 fs/inode.c
> > >   73 
> > >   74 static DEFINE_PER_CPU(unsigned long, nr_inodes);
> > >   75 static DEFINE_PER_CPU(unsigned long, nr_unused);
> > >   76 
> 
> Hmmm, so, if FORCE_WEAK_PER_CPU is set or the arch needs WEAK_PER_CPU,
> DEFINE_PER_CPU() explodes into mind-bending series of definitions to
> ensure that the symbol is globally unique to avoid breaking weak decl
> requirements on a few archs.  In the process static is dropped from
> the actual declaration and it's apparently missing extern decl in
> front of it.
> 
> Does the following patch make it go away?

It should, but is there some reason why you couldn't make the definition
on the line immediately below that static?

> Thanks.
> 
> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> index 57e890a..8c490cc 100644
> --- a/include/linux/percpu-defs.h
> +++ b/include/linux/percpu-defs.h
> @@ -69,6 +69,7 @@
>  	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
>  	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
>  	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
> +	extern __typeof__(type) name;					\
>  	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
>  	__typeof__(type) name
>  #else
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Tejun Heo]

On Tue, Dec 03, 2013 at 03:43:20PM -0800, Josh Triplett wrote:
> > Does the following patch make it go away?
> 
> It should, but is there some reason why you couldn't make the definition
> on the line immediately below that static?

Weak symbols can't be static.  That's the point of the whole thing.

Thanks.

-- 
tejun

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Wanlong Gao]

On 12/04/2013 06:25 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Nov 26, 2013 at 11:37:03AM +0800, Wanlong Gao wrote:
>> If kernel config CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y, then the sparse will report non-static
>> warnings like following:
>>
>>> /git/linux/fs/inode.c:74:8: warning: symbol 'nr_inodes' was not declared. Should it be static?
>>> /git/linux/fs/inode.c:75:8: warning: symbol 'nr_unused' was not declared. Should it be static?
>>>
>>> $ vi +74 fs/inode.c
>>>   73 
>>>   74 static DEFINE_PER_CPU(unsigned long, nr_inodes);
>>>   75 static DEFINE_PER_CPU(unsigned long, nr_unused);
>>>   76 
> 
> Hmmm, so, if FORCE_WEAK_PER_CPU is set or the arch needs WEAK_PER_CPU,
> DEFINE_PER_CPU() explodes into mind-bending series of definitions to
> ensure that the symbol is globally unique to avoid breaking weak decl
> requirements on a few archs.  In the process static is dropped from
> the actual declaration and it's apparently missing extern decl in
> front of it.
> 
> Does the following patch make it go away?

Goes away but comes new error:

/git/linux/fs/inode.c:74:8: error: symbol 'nr_inodes' redeclared with different type (originally declared at /git/linux/fs/inode.c:74) - different address spaces
/git/linux/fs/inode.c:75:8: error: symbol 'nr_unused' redeclared with different type (originally declared at /git/linux/fs/inode.c:75) - different address spaces
/git/linux/fs/inode.c:835:8: error: symbol 'last_ino' redeclared with different type (originally declared at /git/linux/fs/inode.c:835) - different address spaces


Thanks,
Wanlong Gao

> 
> Thanks.
> 
> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> index 57e890a..8c490cc 100644
> --- a/include/linux/percpu-defs.h
> +++ b/include/linux/percpu-defs.h
> @@ -69,6 +69,7 @@
>  	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
>  	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
>  	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
> +	extern __typeof__(type) name;					\
>  	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
>  	__typeof__(type) name
>  #else
> 

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Tejun Heo]

On Wed, Dec 04, 2013 at 11:26:44AM +0800, Wanlong Gao wrote:
> Goes away but comes new error:
> 
> /git/linux/fs/inode.c:74:8: error: symbol 'nr_inodes' redeclared with different type (originally declared at /git/linux/fs/inode.c:74) - different address spaces
> /git/linux/fs/inode.c:75:8: error: symbol 'nr_unused' redeclared with different type (originally declared at /git/linux/fs/inode.c:75) - different address spaces
> /git/linux/fs/inode.c:835:8: error: symbol 'last_ino' redeclared with different type (originally declared at /git/linux/fs/inode.c:835) - different address spaces

Oops, my bad.  How about the following?

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index 57e890a..a5fc7d0 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -69,6 +69,7 @@
 	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
 	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
 	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
+	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
 	__typeof__(type) name
 #else

Re: [BUG report]sparse warnings on DEFINE_PER_CPU() symbols non-static

[Wanlong Gao]

On 12/04/2013 11:12 PM, Tejun Heo wrote:
> On Wed, Dec 04, 2013 at 11:26:44AM +0800, Wanlong Gao wrote:
>> Goes away but comes new error:
>>
>> /git/linux/fs/inode.c:74:8: error: symbol 'nr_inodes' redeclared with different type (originally declared at /git/linux/fs/inode.c:74) - different address spaces
>> /git/linux/fs/inode.c:75:8: error: symbol 'nr_unused' redeclared with different type (originally declared at /git/linux/fs/inode.c:75) - different address spaces
>> /git/linux/fs/inode.c:835:8: error: symbol 'last_ino' redeclared with different type (originally declared at /git/linux/fs/inode.c:835) - different address spaces
> 
> Oops, my bad.  How about the following?

It works, thank you.

Tested-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>

> 
> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> index 57e890a..a5fc7d0 100644
> --- a/include/linux/percpu-defs.h
> +++ b/include/linux/percpu-defs.h
> @@ -69,6 +69,7 @@
>  	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
>  	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
>  	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
> +	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
>  	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
>  	__typeof__(type) name
>  #else
> 

[PATCH] percpu: fix spurious sparse warnings from DEFINE_PER_CPU()

[Tejun Heo]

Applied the following to percpu/for-3.13-fixes.

Thanks!
------ 8< ------
From b1a0fbfdde65dffd83c84c006f84fa12041907c5 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Wed, 4 Dec 2013 10:12:40 -0500

When CONFIG_DEBUG_FORCE_WEAK_PER_CPU or CONFIG_ARCH_NEEDS_WEAK_PER_CPU
is set, DEFINE_PER_CPU() explodes into cryptic series of definitions
to still allow using "static" for percpu variables while keeping all
per-cpu symbols unique in the kernel image which is required for weak
symbols.  This ultimately converts the actual symbol to global whether
DEFINE_PER_CPU() is prefixed with static or not.

Unfortunately, the macro forgot to add explicit extern declartion of
the actual symbol ending up defining global symbol without preceding
declaration for static definitions which naturally don't have matching
DECLARE_PER_CPU().  The only ill effect is triggering of the following
warnings.

 fs/inode.c:74:8: warning: symbol 'nr_inodes' was not declared. Should it be static?
 fs/inode.c:75:8: warning: symbol 'nr_unused' was not declared. Should it be static?

Fix it by adding extern declaration in the DEFINE_PER_CPU() macro.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Tested-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 include/linux/percpu-defs.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index 57e890a..a5fc7d0 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -69,6 +69,7 @@
 	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
 	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
 	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
+	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
 	__typeof__(type) name
 #else
-- 
1.8.4.2