Re: Hammer on snapshot cd's
:> cc -Wall x.c -c -O2 :> x.c: In function 'fubar2': :> x.c:16: warning: 'error' is used uninitialized in this function :> :> (edit so *valuep is set to 0) :> :> cc -Wall x.c -c -O2 :> (no warning reported) : :So you need to go -O2? -O alone doesn't work? Maybe we should -O2 :after the release then :) : :cheers : simon No, we will always stick to -O. GCC is a moving target too, even if -O2 works now there is a high chance it will break something in future GCC rolls. -Matt Matthew Dillon <[EMAIL PROTECTED]>
Re: Hammer on snapshot cd's
On Wed, Jul 16, 2008 at 12:35:38AM +0100, Simon 'corecode' Schubert wrote: > Matthew Dillon wrote: >>One interesting thing I've found on GCC-4 is that the callgraph >> analyzer >>will cross procedure boundaries for all procedures in that particular >>source file. It can actually detect that error is left uninitialized >>in this situation: >> cc -Wall x.c -c -O2 >> x.c: In function 'fubar2': >> x.c:16: warning: 'error' is used uninitialized in this function >> (edit so *valuep is set to 0) >> cc -Wall x.c -c -O2 >> (no warning reported) > > So you need to go -O2? -O alone doesn't work? Maybe we should -O2 after > the release then :) I just tested with the same piece of code I was discussing earlier with gcc 3.4.6 and "-Wall -O" did produce the "... might be used uninitialized in this function" warnings. So apparently -O2 is not necessary.
Re: Hammer on snapshot cd's
Matthew Dillon wrote: One interesting thing I've found on GCC-4 is that the callgraph analyzer will cross procedure boundaries for all procedures in that particular source file. It can actually detect that error is left uninitialized in this situation: cc -Wall x.c -c -O2 x.c: In function 'fubar2': x.c:16: warning: 'error' is used uninitialized in this function (edit so *valuep is set to 0) cc -Wall x.c -c -O2 (no warning reported) So you need to go -O2? -O alone doesn't work? Maybe we should -O2 after the release then :) cheers simon
Re: Hammer on snapshot cd's
On Tue, Jul 15, 2008 at 10:43:29PM +0200, Joerg Sonnenberger wrote: > On Tue, Jul 15, 2008 at 03:22:34PM -0500, Vincent Stemen wrote: > > Compiling with "gcc -o query -g -Wall main.c" produced no warnings at > > all. But when I added #include "query.c" to the main code module after > > it was working stand alone, I got a number of compiler warnings about > > possible use of initialized variables in functions that are in query.c. > > This is depending a lot on the call graph analyser and without > optimising it simply isn't done. > > Joerg Ah! I finally understand what you were trying to tell me here! It finally dawned on me when Johannes said -O2 revealed another uninitialized variable in hammer_btree.c. Sure enough, I was using -O2 in my top level makefile but not when compiling standalone with my test main.c. After grabbing my previous version, from before I fixed the warnings, and compiling with "-O2 -Wall", the warnings did indeed appear. I had no idea that the two options combined would affect the level of warning output. I still have not found in the gcc manual where it tells me that. Thanks, Joerge, for solving that mystery for me :-).
Re: Hammer on snapshot cd's
On Tue, Jul 15, 2008 at 02:40:42PM -0700, Matthew Dillon wrote:
>One interesting thing I've found on GCC-4 is that the callgraph analyzer
>will cross procedure boundaries for all procedures in that particular
>source file. It can actually detect that error is left uninitialized
>in this situation:
>
> cc -Wall x.c -c -O2
> x.c: In function 'fubar2':
> x.c:16: warning: 'error' is used uninitialized in this function
>
> (edit so *valuep is set to 0)
>
> cc -Wall x.c -c -O2
> (no warning reported)
>
> #include
>
> void
> fubar1(int *valuep)
> {
> /* *valuep = 0; */
> }
>
> void
> fubar2(void)
> {
> int error;
>
> fubar1(&error);
> printf("error = %d\n", error);
> }
>
> GCC-4 actually caught a bug during HAMMER development from that sort
> of thing. I was impressed.
>
> -Matt
> Matthew Dillon
> <[EMAIL PROTECTED]>
>
Interesting. Yep. gcc 3.4.6 definitely does not warn on this.
Re: Hammer on snapshot cd's
:> This is depending a lot on the call graph analyser and without :> optimising it simply isn't done. : :Ah nice! Compiling with -O2 revealed another one: Ach. It sure did. hammer_btree_iterate_reverse() is almost identical to hammer_btree_iterate(). It was exactly the same bug... a missing error assignment deep in a switch statement. -Matt
Re: Hammer on snapshot cd's
Joerg Sonnenberger <[EMAIL PROTECTED]> wrote:
> On Tue, Jul 15, 2008 at 03:22:34PM -0500, Vincent Stemen wrote:
>> Compiling with "gcc -o query -g -Wall main.c" produced no warnings at
>> all. But when I added #include "query.c" to the main code module after
>> it was working stand alone, I got a number of compiler warnings about
>> possible use of initialized variables in functions that are in query.c.
>
> This is depending a lot on the call graph analyser and without
> optimising it simply isn't done.
Ah nice! Compiling with -O2 revealed another one:
Index: /hammer/usr/src/sys/vfs/hammer/hammer_btree.c
===
RCS file: /home/dcvs/src/sys/vfs/hammer/hammer_btree.c,v
retrieving revision 1.72
diff -p -u -r1.72 hammer_btree.c
--- /hammer/usr/src/sys/vfs/hammer/hammer_btree.c 15 Jul 2008 18:01:58
- 1.72
+++ /hammer/usr/src/sys/vfs/hammer/hammer_btree.c 15 Jul 2008 21:45:42
-
@@ -375,7 +375,7 @@ hammer_btree_iterate_reverse(hammer_curs
{
hammer_node_ondisk_t node;
hammer_btree_elm_t elm;
- int error;
+ int error = 0;
int r;
int s;
Cheers,
Johannes
>
> Joerg
Re: Hammer on snapshot cd's
One interesting thing I've found on GCC-4 is that the callgraph analyzer
will cross procedure boundaries for all procedures in that particular
source file. It can actually detect that error is left uninitialized
in this situation:
cc -Wall x.c -c -O2
x.c: In function 'fubar2':
x.c:16: warning: 'error' is used uninitialized in this function
(edit so *valuep is set to 0)
cc -Wall x.c -c -O2
(no warning reported)
#include
void
fubar1(int *valuep)
{
/* *valuep = 0; */
}
void
fubar2(void)
{
int error;
fubar1(&error);
printf("error = %d\n", error);
}
GCC-4 actually caught a bug during HAMMER development from that sort
of thing. I was impressed.
-Matt
Matthew Dillon
<[EMAIL PROTECTED]>
Re: Hammer on snapshot cd's
On Tue, Jul 15, 2008 at 03:22:34PM -0500, Vincent Stemen wrote: > Compiling with "gcc -o query -g -Wall main.c" produced no warnings at > all. But when I added #include "query.c" to the main code module after > it was working stand alone, I got a number of compiler warnings about > possible use of initialized variables in functions that are in query.c. This is depending a lot on the call graph analyser and without optimising it simply isn't done. Joerg
Re: Hammer on snapshot cd's
On Tue, Jul 15, 2008 at 07:34:10PM +0100, Simon 'corecode' Schubert wrote: > Matthew Dillon wrote: >> :.. >> :> : >> :> :>That is very odd. Maybe there's a 64-bit arithmatic problem >> somewhere >> :>w/ gcc-34. >> : >> :Hi Matt, >> : >> :the following patch fixes the problem for me. Not sure whether it is the >> :correct solution though. >> : >> :Cheers, >> :Johannes >> : >> :Index: hammer_btree.c >> :+ int error = 0; >> :int r; >> Wow. That is one nasty bug and I am very happy that you found it. >> I will fix it right now. >> I blame my own convalescence. I was relying on gcc-4 to warn me about >> possible uninitialized variable use and got a bit too fancy in my >> handling of 'error' in that routine. GCC missed a case where I was >> not >> initializing error in a switch deeper down in that routine. > > How the heck can a compiler "miss" this? I thought whenever there existed > a possible execution path which would not initialize the variable, it would > complain. > > cheers > simon That is what I thought also, but recently I encountered a similar situation that lead me to believe you cannot always depend on gcc reporting such issues. I was working on a set of functions in a file called query.c. I was testing before including it in the rest of the code by compiling it stand alone with a simple main.c that included query.c, i.e #include "query.c". Compiling with "gcc -o query -g -Wall main.c" produced no warnings at all. But when I added #include "query.c" to the main code module after it was working stand alone, I got a number of compiler warnings about possible use of initialized variables in functions that are in query.c. I still don't know why I got no warnings when it was included in the main.c test module. This was with gcc 3.4.6 on DragonFly.
Re: Hammer on snapshot cd's
:How the heck can a compiler "miss" this? I thought whenever there :existed a possible execution path which would not initialize the :variable, it would complain. : :cheers : simon Meh. It isn't really GCC's fault. Frankly up until a few years ago compilers generally didn't even try to check such use cases beyond simple obvious things. GCC added a ton of code to try to check it through loops, switches, conditionals, and so forth. GCC even tries to figure out invariants created by constants. Sometimes it just misses a case. That bit of code is really complex, I'm not surprised. The funny thing is that I have actually come to depend on GCC warning me. Two years ago I would never have coded 'error' the way I did in that procedure. I changed the way I code because creating an invariant where all the flow paths fall through to the same variable assignment at the end allows GCC to optimize the variable into a short-lived temporary register that don't have to be reserved in the prior code block, and also allows GCC to better partition the coding blocks and optimize other bits of the code better too. You will even occassionally see me do an unnecessary 'blah = 0' to force an invariant and allow GCC to better partition the procedure. Am I nuts? Probably. -Matt Matthew Dillon <[EMAIL PROTECTED]>
Re: Hammer on snapshot cd's
Matthew Dillon wrote: :.. :> : :> :>That is very odd. Maybe there's a 64-bit arithmatic problem somewhere :>w/ gcc-34. : :Hi Matt, : :the following patch fixes the problem for me. Not sure whether it is the :correct solution though. : :Cheers, :Johannes : :Index: hammer_btree.c :+ int error = 0; : int r; Wow. That is one nasty bug and I am very happy that you found it. I will fix it right now. I blame my own convalescence. I was relying on gcc-4 to warn me about possible uninitialized variable use and got a bit too fancy in my handling of 'error' in that routine. GCC missed a case where I was not initializing error in a switch deeper down in that routine. How the heck can a compiler "miss" this? I thought whenever there existed a possible execution path which would not initialize the variable, it would complain. cheers simon
Re: Hammer on snapshot cd's
:.. :> : :> :>That is very odd. Maybe there's a 64-bit arithmatic problem somewhere :>w/ gcc-34. : :Hi Matt, : :the following patch fixes the problem for me. Not sure whether it is the :correct solution though. : :Cheers, :Johannes : :Index: hammer_btree.c :+ int error = 0; : int r; Wow. That is one nasty bug and I am very happy that you found it. I will fix it right now. I blame my own convalescence. I was relying on gcc-4 to warn me about possible uninitialized variable use and got a bit too fancy in my handling of 'error' in that routine. GCC missed a case where I was not initializing error in a switch deeper down in that routine. -Matt Matthew Dillon <[EMAIL PROTECTED]>
Re: Hammer on snapshot cd's
Matthew Dillon <[EMAIL PROTECTED]> wrote:
>
> :Yes it's weird. It's reproducible with gcc34 compiled vkernels btw.
> :
> :Cheers,
> :Johannes
> :
>
>That is very odd. Maybe there's a 64-bit arithmatic problem somewhere
>w/ gcc-34.
Hi Matt,
the following patch fixes the problem for me. Not sure whether it is the
correct solution though.
Cheers,
Johannes
Index: hammer_btree.c
===
RCS file: /home/dcvs/src/sys/vfs/hammer/hammer_btree.c,v
retrieving revision 1.71
diff -u -r1.71 hammer_btree.c
--- hammer_btree.c 13 Jul 2008 09:32:48 - 1.71
+++ hammer_btree.c 15 Jul 2008 16:39:23 -
@@ -117,7 +117,7 @@
{
hammer_node_ondisk_t node;
hammer_btree_elm_t elm;
- int error;
+ int error = 0;
int r;
int s;
