Fix sparse warning

Fix sparse warning

[@ 2009-10-19 21:08 UTC (permalink / raw)]

Hi,

I'm starting to do some cleanup for the project, and I'd like to begin
fixing sparse errors.

By the way, at first glance, it seems that the warnings that sparse
catches may need some depth look (i.e. not a newbiew) not to mess things.

For example, my first compilation sparse checking throws this:


init/main.c:763:13: warning: symbol 'call' shadows an earlier one
init/main.c:710:31: originally declared here
init/main.c:793:13: warning: symbol 'call' shadows an earlier one
init/main.c:710:31: originally declared here

If you take a look at the code, the global "call" variable is defined as:

static struct boot_trace_call call;

And what makes sparse whine is a local variable definition, totally
unrelated (i think):

static void __init do_initcalls(void)
{
    initcall_t *call;

    for (call = __early_initcall_end; call < __initcall_end; call++)
        do_one_initcall(*call);

    /* Make sure there is no pending stuff from the initcall sequence */
    flush_scheduled_work();
}

So, it seems that I could "fix" this by changing the local variable
name. The question is: is this _really_ needed apart from "code
hygiene"? Should I "fix" (if you agree to call that "to fix") that issue?

Or would you otherwise recommend me to start with the official janitor
TODO list and address more mechanical work at first? Thanks in advance.


Regards,

-- 
L. Alberto Giménez
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fix sparse warning

[@ 2009-10-19 21:35 UTC (permalink / raw)]

Hi,

I'm starting to do some cleanup for the project, and I'd like to begin
fixing sparse errors.

By the way, at first glance, it seems that the warnings that sparse
catches may need some depth look (i.e. not a newbiew) not to mess things.

For example, my first compilation sparse checking throws this:


init/main.c:763:13: warning: symbol 'call' shadows an earlier one
init/main.c:710:31: originally declared here
init/main.c:793:13: warning: symbol 'call' shadows an earlier one
init/main.c:710:31: originally declared here

If you take a look at the code, the global "call" variable is defined as:

static struct boot_trace_call call;

And what makes sparse whine is a local variable definition, totally
unrelated (i think):

static void __init do_initcalls(void)
{
    initcall_t *call;

    for (call = __early_initcall_end; call < __initcall_end; call++)
        do_one_initcall(*call);

    /* Make sure there is no pending stuff from the initcall sequence */
    flush_scheduled_work();
}

So, it seems that I could "fix" this by changing the local variable
name. The question is: is this _really_ needed apart from "code
hygiene"? Should I "fix" (if you agree to call that "to fix") that issue?

Or would you otherwise recommend me to start with the official janitor
TODO list and address more mechanical work at first? Thanks in advance.


Regards,

-- 
L. Alberto Giménez
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[Matthew Wilcox]

On Mon, Oct 19, 2009 at 11:08:41PM +0200, "L. Alberto Gim??nez" wrote:
> I'm starting to do some cleanup for the project, and I'd like to begin
> fixing sparse errors.

Fixing sparse errors is a worthy place to start; it's certainly more
useful than doing checkpatch warning fixes.

> By the way, at first glance, it seems that the warnings that sparse
> catches may need some depth look (i.e. not a newbiew) not to mess things.

There's certainly a matter of style and taste when deciding how to fix
a warning ... if it were automatable, we could automate the fixes ;-)

> For example, my first compilation sparse checking throws this:
> 
> init/main.c:763:13: warning: symbol 'call' shadows an earlier one
> init/main.c:710:31: originally declared here
> init/main.c:793:13: warning: symbol 'call' shadows an earlier one
> init/main.c:710:31: originally declared here
> 
> If you take a look at the code, the global "call" variable is defined as:
> 
> static struct boot_trace_call call;
> 
> And what makes sparse whine is a local variable definition, totally
> unrelated (i think):
> 
> static void __init do_initcalls(void)
> {
>     initcall_t *call;
> 
>     for (call = __early_initcall_end; call < __initcall_end; call++)
>         do_one_initcall(*call);
> 
>     /* Make sure there is no pending stuff from the initcall sequence */
>     flush_scheduled_work();
> }
> 
> So, it seems that I could "fix" this by changing the local variable
> name. The question is: is this _really_ needed apart from "code
> hygiene"? Should I "fix" (if you agree to call that "to fix") that issue?

It can certainly be confusing to have two variables with the same name.
Since gcc can do this, I'm not sure why we don't enable -Wshadow.

I think in this particular case, there's no reason for 'boot_trace_call'
to have file-level scope.  I would move it inside the do_one_initcall()
function (and I'd do the same with msgbuf and boot_trace_ret too).

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Fix sparse warning

[walter harms]

L. Alberto Giménez schrieb:
> Hi,
> 
> I'm starting to do some cleanup for the project, and I'd like to begin
> fixing sparse errors.
> 
> By the way, at first glance, it seems that the warnings that sparse
> catches may need some depth look (i.e. not a newbiew) not to mess things.
> 
> For example, my first compilation sparse checking throws this:
> 
> 
> init/main.c:763:13: warning: symbol 'call' shadows an earlier one
> init/main.c:710:31: originally declared here
> init/main.c:793:13: warning: symbol 'call' shadows an earlier one
> init/main.c:710:31: originally declared here
> 
> If you take a look at the code, the global "call" variable is defined as:
> 
> static struct boot_trace_call call;
> 
> And what makes sparse whine is a local variable definition, totally
> unrelated (i think):
> 
> static void __init do_initcalls(void)
> {
>     initcall_t *call;
> 
>     for (call = __early_initcall_end; call < __initcall_end; call++)
>         do_one_initcall(*call);
> 
>     /* Make sure there is no pending stuff from the initcall sequence */
>     flush_scheduled_work();
> }
> 
> So, it seems that I could "fix" this by changing the local variable
> name. The question is: is this _really_ needed apart from "code
> hygiene"? Should I "fix" (if you agree to call that "to fix") that issue?
> 
> Or would you otherwise recommend me to start with the official janitor
> TODO list and address more mechanical work at first? Thanks in advance.
> 


Having two variables with the same name is bad practice at least.
But before renaming "initcall_t *call;" (the easy way) you should check
if the global "struct boot_trace_call call;" really really is needed (global is evil).

re,
 wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[@ 2009-10-20 22:02 UTC (permalink / raw)]

Matthew Wilcox wrote:
> On Mon, Oct 19, 2009 at 11:08:41PM +0200, "L. Alberto Gim??nez" wrote:

> I think in this particular case, there's no reason for 'boot_trace_call'
> to have file-level scope.  I would move it inside the do_one_initcall()
> function (and I'd do the same with msgbuf and boot_trace_ret too).

Matthew, Walter, thanks for your reply.

The static (file-scope) variable was introduced by Ingo Molnar in commit
4a683bf9.

It seems that originally those three static variables were local
variables, what turned out to cause a stack overflow. So, I think they
will stay just like they are right now :) (thank god we have git-blame).

Instead, I'll try to work on renaming the "offending" local variables
and cleaning file by file sparse warnings.


Best regards,
-- 
L. Alberto Giménez
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[@ 2009-10-20 22:08 UTC (permalink / raw)]

Matthew Wilcox wrote:
> On Mon, Oct 19, 2009 at 11:08:41PM +0200, "L. Alberto Gim??nez" wrote:

> I think in this particular case, there's no reason for 'boot_trace_call'
> to have file-level scope.  I would move it inside the do_one_initcall()
> function (and I'd do the same with msgbuf and boot_trace_ret too).

Matthew, Walter, thanks for your reply.

The static (file-scope) variable was introduced by Ingo Molnar in commit
4a683bf9.

It seems that originally those three static variables were local
variables, what turned out to cause a stack overflow. So, I think they
will stay just like they are right now :) (thank god we have git-blame).

Instead, I'll try to work on renaming the "offending" local variables
and cleaning file by file sparse warnings.


Best regards,
-- 
L. Alberto Giménez
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[Matthew Wilcox]

On Wed, Oct 21, 2009 at 12:02:17AM +0200, "L. Alberto Gim?nez" wrote:
> Matthew Wilcox wrote:
> > On Mon, Oct 19, 2009 at 11:08:41PM +0200, "L. Alberto Gim??nez" wrote:
> 
> > I think in this particular case, there's no reason for 'boot_trace_call'
> > to have file-level scope.  I would move it inside the do_one_initcall()
> > function (and I'd do the same with msgbuf and boot_trace_ret too).
> 
> Matthew, Walter, thanks for your reply.
> 
> The static (file-scope) variable was introduced by Ingo Molnar in commit
> 4a683bf9.
> 
> It seems that originally those three static variables were local
> variables, what turned out to cause a stack overflow. So, I think they
> will stay just like they are right now :) (thank god we have git-blame).
> 
> Instead, I'll try to work on renaming the "offending" local variables
> and cleaning file by file sparse warnings.

Ah, you're conflating two things here (and this is a subtle corner of C,
but it's very important to understand the distinction.  I'm not quite
sure why Ingo got it wrong ;-)

Here's the current situation:

static struct boot_trace_call call;
int do_one_initcall(initcall_t fn)
{
	...
}

That allocates 'call' in the data segment, and gives it file scope.


You thought I meant this:

int do_one_initcall(initcall_t fn)
{
	struct boot_trace_call call;
	...
}

That would change the scope to be function-local, and allocate 'call'
on the stack.


What I actually meant was this:

int do_one_initcall(initcall_t fn)
{
	static struct boot_trace_call call;
	...
}

That makes the scope function-local, but allocates 'call' from the data
segment, not on the stack.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Fix sparse warning

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Oct 21, 2009 at 12:02:17AM +0200, "L. Alberto Gim?nez" wrote:
> > Matthew Wilcox wrote:
> > > On Mon, Oct 19, 2009 at 11:08:41PM +0200, "L. Alberto Gim??nez" wrote:
> > 
> > > I think in this particular case, there's no reason for 'boot_trace_call'
> > > to have file-level scope.  I would move it inside the do_one_initcall()
> > > function (and I'd do the same with msgbuf and boot_trace_ret too).
> > 
> > Matthew, Walter, thanks for your reply.
> > 
> > The static (file-scope) variable was introduced by Ingo Molnar in commit
> > 4a683bf9.
> > 
> > It seems that originally those three static variables were local
> > variables, what turned out to cause a stack overflow. So, I think they
> > will stay just like they are right now :) (thank god we have git-blame).
> > 
> > Instead, I'll try to work on renaming the "offending" local variables
> > and cleaning file by file sparse warnings.
> 
> Ah, you're conflating two things here (and this is a subtle corner of C,
> but it's very important to understand the distinction.  I'm not quite
> sure why Ingo got it wrong ;-)
> 
> Here's the current situation:
> 
> static struct boot_trace_call call;
> int do_one_initcall(initcall_t fn)
> {
> 	...
> }
> 
> That allocates 'call' in the data segment, and gives it file scope.
> 
> 
> You thought I meant this:
> 
> int do_one_initcall(initcall_t fn)
> {
> 	struct boot_trace_call call;
> 	...
> }
> 
> That would change the scope to be function-local, and allocate 'call'
> on the stack.
> 
> 
> What I actually meant was this:
> 
> int do_one_initcall(initcall_t fn)
> {
> 	static struct boot_trace_call call;
> 	...
> }
> 
> That makes the scope function-local, but allocates 'call' from the data
> segment, not on the stack.

I made that change in 4a683bf9 intentionally. In the kernel we try to 
avoid function scope statics as much as possible:

 - Overlooking them and confusing them with local, on-stack variables is 
   very easy and we've had subtle bugs in the past due to that.

 - They end up in .data (or .bss) and to keep data storage/bloat under
   control it's best to see all globally allocated variables at file 
   scope.

 - All such variables need proper locking, as they can be re-entered 
   from multiple CPUs (while local stack variables cannot and generally 
   dont need such locking). Moving them to file scope makes sure it's 
   all visible and makes sure proper locking is done.

All in one, there's a constant trend away from statics at function 
scope. It can be done correctly but is subtly fragile for the above 
reasons.

	Ingo

Re: Fix sparse warning

[@ 2009-10-21 21:18 UTC (permalink / raw)]

Hi,

I understand your point, but it's not clear to me that function scope
static variables are "bad".

On 10/21/2009 01:37 PM, Ingo Molnar wrote:
 > I made that change in 4a683bf9 intentionally. In the kernel we try to
> avoid function scope statics as much as possible:
> 
>  - Overlooking them and confusing them with local, on-stack variables is 
>    very easy and we've had subtle bugs in the past due to that.

I think that changing your code to make it "bug-safe" is a bad idea. It
comes to my mind the ugly thing that some programmers do not to
overwrite their variables by mistake when they want to compare:

if (2 = myvar) {
	do_things();
}


>  - They end up in .data (or .bss) and to keep data storage/bloat under
>    control it's best to see all globally allocated variables at file 
>    scope.

That makes sense. Anyway, in our example, the definitions begin at line
706. In my opinion, that does not help to increase visibility, and since
we have syntax-aware editors, the "static" thing should be seen easily.


>  - All such variables need proper locking, as they can be re-entered 
>    from multiple CPUs (while local stack variables cannot and generally 
>    dont need such locking). Moving them to file scope makes sure it's 
>    all visible and makes sure proper locking is done.

The same thing about visibility applies as above. Moreover, I can't see
any explicit lock inside the main.c file. I haven't yet dig into the
functions that use those variables, but if they do acquire some kind of
lock, the visibility problem is the same. You are already in another
funcion, far away of the one that defines the static variables (or the
one that, like in this case, has the definition close to it), and maybe
in another file.

> 
> All in one, there's a constant trend away from statics at function 
> scope. It can be done correctly but is subtly fragile for the above 
> reasons.

Indeed it may be fragile. And I'm sure that you've faced tons of bugs
because of declaring static variables in funcion scope and not having it
in mind when using those variables. I'm just a newbie and I have not
faced serious challenges about kernel development, but I think that
those "sanity" things should be fixed (I'm kind of standards taliban :)
), but I need your expertise to know *how* to fix it, from the point of
view of semantics and not just syntax.

Many thanks.


Regards,
-- 
L. Alberto Giménez
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[Ingo Molnar]

* "L. Alberto Giménez" <agimenez@sysvalve.homelinux.net> wrote:

> >  - All such variables need proper locking, as they can be re-entered 
> >    from multiple CPUs (while local stack variables cannot and generally 
> >    dont need such locking). Moving them to file scope makes sure it's 
> >    all visible and makes sure proper locking is done.
> 
> The same thing about visibility applies as above. Moreover, I can't 
> see any explicit lock inside the main.c file. [...]

It's implicit - all bootup execution is serialized.

So it's double important to make any global state clearly visible - 
should any of this be pushed to async bootup init functions (which would 
require the addition of a lock).

> > All in one, there's a constant trend away from statics at function 
> > scope. It can be done correctly but is subtly fragile for the above 
> > reasons.
> 
> Indeed it may be fragile. And I'm sure that you've faced tons of bugs 
> because of declaring static variables in funcion scope and not having 
> it in mind when using those variables. I'm just a newbie and I have 
> not faced serious challenges about kernel development, but I think 
> that those "sanity" things should be fixed (I'm kind of standards 
> taliban :) ), but I need your expertise to know *how* to fix it, from 
> the point of view of semantics and not just syntax.

I think it's subtly wrong if you try to 'fix a warning', as the subject 
line already says it.

Warnings are not something that should be 'fixed' - they are just a sign 
of some sort of trouble:

 1- bug/limitation in the tool (Sparse)
 2- uncleanliness in the source code
 3- bug in the source code

here it seems to be case #1, as file scope statics are perfectly fine, 
even if used by a single function.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix sparse warning

[Matthew Wilcox]

On Fri, Oct 23, 2009 at 09:59:11AM +0200, Ingo Molnar wrote:
> Warnings are not something that should be 'fixed' - they are just a sign 
> of some sort of trouble:
> 
>  1- bug/limitation in the tool (Sparse)
>  2- uncleanliness in the source code
>  3- bug in the source code
> 
> here it seems to be case #1, as file scope statics are perfectly fine, 
> even if used by a single function.

This is case #2.  We used to have:

int do_one_initcall(initcall_t fn)
{
	struct boot_trace_call call;
}

static void __init do_initcalls(void)
{
        initcall_t *call;
}

static void __init do_pre_smp_initcalls(void)
{
        initcall_t *call;
}

and you changed it so that the boot_trace_call is now file-scope.  Now
sparse rightfully warns about the shadow definitions in do_initcalls and
do_pre_smp_initcalls.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Fix sparse warning

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Fri, Oct 23, 2009 at 09:59:11AM +0200, Ingo Molnar wrote:
> > Warnings are not something that should be 'fixed' - they are just a sign 
> > of some sort of trouble:
> > 
> >  1- bug/limitation in the tool (Sparse)
> >  2- uncleanliness in the source code
> >  3- bug in the source code
> > 
> > here it seems to be case #1, as file scope statics are perfectly fine, 
> > even if used by a single function.
> 
> This is case #2.  We used to have:
> 
> int do_one_initcall(initcall_t fn)
> {
> 	struct boot_trace_call call;
> }
> 
> static void __init do_initcalls(void)
> {
>         initcall_t *call;
> }
> 
> static void __init do_pre_smp_initcalls(void)
> {
>         initcall_t *call;
> }
> 
> and you changed it so that the boot_trace_call is now file-scope.  Now 
> sparse rightfully warns about the shadow definitions in do_initcalls 
> and do_pre_smp_initcalls.

Doh, right you are!

Unfortunately the discussion was not Cc:-ed to lkml so i couldnt review 
the original patch and i assumed it moved the static back into function 
scope.

The right fix would be to rename the variables to not be colliding. (if 
we used -Wshadow like tools/perf/ does we'd have gotten this warning 
from GCC too btw) Does the patch do that? Could we please Cc: patches to 
lkml?

Thanks,

	Ingo

Re: Fix sparse warning

[Matthew Wilcox]

On Fri, Oct 23, 2009 at 01:38:15PM +0200, Ingo Molnar wrote:
> * Matthew Wilcox <matthew@wil.cx> wrote:
> > and you changed it so that the boot_trace_call is now file-scope.  Now 
> > sparse rightfully warns about the shadow definitions in do_initcalls 
> > and do_pre_smp_initcalls.
> 
> Doh, right you are!
> 
> Unfortunately the discussion was not Cc:-ed to lkml so i couldnt review 
> the original patch and i assumed it moved the static back into function 
> scope.
> 
> The right fix would be to rename the variables to not be colliding. (if 
> we used -Wshadow like tools/perf/ does we'd have gotten this warning 
> from GCC too btw) Does the patch do that? Could we please Cc: patches to 
> lkml?

There wasn't a patch ... it was a question about the right way to fix
something: http://marc.info/?l=kernel-janitors&m\x125598851800338&w=2

I agree with you that we should add -Wshadow to the CFLAGS, rather than
requiring sparse to find these problems.

If you're dead-set on not moving this statis variable back to
function-scope, then it needs to be renamed; and probably best to rename
all of 'msgbuf', 'call' and 'ret' to have the same prefix.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."