Thread: 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: https://postgr.es/m/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. ^ there > > What'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 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 linksto 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
clanglinks 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: 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? 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