diff mbox

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

Message ID 20131203222543.GQ8277@htj.dyndns.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Tejun Heo Dec. 3, 2013, 10:25 p.m. UTC
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.

--
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

Comments

Josh Triplett Dec. 3, 2013, 11:43 p.m. UTC | #1
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
--
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
Wanlong Gao Dec. 4, 2013, 3:26 a.m. UTC | #2
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
> 

--
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
Tejun Heo Dec. 4, 2013, 3:10 p.m. UTC | #3
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.
diff mbox

Patch

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