Re: Re: [COMMITTERS] pgsql: Silence compiler warning about ignored return value.

Log Message:
-----------
Silence compiler warning about ignored return value. Our comment already
clearly stated that we are aware that we're ignoring it.

Modified Files:
--------------
pgsql/src/test/regress:
pg_regress.c (r1.49 -> r1.50)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/pg_regress.c?r1=1.49&r2=1.50)

Magnus Hagander wrote:
> Log Message:
> -----------
> Silence compiler warning about ignored return value. Our comment already
> clearly stated that we are aware that we're ignoring it.

I think the usual way is to call the function like:

(void) function_with_return_value()

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Magnus Hagander wrote:
>> Log Message:
>> -----------
>> Silence compiler warning about ignored return value. Our comment already
>> clearly stated that we are aware that we're ignoring it.
>
> I think the usual way is to call the function like:
>
> (void) function_with_return_value()

I tried that first, of course. gcc is too smart about that - it still
throws the warning in this case.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Heikki Linnakangas wrote:
>> I think the usual way is to call the function like:
>> (void) function_with_return_value()

> I tried that first, of course. gcc is too smart about that - it still
> throws the warning in this case.

I think you must have a broken version of gcc. I don't like this
patch either. The (void) is the standard way and should work;
futhermore, if you're getting a warning here, why aren't you getting
a whole lot of others? It's not like we are careful to use (void)
everywhere.

regards, tom lane

Magnus Hagander wrote:
> Heikki Linnakangas wrote:
>> Magnus Hagander wrote:
>>> Log Message:
>>> -----------
>>> Silence compiler warning about ignored return value. Our comment already
>>> clearly stated that we are aware that we're ignoring it.
>> I think the usual way is to call the function like:
>>
>> (void) function_with_return_value()
>
> I tried that first, of course. gcc is too smart about that - it still
> throws the warning in this case.

Well, the warning is explicitly put in there for this specific function
because you are supposed to process the return value. I'm sure a more
smarter compiler would even warn "variable is assigned a value that is
never used". ;-) (Note that gcc in general doesn't work about unused
return values, only for those functions that glibc explicitly marks as
candidates.)

It looks like you are building in fortify mode? I tried that a while
ago and got a few more warnings. Are we trying to be fortify clean, and
if so, what is our approach?

Also, considering my recent complaint about various brittleness in the
regression test driver, more well hidden ignorings of errors are not
exactly my favorite solution.

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Heikki Linnakangas wrote:
>>> I think the usual way is to call the function like:
>>> (void) function_with_return_value()
>
>> I tried that first, of course. gcc is too smart about that - it still
>> throws the warning in this case.
>
> I think you must have a broken version of gcc. I don't like this
> patch either. The (void) is the standard way and should work;
> futhermore, if you're getting a warning here, why aren't you getting
> a whole lot of others? It's not like we are careful to use (void)
> everywhere.

It's:
gcc (Ubuntu 4.3.2-1ubuntu11) 4.3.2

Error was:
pg_regress.c:282: warning: ignoring return value of ‘system’, declared
with attribute warn_unused_result

It's because system() is flagged with __attribute(warn_unused_result)__.
That's why we're not seeing it for other functions. There's a paragraph
about the difference in the GCC docs.

If that's in the new versions of gcc, I expect it to show up on other
platforms as well as time passes.

//Magnus

Peter Eisentraut wrote:
> Magnus Hagander wrote:
>> Heikki Linnakangas wrote:
>>> Magnus Hagander wrote:
>>>> Log Message:
>>>> -----------
>>>> Silence compiler warning about ignored return value. Our comment
>>>> already
>>>> clearly stated that we are aware that we're ignoring it.
>>> I think the usual way is to call the function like:
>>>
>>> (void) function_with_return_value()
>>
>> I tried that first, of course. gcc is too smart about that - it still
>> throws the warning in this case.
>
> Well, the warning is explicitly put in there for this specific function
> because you are supposed to process the return value. I'm sure a more
> smarter compiler would even warn "variable is assigned a value that is
> never used". ;-) (Note that gcc in general doesn't work about unused
> return values, only for those functions that glibc explicitly marks as
> candidates.)
>
> It looks like you are building in fortify mode? I tried that a while
> ago and got a few more warnings. Are we trying to be fortify clean, and
> if so, what is our approach?

I'm building in whatever is the default on Ubuntu 8.10. It may be that
they have switched to fortify mode. How do I check that?

> Also, considering my recent complaint about various brittleness in the
> regression test driver, more well hidden ignorings of errors are not
> exactly my favorite solution.

Right, the other thing to do would be to check the error value :-) I
just assumed that since the comment explicitly said we wanted to ignore
it, we'd want to get rid of the warning. If the comment hadn't been
there, I'd have looked at a different way to do it.

//Magnus

On 2008-11-20, at 15:29, Peter Eisentraut wrote:
>
> Well, the warning is explicitly put in there for this specific
> function because you are supposed to process the return value. I'm
> sure a more smarter compiler would even warn "variable is assigned a
> value that is never used". ;-) (Note that gcc in general doesn't
> work about unused return values, only for those functions that glibc
> explicitly marks as candidates.)

afaik you need to use -Wall and -O3 to get that type of warning with 4.3

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> It looks like you are building in fortify mode? I tried that a while
> ago and got a few more warnings. Are we trying to be fortify clean, and
> if so, what is our approach?

We're definitely *not* fortify-clean, although maybe trying to become so
would be a good idea. From memory, what I have seen in past builds on
Red Hat systems is a lot of warnings about ignoring the return value
from fwrite() and related functions. Which might not be an unreasonable
thing to try to get rid of, though I think many of the cases are in
places where there's no useful recovery action to be taken anyway.
(Whatcha gonna do if you fail to write the postmaster log ... try to
log a complaint message?)

In any case I agree that anything we try to do about this should be a
system-wide effort not a one-line hack.

> Also, considering my recent complaint about various brittleness in the
> regression test driver, more well hidden ignorings of errors are not
> exactly my favorite solution.

+1

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> It looks like you are building in fortify mode? I tried that a while
>> ago and got a few more warnings. Are we trying to be fortify clean, and
>> if so, what is our approach?
>
> We're definitely *not* fortify-clean, although maybe trying to become so
> would be a good idea. From memory, what I have seen in past builds on
> Red Hat systems is a lot of warnings about ignoring the return value
> from fwrite() and related functions. Which might not be an unreasonable
> thing to try to get rid of, though I think many of the cases are in
> places where there's no useful recovery action to be taken anyway.
> (Whatcha gonna do if you fail to write the postmaster log ... try to
> log a complaint message?)
>
> In any case I agree that anything we try to do about this should be a
> system-wide effort not a one-line hack.
>
>> Also, considering my recent complaint about various brittleness in the
>> regression test driver, more well hidden ignorings of errors are not
>> exactly my favorite solution.
>
> +1

So. Should I revert it?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> We're definitely *not* fortify-clean, although maybe trying to become so
>> would be a good idea. From memory, what I have seen in past builds on
>> Red Hat systems is a lot of warnings about ignoring the return value
>> from fwrite() and related functions.

> So. Should I revert it?

I dunno. Was that the only such warning you got on your build? That
would be a bit odd given that I see many more ...

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> We're definitely *not* fortify-clean, although maybe trying to become so
>>> would be a good idea. From memory, what I have seen in past builds on
>>> Red Hat systems is a lot of warnings about ignoring the return value
>>> from fwrite() and related functions.
>
>> So. Should I revert it?
>
> I dunno. Was that the only such warning you got on your build? That
> would be a bit odd given that I see many more ...

Yeah, it happens in more places. I wasn't doing a make clean first, so
that was the only one of the files that were built *that time* that gave
the warning. I see it in more places now.

I think I'll go figure out how to turn fortify off :-)

//Magnus

Magnus Hagander wrote:
> Tom Lane wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> Tom Lane wrote:
>>>> We're definitely *not* fortify-clean, although maybe trying to become so
>>>> would be a good idea. From memory, what I have seen in past builds on
>>>> Red Hat systems is a lot of warnings about ignoring the return value
>>>> from fwrite() and related functions.
>>> So. Should I revert it?
>> I dunno. Was that the only such warning you got on your build? That
>> would be a bit odd given that I see many more ...
>
> Yeah, it happens in more places. I wasn't doing a make clean first, so
> that was the only one of the files that were built *that time* that gave
> the warning. I see it in more places now.
>
> I think I'll go figure out how to turn fortify off :-)

Actually, better not. That's what caught that string format bug after all...

//Magnus

Tom Lane wrote:
> (Whatcha gonna do if you fail to write the postmaster log ... try to
> log a complaint message?)

Well, good question, but I'm not convinced that doing nothing is always
the best answer. Something along the lines of SIGSTOP'ing oneself or
calling abort() might be appropriate in some cases.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane wrote:
>> (Whatcha gonna do if you fail to write the postmaster log ... try to
>> log a complaint message?)

> Well, good question, but I'm not convinced that doing nothing is always
> the best answer.

I didn't say it was --- merely opined that we need a consistent plan
about how to handle corner cases like that if we want to try to wipe out
all the warnings of this class.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Tom Lane wrote:
> >> (Whatcha gonna do if you fail to write the postmaster log ... try to
> >> log a complaint message?)
>
> > Well, good question, but I'm not convinced that doing nothing is always
> > the best answer.
>
> I didn't say it was --- merely opined that we need a consistent plan
> about how to handle corner cases like that if we want to try to wipe out
> all the warnings of this class.

Maybe syslog?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> I didn't say it was --- merely opined that we need a consistent plan
>> about how to handle corner cases like that if we want to try to wipe out
>> all the warnings of this class.

> Maybe syslog?

Doesn't exist on Windows ...

regards, tom lane

>> Maybe syslog?
>
> Doesn't exist on Windows ...

There is an event log though which is fairly comparable. Of course
that still leaves you with the problem of what to do when the syslog
or event log write fails.

...Robert

Robert Haas escribió:
> >> Maybe syslog?
> >
> > Doesn't exist on Windows ...
>
> There is an event log though which is fairly comparable. Of course
> that still leaves you with the problem of what to do when the syslog
> or event log write fails.

Well, syslog does not have a return code, which probably means that it
can't fail (and if it does, there's no way for you to know).

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Magnus Hagander wrote:
> Heikki Linnakangas wrote:
>> Magnus Hagander wrote:
>>> Log Message:
>>> -----------
>>> Silence compiler warning about ignored return value. Our comment already
>>> clearly stated that we are aware that we're ignoring it.
>> I think the usual way is to call the function like:
>>
>> (void) function_with_return_value()
>
> I tried that first, of course. gcc is too smart about that - it still
> throws the warning in this case.

I have equipped this part with a proper error message now. At the time
I was thinking this is a "can't happen" error, but it has in fact
already helped clarifying some other regression test run failures for me:

./pg_regress --inputdir=. --dlpath=. --multibyte=SQL_ASCII
--load-language=plpgsql --temp-install=./tmp_check
--top-builddir=../../.. --temp-port=565432 --schedule=./parallel_schedule
============== removing existing temp installation ==============
============== creating temporary installation ==============
============== initializing database system ==============
============== starting postmaster ==============
running on port 65432 with pid 44940
============== creating database "regression" ==============
ERROR: database "regression" already exists
command failed:
"/Users/peter/devel/postgresql/cvs/pg84/pgsql/src/test/regress/./tmp_check/install//Users/peter/devel/postgresql/cvs/pg84/pg-install/bin/psql"
-X -c "CREATE DATABASE \"regression\" TEMPLATE=template0
ENCODING='SQL_ASCII'" "postgres"
pg_ctl: server does not shut down

pg_regress: could not stop postmaster: exit code was 256 <<< NEW
make: *** [check] Error 2