Thread: Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.

On Thu, 2011-02-17 at 00:53 +0000, Tom Lane wrote:

> Doesn't anybody around here pay attention to compiler warnings?

If you see one, then I accept one was there. I didn't see one, and a
full make distclean and re-compile doesn't show a compiler warning for
that either. So I guess I'm doing something wrong, on this platform:

I'm using Ubuntu 10.04 LTS, with commands for development:
./configure --enable-cassert --enable-depend --enable-debug
make -j4

The compile output has been somewhat dirty of late, with various
messages, which if nothing else indicated to me that fairly strict
warnings were enabled... though I guess not.

In file included from gram.y:12758:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:16256: warning: unused variable ‘yyg’
elog.c: In function ‘write_console’:
elog.c:1702: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result
elog.c: In function ‘write_pipe_chunks’:
elog.c:2395: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result
elog.c:2404: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result
common.c: In function ‘handle_sigint’:
common.c:247: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result
common.c:250: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result
common.c:251: warning: ignoring return value of ‘write’, declared with
attribute warn_unused_result

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On 17 February 2011 08:30, Simon Riggs <simon@2ndquadrant.com> wrote:
> In file included from gram.y:12758:
> scan.c: In function ‘yy_try_NUL_trans’:
> scan.c:16256: warning: unused variable ‘yyg’

Lots of people have reported that one. It's been around since August
of last year, if not earlier.

--
Regards,
Peter Geoghegan

On Thu, 2011-02-17 at 14:37 +0000, Peter Geoghegan wrote:
> On 17 February 2011 08:30, Simon Riggs <simon@2ndquadrant.com> wrote:
> > In file included from gram.y:12758:
> > scan.c: In function ‘yy_try_NUL_trans’:
> > scan.c:16256: warning: unused variable ‘yyg’
> 
> Lots of people have reported that one. It's been around since August
> of last year, if not earlier.

Yeh. I wasn't reporting it as an error, just showing the absence of a
warning for an uninitialized variable in the case shown.

What I should have said was: please share any tips for improving error
checking.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On Thu, Feb 17, 2011 at 9:50 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, 2011-02-17 at 14:37 +0000, Peter Geoghegan wrote:
>> On 17 February 2011 08:30, Simon Riggs <simon@2ndquadrant.com> wrote:
>> > In file included from gram.y:12758:
>> > scan.c: In function ‘yy_try_NUL_trans’:
>> > scan.c:16256: warning: unused variable ‘yyg’
>>
>> Lots of people have reported that one. It's been around since August
>> of last year, if not earlier.
>
> Yeh. I wasn't reporting it as an error, just showing the absence of a
> warning for an uninitialized variable in the case shown.
>
> What I should have said was: please share any tips for improving error
> checking.

On MacOS X and Fedora, I put COPT=-Werror in src/Makefile.custom.
(There's a -Wno-error in the rule that compiles scan.c, so it all
works.)  But I can't do that on my Ubuntu machine because of those
stupid warnings about write().

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Thu, 2011-02-17 at 00:53 +0000, Tom Lane wrote:
>> Doesn't anybody around here pay attention to compiler warnings?

> If you see one, then I accept one was there. I didn't see one, and a
> full make distclean and re-compile doesn't show a compiler warning for
> that either. So I guess I'm doing something wrong, on this platform:

> I'm using Ubuntu 10.04 LTS, with commands for development:
> ./configure --enable-cassert --enable-depend --enable-debug
> make -j4

Hmm ... the only plausible reason I can think of for gcc not showing
that warning would be building with -O0 (which disables the flow graph
computations needed to detect uses of uninitialized values).  Your
configure command doesn't betray any such thing, but maybe you've got
some CFLAGS overrides you're not showing us?

I usually find that -O1 is the best compromise setting for development
builds.  It enables uninitialized-variable warnings but doesn't produce
code that's completely unfriendly to gdb.  (Sometimes I do recompile a
specific file at -O0 if it's making no sense during single-stepping.)

> The compile output has been somewhat dirty of late, with various
> messages, which if nothing else indicated to me that fairly strict
> warnings were enabled... though I guess not.

In my builds, the only warning anywhere is the unused variable in
gram.y, which is a bison bug that we can't do anything about (except
complain to the bison folk, which I've done).  It might be worth trying
to clean up those warn_unused_result things, if other people are seeing
those.
        regards, tom lane

On Thu, 2011-02-17 at 10:09 -0500, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Thu, 2011-02-17 at 00:53 +0000, Tom Lane wrote:
> >> Doesn't anybody around here pay attention to compiler warnings?
> 
> > If you see one, then I accept one was there. I didn't see one, and a
> > full make distclean and re-compile doesn't show a compiler warning for
> > that either. So I guess I'm doing something wrong, on this platform:
> 
> > I'm using Ubuntu 10.04 LTS, with commands for development:
> > ./configure --enable-cassert --enable-depend --enable-debug
> > make -j4
> 
> Hmm ... the only plausible reason I can think of for gcc not showing
> that warning would be building with -O0 (which disables the flow graph
> computations needed to detect uses of uninitialized values).  Your
> configure command doesn't betray any such thing, but maybe you've got
> some CFLAGS overrides you're not showing us?

No, nothing set

> I usually find that -O1 is the best compromise setting for development
> builds.  It enables uninitialized-variable warnings but doesn't produce
> code that's completely unfriendly to gdb.  (Sometimes I do recompile a
> specific file at -O0 if it's making no sense during single-stepping.)
> 
> > The compile output has been somewhat dirty of late, with various
> > messages, which if nothing else indicated to me that fairly strict
> > warnings were enabled... though I guess not.
> 
> In my builds, the only warning anywhere is the unused variable in
> gram.y, which is a bison bug that we can't do anything about (except
> complain to the bison folk, which I've done).  It might be worth trying
> to clean up those warn_unused_result things, if other people are seeing
> those.

Thanks, will investigate.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It might be worth trying to clean up those warn_unused_result
> things, if other people are seeing those.
In `make world` I'm seeing:
scan.c: In function *yy_try_NUL_trans*:
scan.c:16256: warning: unused variable *yyg*
--
elog.c: In function *write_console*:
elog.c:1708: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
elog.c: In function *write_pipe_chunks*:
elog.c:2401: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
elog.c:2410: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
--
common.c: In function *handle_sigint*:
common.c:247: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
common.c:250: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
common.c:251: warning: ignoring return value of *write*, declared
with attribute warn_unused_result
--
option.c: In function *parseCommandLine*:
option.c:92: warning: ignoring return value of *getcwd*, declared
with attribute warn_unused_result
option.c: In function *get_pkglibdir*:
option.c:336: warning: ignoring return value of *fgets*, declared
with attribute warn_unused_result
Example of a compile line showing a warning:
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wformat-security
-fno-strict-aliasing -fwrapv -g -I../../../../src/include
-D_GNU_SOURCE -I/usr/include/libxml2   -c -o elog.o elog.c -MMD -MP
-MF .deps/elog.Po
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It might be worth trying to clean up those warn_unused_result
>> things, if other people are seeing those.

> elog.c: In function *write_console*:
> elog.c:1708: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result
> elog.c: In function *write_pipe_chunks*:
> elog.c:2401: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result
> elog.c:2410: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result
> --
> common.c: In function *handle_sigint*:
> common.c:247: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result
> common.c:250: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result
> common.c:251: warning: ignoring return value of *write*, declared
> with attribute warn_unused_result

In at least some of these cases, I think ignoring the write() result
is intentional, because there's really nothing useful we can do about
it if it fails (oh, you wish we'd log a failure to write to the log?).
Would you check whether just casting the function result to (void)
shuts it up?

> option.c: In function *parseCommandLine*:
> option.c:92: warning: ignoring return value of *getcwd*, declared
> with attribute warn_unused_result
> option.c: In function *get_pkglibdir*:
> option.c:336: warning: ignoring return value of *fgets*, declared
> with attribute warn_unused_result

These look like they probably are genuine bugs, or if not bugs at least
the kind of sloppy coding the warning is meant to catch.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Would you check whether just casting the function result to (void)
> shuts it up?
Casting the result to (void) didn't change the warning.  It shut up
when I declared a local variable and assigned the value to it (which
was then never used).
-Kevin

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In at least some of these cases, I think ignoring the write()
> result is intentional, because there's really nothing useful we
> can do about it if it fails (oh, you wish we'd log a failure to
> write to the log?).
I know that in Java you can get a positive number less than the full
size as an indication that part of the block was written, and you
must loop to write until you get all of it written (or get an error
return).  At this page, it appears that the same is true of the
write function in C:
http://www.gnu.org/s/libc/manual/html_node/I_002fO-Primitives.html
Quoting from that page:
| The return value is the number of bytes actually written. This may
| be size, but can always be smaller. Your program should always
| call write in a loop, iterating until all the data is written. 
Isn't that the write function we're calling?  If so, are you
maintaining that the gnu.org documentation of this function is
wrong?
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Would you check whether just casting the function result to (void)
>> shuts it up?
> Casting the result to (void) didn't change the warning.  It shut up
> when I declared a local variable and assigned the value to it (which
> was then never used).

Too bad.  I believe gcc 4.6 will warn about *that*, so it's not going to
be much of an improvement for long.
        regards, tom lane

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> I know that in Java you can get a positive number less than the full
> size as an indication that part of the block was written, and you
> must loop to write until you get all of it written (or get an error
> return).  At this page, it appears that the same is true of the
> write function in C:

This is appropriate when writing to sockets etc, where the kernel is
willing to reflect details like packet boundaries back to userspace.
I have never seen nor heard of it being true for writes to disk files,
except for the case of out-of-disk-space, in which it is quite unlikely
that looping is a good thing to do.
        regards, tom lane

On 02/17/2011 12:19 PM, Tom Lane wrote:
> "Kevin Grittner"<Kevin.Grittner@wicourts.gov>  writes:
>> Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>>> Would you check whether just casting the function result to (void)
>>> shuts it up?
>
>> Casting the result to (void) didn't change the warning.  It shut up
>> when I declared a local variable and assigned the value to it (which
>> was then never used).
> Too bad.  I believe gcc 4.6 will warn about *that*, so it's not going to
> be much of an improvement for long.
>
>             

Ugh. Isn't there some sort of pragma or similar we can use to shut it up?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> wrote:
> Ugh. Isn't there some sort of pragma or similar we can use to shut
> it up?
If that fails, maybe use some function like the below?  That would
also have the advantage of not relying on assumptions beyond the
documented API, which I tend to feel good about no matter how sure I
am that the implementation details upon which I'm relying won't
change.
No claims that this is good final form, especially when it comes to
using ssize_t, but just trying to get across the general idea:
void
write_completely_ignore_errors(int filedes, const void *buffer,                              size_t size)
{   size_t t = 0;   while (t < size)   {       ssize_t n = write(filedes, buffer, size - t);       if (n <= 0)
break;       t += n;   }
 
}
-Kevin

On 02/17/2011 12:54 PM, Kevin Grittner wrote:
> Andrew Dunstan<andrew@dunslane.net>  wrote:
>
>> Ugh. Isn't there some sort of pragma or similar we can use to shut
>> it up?
>
> If that fails, maybe use some function like the below?  That would
> also have the advantage of not relying on assumptions beyond the
> documented API, which I tend to feel good about no matter how sure I
> am that the implementation details upon which I'm relying won't
> change.
>
> No claims that this is good final form, especially when it comes to
> using ssize_t, but just trying to get across the general idea:
>
> void
> write_completely_ignore_errors(int filedes, const void *buffer,
>                                 size_t size)
> {
>      size_t t = 0;
>      while (t<  size)
>      {
>          ssize_t n = write(filedes, buffer, size - t);
>          if (n<= 0)
>              break;
>          t += n;
>      }
> }


In a very modern gcc, where we seem to be getting the errors from, maybe
   |#pragma GCC diagnostic push   |#pragma GCC diagnostic ignored "-Wunused-result"   write( ...);   #pragma GCC
diagnosticpop
 


would work. See 
<http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas>

cheers

andrew

On Thu, 2011-02-17 at 10:09 -0500, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Thu, 2011-02-17 at 00:53 +0000, Tom Lane wrote:
> >> Doesn't anybody around here pay attention to compiler warnings?
> 
> > If you see one, then I accept one was there. I didn't see one, and a
> > full make distclean and re-compile doesn't show a compiler warning for
> > that either. So I guess I'm doing something wrong, on this platform:
> 
> > I'm using Ubuntu 10.04 LTS, with commands for development:
> > ./configure --enable-cassert --enable-depend --enable-debug
> > make -j4
> 
> Hmm ... the only plausible reason I can think of for gcc not showing
> that warning would be building with -O0 (which disables the flow graph
> computations needed to detect uses of uninitialized values).  Your
> configure command doesn't betray any such thing, but maybe you've got
> some CFLAGS overrides you're not showing us?
> 
> I usually find that -O1 is the best compromise setting for development
> builds.  It enables uninitialized-variable warnings but doesn't produce
> code that's completely unfriendly to gdb.  (Sometimes I do recompile a
> specific file at -O0 if it's making no sense during single-stepping.)

Just recompiled with explicit CFLAGS=-O1 using my distro's gcc 4.4.3

The only difference in messages I got was

dbsize.c: In function ‘pg_relation_filepath’:
dbsize.c:570: warning: ‘rnode.dbNode’ may be used uninitialized in this
function
dbsize.c:570: warning: ‘rnode.spcNode’ may be used uninitialized in this
function

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services

On Thu, Feb 17, 2011 at 3:47 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The only difference in messages I got was
>
> dbsize.c: In function ‘pg_relation_filepath’:
> dbsize.c:570: warning: ‘rnode.dbNode’ may be used uninitialized in this
> function
> dbsize.c:570: warning: ‘rnode.spcNode’ may be used uninitialized in this
> function

Well, at least these are easily fixed.  Done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company