RE: [PATCH] bio: gcc warning fix.

RE: [PATCH] bio: gcc warning fix.

[Khushil Dep]

I wonder however whether this is not correct? I was always taught to
initialise variables so there is no doubt as to their starting value?

-----------------------
Khushil Dep

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org
[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Al Viro
Sent: 06 January 2006 15:40
To: Luiz Fernando Capitulino
Cc: akpm; axboe@suse.de; lkml
Subject: Re: [PATCH] bio: gcc warning fix.

On Fri, Jan 06, 2006 at 01:07:29PM -0200, Luiz Fernando Capitulino
wrote:
> 
>  Fixes the following gcc 4.0.2 warning:
> 
> fs/bio.c: In function 'bio_alloc_bioset':
> fs/bio.c:167: warning: 'idx' may be used uninitialized in this
function

NAK.  There is nothing to fix except for broken logics in gcc.
Please, do not obfuscate correct code just to make gcc to shut up.
Especially for this call of warnings; gcc4 *blows* in that area.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bio: gcc warning fix.

[Jesper Juhl]

On 1/6/06, Khushil Dep <khushil.dep@help.basilica.co.uk> wrote:
> I wonder however whether this is not correct? I was always taught to
> initialise variables so there is no doubt as to their starting value?
>
There is no doubt, the idx variable is used on the very next line,
it's address is being passed to bvec_alloc_bs() which as the very
first thing it does fills in a value or returns NULL (in which case
idx is undefined anyway).

So there's no doubt at all that idx will always get a value assigned to it.

gcc is right to warn in the sense that it doesn't know if
bvec_alloc_bs() will read or write into idx when its address is passed
to it. But since we know that bvec_alloc_bs() only reads from it after
having assigned a value we know that gcc's warning is wrong, idx can
never *actually* be used uninitialized.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] bio: gcc warning fix.

[Jens Axboe]

On Fri, Jan 06 2006, Jesper Juhl wrote:
> gcc is right to warn in the sense that it doesn't know if
> bvec_alloc_bs() will read or write into idx when its address is passed

The function is right there, on top of bio_alloc_bioset(). It's even
inlined. gcc has absolutely no reason to complain.

> to it. But since we know that bvec_alloc_bs() only reads from it after

bio_alloc_bioset() you mean.

> having assigned a value we know that gcc's warning is wrong, idx can
> never *actually* be used uninitialized.

Indeed, that's the whole point. For the original submitter, you are not
the first to submit this. See archives for basically the same thread as
this one...

-- 
Jens Axboe

Re: [PATCH] bio: gcc warning fix.

[Jesper Juhl]

On 1/6/06, Jens Axboe <axboe@suse.de> wrote:
> On Fri, Jan 06 2006, Jesper Juhl wrote:
> > gcc is right to warn in the sense that it doesn't know if
> > bvec_alloc_bs() will read or write into idx when its address is passed
>
> The function is right there, on top of bio_alloc_bioset(). It's even
> inlined. gcc has absolutely no reason to complain.
>
Right, gcc should be smarter in that case... hmm, I wonder if it warns
if you build with -funit-at-a-time ...

> > to it. But since we know that bvec_alloc_bs() only reads from it after
>
> bio_alloc_bioset() you mean.
>
Actually I did mean that bvec_alloc_bs() only reads the value of *idx
after it has assigned a value to it, but ofcourse there's no way for
gcc to warn about the use inside that function since there it has to
assume that whatever value of the variable being passed as a pointer
is the intended one.
Of course bio_alloc_bioset() also only reads from idx after
bvex_alloc_bs() has initialized it which is why the warning is bogus.

> > having assigned a value we know that gcc's warning is wrong, idx can
> > never *actually* be used uninitialized.
>
> Indeed, that's the whole point. For the original submitter, you are not
> the first to submit this. See archives for basically the same thread as
> this one...
>
> --
> Jens Axboe
>
>

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] bio: gcc warning fix.

[Luiz Fernando Capitulino]

On Fri, 6 Jan 2006 19:48:11 +0100
Jens Axboe <axboe@suse.de> wrote:

| > having assigned a value we know that gcc's warning is wrong, idx can
| > never *actually* be used uninitialized.
| 
| Indeed, that's the whole point. For the original submitter, you are not
| the first to submit this. See archives for basically the same thread as
| this one...

 Al Viro got it: I just wanted to make gcc not complain. But
'obfuscate correct code' for it is wrong.

 The code is right, the patch is bad. That's it.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] bio: gcc warning fix.

[Jens Axboe]

On Fri, Jan 06 2006, Luiz Fernando Capitulino wrote:
> 
> On Fri, 6 Jan 2006 19:48:11 +0100
> Jens Axboe <axboe@suse.de> wrote:
> 
> | > having assigned a value we know that gcc's warning is wrong, idx can
> | > never *actually* be used uninitialized.
> | 
> | Indeed, that's the whole point. For the original submitter, you are not
> | the first to submit this. See archives for basically the same thread as
> | this one...
> 
>  Al Viro got it: I just wanted to make gcc not complain. But
> 'obfuscate correct code' for it is wrong.

Yes I realize this is what you wanted to do, the warning annoys me to
(using 4.0.2 as well on one machine).

>  The code is right, the patch is bad. That's it.

Indeed :-)

-- 
Jens Axboe

Re: [PATCH] bio: gcc warning fix.

[Jesper Juhl]

On 1/6/06, Luiz Fernando Capitulino <lcapitulino@mandriva.com.br> wrote:
>
> On Fri, 6 Jan 2006 19:48:11 +0100
> Jens Axboe <axboe@suse.de> wrote:
>
> | > having assigned a value we know that gcc's warning is wrong, idx can
> | > never *actually* be used uninitialized.
> |
> | Indeed, that's the whole point. For the original submitter, you are not
> | the first to submit this. See archives for basically the same thread as
> | this one...
>
>  Al Viro got it: I just wanted to make gcc not complain. But
> 'obfuscate correct code' for it is wrong.
>
>  The code is right, the patch is bad. That's it.
>
Yeah, but thanks for trying.
You may have been wrong in this case (and learned to verify your
patches more thoroughly), but the effort to try and review and help
improve the kernel is apreciated.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] bio: gcc warning fix.

[Zan Lynx]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Fri, 2006-01-06 at 19:48 +0100, Jens Axboe wrote:
> On Fri, Jan 06 2006, Jesper Juhl wrote:
> > gcc is right to warn in the sense that it doesn't know if
> > bvec_alloc_bs() will read or write into idx when its address is passed
> 
> The function is right there, on top of bio_alloc_bioset(). It's even
> inlined. gcc has absolutely no reason to complain.

GCC complains because it is possible for that function to return without
ever setting a value into idx.  It's the "default" case in the switch.
Of course, if that happens, idx will not be used and so it is not
actually a problem.
-- 
Zan Lynx <zlynx@acm.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bio: gcc warning fix.

[Al Viro]

On Fri, Jan 06, 2006 at 12:33:56PM -0700, Zan Lynx wrote:
> On Fri, 2006-01-06 at 19:48 +0100, Jens Axboe wrote:
> > On Fri, Jan 06 2006, Jesper Juhl wrote:
> > > gcc is right to warn in the sense that it doesn't know if
> > > bvec_alloc_bs() will read or write into idx when its address is passed
> > 
> > The function is right there, on top of bio_alloc_bioset(). It's even
> > inlined. gcc has absolutely no reason to complain.
> 
> GCC complains because it is possible for that function to return without
> ever setting a value into idx.  It's the "default" case in the switch.
> Of course, if that happens, idx will not be used and so it is not
> actually a problem.

If gcc would look at that code *after* it expands the call, it would
actually notice that everything's fine.  The codepath leaving the
inlined block without setting idx would look like

	bvl = NULL;
	goto l1;
...
l1:
	if (!bvl)
		goto l2;
	use idx
...
l2:
	mempool_free(bio, bs->bio_pool);
	bio = NULL;
	goto out;

and after that exit collapses into jump directly to l2, we end up with
situation when every path to use of idx obviously going through l1 and,
before that, end of switch() in inlined block.  All possible precursors
of that end of switch assign idx.

And yes, if you inline it manually gcc _will_ see that everything's OK.
Path that confuses it is
	default in switch -> exit from bio_alloc_bs() -> l1 -> use of idx
and 
		return value will be NULL	=>	  we will go to l2
is what it doesn't notice when it inlines itself.

Re: [PATCH] bio: gcc warning fix.

[Peter Osterlund]

Al Viro <viro@ftp.linux.org.uk> writes:

> On Fri, Jan 06, 2006 at 12:33:56PM -0700, Zan Lynx wrote:
> > On Fri, 2006-01-06 at 19:48 +0100, Jens Axboe wrote:
> > > On Fri, Jan 06 2006, Jesper Juhl wrote:
> > > > gcc is right to warn in the sense that it doesn't know if
> > > > bvec_alloc_bs() will read or write into idx when its address is passed
> > > 
> > > The function is right there, on top of bio_alloc_bioset(). It's even
> > > inlined. gcc has absolutely no reason to complain.

> And yes, if you inline it manually gcc _will_ see that everything's OK.
> Path that confuses it is
> 	default in switch -> exit from bio_alloc_bs() -> l1 -> use of idx
> and 
> 		return value will be NULL	=>	  we will go to l2
> is what it doesn't notice when it inlines itself.

With my compiler (gcc 4.0.2 from FC4), it's the "unlikely" construct
that confuses the gcc warning logic, not the inlining. I get the
warning if I compile the following code with "gcc -Wall -O2 -S
test.c":

	int f(int x)
	{
		int a;
		int b = 0;
		if (x) {
			a = 1;
			b = 1;
		}
		if (__builtin_expect(!b, 0))
			return 0;
		return a;
	}

However, removing the __builtin_expect makes the warning go away.

Interestingly, changing the source so that a is initialized to some
value will make the warning go away without changing the compiled
code. Apparently, the compiler eventually realizes that the initial
value of a is never used, but it realizes it after deciding if the
warning should be generated.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

[PATCH] bio: gcc warning fix.

[Luiz Fernando Capitulino]

 Fixes the following gcc 4.0.2 warning:

fs/bio.c: In function 'bio_alloc_bioset':
fs/bio.c:167: warning: 'idx' may be used uninitialized in this function

Signed-off-by: Luiz Capitulino <lcapitulino@mandriva.com.br>

 fs/bio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bio.c b/fs/bio.c
index 38d3e80..55a5688 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -164,7 +164,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
 
 		bio_init(bio);
 		if (likely(nr_iovecs)) {
-			unsigned long idx;
+			unsigned long idx = 0;
 
 			bvl = bvec_alloc_bs(gfp_mask, nr_iovecs, &idx, bs);
 			if (unlikely(!bvl)) {


-- 
Luiz Fernando N. Capitulino

Re: [PATCH] bio: gcc warning fix.

[Jens Axboe]

On Fri, Jan 06 2006, Luiz Fernando Capitulino wrote:
> 
>  Fixes the following gcc 4.0.2 warning:
> 
> fs/bio.c: In function 'bio_alloc_bioset':
> fs/bio.c:167: warning: 'idx' may be used uninitialized in this function
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@mandriva.com.br>

NACK, the warning is bogus.

-- 
Jens Axboe

Re: [PATCH] bio: gcc warning fix.

[Al Viro]

On Fri, Jan 06, 2006 at 01:07:29PM -0200, Luiz Fernando Capitulino wrote:
> 
>  Fixes the following gcc 4.0.2 warning:
> 
> fs/bio.c: In function 'bio_alloc_bioset':
> fs/bio.c:167: warning: 'idx' may be used uninitialized in this function

NAK.  There is nothing to fix except for broken logics in gcc.
Please, do not obfuscate correct code just to make gcc to shut up.
Especially for this call of warnings; gcc4 *blows* in that area.

Re: [PATCH] bio: gcc warning fix.

[Daniel Barkalow]

On Fri, 6 Jan 2006, Al Viro wrote:

> On Fri, Jan 06, 2006 at 01:07:29PM -0200, Luiz Fernando Capitulino wrote:
> > 
> >  Fixes the following gcc 4.0.2 warning:
> > 
> > fs/bio.c: In function 'bio_alloc_bioset':
> > fs/bio.c:167: warning: 'idx' may be used uninitialized in this function
> 
> NAK.  There is nothing to fix except for broken logics in gcc.
> Please, do not obfuscate correct code just to make gcc to shut up.
> Especially for this call of warnings; gcc4 *blows* in that area.

Wouldn't it be worthwhile to add -Wno-uninitialized for gcc4, since those 
warnings mostly have to be ignored (and are therefore not useful for 
finding actual uninitialized variables) and make it harder to see other 
types of warnings which might be informative?

This would also reduce the number of patches submitted for correct code, 
since people wouldn't keep having to be told to ignore those warnings.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] bio: gcc warning fix.

[Jesper Juhl]

On 1/6/06, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Fri, 6 Jan 2006, Al Viro wrote:
>
> > On Fri, Jan 06, 2006 at 01:07:29PM -0200, Luiz Fernando Capitulino wrote:
> > >
> > >  Fixes the following gcc 4.0.2 warning:
> > >
> > > fs/bio.c: In function 'bio_alloc_bioset':
> > > fs/bio.c:167: warning: 'idx' may be used uninitialized in this function
> >
> > NAK.  There is nothing to fix except for broken logics in gcc.
> > Please, do not obfuscate correct code just to make gcc to shut up.
> > Especially for this call of warnings; gcc4 *blows* in that area.
>
> Wouldn't it be worthwhile to add -Wno-uninitialized for gcc4, since those
> warnings mostly have to be ignored (and are therefore not useful for
> finding actual uninitialized variables) and make it harder to see other
> types of warnings which might be informative?
>
> This would also reduce the number of patches submitted for correct code,
> since people wouldn't keep having to be told to ignore those warnings.
>

Hmm, the uninitialized warnings do find real bugs now and then.
Generate some noice as well, true, but won't we rather tollerate a
little noice rather than miss bugs?

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] bio: gcc warning fix.

[Daniel Barkalow]

On Fri, 6 Jan 2006, Jesper Juhl wrote:

> On 1/6/06, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > On Fri, 6 Jan 2006, Al Viro wrote:
> >
> > > On Fri, Jan 06, 2006 at 01:07:29PM -0200, Luiz Fernando Capitulino wrote:
> > > >
> > > >  Fixes the following gcc 4.0.2 warning:
> > > >
> > > > fs/bio.c: In function 'bio_alloc_bioset':
> > > > fs/bio.c:167: warning: 'idx' may be used uninitialized in this function
> > >
> > > NAK.  There is nothing to fix except for broken logics in gcc.
> > > Please, do not obfuscate correct code just to make gcc to shut up.
> > > Especially for this call of warnings; gcc4 *blows* in that area.
> >
> > Wouldn't it be worthwhile to add -Wno-uninitialized for gcc4, since those
> > warnings mostly have to be ignored (and are therefore not useful for
> > finding actual uninitialized variables) and make it harder to see other
> > types of warnings which might be informative?
> >
> > This would also reduce the number of patches submitted for correct code,
> > since people wouldn't keep having to be told to ignore those warnings.
> >
> 
> Hmm, the uninitialized warnings do find real bugs now and then.
> Generate some noice as well, true, but won't we rather tollerate a
> little noice rather than miss bugs?

Uninitialized warnings from some gcc versions do find real bugs, but I bet 
that randomly choosing variable declarations to inspect would be more 
likely to find bugs than looking at gcc4 warnings. If we tollerate noise, 
we're likely to ignore the noise that identifies a bug.

	-Daniel
*This .sig left intentionally blank*