Horton

Thread

Commits

  1. Try harder to detect unavailability of __builtin_mul_overflow(int64).

  2. Try to detect runtime unavailability of __builtin_mul_overflow(int64).

  3. Fix a number of copy & paste comment errors in common/int.h.

  4. Provide overflow safe integer math inline functions.

  1. pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-13T01:01:19Z

    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(+)

  2. Re: pgsql: Provide overflow safe integer math inline functions.

    Christoph Berg <myon@debian.org> — 2017-12-13T10:10:37Z

    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

  3. Re: pgsql: Provide overflow safe integer math inline functions.

    Robert Haas <robertmhaas@gmail.com> — 2017-12-13T14:37:25Z

    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

  4. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-13T17:35:24Z

    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

  5. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-13T21:37:54Z

    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

  6. Re: pgsql: Provide overflow safe integer math inline functions.

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-14T04:05:32Z

    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

  7. Re: pgsql: Provide overflow safe integer math inline functions.

    Christoph Berg <myon@debian.org> — 2017-12-14T08:28:08Z

    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.                                                         ^ 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...

I believe the attached is the correct version.

Christoph

  8. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-14T20:38:03Z

    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

  9. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-17T00:32:07Z

    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

  10. Re: pgsql: Provide overflow safe integer math inline functions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-17T01:31:01Z

    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

  11. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-17T01:35:05Z

    
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.

  12. Re: pgsql: Provide overflow safe integer math inline functions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-17T01:48:32Z

    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

  13. Re: pgsql: Provide overflow safe integer math inline functions.

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-17T07:00:31Z

    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

  14. Re: pgsql: Provide overflow safe integer math inline functions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-17T16:03:49Z

    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

  15. Re: pgsql: Provide overflow safe integer math inline functions.

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-18T00:20:39Z

    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

  16. Re: pgsql: Provide overflow safe integer math inline functions.

    Andres Freund <andres@anarazel.de> — 2017-12-18T16:40:06Z

    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