Bug 54544 - Option -Wuninitialized does not work as documented with volatile
Summary: Option -Wuninitialized does not work as documented with volatile
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.6.3
: P3 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-10 21:19 UTC by Zakhar
Modified: 2012-09-11 21:09 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zakhar 2012-09-10 21:19:10 UTC
I first filed this bug to Ubuntu Launchpad under the reference 1008090:
https://bugs.launchpad.net/ubuntu/+source/gcc-4.6/+bug/1008090

A the finding of the Ubuntu guys is that it is an upstream bug, I'll just copy/paste it here (with additional findings).

I classified is as "minor" as we are talking "only" of warnings (but still it is an annoyance) and/or a documentation bug.
==============================================================================

Versions of Ubuntu and gcc:
-------------------------------------

(Precise amd64, out-of-the-box gcc)

$ LANG=ENG && uname -a && echo && gcc -v
Linux zakhar-desktop 3.2.0-24-generic #39-Ubuntu SMP Mon May 21 16:52:17 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.6/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 4.6.3-1ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.6/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.6 --enable-shared --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.6 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --enable-plugin --enable-objc-gc --disable-werror --with-arch-32=i686 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)

WHAT HAPPENS:
--------------------

Consider the source code below:

/*01*/ int fct(volatile int *p);
/*02*/
/*03*/ int
/*04*/ foo( p )
/*05*/ volatile int *p;
/*06*/ {
/*07*/   volatile int foobar,barfoo;
/*08*/   volatile int flag=0;
/*09*/   volatile int *bar;
/*10*/
/*11*/   do
/*12*/     {
/*13*/       if ( *p )
/*14*/         {
/*15*/           flag= fct( p );
/*16*/           bar = p;
/*17*/         }
/*18*/       if ( fct( p ) ) break;
/*19*/       if ( flag )
/*20*/         {
/*21*/           barfoo = *bar;
/*22*/           if ( bar == (int *)0 ) break;
/*23*/           foobar = *bar;
/*24*/           return foobar + barfoo;
/*25*/         }
/*26*/     }
/*27*/   while ( fct( p ) );
/*28*/
/*29*/   return 0;
/*30*/ }

Compile it and you get:

$ gcc -O3 -c uninit.c -o /dev/null -Wall

uninit.c: In function 'foo':
uninit.c:23:27: warning: 'bar' may be used uninitialized in this function [-Wuninitialized]

WHAT SHOULD HAPPEN:
------------------------------

1) gcc makes a *VERY BAD* job at detecting uninitialized!
Per se, this is not a bug, because it is duly documented that in some situations, gcc cannot guess.
Here, simple logic proves that the detection is bad:
- you cannot reach lines 21-24 unless flag is not zero.
- flag is initialized to zero, and the only place it can take another value is line 15, where it gets the result of our external function.
- if we go to line 15, the next line in sequence would initialize 'bar'.
- thus when we go to lines 21-24 'bar' is definitely initialized.

Other strange things about this false detection:
- it warns on line 23, and not on lines 21 or 22 that already use the same variable BEFORE line 23!
- it stops warning if you remove line 18! This is odd, because line 18 only breaks out of the flow on certain condition, and as this is before we use the allegedly uninitialized variable, it can do no harm. One could argue that gcc can move line 18 up, but I hope it does not... as we don't know whether our external function has side effect (and no way I know in C to instruct the compiler it does not!), if it has side effects this would break the behaviour of the code as fct is already called on line 15, prior to line 18.

2) Apart from that bad detection of uninitialized, the behaviour is *NOT COMPLIANT with the documentation*.

If you look at the documentation page here:
http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

It says:
-Wuninitialized
    Warn if an automatic variable is used without first being initialized (...)

   (...)

   (...) They do not occur for *variables or elements declared volatile*.

As you see, in the code: 'bar' is an automatic variable, 'bar' has been declared volatile (and as demonstrated in 1: it *IS* initialized!), and gcc continues to spit out the warning.

CONCLUSION:
---------------

We do certainly have a bad detection of 'uninitialized'

Plus,
- either we have a bug in gcc that do not remove the warning in spite of the volatile qualifier
- either we have a bug in the documentation, and 'volatile' is irrelevant to that warning!.. but then gcc should provide a way so that the programmer can make the warning disappear when he does perfectly know un-initialization can't happen.


ADDITIONNAL FINDINGS
--------------------
I found that if we make the 'foo' function 'static' the bug disappears.
Does gcc think someone can jump inside the function (which has no label whatsoever for that purpose)? If we could jump anywhere from "outside" to the "inside" of any function with no label... there would surely be plenty of uninitialized variables, wouldn't it?

Unfortunately for my use-case, 'static' is not an option as the function I am writing is part of a (small) library, and meant to be called from other source files. Thus the function has to be 'extern' for me.
Comment 1 Andrew Pinski 2012-09-10 21:22:44 UTC
> As you see, in the code: 'bar' is an automatic variable, 'bar' has been
> declared volatile 

No it is not declared as volatile.  Its type is a pointer to a volatile memory location.  Not a volatile pointer :).
Comment 2 Zakhar 2012-09-10 21:33:15 UTC
Oh yes... my bad!

So we replace lines 03 to 09 by 

/*03*/ extern int
/*04*/ foo( p )
/*05*/ int * volatile p;
/*06*/ {
/*07*/ volatile int foobar,barfoo;
/*08*/ volatile int flag=0;
/*09*/ int * volatile bar;

now it's volatile pointer and all works fine, no warning whatsoever.

Thank a lot, to have solved that.

I'll markd it as RESOLVED/INVALID then.

Best regards.
Comment 3 Zakhar 2012-09-11 16:41:43 UTC
On second thoughs, I reopen the issue!

WHY:
---
The 'correction' of the code above is a contrary to what the function intended to do and thus breaks its logic: the declaration in the second version of the code are inconsistent with what the function was intented to. That is because, I suppose, the message gcc delivers and its correction are not OK with what really happens.



FURTHER EXPLANATIONS:
---------------------
You are right Andrew, the variable 'bar' is indeed (first version of the code) a 'pointer to a volatile memory location' (location of an int).
*And that was intended so*.
The code is a simplification of a 'memory protection algorithm' where the int that is pointed represents the count of threads using a given piece of memory. This count being accessed from several threads, is indeed volatile as no thread can assume the value didn't change from last time it read/wrote the value.

(atomic instructions where removed from the code example).

The pointer itself is not at all volatile. Anyway, once it is passed to the function, it sits on the stack where its value should be unchanged as long a the function lives, and same goes for the 'bar' automatic variable. I'm not doing crazy threaded things on variables on the stack of this function!

I was confused by the warning message saying:
'bar' may be used uninitialized in this function

... because the message is indeed confusing. 'bar' is NOT used uninitialized (as demonstrated) but the *content pointed by 'bar'* could be. I must confess that I didn't look from far enough to interpret the message this way.



HYPOTHESIS:
-----------
So could it be that gcc saw that, and warns incorrectly on 'bar' instead of '*bar'?


If so, yes, because the function receives a pointer to a memory location, the function itself cannot know whether the location pointed to was initialized or not. 'bar' gets the same address ( bar = p ) thus, indeed, the location pointed by 'bar' could be un-initialized.
This could also be coherent with the fact that when we change the function to static, the warning disappears. Being static, all the callers have to be on the same source, thus the compiler can check if the callers initialize properly the content memory pointed to.


But then, shouldn't the message be:
'*bar' may be used uninitialized in this function.


And that would indeed be correct, because '*bar' being the memory pointed to by bar, could indeed be un-initialized (if the caller didn't initialize it).

And thus, the compiler would do a good job signaling that, as '*bar' (memory which bar points to) is declared as volatile, but it is NOT an automatic variable (the pointer is, not the memory pointed to).


My hypothesis is probably wrong, because if gcc warned about un-initialized memory pointed to, it would have to warn on that:

/*01*/ int
/*02*/ foo( p )
/*03*/   int *p;
/*04*/ {
/*05*/   int foobar;
/*06*/
/*07*/   foobar= *p;
/*08*/
/*09*/   return foobar + 2;
/*10*/ }

(we don't know if '*p' has ever been initialized).
And this short code snipet produces no warning at all, which is fine because a lot of code do that (passing variable 'by reference'), and it is perfectly correct not to warn.



CONCLUSION:
----------
As previously concluded, it is not strictly a 'bug' as the documentation perfectly states that in some case the dectection is broken, but I assume this issue can go in the general thread "better wuninitialized".

- either gcc saw that '*bar' is uninitialized and should report that (and not report the pointer instead)
- or gcc saw 'bar' (the pointer) to be unitialized, and that is a test case where it can do better work, as we can demonstrate it IS initialized.




VERSION 3 of the code:
---------------------
Of course, NOT to break my program logic, the correct declaration of the variable should be:
/*09*/   volatile int * volatile bar;

The 1st 'volatile' because indeed the memory we point to MUST be declared as volatile.
The 2nd 'volatile' to suppress the 'false positive' detection on the pointer.



QUESTION @Andrew:
----------------

Should I post something on the general thread "better wuninitialized" (unless my deductions are wrong again), or do you attach the use case directly from this bug report?
Comment 4 Zakhar 2012-09-11 21:09:28 UTC
MORE
----

Unfortunately, I don't think the hypothesis of the uninitialized pointed memory hold. That should prove it if we add:

/*01*/ int fct(volatile int *p);
/*02*/
/*03*/ static int
/*04*/ foo( p )
/*05*/   volatile int * p;
/*06*/ {
/*07*/   volatile int foobar,barfoo;
/*08*/   volatile int flag=0;
/*09*/   volatile int * bar;
/*10*/
/*11*/   do
/*12*/     {
/*13*/       if ( *p )
/*14*/         {
/*15*/           flag= fct( p );
/*16*/           bar = p;
/*17*/         }
/*18*/       if ( fct( p ) ) break;
/*19*/       if ( flag )
/*20*/         {
/*21*/           barfoo = *bar;
/*22*/           if ( bar == (int *)0 ) break;
/*23*/           foobar = *bar;
/*24*/           return foobar + barfoo;
/*25*/         }
/*26*/     }
/*27*/   while ( fct( p ) );
/*28*/
/*29*/   return 0;
/*30*/ }
/*31*/
/*32*/ int
/*33*/ main()
/*34*/ {
/*35*/   int i;
/*35*/
/*37*/   return foo( &i );
/*38*/
/*40*/ }

Here 'main' calls the 'foo' function with a pointer to a variable which for sure is NOT initialized, and there is no warning whatsoever when we compile with:

$ gcc -O3 -c uninit.c -o /dev/null -Wall

In this example, if we go to line 23, for sure the result of the returned value is totally unpredictable as it depends on the value of 'i' in the main function.
'i' is on the stack, and has not been initialized, so it gets any value that was there previously on the stack!


If we remove 'static' in front of the function, this time we get our warning back... but probably a 'false positive' on 'bar', and not related to tracking down pointed memory.


In this new use-case, if we add 'inline' after static (which -O3 should do by itself here?) we are for sure doing something wrong.

Shouldn't -WUninitialized output something instead of remaining silent?