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 <[email protected]>
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
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 <[email protected]>
NACK, the warning is bogus.
--
Jens Axboe
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.
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: [email protected]
[mailto:[email protected]] On Behalf Of Al Viro
Sent: 06 January 2006 15:40
To: Luiz Fernando Capitulino
Cc: akpm; [email protected]; 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.
On 1/6/06, Khushil Dep <[email protected]> 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 <[email protected]>
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
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
On 1/6/06, Jens Axboe <[email protected]> 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 <[email protected]>
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
On Fri, 6 Jan 2006 19:48:11 +0100
Jens Axboe <[email protected]> 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
On Fri, Jan 06 2006, Luiz Fernando Capitulino wrote:
>
> On Fri, 6 Jan 2006 19:48:11 +0100
> Jens Axboe <[email protected]> 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
On 1/6/06, Luiz Fernando Capitulino <[email protected]> wrote:
>
> On Fri, 6 Jan 2006 19:48:11 +0100
> Jens Axboe <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
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.
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*
On 1/6/06, Daniel Barkalow <[email protected]> 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 <[email protected]>
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
On Fri, 6 Jan 2006, Jesper Juhl wrote:
> On 1/6/06, Daniel Barkalow <[email protected]> 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*
Al Viro <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340