pgsql: Use new overflow aware integer operations.

On 13 December 2017 at 14:01, Andres Freund <andres@anarazel.de> wrote:

Use new overflow aware integer operations.

Thanks for making this happen.

I notice it's caused a small warning in compilers that don't
understand about elog(ERROR) and ereport(ERROR) not returning.

This can be seen on bowerbird's compile log [1]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=bowerbird&amp;dt=2017-12-15%2018%3A21%3A51&amp;stg=make:

c:\prog\bf\root\head\pgsql.build\src\backend\utils\adt\int8.c(131):
warning C4715: 'scanint8' : not all control paths return a value
[c:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]

The attached just shuffles things around to get rid of the warning.
This way seems better than to add another "return false" at the end as
this way saves a couple of lines of code rather than adding one.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=bowerbird&amp;dt=2017-12-15%2018%3A21%3A51&amp;stg=make

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

I notice it's caused a small warning in compilers that don't
understand about elog(ERROR) and ereport(ERROR) not returning.

Wups, I noticed this independently and fixed it before reading your
message. Sorry about failing to credit you in the commit log.

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

Use new overflow aware integer operations.

I just tried to compile manually on prairiedog for the first time since
this went in, and noticed that it now produces a boatload of warnings
like

int.c: In function 'int2pl':
int.c:735: warning: 'result' may be used uninitialized in this function

I think it's entirely within its rights to complain, because
the non-builtin code paths in int.h look like

int32 res = (int32) a + (int32) b;

if (res > PG_INT16_MAX || res < PG_INT16_MIN)
return true;
*result = (int16) res;
return false;

and so if an overflow occurs then *result won't get set.

I do not think it is reasonable for these functions to not set the
output variable at all in the overflow case; it is not their job
to opine on whether the caller may use the result. Furthermore,
this is an incorrect emulation of the built-in functions;
the gcc manual clearly says

These built-in functions promote the first two operands into
infinite precision signed type and perform addition on those
promoted operands. The result is then cast to the type the third
pointer argument points to and stored there. If the stored result
is equal to the infinite precision result, the built-in functions
return false, otherwise they return true.

So as coded, there is a difference of behavior depending on whether
you have the built-in, and that cannot be anything but bad.

Therefore I think all of these need a patch like

int32 res = (int32) a + (int32) b;

+ *result = (int16) res;
if (res > PG_INT16_MAX || res < PG_INT16_MIN)
return true;
- *result = (int16) res;
return false;

regards, tom lane

Hi,

On 2017-12-22 11:00:54 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Use new overflow aware integer operations.

I just tried to compile manually on prairiedog for the first time since
this went in, and noticed that it now produces a boatload of warnings
like

int.c: In function 'int2pl':
int.c:735: warning: 'result' may be used uninitialized in this function

I think it's entirely within its rights to complain, because
the non-builtin code paths in int.h look like

int32 res = (int32) a + (int32) b;

if (res > PG_INT16_MAX || res < PG_INT16_MIN)
return true;
*result = (int16) res;
return false;

and so if an overflow occurs then *result won't get set.

I do not think it is reasonable for these functions to not set the
output variable at all in the overflow case; it is not their job
to opine on whether the caller may use the result.

I don't agree. Please note that that the function's documentation
explicitly says "The content of *result is implementation defined in
case of overflow.".

Furthermore, this is an incorrect emulation of the built-in functions;
the gcc manual clearly says

These built-in functions promote the first two operands into
infinite precision signed type and perform addition on those
promoted operands. The result is then cast to the type the third
pointer argument points to and stored there. If the stored result
is equal to the infinite precision result, the built-in functions
return false, otherwise they return true.

So as coded, there is a difference of behavior depending on whether
you have the built-in, and that cannot be anything but bad.

The problem is that other intrinsic implementation we might want to use
in the future, like microsoft's, do *not* exactly behave that way. So I
think defining this as implementation defined is the right thing to do.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2017-12-22 11:00:54 -0500, Tom Lane wrote:

I do not think it is reasonable for these functions to not set the
output variable at all in the overflow case; it is not their job
to opine on whether the caller may use the result.

I don't agree. Please note that that the function's documentation
explicitly says "The content of *result is implementation defined in
case of overflow.".

I will not accept an implementation that spews compiler warnings
all over the place, which is what this one is doing. Please fix that,
or else I will.

regards, tom lane

On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2017-12-22 11:00:54 -0500, Tom Lane wrote:

I do not think it is reasonable for these functions to not set the
output variable at all in the overflow case; it is not their job
to opine on whether the caller may use the result.

I don't agree. Please note that that the function's documentation
explicitly says "The content of *result is implementation defined in
case of overflow.".

I will not accept an implementation that spews compiler warnings
all over the place, which is what this one is doing. Please fix that,
or else I will.

Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was saying is that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved the discussion of semantics it just seemed pointless to start fixing...

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[ back from Christmas break ]

Andres Freund <andres@anarazel.de> writes:

On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I will not accept an implementation that spews compiler warnings
all over the place, which is what this one is doing. Please fix that,
or else I will.

Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was saying is that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved the discussion of semantics it just seemed pointless to start fixing...

Sorry, I was being unnecessarily grumpy there. I follow your point about
not wanting to constrain the implementation to yield the correct low-order
bits if we don't actually need that behavior ... but I'm still not happy
about the warnings.

What do you think of having the fallback code explicitly set the output
variable to zero (or any other fixed value) on overflow, like

#if defined(HAVE__BUILTIN_OP_OVERFLOW)
return __builtin_add_overflow(a, b, result);
#else
int32 res = (int32) a + (int32) b;

	if (res > PG_INT16_MAX || res < PG_INT16_MIN)
+	{
+		*result = 0;		/* just to keep compiler quiet */
		return true;
+	}
	*result = (int16) res;
	return false;
 #endif

I do not think this would cause any performance loss in our expected
usage, because reasonably bright compilers would detect that the store
is dead code and remove it. But less-bright compilers would not be
issuing warnings.

regards, tom lane

On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote:

Use new overflow aware integer operations.

frogmouth seems to have crashed in the vicinity of various number
processing tests -- could this commit be responsible? A wild guess as
there isn't much to go on in the build log:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&amp;dt=2017-12-28%2011%3A30%3A54

--
Thomas Munro
http://www.enterprisedb.com

On 2017-12-29 23:06:10 +1300, Thomas Munro wrote:

On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote:

Use new overflow aware integer operations.

frogmouth seems to have crashed in the vicinity of various number
processing tests -- could this commit be responsible? A wild guess as
there isn't much to go on in the build log:

That seems not that likely an explanation, given there were a good
number of runs without failure after this commit went in, before the
animal started to fail due to PHJ.

Greetings,

Andres Freund

On 2017-12-27 17:59:26 -0500, Tom Lane wrote:

[ back from Christmas break ]

Andres Freund <andres@anarazel.de> writes:

On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I will not accept an implementation that spews compiler warnings
all over the place, which is what this one is doing. Please fix that,
or else I will.

Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was saying is that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved the discussion of semantics it just seemed pointless to start fixing...

Sorry, I was being unnecessarily grumpy there.

Lack of holidays does that to one ;)

I follow your point about not wanting to constrain the implementation
to yield the correct low-order bits if we don't actually need that
behavior ... but I'm still not happy about the warnings.

Agreed.

What do you think of having the fallback code explicitly set the output
variable to zero (or any other fixed value) on overflow, like

#if defined(HAVE__BUILTIN_OP_OVERFLOW)
return __builtin_add_overflow(a, b, result);
#else
int32 res = (int32) a + (int32) b;

if (res > PG_INT16_MAX || res < PG_INT16_MIN)
+	{
+		*result = 0;		/* just to keep compiler quiet */
return true;
+	}
*result = (int16) res;
return false;
#endif

I do not think this would cause any performance loss in our expected
usage, because reasonably bright compilers would detect that the store
is dead code and remove it. But less-bright compilers would not be
issuing warnings.

Yea, that works for me. I wonder if we should choose an absurd sentinel
value to prevent code from relying on one? 0x0000beef or such. Unless
somebody protests soon-ish I'll make it so.

Greetings,

Andres Freund

On Sat, Dec 30, 2017 at 8:43 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-12-29 23:06:10 +1300, Thomas Munro wrote:

On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote:

Use new overflow aware integer operations.

frogmouth seems to have crashed in the vicinity of various number
processing tests -- could this commit be responsible? A wild guess as
there isn't much to go on in the build log:

That seems not that likely an explanation, given there were a good
number of runs without failure after this commit went in, before the
animal started to fail due to PHJ.

Hmm, yeah, also the more recent failure is bombing in different
places, so I take that back. I was thrown by a couple of bits in the
earlier build log that seemed to evoke this commit:

INSERT INTO FLOAT4_TBL(f1) VALUES ('10e70');
! ERROR: value out of range: overflow

SELECT * FROM generate_series('+4567890123456789'::int8,
'+4567890123456799'::int8, 0);
! WARNING: terminating connection because of crash of another server process

--
Thomas Munro
http://www.enterprisedb.com

On 2017-12-29 12:21:54 -0800, Andres Freund wrote:

On 2017-12-27 17:59:26 -0500, Tom Lane wrote:

#if defined(HAVE__BUILTIN_OP_OVERFLOW)
return __builtin_add_overflow(a, b, result);
#else
int32 res = (int32) a + (int32) b;

if (res > PG_INT16_MAX || res < PG_INT16_MIN)
+	{
+		*result = 0;		/* just to keep compiler quiet */
return true;
+	}
*result = (int16) res;
return false;
#endif

I do not think this would cause any performance loss in our expected
usage, because reasonably bright compilers would detect that the store
is dead code and remove it. But less-bright compilers would not be
issuing warnings.

Yea, that works for me. I wonder if we should choose an absurd sentinel
value to prevent code from relying on one? 0x0000beef or such. Unless
somebody protests soon-ish I'll make it so.

Pushed that way (with 0x5EED as the value, seems more appropriate ;)).

I can't convince any of my compilers to actual emit warnings in this
case, so we'll have to see whether prairiedog like this...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Yea, that works for me. I wonder if we should choose an absurd sentinel
value to prevent code from relying on one? 0x0000beef or such. Unless
somebody protests soon-ish I'll make it so.

Pushed that way (with 0x5EED as the value, seems more appropriate ;)).

LGTM, thanks!

I can't convince any of my compilers to actual emit warnings in this
case, so we'll have to see whether prairiedog like this...

Per my results yesterday, locust and coypu were also complaining;
one of them might come back quicker.

regards, tom lane

Hi,

On 2018-02-14 17:36:38 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I can't convince any of my compilers to actual emit warnings in this
case, so we'll have to see whether prairiedog like this...

Per my results yesterday, locust and coypu were also complaining;
one of them might come back quicker.

All of them seem to be ok with this now.

Greetings,

Andres Freund