pgsql: Provide overflow safe integer math inline functions.
Provide overflow safe integer math inline functions.
It's not easy to get signed integer overflow checks correct and
fast. Therefore abstract the necessary infrastructure into a common
header providing addition, subtraction and multiplication for 16, 32,
64 bit signed integers.
The new macros aren't yet used, but a followup commit will convert
several open coded overflow checks.
Author: Andres Freund, with some code stolen from Greg Stark
Reviewed-By: Robert Haas
Discussion: /messages/by-id/20171024103954.ztmatprlglz3rwke@alap3.anarazel.de
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/4d6ad31257adaf8a51e1c4377d96afa656d9165f
Modified Files
--------------
config/c-compiler.m4 | 22 ++++
configure | 33 ++++++
configure.in | 4 +
src/include/common/int.h | 239 ++++++++++++++++++++++++++++++++++++++++++
src/include/pg_config.h.in | 3 +
src/include/pg_config.h.win32 | 3 +
6 files changed, 304 insertions(+)
Re: Andres Freund 2017-12-13 <E1eOvQF-00075r-Cy@gemulon.postgresql.org>
Provide overflow safe integer math inline functions.
The new _overflow functions have buggy comments:
/*
* If a - b overflows, return true, otherwise store the result of a + b into
* *result. ^ there
Christoph
On Wed, Dec 13, 2017 at 5:10 AM, Christoph Berg <myon@debian.org> wrote:
Re: Andres Freund 2017-12-13 <E1eOvQF-00075r-Cy@gemulon.postgresql.org>
Provide overflow safe integer math inline functions.
The new _overflow functions have buggy comments:
/*
* If a - b overflows, return true, otherwise store the result of a + b into
* *result. ^ there
What's wrong with that?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2017-12-13 09:37:25 -0500, Robert Haas wrote:
On Wed, Dec 13, 2017 at 5:10 AM, Christoph Berg <myon@debian.org> wrote:
Re: Andres Freund 2017-12-13 <E1eOvQF-00075r-Cy@gemulon.postgresql.org>
Provide overflow safe integer math inline functions.
The new _overflow functions have buggy comments:
/*
* If a - b overflows, return true, otherwise store the result of a + b into
* *result. ^ thereWhat's wrong with that?
After staring at it for a while, I seem to have partially mis-copied the
note for addition to the subtraction operation...
Greetings,
Andres Freund
Hi Michael,
On 2017-12-13 01:01:19 +0000, Andres Freund wrote:
Provide overflow safe integer math inline functions.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi&dt=2017-12-13%2018%3A00%3A18
which seems half like a compiler bug to me. But either way, we gotta
work around it. I suspect the reason configure test doesn't
sufficiently detect this here is because it's testing the function with
constant arguments.
Could you perhaps test whether replacing PGAC_C_BUILTIN_OP_OVERFLOW's body with something like
result
PG_INT64_TYPE a;
PG_INT64_TYPE b;
PG_INT64_TYPE result;
__builtin_mul_overflow(*(volatile PG_INT64_TYPE*) &a, *(volatile PG_INT64_TYPE*) &b, &result);
makes it fail? I'd rather not test this via the buildfarm, given that
dangomushi isn't the most frequently running / fastest animal.
- Andres
On Thu, Dec 14, 2017 at 6:37 AM, Andres Freund <andres@anarazel.de> wrote:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi&dt=2017-12-13%2018%3A00%3A18
which seems half like a compiler bug to me. But either way, we gotta
work around it. I suspect the reason configure test doesn't
sufficiently detect this here is because it's testing the function with
constant arguments.
This uses clang 5.0 and I can reproduce the failure manually:
$ clang --version
clang version 5.0.0 (tags/RELEASE_500/final)
Target: armv7l-unknown-linux-gnueabihf
Thread model: posix
InstalledDir: /usr/bin
Note that on the same machine using gcc 7.2.0 I am seeing no
compilation failures. ArchLinux maintainers can be sometimes wild in
the versions they push, I have seen that in the past. And Arch ARM has
less resources I think.
Also, gull is complaining with the same failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gull&dt=2017-12-13%2004%3A20%3A18
ARMv7 is used there as well, with clang 3.8, so this combination is deadly.
Could you perhaps test whether replacing PGAC_C_BUILTIN_OP_OVERFLOW's body with something like
result
PG_INT64_TYPE a;
PG_INT64_TYPE b;
PG_INT64_TYPE result;
__builtin_mul_overflow(*(volatile PG_INT64_TYPE*) &a, *(volatile PG_INT64_TYPE*) &b, &result);makes it fail? I'd rather not test this via the buildfarm, given that
dangomushi isn't the most frequently running / fastest animal.
The tests are run once per day to not push too much its SD card.
Let me see... If I enforce also a cast on "result" as well then I am
able to compile correctly, which gives me the attached. make check
also works, I haven't taken the time to do a complete test run though,
this would take way longer on dangomushi.
--
Michael
Attachments:
builtin-multi-conf.patchapplication/octet-stream; name=builtin-multi-conf.patchDownload
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index ed26644a48..a3dc7f9cac 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -310,8 +310,12 @@ fi])# PGAC_C_BUILTIN_CONSTANT_P
AC_DEFUN([PGAC_C_BUILTIN_OP_OVERFLOW],
[AC_CACHE_CHECK(for __builtin_mul_overflow, pgac_cv__builtin_op_overflow,
[AC_LINK_IFELSE([AC_LANG_PROGRAM([],
-[PG_INT64_TYPE result;
-__builtin_mul_overflow((PG_INT64_TYPE) 1, (PG_INT64_TYPE) 2, &result);]
+[PG_INT64_TYPE a;
+PG_INT64_TYPE b;
+PG_INT64_TYPE result;
+__builtin_mul_overflow(*(volatile PG_INT64_TYPE*) &a,
+ *(volatile PG_INT64_TYPE*) &b,
+ *(volatile PG_INT64_TYPE*) &result);]
)],
[pgac_cv__builtin_op_overflow=yes],
[pgac_cv__builtin_op_overflow=no])])
diff --git a/configure b/configure
index ca76ef0ab2..b6d60c97dc 100755
--- a/configure
+++ b/configure
@@ -14485,8 +14485,12 @@ else
int
main ()
{
+PG_INT64_TYPE a;
+PG_INT64_TYPE b;
PG_INT64_TYPE result;
-__builtin_mul_overflow((PG_INT64_TYPE) 1, (PG_INT64_TYPE) 2, &result);
+__builtin_mul_overflow(*(volatile PG_INT64_TYPE*) &a,
+ *(volatile PG_INT64_TYPE*) &b,
+ *(volatile PG_INT64_TYPE*) &result);
;
return 0;
Re: Andres Freund 2017-12-13 <20171213173524.rjs7b3ahsong5zko@alap3.anarazel.de>
On 2017-12-13 09:37:25 -0500, Robert Haas wrote:
On Wed, Dec 13, 2017 at 5:10 AM, Christoph Berg <myon@debian.org> wrote:
Re: Andres Freund 2017-12-13 <E1eOvQF-00075r-Cy@gemulon.postgresql.org>
Provide overflow safe integer math inline functions.
The new _overflow functions have buggy comments:
/*
* If a - b overflows, return true, otherwise store the result of a + b into
* *result. ^ thereWhat's wrong with that?
After staring at it for a while, I seem to have partially mis-copied the
note for addition to the subtraction operation...
I believe the attached is the correct version.
Christoph
Attachments:
int.h-commentstext/plain; charset=us-asciiDownload
diff --git a/src/include/common/int.h b/src/include/common/int.h
new file mode 100644
index e44d42f..e6907c6
*** a/src/include/common/int.h
--- b/src/include/common/int.h
*************** pg_add_s16_overflow(int16 a, int16 b, in
*** 41,47 ****
}
/*
! * If a - b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 41,47 ----
}
/*
! * If a - b overflows, return true, otherwise store the result of a - b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
*************** pg_sub_s16_overflow(int16 a, int16 b, in
*** 61,67 ****
}
/*
! * If a * b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 61,67 ----
}
/*
! * If a * b overflows, return true, otherwise store the result of a * b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
*************** pg_add_s32_overflow(int32 a, int32 b, in
*** 101,107 ****
}
/*
! * If a - b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 101,107 ----
}
/*
! * If a - b overflows, return true, otherwise store the result of a - b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
*************** pg_sub_s32_overflow(int32 a, int32 b, in
*** 121,127 ****
}
/*
! * If a * b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 121,127 ----
}
/*
! * If a * b overflows, return true, otherwise store the result of a * b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
*************** pg_add_s64_overflow(int64 a, int64 b, in
*** 167,173 ****
}
/*
! * If a - b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 167,173 ----
}
/*
! * If a - b overflows, return true, otherwise store the result of a - b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
*************** pg_sub_s64_overflow(int64 a, int64 b, in
*** 193,199 ****
}
/*
! * If a * b overflows, return true, otherwise store the result of a + b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
--- 193,199 ----
}
/*
! * If a * b overflows, return true, otherwise store the result of a * b into
* *result. The content of *result is implementation defined in case of
* overflow.
*/
Hi,
On 2017-12-14 09:28:08 +0100, Christoph Berg wrote:
Re: Andres Freund 2017-12-13 <20171213173524.rjs7b3ahsong5zko@alap3.anarazel.de>
After staring at it for a while, I seem to have partially mis-copied the
note for addition to the subtraction operation...I believe the attached is the correct version.
Thanks! Pushed.
Sch�nen Abend,
Andres
On 2017-12-13 13:37:54 -0800, Andres Freund wrote:
Hi Michael,
On 2017-12-13 01:01:19 +0000, Andres Freund wrote:
Provide overflow safe integer math inline functions.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi&dt=2017-12-13%2018%3A00%3A18
which seems half like a compiler bug to me. But either way, we gotta
work around it. I suspect the reason configure test doesn't
sufficiently detect this here is because it's testing the function with
constant arguments.Could you perhaps test whether replacing PGAC_C_BUILTIN_OP_OVERFLOW's body with something like
result
PG_INT64_TYPE a;
PG_INT64_TYPE b;
PG_INT64_TYPE result;
__builtin_mul_overflow(*(volatile PG_INT64_TYPE*) &a, *(volatile PG_INT64_TYPE*) &b, &result);makes it fail? I'd rather not test this via the buildfarm, given that
dangomushi isn't the most frequently running / fastest animal.
I've since tried this via the buildfarm, but still:
configure:14480: checking for __builtin_mul_overflow
configure:14500: ccache clang -o conftest -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 -O2 -D_GNU_SOURCE -I/usr/include/et conftest.c -lssl -lcrypto -lz -lreadline -lrt -lcrypt -ldl -lm >&5
configure:14500: $? = 0
configure:14508: result: yes
I'm not quite following. Could you check if the same happens without
-O2? Not because that'd be a solution, but to narrow down how this
happens?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
I'm not quite following. Could you check if the same happens without
-O2? Not because that'd be a solution, but to narrow down how this
happens?
The committed test looks quite broken to me: it's missing some &
operators. Not sure how that translates into failing to fail the
configure test, but personally I'd have done this like
volatile PG_INT64_TYPE a = 1;
volatile PG_INT64_TYPE b = 1;
PG_INT64_TYPE result;
__builtin_mul_overflow(a, b, &result);
regards, tom lane
On December 16, 2017 5:31:01 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
I'm not quite following. Could you check if the same happens without
-O2? Not because that'd be a solution, but to narrow down how this
happens?The committed test looks quite broken to me: it's missing some &
operators. Not sure how that translates into failing to fail the
configure test,
Hm, true. As you say, it doesn't explain the problem though, it's just weird int to ptr cases. Kinda seems like clang links to a different runtime during the configure tests than what's used for postgres...
Andres
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> writes:
On December 16, 2017 5:31:01 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The committed test looks quite broken to me: it's missing some &
operators. Not sure how that translates into failing to fail the
configure test,
Hm, true. As you say, it doesn't explain the problem though, it's just weird int to ptr cases. Kinda seems like clang links to a different runtime during the configure tests than what's used for postgres...
What I'm thinking is that that somehow prevents the "volatile" from
having any effect. On my laptop (recent Apple clang), the function call
certainly does seem to get optimized away with the test-as-committed...
Oh! It seems to get optimized away with my "volatile" version too.
I think the real issue, or part of it, is that you need to assign the
result to a global variable, not a local one, so that the compiler
doesn't decide it can forget the whole thing. And maybe store the
bool result somewhere, too.
Marking the result and bool-result variables as volatile might
be an OK substitute for making them global. But the global way
is what we've done in older configure test programs.
regards, tom lane
On Sun, Dec 17, 2017 at 9:32 AM, Andres Freund <andres@anarazel.de> wrote:
I've since tried this via the buildfarm, but still:
configure:14480: checking for __builtin_mul_overflow
configure:14500: ccache clang -o conftest -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 -O2 -D_GNU_SOURCE -I/usr/include/et conftest.c -lssl -lcrypto -lz -lreadline -lrt -lcrypt -ldl -lm >&5
configure:14500: $? = 0
configure:14508: result: yesI'm not quite following. Could you check if the same happens without
-O2? Not because that'd be a solution, but to narrow down how this
happens?
I have triggered a new test manually this morning on dangomushi
because I knew that it would fail as I have tested as well stuff
similar to the version you have pushed. Please note that if you added
a volatile cast to "result" as well, then compilation was able to
complete and regression tests passed...
--
Michael
Michael Paquier <michael.paquier@gmail.com> writes:
... Please note that if you added
a volatile cast to "result" as well, then compilation was able to
complete and regression tests passed...
Yeah, that squares with my analysis: the problem with the existing test
is that the compiler is throwing away the function call because its
output is unused.
If I haven't seen something from Andres shortly, I'll push a fix.
regards, tom lane
On Mon, Dec 18, 2017 at 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
... Please note that if you added
a volatile cast to "result" as well, then compilation was able to
complete and regression tests passed...Yeah, that squares with my analysis: the problem with the existing test
is that the compiler is throwing away the function call because its
output is unused.If I haven't seen something from Andres shortly, I'll push a fix.
And the buildfarm is happy again. Thanks for the fix.
--
Michael
On 2017-12-17 11:03:49 -0500, Tom Lane wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
... Please note that if you added
a volatile cast to "result" as well, then compilation was able to
complete and regression tests passed...Yeah, that squares with my analysis: the problem with the existing test
is that the compiler is throwing away the function call because its
output is unused.If I haven't seen something from Andres shortly, I'll push a fix.
Thanks! Was off for most of yesterday...
Greetings,
Andres Freund