[HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, 2011-02-17 at 00:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On 17 February 2011 08:30, Simon Riggs si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, 2011-02-17 at 14:37 +, Peter Geoghegan wrote: On 17 February 2011 08:30, Simon Riggs si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, Feb 17, 2011 at 9:50 AM, Simon Riggs si...@2ndquadrant.com wrote: On Thu, 2011-02-17 at 14:37 +, Peter Geoghegan wrote: On 17 February 2011 08:30, Simon Riggs si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Simon Riggs si...@2ndquadrant.com writes: On Thu, 2011-02-17 at 00:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, 2011-02-17 at 10:09 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com writes: On Thu, 2011-02-17 at 00:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Kevin Grittner kevin.gritt...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On 02/17/2011 12:19 PM, Tom Lane wrote: Kevin Grittnerkevin.gritt...@wicourts.gov writes: Tom Lanet...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
Andrew Dunstan and...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On 02/17/2011 12:54 PM, Kevin Grittner wrote:
Andrew Dunstanand...@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 diagnostic pop
would work. See
http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, 2011-02-17 at 10:09 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com writes: On Thu, 2011-02-17 at 00:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix blatantly uninitialized variable in recent commit.
On Thu, Feb 17, 2011 at 3:47 PM, Simon Riggs si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
