pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedC99 compliance for src/port/snprintf.c
46+ messages / 11 participants
[nested] [flat]
* C99 compliance for src/port/snprintf.c
@ 2018-08-14 23:28 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-14 23:28 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I noticed that src/port/snprintf.c still follows the old (pre-C99)
semantics for what to return from snprintf() after buffer overrun.
This seems bad for a couple of reasons:
* It results in potential performance loss in psnprintf and
appendPQExpBufferVA, since those have to fly blind about how much
to enlarge the buffer before retrying.
* Given that we override the system-supplied *printf if it doesn't
support the 'z' width modifier, it seems highly unlikely that we are
still using any snprintf's with pre-C99 semantics, except when this
code is used. So there's not much point in keeping this behavior
as a test case to ensure compatibility with such libraries.
* When we install snprintf.c, port.h forces it to be used everywhere
that includes c.h, including extensions. It seems quite possible that
there is extension code out there that assumes C99 snprintf behavior.
Such code would work today everywhere except Windows, since that's the
only platform made in this century that requires snprintf.c. Between
the existing Windows porting hazard and our nearby discussion about
using snprintf.c on more platforms, I'd say this is a gotcha waiting
to bite somebody.
Hence, PFA a patch that changes snprintf.c to follow C99 by counting
dropped bytes in the result of snprintf(), including in the case where
the passed count is zero.
(I also dropped the tests for str == NULL when count isn't zero; that's
not permitted by either C99 or SUSv2, so I see no reason for this code
to support it. Also, avoid wasting one byte in the local buffer for
*fprintf.)
I'm almost tempted to think that the reasons above make this a
back-patchable bug fix. Comments?
regards, tom lane
Attachments:
[text/x-diff] c99-compliant-snprintf-result-1.patch (9.0K, ../../17245.1534289329@sss.pgh.pa.us/2-c99-compliant-snprintf-result-1.patch)
download | inline diff:
diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index a184134..851e2ae 100644
*** a/src/port/snprintf.c
--- b/src/port/snprintf.c
***************
*** 2,7 ****
--- 2,8 ----
* Copyright (c) 1983, 1995, 1996 Eric P. Allman
* Copyright (c) 1988, 1993
* The Regents of the University of California. All rights reserved.
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
***************
*** 49,56 ****
* SNPRINTF, VSNPRINTF and friends
*
* These versions have been grabbed off the net. They have been
! * cleaned up to compile properly and support for most of the Single Unix
! * Specification has been added. Remaining unimplemented features are:
*
* 1. No locale support: the radix character is always '.' and the '
* (single quote) format flag is ignored.
--- 50,57 ----
* SNPRINTF, VSNPRINTF and friends
*
* These versions have been grabbed off the net. They have been
! * cleaned up to compile properly and support for most of the C99
! * specification has been added. Remaining unimplemented features are:
*
* 1. No locale support: the radix character is always '.' and the '
* (single quote) format flag is ignored.
***************
*** 64,88 ****
* 5. Space and '#' flags are not implemented.
*
*
! * The result values of these functions are not the same across different
! * platforms. This implementation is compatible with the Single Unix Spec:
*
! * 1. -1 is returned only if processing is abandoned due to an invalid
! * parameter, such as incorrect format string. (Although not required by
! * the spec, this happens only when no characters have yet been transmitted
! * to the destination.)
*
! * 2. For snprintf and sprintf, 0 is returned if str == NULL or count == 0;
! * no data has been stored.
*
! * 3. Otherwise, the number of bytes actually transmitted to the destination
! * is returned (excluding the trailing '\0' for snprintf and sprintf).
*
! * For snprintf with nonzero count, the result cannot be more than count-1
! * (a trailing '\0' is always stored); it is not possible to distinguish
! * buffer overrun from exact fit. This is unlike some implementations that
! * return the number of bytes that would have been needed for the complete
! * result string.
*/
/**************************************************************
--- 65,88 ----
* 5. Space and '#' flags are not implemented.
*
*
! * Historically the result values of sprintf/snprintf varied across platforms.
! * This implementation now follows the C99 standard:
*
! * 1. -1 is returned if an error is detected in the format string, or if
! * a write to the target stream fails (as reported by fwrite). Note that
! * overrunning snprintf's target buffer is *not* an error.
*
! * 2. For successful writes to streams, the actual number of bytes written
! * to the stream is returned.
*
! * 3. For successful sprintf/snprintf, the number of bytes that would have
! * been written to an infinite-size buffer (excluding the trailing '\0')
! * is returned. snprintf will truncate its output to fit in the buffer
! * (ensuring a trailing '\0' unless count == 0), but this is not reflected
! * in the function result.
*
! * snprintf buffer overrun can be detected by checking for function result
! * greater than or equal to the supplied count.
*/
/**************************************************************
***************
*** 101,115 ****
#undef fprintf
#undef printf
! /* Info about where the formatted output is going */
typedef struct
{
char *bufptr; /* next buffer output position */
char *bufstart; /* first buffer element */
! char *bufend; /* last buffer element, or NULL */
/* bufend == NULL is for sprintf, where we assume buf is big enough */
FILE *stream; /* eventual output destination, or NULL */
! int nchars; /* # chars already sent to stream */
bool failed; /* call is a failure; errno is set */
} PrintfTarget;
--- 101,127 ----
#undef fprintf
#undef printf
! /*
! * Info about where the formatted output is going.
! *
! * dopr and subroutines will not write at/past bufend, but snprintf
! * reserves one byte, ensuring it may place the trailing '\0' there.
! *
! * In snprintf, we use nchars to count the number of bytes dropped on the
! * floor due to buffer overrun. The correct result of snprintf is thus
! * (bufptr - bufstart) + nchars. (This isn't as inconsistent as it might
! * seem: nchars is the number of emitted bytes that are not in the buffer now,
! * either because we sent them to the stream or because we couldn't fit them
! * into the buffer to begin with.)
! */
typedef struct
{
char *bufptr; /* next buffer output position */
char *bufstart; /* first buffer element */
! char *bufend; /* last+1 buffer element, or NULL */
/* bufend == NULL is for sprintf, where we assume buf is big enough */
FILE *stream; /* eventual output destination, or NULL */
! int nchars; /* # chars sent to stream, or dropped */
bool failed; /* call is a failure; errno is set */
} PrintfTarget;
*************** int
*** 147,163 ****
pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
{
PrintfTarget target;
! if (str == NULL || count == 0)
! return 0;
target.bufstart = target.bufptr = str;
target.bufend = str + count - 1;
target.stream = NULL;
! /* target.nchars is unused in this case */
target.failed = false;
dopr(&target, fmt, args);
*(target.bufptr) = '\0';
! return target.failed ? -1 : (target.bufptr - target.bufstart);
}
int
--- 159,186 ----
pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
{
PrintfTarget target;
+ char onebyte[1];
! /*
! * C99 allows the case str == NULL when count == 0. Rather than
! * special-casing this situation further down, we substitute a one-byte
! * local buffer. Callers cannot tell, since the function result doesn't
! * depend on count.
! */
! if (count == 0)
! {
! str = onebyte;
! count = 1;
! }
target.bufstart = target.bufptr = str;
target.bufend = str + count - 1;
target.stream = NULL;
! target.nchars = 0;
target.failed = false;
dopr(&target, fmt, args);
*(target.bufptr) = '\0';
! return target.failed ? -1 : (target.bufptr - target.bufstart
! + target.nchars);
}
int
*************** pg_vsprintf(char *str, const char *fmt,
*** 177,192 ****
{
PrintfTarget target;
- if (str == NULL)
- return 0;
target.bufstart = target.bufptr = str;
target.bufend = NULL;
target.stream = NULL;
! /* target.nchars is unused in this case */
target.failed = false;
dopr(&target, fmt, args);
*(target.bufptr) = '\0';
! return target.failed ? -1 : (target.bufptr - target.bufstart);
}
int
--- 200,214 ----
{
PrintfTarget target;
target.bufstart = target.bufptr = str;
target.bufend = NULL;
target.stream = NULL;
! target.nchars = 0; /* not really used in this case */
target.failed = false;
dopr(&target, fmt, args);
*(target.bufptr) = '\0';
! return target.failed ? -1 : (target.bufptr - target.bufstart
! + target.nchars);
}
int
*************** pg_vfprintf(FILE *stream, const char *fm
*** 213,219 ****
return -1;
}
target.bufstart = target.bufptr = buffer;
! target.bufend = buffer + sizeof(buffer) - 1;
target.stream = stream;
target.nchars = 0;
target.failed = false;
--- 235,241 ----
return -1;
}
target.bufstart = target.bufptr = buffer;
! target.bufend = buffer + sizeof(buffer); /* use the whole buffer */
target.stream = stream;
target.nchars = 0;
target.failed = false;
*************** flushbuffer(PrintfTarget *target)
*** 256,261 ****
--- 278,287 ----
{
size_t nc = target->bufptr - target->bufstart;
+ /*
+ * Don't write anything if we already failed; this is to ensure we
+ * preserve the original failure's errno.
+ */
if (!target->failed && nc > 0)
{
size_t written;
*************** dostr(const char *str, int slen, PrintfT
*** 1035,1041 ****
{
/* buffer full, can we dump to stream? */
if (target->stream == NULL)
! return; /* no, lose the data */
flushbuffer(target);
continue;
}
--- 1061,1070 ----
{
/* buffer full, can we dump to stream? */
if (target->stream == NULL)
! {
! target->nchars += slen; /* no, lose the data */
! return;
! }
flushbuffer(target);
continue;
}
*************** dopr_outch(int c, PrintfTarget *target)
*** 1054,1060 ****
{
/* buffer full, can we dump to stream? */
if (target->stream == NULL)
! return; /* no, lose the data */
flushbuffer(target);
}
*(target->bufptr++) = c;
--- 1083,1092 ----
{
/* buffer full, can we dump to stream? */
if (target->stream == NULL)
! {
! target->nchars++; /* no, lose the data */
! return;
! }
flushbuffer(target);
}
*(target->bufptr++) = c;
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 14:58 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 46+ messages in thread
From: Robert Haas @ 2018-08-15 14:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Aug 14, 2018 at 7:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm almost tempted to think that the reasons above make this a
> back-patchable bug fix. Comments?
No objections to changing the behavior. Have you checked whether
there are any noticeable performance consequences?
Back-patching seems a bit aggressive to me considering that the danger
is hypothetical. I'd want to have some tangible evidence that
back-patching was going help somebody. For all we know somebody's got
an extension which they only use on Windows that happens to be relying
on the current behavior, although more likely still (IMHO) is that it
there is little or no code relying on either behavior.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 15:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 2 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 15:41 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 14, 2018 at 7:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm almost tempted to think that the reasons above make this a
>> back-patchable bug fix. Comments?
> No objections to changing the behavior. Have you checked whether
> there are any noticeable performance consequences?
Hard to believe there could be any performance loss. The normal (non
buffer overflowing) code path gets two additional instructions, storing
zero into .nchars and then adding it to the result. There would be more
work added to buffer-overflow cases, but we'd surely more than win that
back by avoiding repeated repalloc-and-try-again cycles.
> Back-patching seems a bit aggressive to me considering that the danger
> is hypothetical. I'd want to have some tangible evidence that
> back-patching was going help somebody. For all we know somebody's got
> an extension which they only use on Windows that happens to be relying
> on the current behavior, although more likely still (IMHO) is that it
> there is little or no code relying on either behavior.
Yeah, it's theoretically possible that there's somebody out there who's
depending on the pre-C99 behavior and never tested their code anywhere
but Windows. But come on, how likely is that really, compared to the
much more plausible risks I already cited? It's not even that easy
to imagine how someone would construct code that had such a dependency
while not being outright buggy in the face of buffer overrun.
BTW, independently of whether to back-patch, it strikes me that what
we ought to do in HEAD (after applying this) is to just assume we have
C99-compliant behavior, and rip out the baroque logic in psnprintf
and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.
I don't expect that'd save any really meaningful number of cycles,
but at least it'd buy back the two added instructions mentioned above.
I suppose we could put in a configure check that verifies whether
the system snprintf returns the right value for overrun cases, though
it's hard to believe there are any platforms that pass the 'z' check
and would fail this one.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 15:52 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 46+ messages in thread
From: Andres Freund @ 2018-08-15 15:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2018-08-15 11:41:46 -0400, Tom Lane wrote:
> BTW, independently of whether to back-patch, it strikes me that what
> we ought to do in HEAD (after applying this) is to just assume we have
> C99-compliant behavior, and rip out the baroque logic in psnprintf
> and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.
> I don't expect that'd save any really meaningful number of cycles,
> but at least it'd buy back the two added instructions mentioned above.
> I suppose we could put in a configure check that verifies whether
> the system snprintf returns the right value for overrun cases, though
> it's hard to believe there are any platforms that pass the 'z' check
> and would fail this one.
We could just mandate C99, more generally.
/me goes and hides in a bush.
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 15:56 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 46+ messages in thread
From: Alvaro Herrera @ 2018-08-15 15:56 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 2018-Aug-15, Robert Haas wrote:
> On Tue, Aug 14, 2018 at 7:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm almost tempted to think that the reasons above make this a
> > back-patchable bug fix. Comments?
>
> No objections to changing the behavior. Have you checked whether
> there are any noticeable performance consequences?
>
> Back-patching seems a bit aggressive to me considering that the danger
> is hypothetical.
That was my first thought too, and my preferred path would be to make
this master-only and only consider a backpatch later if we find some
practical reason to do so.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 16:01 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 46+ messages in thread
From: Robert Haas @ 2018-08-15 16:01 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Aug 15, 2018 at 11:52 AM, Andres Freund <andres@anarazel.de> wrote:
> We could just mandate C99, more generally.
>
> /me goes and hides in a bush.
It's hard to believe that would cost much. Personally, I'd prefer to
continue avoiding // comments and intermingled declarations of
variables and code on grounds of style and readability. But it's kind
of difficult to believe that we really need to worry about people
still running 20-year old compilers very much. My first exposure to
Tom arguing that we ought to continue supporting C89 was about a
decade ago, but the argument that people might still have older
systems in service was a lot more plausible then than it is now.
BTW, I think a bush is probably not a nearly sufficient place to hide.
The wrath of Tom will find you wherever you may go... :-)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 16:07 Stephen Frost <sfrost@snowman.net>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 46+ messages in thread
From: Stephen Frost @ 2018-08-15 16:07 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Greetings,
* Andres Freund (andres@anarazel.de) wrote:
> On 2018-08-15 11:41:46 -0400, Tom Lane wrote:
> > BTW, independently of whether to back-patch, it strikes me that what
> > we ought to do in HEAD (after applying this) is to just assume we have
> > C99-compliant behavior, and rip out the baroque logic in psnprintf
> > and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.
> > I don't expect that'd save any really meaningful number of cycles,
> > but at least it'd buy back the two added instructions mentioned above.
> > I suppose we could put in a configure check that verifies whether
> > the system snprintf returns the right value for overrun cases, though
> > it's hard to believe there are any platforms that pass the 'z' check
> > and would fail this one.
>
> We could just mandate C99, more generally.
*cough* +1 *cough*
> /me goes and hides in a bush.
/me runs
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20180815160740.GD3326@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 16:11 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 46+ messages in thread
From: Andres Freund @ 2018-08-15 16:11 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2018-08-15 12:01:28 -0400, Robert Haas wrote:
> On Wed, Aug 15, 2018 at 11:52 AM, Andres Freund <andres@anarazel.de> wrote:
> > We could just mandate C99, more generally.
> >
> > /me goes and hides in a bush.
>
> It's hard to believe that would cost much.
Yea.
> Personally, I'd prefer to continue avoiding // comments and
> intermingled declarations of variables and code on grounds of style
> and readability.
I don't really care much about either. The calculus for intermingled
declarations would personally change the minute that we decided to allow
some C++ - allowing for scoped locks etc via RAII - but not earlier.
The thing I'd really want is designated initializers for structs. Makes
code for statically allocated structs *so* much more readable. And guess
who's working on code that adds statically allocated structs with lots
of members...
> BTW, I think a bush is probably not a nearly sufficient place to hide.
> The wrath of Tom will find you wherever you may go... :-)
That's why I keep moving. At 200 mph on a train :P. A continent away.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 16:17 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 2 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 16:17 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 15, 2018 at 11:52 AM, Andres Freund <andres@anarazel.de> wrote:
>> We could just mandate C99, more generally.
>>
>> /me goes and hides in a bush.
> It's hard to believe that would cost much.
I think we have done that, piece by piece, where it was actually buying us
something. In particular we've gradually moved the goalposts for *printf
compliance, and I'm proposing here to move them a bit further. I'm not
sure what "we're going to insist on C99" even means concretely, given
this position ...
> Personally, I'd prefer to
> continue avoiding // comments and intermingled declarations of
> variables and code on grounds of style and readability.
... which I agree with.
> But it's kind
> of difficult to believe that we really need to worry about people
> still running 20-year old compilers very much.
Sure. It's been a long time since anybody worried about those as
optimization targets, for instance. Still, I'm not in favor of
actively breaking compatibility unless it buys us something.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 17:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-15 17:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Aug-15, Robert Haas wrote:
>> Back-patching seems a bit aggressive to me considering that the danger
>> is hypothetical.
> That was my first thought too, and my preferred path would be to make
> this master-only and only consider a backpatch later if we find some
> practical reason to do so.
Meh --- the hazards of back-patching seem to me to be more hypothetical
than the benefits. Still, I seem to be in the minority, so I withdraw
the proposal to back-patch.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 18:03 David Steele <david@pgmasters.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 46+ messages in thread
From: David Steele @ 2018-08-15 18:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Stephen Frost <sfrost@snowman.net>
On 8/15/18 12:17 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>
>> Personally, I'd prefer to
>> continue avoiding // comments and intermingled declarations of
>> variables and code on grounds of style and readability.
>
> ... which I agree with.
We already have -Wdeclaration-after-statement to prevent mixed
declarations. Not sure what to do about comments except manual enforcement.
>> But it's kind
>> of difficult to believe that we really need to worry about people
>> still running 20-year old compilers very much.
>
> Sure. It's been a long time since anybody worried about those as
> optimization targets, for instance. Still, I'm not in favor of
> actively breaking compatibility unless it buys us something.
We use C99 for the pgBackRest project and we've found designated
initializers, compound declarations, and especially variadic macros to
be very helpful. Only the latter really provides new functionality but
simplifying and clarifying code is always a bonus.
So, +1 from me!
Regards,
--
-David
david@pgmasters.net
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 18:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-15 18:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> Meh --- the hazards of back-patching seem to me to be more hypothetical
> than the benefits. Still, I seem to be in the minority, so I withdraw
> the proposal to back-patch.
Actually, after digging around a bit, I'm excited about this again.
There are only a couple dozen places in our tree that pay any attention
to the result of (v)snprintf, but with the exception of psnprintf,
appendPQExpBufferVA, and one or two other places, *they're all assuming
C99 semantics*, and will fail to detect buffer overflow with the pre-C99
behavior.
Probably a lot of these are not live bugs because buffer overrun is
not ever going to occur in practice. But at least pg_upgrade and
pg_regress are constructing command strings including externally
supplied paths, so overrun doesn't seem impossible. If it happened,
they'd merrily proceed to execute a truncated command.
If we don't backpatch the snprintf change, we're morally obliged to
back-patch some other fix for these places. At least one of them,
in plperl's pport.h, is not our code and so changing it seems like
a bad idea.
Still want to argue for no backpatch?
regards, tom lane
PS: I also found a couple of places that are just wrong regardless
of semantics: they're checking overflow by "result > bufsize", not
"result >= bufsize". Will fix those in any case.
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 18:09 Tom Lane <tgl@sss.pgh.pa.us>
parent: David Steele <david@pgmasters.net>
0 siblings, 0 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 18:09 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Stephen Frost <sfrost@snowman.net>
David Steele <david@pgmasters.net> writes:
> On 8/15/18 12:17 PM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Personally, I'd prefer to
>>> continue avoiding // comments and intermingled declarations of
>>> variables and code on grounds of style and readability.
>> ... which I agree with.
> We already have -Wdeclaration-after-statement to prevent mixed
> declarations. Not sure what to do about comments except manual enforcement.
pgindent will change // comments to /* style, so at least on the timescale
of a release cycle, we have enforcement for that.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 18:59 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 46+ messages in thread
From: Andrew Dunstan @ 2018-08-15 18:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 08/15/2018 12:17 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>
>> Personally, I'd prefer to
>> continue avoiding // comments and intermingled declarations of
>> variables and code on grounds of style and readability.
> ... which I agree with.
>
>
A decade or so ago I would have strongly agreed with you. But the
language trend seems to be in the other direction. And there is
something to be said for declaration near use without having to use an
inner block. I'm not advocating that we change policy, however.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 19:18 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
0 siblings, 2 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 19:18 UTC (permalink / raw)
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 08/15/2018 12:17 PM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Personally, I'd prefer to
>>> continue avoiding // comments and intermingled declarations of
>>> variables and code on grounds of style and readability.
>> ... which I agree with.
> A decade or so ago I would have strongly agreed with you. But the
> language trend seems to be in the other direction. And there is
> something to be said for declaration near use without having to use an
> inner block. I'm not advocating that we change policy, however.
FWIW, the issue I've got with what C99 did is that you can narrow the
*start* of the scope of a local variable easily, but not the *end* of
its scope, which seems to me to be solving at most half of the problem.
To solve the whole problem, you end up needing a nested block anyway.
I do dearly miss the ability to easily limit the scope of a loop's
control variable to just the loop, eg
for (int i = 0; ...) { ... }
But AFAIK that's C++ not C99.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 19:21 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 46+ messages in thread
From: Andrew Dunstan @ 2018-08-15 19:21 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 08/15/2018 03:18 PM, Tom Lane wrote:
>
> FWIW, the issue I've got with what C99 did is that you can narrow the
> *start* of the scope of a local variable easily, but not the *end* of
> its scope, which seems to me to be solving at most half of the problem.
> To solve the whole problem, you end up needing a nested block anyway.
>
> I do dearly miss the ability to easily limit the scope of a loop's
> control variable to just the loop, eg
>
> for (int i = 0; ...) { ... }
>
> But AFAIK that's C++ not C99.
>
>
Agree completely.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 19:37 David Steele <david@pgmasters.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 46+ messages in thread
From: David Steele @ 2018-08-15 19:37 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
On 8/15/18 3:18 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 08/15/2018 12:17 PM, Tom Lane wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> Personally, I'd prefer to
>>>> continue avoiding // comments and intermingled declarations of
>>>> variables and code on grounds of style and readability.
>
>>> ... which I agree with.
>
>> A decade or so ago I would have strongly agreed with you. But the
>> language trend seems to be in the other direction. And there is
>> something to be said for declaration near use without having to use an
>> inner block. I'm not advocating that we change policy, however.
>
> FWIW, the issue I've got with what C99 did is that you can narrow the
> *start* of the scope of a local variable easily, but not the *end* of
> its scope, which seems to me to be solving at most half of the problem.
> To solve the whole problem, you end up needing a nested block anyway.
>
> I do dearly miss the ability to easily limit the scope of a loop's
> control variable to just the loop, eg
>
> for (int i = 0; ...) { ... }
>
> But AFAIK that's C++ not C99.
This works in C99 -- and I'm a really big fan.
--
-David
david@pgmasters.net
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 19:57 Tom Lane <tgl@sss.pgh.pa.us>
parent: David Steele <david@pgmasters.net>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-15 19:57 UTC (permalink / raw)
To: David Steele <david@pgmasters.net>; +Cc: pgsql-hackers@lists.postgresql.org
David Steele <david@pgmasters.net> writes:
> On 8/15/18 3:18 PM, Tom Lane wrote:
>> I do dearly miss the ability to easily limit the scope of a loop's
>> control variable to just the loop, eg
>> for (int i = 0; ...) { ... }
>> But AFAIK that's C++ not C99.
> This works in C99 -- and I'm a really big fan.
It does? [ checks standard... ] Oh wow:
6.8.5 Iteration statements
Syntax
iteration-statement:
while ( expression ) statement
do statement while ( expression ) ;
for ( expr-opt ; expr-opt ; expr-opt ) statement
for ( declaration ; expr-opt ; expr-opt ) statement
I'd always thought this was only in C++. This alone might be a sufficient
reason to drop C89 compiler support ...
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 21:49 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-15 21:49 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-15 15:57:43 -0400, Tom Lane wrote:
> I'd always thought this was only in C++. This alone might be a sufficient
> reason to drop C89 compiler support ...
It's also IIRC reasonably widely supported from before C99. So, for the
sake of designated initializers, for loop scoping, snprintf, let's do
this in master?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 21:52 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-15 21:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 2018-08-15 14:05:29 -0400, Tom Lane wrote:
> I wrote:
> > Meh --- the hazards of back-patching seem to me to be more hypothetical
> > than the benefits. Still, I seem to be in the minority, so I withdraw
> > the proposal to back-patch.
>
> Actually, after digging around a bit, I'm excited about this again.
> There are only a couple dozen places in our tree that pay any attention
> to the result of (v)snprintf, but with the exception of psnprintf,
> appendPQExpBufferVA, and one or two other places, *they're all assuming
> C99 semantics*, and will fail to detect buffer overflow with the pre-C99
> behavior.
>
> Probably a lot of these are not live bugs because buffer overrun is
> not ever going to occur in practice. But at least pg_upgrade and
> pg_regress are constructing command strings including externally
> supplied paths, so overrun doesn't seem impossible. If it happened,
> they'd merrily proceed to execute a truncated command.
>
> If we don't backpatch the snprintf change, we're morally obliged to
> back-patch some other fix for these places. At least one of them,
> in plperl's pport.h, is not our code and so changing it seems like
> a bad idea.
>
> Still want to argue for no backpatch?
>
> regards, tom lane
>
> PS: I also found a couple of places that are just wrong regardless
> of semantics: they're checking overflow by "result > bufsize", not
> "result >= bufsize". Will fix those in any case.
I'm a bit confused. Why did you just backpatch this ~two hours after
people objected to the idea? Even if it were during my current work
hours, I don't even read mail that often if I'm hacking on something
complicated.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-15 22:13 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-15 15:57:43 -0400, Tom Lane wrote:
>> I'd always thought this was only in C++. This alone might be a sufficient
>> reason to drop C89 compiler support ...
> It's also IIRC reasonably widely supported from before C99. So, for the
> sake of designated initializers, for loop scoping, snprintf, let's do
> this in master?
Nitpick: snprintf is an independent concern: that's from the C library,
not from the compiler. To drive the point home, I could still test
master on "gaur" if I were to install a just-slightly-newer gcc on that
machine (its existing gcc installation isn't native either...); but
replacing its libc is a nonstarter.
Experimenting here says that even reasonably modern gcc's won't take
declarations-inside-for without "--std=c99" or such. No idea about
other compilers. So we'd have a little bit of work to do on
configuration before we could open the floodgates on this.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:18 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 22:18 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-15 14:05:29 -0400, Tom Lane wrote:
>> Still want to argue for no backpatch?
> I'm a bit confused. Why did you just backpatch this ~two hours after
> people objected to the idea? Even if it were during my current work
> hours, I don't even read mail that often if I'm hacking on something
> complicated.
If a consensus emerges to deal with this some other way, reverting
isn't hard. But I think it's pretty clear at this point that we're
dealing with real bugs versus entirely hypothetical bugs, and that's
not a decision that's hard to make IMO.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:24 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-15 22:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-15 18:13:59 -0400, Tom Lane wrote:
> Experimenting here says that even reasonably modern gcc's won't take
> declarations-inside-for without "--std=c99" or such. No idea about
> other compilers. So we'd have a little bit of work to do on
> configuration before we could open the floodgates on this.
I think autoconf's magic knows about most of that:
— Macro: AC_PROG_CC_C99
If the C compiler is not in C99 mode by default, try to add an
option to output variable CC to make it so. This macro tries various
options that select C99 on some system or another. It considers the
compiler to be in C99 mode if it handles _Bool, flexible arrays,
inline, long long int, mixed code and declarations, named
initialization of structs, restrict, varargs macros, variable
declarations in for loops and variable length arrays.
After calling this macro you can check whether the C compiler has
been set to accept C99; if not, the shell variable ac_cv_prog_cc_c99
is set to ‘no’. ~
I think we could get a start by adding that test to configure, without
relying on it for now (i.e. keeping mylodon with -Wc99-extensions
-Werror=c99-extensions alive). That'd tell us about which machines,
besides presumably gaur, we'd need to either kick to the curb or change.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:31 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 22:31 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-15 18:13:59 -0400, Tom Lane wrote:
>> Experimenting here says that even reasonably modern gcc's won't take
>> declarations-inside-for without "--std=c99" or such.
> I think autoconf's magic knows about most of that:
> — Macro: AC_PROG_CC_C99
Ah, of course. What about the MSVC build?
> I think we could get a start by adding that test to configure, without
> relying on it for now (i.e. keeping mylodon with -Wc99-extensions
> -Werror=c99-extensions alive). That'd tell us about which machines,
> besides presumably gaur, we'd need to either kick to the curb or change.
Sure, no objection to putting that in just to see how much of the
buildfarm can handle it. If the answer turns out to be "a lot",
we might have to reconsider, but gathering data seems like the
first thing to do.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:40 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-15 22:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-15 18:31:10 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-08-15 18:13:59 -0400, Tom Lane wrote:
> >> Experimenting here says that even reasonably modern gcc's won't take
> >> declarations-inside-for without "--std=c99" or such.
>
> > I think autoconf's magic knows about most of that:
> > — Macro: AC_PROG_CC_C99
>
> Ah, of course. What about the MSVC build?
It looks like it mostly just enables that by default. But I only looked
cursorily. It's a bit annoying because that makes it harder to be sure
which animals support what. Looks like e.g. hammerkop (supposedly msvc
2005) might not support the subset we want; not that I'd loose sleep
over raising the minimum msvc in master a bit.
> > I think we could get a start by adding that test to configure, without
> > relying on it for now (i.e. keeping mylodon with -Wc99-extensions
> > -Werror=c99-extensions alive). That'd tell us about which machines,
> > besides presumably gaur, we'd need to either kick to the curb or change.
>
> Sure, no objection to putting that in just to see how much of the
> buildfarm can handle it. If the answer turns out to be "a lot",
> we might have to reconsider, but gathering data seems like the
> first thing to do.
Cool. Too late today (in Europe for a few more days), but I'll try to
come up with something tomorrow.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 22:54 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 46+ messages in thread
From: Thomas Munro @ 2018-08-15 22:54 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Aug 16, 2018 at 10:40 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2018-08-15 18:31:10 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>> > On 2018-08-15 18:13:59 -0400, Tom Lane wrote:
>> >> Experimenting here says that even reasonably modern gcc's won't take
>> >> declarations-inside-for without "--std=c99" or such.
>>
>> > I think autoconf's magic knows about most of that:
>> > — Macro: AC_PROG_CC_C99
>>
>> Ah, of course. What about the MSVC build?
>
> It looks like it mostly just enables that by default. But I only looked
> cursorily. It's a bit annoying because that makes it harder to be sure
> which animals support what. Looks like e.g. hammerkop (supposedly msvc
> 2005) might not support the subset we want; not that I'd loose sleep
> over raising the minimum msvc in master a bit.
Really? I am not an MSVC user but I had the impression that their C
mode (/TC or files named .c) was stuck on C89/C90 as a matter of
policy, as Herb Sutter explained here (though maybe the situation has
changed since then):
https://herbsutter.com/2012/05/03/reader-qa-what-about-vc-and-c99/
That's presumably why cfbot's appveyor build always complains about
people declaring variables after statements. To allow that particular
language feature, it looks like you have to tell it that your .c file
is really a C++ program with /TP. But that opens a separate can of
worms, doesn't it?
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 23:00 Tom Lane <tgl@sss.pgh.pa.us>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
1 sibling, 0 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-15 23:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Really? I am not an MSVC user but I had the impression that their C
> mode (/TC or files named .c) was stuck on C89/C90 as a matter of
> policy, as Herb Sutter explained here (though maybe the situation has
> changed since then):
> https://herbsutter.com/2012/05/03/reader-qa-what-about-vc-and-c99/
Hm, I read that and what it says is that they don't plan to support
C99 features that aren't also in C++. But this one surely is.
How you turn it on (without enabling all of C++) is not very clear
though.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-15 23:06 Andres Freund <andres@anarazel.de>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
1 sibling, 2 replies; 46+ messages in thread
From: Andres Freund @ 2018-08-15 23:06 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2018-08-16 10:54:01 +1200, Thomas Munro wrote:
> Really? I am not an MSVC user but I had the impression that their C
> mode (/TC or files named .c) was stuck on C89/C90 as a matter of
> policy, as Herb Sutter explained here (though maybe the situation has
> changed since then):
They revised their position gradually, starting soon after. They claim
full C99 "language" (vs library, which is also pretty complete)
compliance now. I've not yet bothered to fully figure out which version
supports what however. Nor am I really sure about the whole flag thing,
it appears there's a gui element to choose, which we might need to mirror on
the xml level.
A bit of googling shows
https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2013/hh409293(v=vs.120)
"
Supports these ISO C99 language features:
_Bool
Compound literals.
Designated initializers.
Mixing declarations with code.
"
which I think is what we roughly would want. So it looks like msvc 2013
might be the relevant requirement.
> That's presumably why cfbot's appveyor build always complains about
> people declaring variables after statements.
IIRC even in older versions there's a flag to allow that.
> To allow that particular language feature, it looks like you have to
> tell it that your .c file is really a C++ program with /TP. But that
> opens a separate can of worms, doesn't it?
Yea, I don't think we want to go there by default soon.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 00:15 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 46+ messages in thread
From: Tom Lane @ 2018-08-16 00:15 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> BTW, independently of whether to back-patch, it strikes me that what
> we ought to do in HEAD (after applying this) is to just assume we have
> C99-compliant behavior, and rip out the baroque logic in psnprintf
> and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.
Here's a proposed patch for that. It also gets rid of some ancient
code that tried to deal with snprintfs that were outright broken,
such as writing past the end of the specified buffer. Even if anyone
is still using platforms where that's a problem, I'd expect that we'd
have rejected the system snprintf thanks to configure's feature checks.
regards, tom lane
Attachments:
[text/x-diff] assume-snprintf-follows-c99.patch (15.8K, ../../2886.1534378558@sss.pgh.pa.us/2-assume-snprintf-follows-c99.patch)
download | inline diff:
diff --git a/config/c-library.m4 b/config/c-library.m4
index 4446a5b..6bcfcee 100644
*** a/config/c-library.m4
--- b/config/c-library.m4
*************** int main()
*** 238,243 ****
--- 238,270 ----
AC_MSG_RESULT([$pgac_cv_snprintf_size_t_support])
])# PGAC_FUNC_SNPRINTF_SIZE_T_SUPPORT
+ # PGAC_FUNC_SNPRINTF_C99_RESULT
+ # -----------------------------
+ # Determine whether snprintf returns the desired buffer length when
+ # it overruns the actual buffer length. That's required by C99 and POSIX
+ # but ancient platforms don't behave that way, so we must test.
+ #
+ AC_DEFUN([PGAC_FUNC_SNPRINTF_C99_RESULT],
+ [AC_MSG_CHECKING([whether snprintf reports buffer overrun per C99])
+ AC_CACHE_VAL(pgac_cv_snprintf_c99_result,
+ [AC_RUN_IFELSE([AC_LANG_SOURCE([[#include <stdio.h>
+ #include <string.h>
+
+ int main()
+ {
+ char buf[10];
+
+ if (snprintf(buf, sizeof(buf), "12345678901234567890") != 20)
+ return 1;
+ return 0;
+ }]])],
+ [pgac_cv_snprintf_c99_result=yes],
+ [pgac_cv_snprintf_c99_result=no],
+ [pgac_cv_snprintf_c99_result=cross])
+ ])dnl AC_CACHE_VAL
+ AC_MSG_RESULT([$pgac_cv_snprintf_c99_result])
+ ])# PGAC_FUNC_SNPRINTF_C99_RESULT
+
# PGAC_TYPE_LOCALE_T
# ------------------
diff --git a/configure b/configure
index 067fc43..863d07f 100755
*** a/configure
--- b/configure
*************** fi
*** 15965,15971 ****
# Run tests below here
# --------------------
! # Force use of our snprintf if system's doesn't do arg control
# See comment above at snprintf test for details.
if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether snprintf supports argument control" >&5
--- 15965,15971 ----
# Run tests below here
# --------------------
! # For NLS, force use of our snprintf if system's doesn't do arg control.
# See comment above at snprintf test for details.
if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether snprintf supports argument control" >&5
*************** $as_echo "$pgac_cv_snprintf_size_t_suppo
*** 16259,16264 ****
--- 16259,16308 ----
fi
fi
+ # Force use of our snprintf if the system's doesn't handle buffer overrun
+ # as specified by C99.
+ if test "$pgac_need_repl_snprintf" = no; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether snprintf reports buffer overrun per C99" >&5
+ $as_echo_n "checking whether snprintf reports buffer overrun per C99... " >&6; }
+ if ${pgac_cv_snprintf_c99_result+:} false; then :
+ $as_echo_n "(cached) " >&6
+ else
+ if test "$cross_compiling" = yes; then :
+ pgac_cv_snprintf_c99_result=cross
+ else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+ /* end confdefs.h. */
+ #include <stdio.h>
+ #include <string.h>
+
+ int main()
+ {
+ char buf[10];
+
+ if (snprintf(buf, sizeof(buf), "12345678901234567890") != 20)
+ return 1;
+ return 0;
+ }
+ _ACEOF
+ if ac_fn_c_try_run "$LINENO"; then :
+ pgac_cv_snprintf_c99_result=yes
+ else
+ pgac_cv_snprintf_c99_result=no
+ fi
+ rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
+ conftest.$ac_objext conftest.beam conftest.$ac_ext
+ fi
+
+
+ fi
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_snprintf_c99_result" >&5
+ $as_echo "$pgac_cv_snprintf_c99_result" >&6; }
+
+ if test "$pgac_cv_snprintf_c99_result" != yes; then
+ pgac_need_repl_snprintf=yes
+ fi
+ fi
+
# Now we have checked all the reasons to replace snprintf
if test $pgac_need_repl_snprintf = yes; then
diff --git a/configure.in b/configure.in
index 49257e5..1c529e7 100644
*** a/configure.in
--- b/configure.in
*************** for the exact reason.]])],
*** 1805,1811 ****
# Run tests below here
# --------------------
! # Force use of our snprintf if system's doesn't do arg control
# See comment above at snprintf test for details.
if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then
PGAC_FUNC_SNPRINTF_ARG_CONTROL
--- 1805,1811 ----
# Run tests below here
# --------------------
! # For NLS, force use of our snprintf if system's doesn't do arg control.
# See comment above at snprintf test for details.
if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then
PGAC_FUNC_SNPRINTF_ARG_CONTROL
*************** if test "$pgac_need_repl_snprintf" = no;
*** 1857,1862 ****
--- 1857,1871 ----
fi
fi
+ # Force use of our snprintf if the system's doesn't handle buffer overrun
+ # as specified by C99.
+ if test "$pgac_need_repl_snprintf" = no; then
+ PGAC_FUNC_SNPRINTF_C99_RESULT
+ if test "$pgac_cv_snprintf_c99_result" != yes; then
+ pgac_need_repl_snprintf=yes
+ fi
+ fi
+
# Now we have checked all the reasons to replace snprintf
if test $pgac_need_repl_snprintf = yes; then
AC_DEFINE(USE_REPL_SNPRINTF, 1, [Use replacement snprintf() functions.])
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f458c0e..9989d3a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** do_serialize(char **destptr, Size *maxby
*** 9441,9466 ****
if (*maxbytes <= 0)
elog(ERROR, "not enough space to serialize GUC state");
- errno = 0;
-
va_start(vargs, fmt);
n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
va_end(vargs);
! /*
! * Cater to portability hazards in the vsnprintf() return value just like
! * appendPQExpBufferVA() does. Note that this requires an extra byte of
! * slack at the end of the buffer. Since serialize_variable() ends with a
! * do_serialize_binary() rather than a do_serialize(), we'll always have
! * that slack; estimate_variable_size() need not add a byte for it.
! */
! if (n < 0 || n >= *maxbytes - 1)
{
! if (n < 0 && errno != 0 && errno != ENOMEM)
! /* Shouldn't happen. Better show errno description. */
! elog(ERROR, "vsnprintf failed: %m");
! else
! elog(ERROR, "not enough space to serialize GUC state");
}
/* Shift the destptr ahead of the null terminator */
--- 9441,9459 ----
if (*maxbytes <= 0)
elog(ERROR, "not enough space to serialize GUC state");
va_start(vargs, fmt);
n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
va_end(vargs);
! if (n < 0)
{
! /* Shouldn't happen. Better show errno description. */
! elog(ERROR, "vsnprintf failed: %m");
! }
! if (n >= *maxbytes)
! {
! /* This shouldn't happen either, really. */
! elog(ERROR, "not enough space to serialize GUC state");
}
/* Shift the destptr ahead of the null terminator */
diff --git a/src/common/psprintf.c b/src/common/psprintf.c
index b974a99..04465a1 100644
*** a/src/common/psprintf.c
--- b/src/common/psprintf.c
*************** psprintf(const char *fmt,...)
*** 77,83 ****
* pvsnprintf
*
* Attempt to format text data under the control of fmt (an sprintf-style
! * format string) and insert it into buf (which has length len, len > 0).
*
* If successful, return the number of bytes emitted, not counting the
* trailing zero byte. This will always be strictly less than len.
--- 77,83 ----
* pvsnprintf
*
* Attempt to format text data under the control of fmt (an sprintf-style
! * format string) and insert it into buf (which has length len).
*
* If successful, return the number of bytes emitted, not counting the
* trailing zero byte. This will always be strictly less than len.
*************** psprintf(const char *fmt,...)
*** 89,102 ****
* Other error cases do not return, but exit via elog(ERROR) or exit().
* Hence, this shouldn't be used inside libpq.
*
- * This function exists mainly to centralize our workarounds for
- * non-C99-compliant vsnprintf implementations. Generally, any call that
- * pays any attention to the return value should go through here rather
- * than calling snprintf or vsnprintf directly.
- *
* Note that the semantics of the return value are not exactly C99's.
* First, we don't promise that the estimated buffer size is exactly right;
* callers must be prepared to loop multiple times to get the right size.
* Second, we return the recommended buffer size, not one less than that;
* this lets overflow concerns be handled here rather than in the callers.
*/
--- 89,99 ----
* Other error cases do not return, but exit via elog(ERROR) or exit().
* Hence, this shouldn't be used inside libpq.
*
* Note that the semantics of the return value are not exactly C99's.
* First, we don't promise that the estimated buffer size is exactly right;
* callers must be prepared to loop multiple times to get the right size.
+ * (Given a C99-compliant vsnprintf, that won't happen, but it is rumored
+ * that some implementations don't always return the same value ...)
* Second, we return the recommended buffer size, not one less than that;
* this lets overflow concerns be handled here rather than in the callers.
*/
*************** pvsnprintf(char *buf, size_t len, const
*** 105,132 ****
{
int nprinted;
- Assert(len > 0);
-
- errno = 0;
-
- /*
- * Assert check here is to catch buggy vsnprintf that overruns the
- * specified buffer length. Solaris 7 in 64-bit mode is an example of a
- * platform with such a bug.
- */
- #ifdef USE_ASSERT_CHECKING
- buf[len - 1] = '\0';
- #endif
-
nprinted = vsnprintf(buf, len, fmt, args);
! Assert(buf[len - 1] == '\0');
!
! /*
! * If vsnprintf reports an error other than ENOMEM, fail. The possible
! * causes of this are not user-facing errors, so elog should be enough.
! */
! if (nprinted < 0 && errno != 0 && errno != ENOMEM)
{
#ifndef FRONTEND
elog(ERROR, "vsnprintf failed: %m");
--- 102,111 ----
{
int nprinted;
nprinted = vsnprintf(buf, len, fmt, args);
! /* We assume failure means the fmt is bogus, hence hard failure is OK */
! if (unlikely(nprinted < 0))
{
#ifndef FRONTEND
elog(ERROR, "vsnprintf failed: %m");
*************** pvsnprintf(char *buf, size_t len, const
*** 136,177 ****
#endif
}
! /*
! * Note: some versions of vsnprintf return the number of chars actually
! * stored, not the total space needed as C99 specifies. And at least one
! * returns -1 on failure. Be conservative about believing whether the
! * print worked.
! */
! if (nprinted >= 0 && (size_t) nprinted < len - 1)
{
/* Success. Note nprinted does not include trailing null. */
return (size_t) nprinted;
}
- if (nprinted >= 0 && (size_t) nprinted > len)
- {
- /*
- * This appears to be a C99-compliant vsnprintf, so believe its
- * estimate of the required space. (If it's wrong, the logic will
- * still work, but we may loop multiple times.) Note that the space
- * needed should be only nprinted+1 bytes, but we'd better allocate
- * one more than that so that the test above will succeed next time.
- *
- * In the corner case where the required space just barely overflows,
- * fall through so that we'll error out below (possibly after
- * looping).
- */
- if ((size_t) nprinted <= MaxAllocSize - 2)
- return nprinted + 2;
- }
-
/*
! * Buffer overrun, and we don't know how much space is needed. Estimate
! * twice the previous buffer size, but not more than MaxAllocSize; if we
! * are already at MaxAllocSize, choke. Note we use this palloc-oriented
! * overflow limit even when in frontend.
*/
! if (len >= MaxAllocSize)
{
#ifndef FRONTEND
ereport(ERROR,
--- 115,135 ----
#endif
}
! if ((size_t) nprinted < len)
{
/* Success. Note nprinted does not include trailing null. */
return (size_t) nprinted;
}
/*
! * We assume a C99-compliant vsnprintf, so believe its estimate of the
! * required space, and add one for the trailing null. (If it's wrong, the
! * logic will still work, but we may loop multiple times.)
! *
! * Choke if the required space would exceed MaxAllocSize. Note we use
! * this palloc-oriented overflow limit even when in frontend.
*/
! if (unlikely((size_t) nprinted > MaxAllocSize - 1))
{
#ifndef FRONTEND
ereport(ERROR,
*************** pvsnprintf(char *buf, size_t len, const
*** 183,190 ****
#endif
}
! if (len >= MaxAllocSize / 2)
! return MaxAllocSize;
!
! return len * 2;
}
--- 141,145 ----
#endif
}
! return nprinted + 1;
}
diff --git a/src/interfaces/libpq/pqexpbuffer.c b/src/interfaces/libpq/pqexpbuffer.c
index 86b16e6..0814ec6 100644
*** a/src/interfaces/libpq/pqexpbuffer.c
--- b/src/interfaces/libpq/pqexpbuffer.c
*************** appendPQExpBufferVA(PQExpBuffer str, con
*** 295,370 ****
*/
if (str->maxlen > str->len + 16)
{
! /*
! * Note: we intentionally leave one byte unused, as a guard against
! * old broken versions of vsnprintf.
! */
! avail = str->maxlen - str->len - 1;
!
! errno = 0;
nprinted = vsnprintf(str->data + str->len, avail, fmt, args);
/*
! * If vsnprintf reports an error other than ENOMEM, fail.
*/
! if (nprinted < 0 && errno != 0 && errno != ENOMEM)
{
markPQExpBufferBroken(str);
return true;
}
! /*
! * Note: some versions of vsnprintf return the number of chars
! * actually stored, not the total space needed as C99 specifies. And
! * at least one returns -1 on failure. Be conservative about
! * believing whether the print worked.
! */
! if (nprinted >= 0 && (size_t) nprinted < avail - 1)
{
/* Success. Note nprinted does not include trailing null. */
str->len += nprinted;
return true;
}
! if (nprinted >= 0 && (size_t) nprinted > avail)
! {
! /*
! * This appears to be a C99-compliant vsnprintf, so believe its
! * estimate of the required space. (If it's wrong, the logic will
! * still work, but we may loop multiple times.) Note that the
! * space needed should be only nprinted+1 bytes, but we'd better
! * allocate one more than that so that the test above will succeed
! * next time.
! *
! * In the corner case where the required space just barely
! * overflows, fail.
! */
! if (nprinted > INT_MAX - 2)
! {
! markPQExpBufferBroken(str);
! return true;
! }
! needed = nprinted + 2;
! }
! else
{
! /*
! * Buffer overrun, and we don't know how much space is needed.
! * Estimate twice the previous buffer size, but not more than
! * INT_MAX.
! */
! if (avail >= INT_MAX / 2)
! needed = INT_MAX;
! else
! needed = avail * 2;
}
}
else
{
/*
* We have to guess at how much to enlarge, since we're skipping the
! * formatting work.
*/
needed = 32;
}
--- 295,344 ----
*/
if (str->maxlen > str->len + 16)
{
! avail = str->maxlen - str->len;
nprinted = vsnprintf(str->data + str->len, avail, fmt, args);
/*
! * If vsnprintf reports an error, fail (we assume this means there's
! * something wrong with the format string).
*/
! if (unlikely(nprinted < 0))
{
markPQExpBufferBroken(str);
return true;
}
! if ((size_t) nprinted < avail)
{
/* Success. Note nprinted does not include trailing null. */
str->len += nprinted;
return true;
}
! /*
! * We assume a C99-compliant vsnprintf, so believe its estimate of the
! * required space, and add one for the trailing null. (If it's wrong,
! * the logic will still work, but we may loop multiple times.)
! *
! * Choke if the required space would exceed INT_MAX, since str->maxlen
! * can't represent more than that.
! */
! if (unlikely(nprinted > INT_MAX - 1))
{
! markPQExpBufferBroken(str);
! return true;
}
+ needed = nprinted + 1;
}
else
{
/*
* We have to guess at how much to enlarge, since we're skipping the
! * formatting work. Fortunately, because of enlargePQExpBuffer's
! * preference for power-of-2 sizes, this number isn't very sensitive;
! * the net effect is that we'll double the buffer size before trying
! * to run vsnprintf, which seems sensible.
*/
needed = 32;
}
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 00:24 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 46+ messages in thread
From: Thomas Munro @ 2018-08-16 00:24 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Aug 16, 2018 at 11:06 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2018-08-16 10:54:01 +1200, Thomas Munro wrote:
>> Really? I am not an MSVC user but I had the impression that their C
>> mode (/TC or files named .c) was stuck on C89/C90 as a matter of
>> policy, as Herb Sutter explained here (though maybe the situation has
>> changed since then):
>
> They revised their position gradually, starting soon after. They claim
> full C99 "language" (vs library, which is also pretty complete)
> compliance now. I've not yet bothered to fully figure out which version
> supports what however. Nor am I really sure about the whole flag thing,
> it appears there's a gui element to choose, which we might need to mirror on
> the xml level.
Hah, I see. Thanks apparently due to FFmpeg for helping them change
their minds. That seems like a bit of a catch-22 for projects that
care about portability. (Maybe if we start writing C11 they'll change
the compiler to keep up? Actually I already did that once, with an
anonymous union that turned the build farm slightly red...)
https://stackoverflow.com/questions/27826409/what-is-the-official-status-of-c99-support-in-vs2013
> ...
> which I think is what we roughly would want. So it looks like msvc 2013
> might be the relevant requirement.
FWIW cfbot is using Visual Studio 2010 right now. Appveyor provides
2008, 2010, 2012, 2013 (AKA 12.0), 2015, 2017, and to test with a
different toolchain you can take the example patch from
https://wiki.postgresql.org/wiki/Continuous_Integration and add a line
like this to the end of the install section (worked for me; for 2015+
you probably also need to request a different image):
- 'call "C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat" x86_amd64'
I'll make that change to cfbot if we decree that it is the new
baseline for PostgreSQL on Windows. Or I could do it sooner if anyone
wants to be able to post test C99 patches in the Commitfest before we
decide.
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 02:57 Nico Williams <nico@cryptonector.com>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
1 sibling, 0 replies; 46+ messages in thread
From: Nico Williams @ 2018-08-16 02:57 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
There's also clang on Windows, which VS apparently supports. With clang
on Windows PG could even make use of GCC/Clang C extensions :^)
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 08:41 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-16 08:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-15 18:31:10 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I think we could get a start by adding that test to configure, without
> > relying on it for now (i.e. keeping mylodon with -Wc99-extensions
> > -Werror=c99-extensions alive). That'd tell us about which machines,
> > besides presumably gaur, we'd need to either kick to the curb or change.
>
> Sure, no objection to putting that in just to see how much of the
> buildfarm can handle it. If the answer turns out to be "a lot",
> we might have to reconsider, but gathering data seems like the
> first thing to do.
I've pushed a minimal version adding the C99 test. If we were to
actually go for this permanently, we'd likely want to clean up a bunch
of other tests (say removing PGAC_C_VA_ARGS), but I don't see much point
in doing that while just gathering evidence (to the contrary, it seems
like it'd just muddy the water a bit).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 11:18 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 46+ messages in thread
From: Andres Freund @ 2018-08-16 11:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
On 2018-08-16 01:41:34 -0700, Andres Freund wrote:
> I've pushed a minimal version adding the C99 test.
So, we get:
* lotsa animals, unsurprisingly, showing C99 work without any flags.
checking for ccache gcc option to accept ISO C99... none needed
* rhinoceros, nudibranch, grouse, ...:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rhinoceros&dt=2018-08-16%2008%3A45...
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=nudibranch&dt=2018-08-16%2009%3A16...
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=grouse&dt=2018-08-16%2009%3A17%3A2...
checking for ccache gcc option to accept ISO C99... -std=gnu99 (or variations thereof)
So, the autoconf magic is doing it's thing here.
* dunlin (icc):
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=dunlin&dt=2018-08-16%2009%3A35%3A1...
checking for icc option to accept ISO C99... -std=gnu99
(later fails, but not newly so, and just because of ENOSPC)
* anole (HP C compiler)
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole&dt=2018-08-16 09%3A32%3A19
checking for cc option to accept ISO C99... none needed
* dromedary:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=dromedary&dt=2018-08-16%2008%3A37%...
checking for ccache gcc option to accept ISO C99... unsupported
I suspect that's because of the '-ansi' flag in CFLAGS, not because
the compiler is incapable of actually supporting C99.
Besides gaur, I'm also awaiting casteroides' results. The latter
definitely does support C99, but I'm not sure autconf pushes hard
enough. I think every other relevant animal has reported back.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 12:26 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 46+ messages in thread
From: Peter Eisentraut @ 2018-08-16 12:26 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
On 16/08/2018 13:18, Andres Freund wrote:
> checking for ccache gcc option to accept ISO C99... unsupported
>
> I suspect that's because of the '-ansi' flag in CFLAGS, not because
> the compiler is incapable of actually supporting C99.
-ansi is equivalent to -std=c90. If we make the switch, the build farm
configuration should just probably replace -ansi with -std=c99.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 12:28 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 46+ messages in thread
From: Peter Eisentraut @ 2018-08-16 12:28 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 16/08/2018 01:06, Andres Freund wrote:
> So it looks like msvc 2013 might be the relevant requirement.
According to my research (completely untested in practice), you need
2010 for mixed code and declarations and 2013 for named initialization
of structs.
I wonder what raising the msvc requirement would imply for supporting
older Windows versions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 12:30 Andres Freund <andres@anarazel.de>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-16 12:30 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-16 14:26:07 +0200, Peter Eisentraut wrote:
> On 16/08/2018 13:18, Andres Freund wrote:
> > checking for ccache gcc option to accept ISO C99... unsupported
> >
> > I suspect that's because of the '-ansi' flag in CFLAGS, not because
> > the compiler is incapable of actually supporting C99.
>
> -ansi is equivalent to -std=c90. If we make the switch, the build farm
> configuration should just probably replace -ansi with -std=c99.
Right. I just hadn't checked the addition of -std=c99 doesn't override
the -ansi. Presumably just an ordering thing.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 12:54 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 46+ messages in thread
From: Peter Eisentraut @ 2018-08-16 12:54 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
On 16/08/2018 14:30, Andres Freund wrote:
> Hi,
>
> On 2018-08-16 14:26:07 +0200, Peter Eisentraut wrote:
>> On 16/08/2018 13:18, Andres Freund wrote:
>>> checking for ccache gcc option to accept ISO C99... unsupported
>>>
>>> I suspect that's because of the '-ansi' flag in CFLAGS, not because
>>> the compiler is incapable of actually supporting C99.
>>
>> -ansi is equivalent to -std=c90. If we make the switch, the build farm
>> configuration should just probably replace -ansi with -std=c99.
>
> Right. I just hadn't checked the addition of -std=c99 doesn't override
> the -ansi. Presumably just an ordering thing.
The mistake is putting -ansi or similar options into CFLAGS. You need
to put them into CC.
The Autoconf test for C99 correctly puts the candidate options into CC,
but then if the test compilation invocation is something like "$CC
$CFLAGS -c conftest.c", the -ansi cancels out the earlier -std=c99 or
whatever.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 13:00 Andres Freund <andres@anarazel.de>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-16 13:00 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; sandeep.thakkar@enterprisedb.com; +Cc: Thomas Munro <thomas.munro@enterprisedb.com>; Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2018-08-16 14:28:25 +0200, Peter Eisentraut wrote:
> On 16/08/2018 01:06, Andres Freund wrote:
> > So it looks like msvc 2013 might be the relevant requirement.
>
> According to my research (completely untested in practice), you need
> 2010 for mixed code and declarations and 2013 for named initialization
> of structs.
>
> I wonder what raising the msvc requirement would imply for supporting
> older Windows versions.
One relevant tidbit is that afaict 2013 still allows *targeting* older
versions of windows, down to XP and 2003, while requiring a newer
platforms to run. See:
https://docs.microsoft.com/en-us/visualstudio/productinfo/vs2013-compatibility-vs
I don't know if that's hard to do, but I strongly suspect that the
existing installers already do that (otherwise supporting newer versions
would likely require separate builds).
2013 still runs on Windows 7, should you want that:
https://docs.microsoft.com/en-us/visualstudio/productinfo/vs2013-sysrequirements-vs
According to https://www.postgresql.org/download/windows/
the available binaries already effectively restrict windows support:
EDB installers, for 10, restrict to:
64 Bit Windows: 2016, 2012 R2 & R1, 2008 R2, 7, 8, 10
32 Bit Windows: 2008 R1, 7, 8, 10
BIGSQL to: Windows 10 and Windows Server 2012.
Of those 2013 only doesn't run on 2008 R1 anymore. Which still can be
targeted from the newer windows versions.
It'd be good to get confirmation that the windows binaries / installers
are indeed built on newer platforms than the oldest supported version.
Random observation: http://www.openscg.com/bigsql/postgresql/installers/
seems to indicate that packages aren't updated anymore. While it says
"(09-Aug-18)" besides the major versions, it does not actually in fact
have the last set of minor releases. I suspect that's related to
openscg's acquisition by amazon? Either they need to catch up, or we
need to take down the page and probably alert people about that fact.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 14:22 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 46+ messages in thread
From: Andres Freund @ 2018-08-16 14:22 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Dave Page <dpage@pgadmin.org>; +Cc: David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-16 04:18:59 -0700, Andres Freund wrote:
> Besides gaur, I'm also awaiting casteroides' results. The latter
> definitely does support C99, but I'm not sure autconf pushes hard
> enough. I think every other relevant animal has reported back.
Casteroides wasn't a problem, -D_STDC_C99= does the trick.
But enabling C99 support triggered a somewhat weird failure on
protosciurus (also solaris, but gcc)
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=protosciurus&dt=2018-08-16%2013%3A37%3A4...
It detects C99 support successfully via -std=gnu99 but then fails
somewhere during build with:
gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -I/usr/local/include -m64 -I../../../../src/include -c -o gistutil.o gistutil.c
In file included from gistutil.c:24:
../../../../src/include/utils/float.h: In function `get_float4_infinity':
../../../../src/include/utils/float.h:71: error: `__builtin_infinity' undeclared (first use in this function)
../../../../src/include/utils/float.h:71: error: (Each undeclared identifier is reported only once
../../../../src/include/utils/float.h:71: error: for each function it appears in.)
../../../../src/include/utils/float.h: In function `get_float8_infinity':
../../../../src/include/utils/float.h:91: error: `__builtin_infinity' undeclared (first use in this function)
../../../../src/include/utils/float.h: In function `get_float4_nan':
../../../../src/include/utils/float.h:108: error: pointer value used where a floating point value was expected
../../../../src/include/utils/float.h: In function `get_float8_nan':
../../../../src/include/utils/float.h:121: error: pointer value used where a floating point value was expected
../../../../src/include/utils/float.h: In function `check_float4_val':
../../../../src/include/utils/float.h:136: warning: implicit declaration of function `__builtin_isinf'
../../../../src/include/utils/float.h: In function `float4_eq':
../../../../src/include/utils/float.h:283: warning: implicit declaration of function `__builtin_isnan'
While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
still confused what causes this error mode. Kinda looks like
out-of-sync headers with gcc or something.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 15:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-16 15:41 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Dave Page <dpage@pgadmin.org>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> But enabling C99 support triggered a somewhat weird failure on
> protosciurus (also solaris, but gcc)
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=protosciurus&dt=2018-08-16%2013%3A37%3A4...
> It detects C99 support successfully via -std=gnu99 but then fails
> somewhere during build with:
> ../../../../src/include/utils/float.h:71: error: `__builtin_infinity' undeclared (first use in this function)
> While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
> still confused what causes this error mode. Kinda looks like
> out-of-sync headers with gcc or something.
Yeah, this is *absolutely* unsurprising for a non-native gcc installation
on an old platform. It only works to the extent that the platform's
library headers are up to snuff. The gcc installation process actually
knows enough to do a certain amount of editing of the platform's headers,
and install "fixed" copies of those headers where gcc will find them
before the ones in /usr/include. But it looks like the fixing didn't
account for __builtin_infinity on this platform. Maybe a newer gcc
would get it right.
I just launched gaur/pademelon builds for you, though I'm quite certain
they'll both report "unsupported". If we go through with this, my plan
would be to retire pademelon (vendor's compiler) and see if I can install
gcc 4.something to replace the 2.95.3 version that gaur is using. The
-ansi option that dromedary is using is certainly losable, too (I'd
probably replace it with either --std=c99 or --std=gnu99, whichever
autoconf *doesn't* pick by default, just to be contrary).
Andrew is going to need to give us a policy decision on whether to
keep using the existing animal names or take out new ones for these
rather-significantly-modified configurations.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 16:05 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 46+ messages in thread
From: Andres Freund @ 2018-08-16 16:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Andrew Dunstan <andrew@dunslane.net>; +Cc: Dave Page <dpage@pgadmin.org>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-16 11:41:30 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > But enabling C99 support triggered a somewhat weird failure on
> > protosciurus (also solaris, but gcc)
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=protosciurus&dt=2018-08-16%2013%3A37%3A4...
>
> > It detects C99 support successfully via -std=gnu99 but then fails
> > somewhere during build with:
> > ../../../../src/include/utils/float.h:71: error: `__builtin_infinity' undeclared (first use in this function)
>
> > While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
> > still confused what causes this error mode. Kinda looks like
> > out-of-sync headers with gcc or something.
>
> Yeah, this is *absolutely* unsurprising for a non-native gcc installation
> on an old platform. It only works to the extent that the platform's
> library headers are up to snuff. The gcc installation process actually
> knows enough to do a certain amount of editing of the platform's headers,
> and install "fixed" copies of those headers where gcc will find them
> before the ones in /usr/include. But it looks like the fixing didn't
> account for __builtin_infinity on this platform. Maybe a newer gcc
> would get it right.
Sure, but that still requires the headers to behave differently between
C89 and C99 mode, as this worked before. But it turns out there's two
different math.h implementation headers, depending on c99 being enabled
(math_c99.h being the troublesome). If I understand correctly the
problem is more that the system library headers are *newer* (and assume
a sun studio emulating/copying quite a bit of gcc) than the gcc that's
being used, and therefore gcc fails.
Gcc 3.4.3 has been released November 4, 2004, whereas solaris 10 is from
January 31, 2005. The sun studio used for castoroides running on, I
assume, the same hardware, is quite a bit newer in turn.
> I just launched gaur/pademelon builds for you, though I'm quite certain
> they'll both report "unsupported".
Yea, that seems fairly likely.
> If we go through with this, my plan would be to retire pademelon
> (vendor's compiler) and see if I can install gcc 4.something to
> replace the 2.95.3 version that gaur is using.
K.
> The -ansi option that dromedary is using is certainly losable, too
> (I'd probably replace it with either --std=c99 or --std=gnu99,
> whichever autoconf *doesn't* pick by default, just to be contrary).
Makes sense.
> Andrew is going to need to give us a policy decision on whether to
> keep using the existing animal names or take out new ones for these
> rather-significantly-modified configurations.
CCed.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-16 20:36 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 46+ messages in thread
From: Andrew Dunstan @ 2018-08-16 20:36 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dave Page <dpage@pgadmin.org>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
On 08/16/2018 12:05 PM, Andres Freund wrote:
> Hi,
>
> On 2018-08-16 11:41:30 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> But enabling C99 support triggered a somewhat weird failure on
>>> protosciurus (also solaris, but gcc)
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=protosciurus&dt=2018-08-16%2013%3A37%3A4...
>>> It detects C99 support successfully via -std=gnu99 but then fails
>>> somewhere during build with:
>>> ../../../../src/include/utils/float.h:71: error: `__builtin_infinity' undeclared (first use in this function)
>>> While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
>>> still confused what causes this error mode. Kinda looks like
>>> out-of-sync headers with gcc or something.
>> Yeah, this is *absolutely* unsurprising for a non-native gcc installation
>> on an old platform. It only works to the extent that the platform's
>> library headers are up to snuff. The gcc installation process actually
>> knows enough to do a certain amount of editing of the platform's headers,
>> and install "fixed" copies of those headers where gcc will find them
>> before the ones in /usr/include. But it looks like the fixing didn't
>> account for __builtin_infinity on this platform. Maybe a newer gcc
>> would get it right.
> Sure, but that still requires the headers to behave differently between
> C89 and C99 mode, as this worked before. But it turns out there's two
> different math.h implementation headers, depending on c99 being enabled
> (math_c99.h being the troublesome). If I understand correctly the
> problem is more that the system library headers are *newer* (and assume
> a sun studio emulating/copying quite a bit of gcc) than the gcc that's
> being used, and therefore gcc fails.
>
> Gcc 3.4.3 has been released November 4, 2004, whereas solaris 10 is from
> January 31, 2005. The sun studio used for castoroides running on, I
> assume, the same hardware, is quite a bit newer in turn.
>
>
>> I just launched gaur/pademelon builds for you, though I'm quite certain
>> they'll both report "unsupported".
> Yea, that seems fairly likely.
>
>
>> If we go through with this, my plan would be to retire pademelon
>> (vendor's compiler) and see if I can install gcc 4.something to
>> replace the 2.95.3 version that gaur is using.
> K.
>
>
>> The -ansi option that dromedary is using is certainly losable, too
>> (I'd probably replace it with either --std=c99 or --std=gnu99,
>> whichever autoconf *doesn't* pick by default, just to be contrary).
> Makes sense.
>
>
>> Andrew is going to need to give us a policy decision on whether to
>> keep using the existing animal names or take out new ones for these
>> rather-significantly-modified configurations.
> CCed.
>
The usual policy is that animal names need to change if the OS or
compiler names change, but not if just the versions of those change.
There is a script in the suite to update those settings, and the admins
can also help if necessary.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-22 11:44 Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 46+ messages in thread
From: Sandeep Thakkar @ 2018-08-22 11:44 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Thomas Munro <thomas.munro@enterprisedb.com>; Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi
On Thu, Aug 16, 2018 at 6:30 PM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2018-08-16 14:28:25 +0200, Peter Eisentraut wrote:
> > On 16/08/2018 01:06, Andres Freund wrote:
> > > So it looks like msvc 2013 might be the relevant requirement.
> >
> > According to my research (completely untested in practice), you need
> > 2010 for mixed code and declarations and 2013 for named initialization
> > of structs.
> >
> > I wonder what raising the msvc requirement would imply for supporting
> > older Windows versions.
>
> One relevant tidbit is that afaict 2013 still allows *targeting* older
> versions of windows, down to XP and 2003, while requiring a newer
> platforms to run. See:
> https://docs.microsoft.com/en-us/visualstudio/productinfo/
> vs2013-compatibility-vs
> I don't know if that's hard to do, but I strongly suspect that the
> existing installers already do that (otherwise supporting newer versions
> would likely require separate builds).
>
> 2013 still runs on Windows 7, should you want that:
> https://docs.microsoft.com/en-us/visualstudio/productinfo/
> vs2013-sysrequirements-vs
>
> According to https://www.postgresql.org/download/windows/
> the available binaries already effectively restrict windows support:
>
> EDB installers, for 10, restrict to:
> 64 Bit Windows: 2016, 2012 R2 & R1, 2008 R2, 7, 8, 10
> 32 Bit Windows: 2008 R1, 7, 8, 10
>
> BIGSQL to: Windows 10 and Windows Server 2012.
>
> Of those 2013 only doesn't run on 2008 R1 anymore. Which still can be
> targeted from the newer windows versions.
>
>
> It'd be good to get confirmation that the windows binaries / installers
> are indeed built on newer platforms than the oldest supported version.
>
> We build windows binaries (>=9.3) on Windows 7 and Windows Server 2012 R2.
For 9.3, the Visual Studio version is 2010 and for 9.4 and v10, we use
2013. For v11, we use 2017.
> Random observation: http://www.openscg.com/bigsql/postgresql/installers/
> seems to indicate that packages aren't updated anymore. While it says
> "(09-Aug-18)" besides the major versions, it does not actually in fact
> have the last set of minor releases. I suspect that's related to
> openscg's acquisition by amazon? Either they need to catch up, or we
> need to take down the page and probably alert people about that fact.
>
> Greetings,
>
> Andres Freund
>
--
Sandeep Thakkar
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-24 02:36 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
1 sibling, 0 replies; 46+ messages in thread
From: Thomas Munro @ 2018-08-24 02:36 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Steele <david@pgmasters.net>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Aug 16, 2018 at 12:24 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> FWIW cfbot is using Visual Studio 2010 right now. Appveyor provides
> 2008, 2010, 2012, 2013 (AKA 12.0), 2015, 2017, and to test with a
> different toolchain you can take the example patch from
> https://wiki.postgresql.org/wiki/Continuous_Integration and add a line
> like this to the end of the install section (worked for me; for 2015+
> you probably also need to request a different image):
>
> - 'call "C:\Program Files (x86)\Microsoft Visual Studio
> 12.0\VC\vcvarsall.bat" x86_amd64'
>
> I'll make that change to cfbot if we decree that it is the new
> baseline for PostgreSQL on Windows.
Done.
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-08-25 17:08 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 46+ messages in thread
From: Tom Lane @ 2018-08-25 17:08 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Andrew Dunstan <andrew@dunslane.net>; Dave Page <dpage@pgadmin.org>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-16 11:41:30 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
>>> still confused what causes this error mode. Kinda looks like
>>> out-of-sync headers with gcc or something.
>> Yeah, this is *absolutely* unsurprising for a non-native gcc installation
>> on an old platform.
> Sure, but that still requires the headers to behave differently between
> C89 and C99 mode, as this worked before. But it turns out there's two
> different math.h implementation headers, depending on c99 being enabled
> (math_c99.h being the troublesome). If I understand correctly the
> problem is more that the system library headers are *newer* (and assume
> a sun studio emulating/copying quite a bit of gcc) than the gcc that's
> being used, and therefore gcc fails.
I have some more info on this issue, based on having successfully
updated "gaur" using gcc 3.4.6 (which I picked because it was the last
of the 3.x release series). It seems very unlikely that there's much
difference between 3.4.3 and 3.4.6 as far as external features go.
What I find in the 3.4.6 documentation is
-- Built-in Function: double __builtin_inf (void)
Similar to `__builtin_huge_val', except a warning is generated if
the target floating-point format does not support infinities.
This function is suitable for implementing the ISO C99 macro
`INFINITY'.
Note that the function is called "__builtin_inf", whereas what we see
protosciurus choking on is "__builtin_infinity". So I don't think this
is a version skew issue at all. I think that the system headers are
written for the Solaris cc, and its name for the equivalent function is
__builtin_infinity, whereas what gcc wants is __builtin_inf. Likewise,
the failures we see for __builtin_isinf and __builtin_isnan are because
Solaris cc provides those but gcc does not.
If we wanted to keep protosciurus going without a compiler update, my
thought would be to modify gcc's copy of math_c99.h to correct the
function name underlying INFINITY, and change the definitions of isinf()
and isnan() back to whatever was being used pre-C99.
It's possible that newer gcc releases have been tweaked so that they
make appropriate corrections in this header file automatically, but
that's not a sure thing.
regards, tom lane
^ permalink raw reply [nested|flat] 46+ messages in thread
* Re: C99 compliance for src/port/snprintf.c
@ 2018-09-28 15:04 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 46+ messages in thread
From: Andres Freund @ 2018-09-28 15:04 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Dunstan <andrew@dunslane.net>; Dave Page <dpage@pgadmin.org>; David Steele <david@pgmasters.net>; pgsql-hackers@lists.postgresql.org
Hi,
On 2018-08-25 13:08:18 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-08-16 11:41:30 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> While I'd personally have no problem kicking gcc 3.4 to the curb, I'm
> >>> still confused what causes this error mode. Kinda looks like
> >>> out-of-sync headers with gcc or something.
>
> >> Yeah, this is *absolutely* unsurprising for a non-native gcc installation
> >> on an old platform.
>
> > Sure, but that still requires the headers to behave differently between
> > C89 and C99 mode, as this worked before. But it turns out there's two
> > different math.h implementation headers, depending on c99 being enabled
> > (math_c99.h being the troublesome). If I understand correctly the
> > problem is more that the system library headers are *newer* (and assume
> > a sun studio emulating/copying quite a bit of gcc) than the gcc that's
> > being used, and therefore gcc fails.
>
> I have some more info on this issue, based on having successfully
> updated "gaur" using gcc 3.4.6 (which I picked because it was the last
> of the 3.x release series). It seems very unlikely that there's much
> difference between 3.4.3 and 3.4.6 as far as external features go.
> What I find in the 3.4.6 documentation is
>
> -- Built-in Function: double __builtin_inf (void)
> Similar to `__builtin_huge_val', except a warning is generated if
> the target floating-point format does not support infinities.
> This function is suitable for implementing the ISO C99 macro
> `INFINITY'.
>
> Note that the function is called "__builtin_inf", whereas what we see
> protosciurus choking on is "__builtin_infinity". So I don't think this
> is a version skew issue at all. I think that the system headers are
> written for the Solaris cc, and its name for the equivalent function is
> __builtin_infinity, whereas what gcc wants is __builtin_inf. Likewise,
> the failures we see for __builtin_isinf and __builtin_isnan are because
> Solaris cc provides those but gcc does not.
>
> If we wanted to keep protosciurus going without a compiler update, my
> thought would be to modify gcc's copy of math_c99.h to correct the
> function name underlying INFINITY, and change the definitions of isinf()
> and isnan() back to whatever was being used pre-C99.
>
> It's possible that newer gcc releases have been tweaked so that they
> make appropriate corrections in this header file automatically, but
> that's not a sure thing.
I've pinged Dave about this, and he said:
On 2018-09-26 17:04:29 -0400, Dave Page wrote:
> Unfortunately, i think that whole machine is basically EOL now. It's
> already skipping OpenSSL for some builds, as being stuck on a very old
> version of the buildfarm client, as some of the modules used in newer
> versions just won't compile or work. I don't have any support contract, or
> access to newer versions of SunStudio, and the guys that used to package
> GCC for Solaris now charge to download their packages.
>
> I could potentially build my own version of GCC, but I question whether
> it's really worth it, given the other problems.
He's now disabled building master on protosciurus and casteroides. We
still have damselfly and rover_firefly so I don't feel too bad about
that. I've pinged their owners to ask whether they could set up a sun
studio (or however that's called in their solaris descendants) version.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 46+ messages in thread
end of thread, other threads:[~2018-09-28 15:04 UTC | newest]
Thread overview: 46+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-08-14 23:28 C99 compliance for src/port/snprintf.c Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 14:58 ` Robert Haas <robertmhaas@gmail.com>
2018-08-15 15:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 15:52 ` Andres Freund <andres@anarazel.de>
2018-08-15 16:01 ` Robert Haas <robertmhaas@gmail.com>
2018-08-15 16:11 ` Andres Freund <andres@anarazel.de>
2018-08-15 16:17 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 18:03 ` David Steele <david@pgmasters.net>
2018-08-15 18:09 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 18:59 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2018-08-15 19:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 19:21 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2018-08-15 19:37 ` David Steele <david@pgmasters.net>
2018-08-15 19:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 21:49 ` Andres Freund <andres@anarazel.de>
2018-08-15 22:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 22:24 ` Andres Freund <andres@anarazel.de>
2018-08-15 22:31 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 22:40 ` Andres Freund <andres@anarazel.de>
2018-08-15 22:54 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-08-15 23:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 23:06 ` Andres Freund <andres@anarazel.de>
2018-08-16 00:24 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-08-16 02:57 ` Nico Williams <nico@cryptonector.com>
2018-08-24 02:36 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-08-16 12:28 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-16 13:00 ` Andres Freund <andres@anarazel.de>
2018-08-22 11:44 ` Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>
2018-08-16 08:41 ` Andres Freund <andres@anarazel.de>
2018-08-16 11:18 ` Andres Freund <andres@anarazel.de>
2018-08-16 12:26 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-16 12:30 ` Andres Freund <andres@anarazel.de>
2018-08-16 12:54 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-16 14:22 ` Andres Freund <andres@anarazel.de>
2018-08-16 15:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-16 16:05 ` Andres Freund <andres@anarazel.de>
2018-08-16 20:36 ` Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
2018-08-25 17:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-09-28 15:04 ` Andres Freund <andres@anarazel.de>
2018-08-15 16:07 ` Stephen Frost <sfrost@snowman.net>
2018-08-16 00:15 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 15:56 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-08-15 17:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 18:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-08-15 21:52 ` Andres Freund <andres@anarazel.de>
2018-08-15 22:18 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox