Re: GCC 7 warnings

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: GCC 7 warnings
Date: 2017-04-12 04:12:10
Message-ID: 24498.1491970330@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> Attached is a more refined patch that I propose for PG10 now. Compared
> to the previous rushed version, this one uses some more precise
> arithmetic to size some of the buffers.

Generally +1 for switching the snprintf calls to use sizeof() rather
than repeating the declared sizes of the arrays.

The change in setup_formatted_log_time seems a bit weird:

- char msbuf[8];
+ char msbuf[10];

The associated call is

sprintf(msbuf, ".%03d", (int) (saved_timeval.tv_usec / 1000));

Now a human can see that saved_timeval.tv_usec must be 0..999999, so
that the %d format item must always emit exactly 3 characters, which
means that really 5 bytes would be enough. I wouldn't expect a
compiler to know that, but if it's making a generic assumption about
the worst-case width of %d, shouldn't it conclude that we might need
as many as 13 bytes for the buffer? Why does msbuf[10] satisfy it
if msbuf[8] doesn't?

IOW, if we're going to touch this at all, I'd be inclined to go with
msbuf[16] or so, as being more likely to satisfy compilers that have
decided to try to warn about this. And maybe we should use snprintf,
just for luck.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavan Deolasee 2017-04-12 04:23:09 Re: Patch: Write Amplification Reduction Method (WARM)
Previous Message Peter Eisentraut 2017-04-12 04:10:26 Re: logical replication apply to run with sync commit off by default