Horton

Thread

Commits

  1. Use C99 restrict via pg_restrict, rather than restrict directly.

  2. Work around overly strict restrict checks by MSVC.

  3. Add configure infrastructure to detect support for C99's restrict.

  1. pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-11T23:11:15Z

    Add configure infrastructure to detect support for C99's restrict.

Will be used in later commits improving performance for a few key
routines where information about aliasing allows for significantly
better code generation.

This allows to use the C99 'restrict' keyword without breaking C89, or
for that matter C++, compilers. If not supported it's defined to be
empty.

Author: Andres Freund
Discussion: https://postgr.es/m/20170914063418.sckdzgjfrsbekae4@alap3.anarazel.de

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/0b974dba2d6b5581ce422ed883209de46f313fb6

Modified Files
--------------
configure                     | 46 +++++++++++++++++++++++++++++++++++++++++++
configure.in                  |  1 +
src/include/pg_config.h.in    | 14 +++++++++++++
src/include/pg_config.h.win32 | 11 +++++++++++
4 files changed, 72 insertions(+)

  2. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T00:13:20Z

    Hi,

On 2017-10-11 23:11:15 +0000, Andres Freund wrote:
> Add configure infrastructure to detect support for C99's restrict.
> 
> Will be used in later commits improving performance for a few key
> routines where information about aliasing allows for significantly
> better code generation.
> 
> This allows to use the C99 'restrict' keyword without breaking C89, or
> for that matter C++, compilers. If not supported it's defined to be
> empty.

Woodlouse doesn't like this, erroring out with:
C:\buildfarm\buildenv\HEAD\pgsql.build\src\include\libpq/pqformat.h(47): error C2219: syntax error : type qualifier must be after '*' (src/backend/access/common/printsimple.c) [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]

It's MSVC being super peculiar about error checks. I think msvc is just
confused by the pointer hiding typedef. Using some online MSVC (and
other compilers) frontend:
https://godbolt.org/g/TD3nmA

I confirmed that removing the pointer hiding typedef indeed resolves the
isssue.

I can't quite decide whether msvc just has taste and dislikes pointer
hiding typedefs as much as I do, or whether it's incredibly stupid ;)

I'll add a comment and use StringInfoData *.

Greetings,

Andres Freund

  3. Re: [COMMITTERS] pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T04:59:53Z

    On 2017-10-11 17:13:20 -0700, Andres Freund wrote:
> Hi,
> 
> On 2017-10-11 23:11:15 +0000, Andres Freund wrote:
> > Add configure infrastructure to detect support for C99's restrict.
> > 
> > Will be used in later commits improving performance for a few key
> > routines where information about aliasing allows for significantly
> > better code generation.
> > 
> > This allows to use the C99 'restrict' keyword without breaking C89, or
> > for that matter C++, compilers. If not supported it's defined to be
> > empty.
> 
> Woodlouse doesn't like this, erroring out with:
> C:\buildfarm\buildenv\HEAD\pgsql.build\src\include\libpq/pqformat.h(47): error C2219: syntax error : type qualifier must be after '*' (src/backend/access/common/printsimple.c) [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]
> 
> It's MSVC being super peculiar about error checks. I think msvc is just
> confused by the pointer hiding typedef. Using some online MSVC (and
> other compilers) frontend:
> https://godbolt.org/g/TD3nmA
> 
> I confirmed that removing the pointer hiding typedef indeed resolves the
> isssue.
> 
> I can't quite decide whether msvc just has taste and dislikes pointer
> hiding typedefs as much as I do, or whether it's incredibly stupid ;)
> 
> I'll add a comment and use StringInfoData *.

That fixed that problem I think. But unfortunately since then another
problem has been reported by some other animals, all with older msvc
versions afaict (thrips - vs 2012, bowerbird - vs 2012). Those report
that the defining of restrict to __restrict interfers with some system
headers:
C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\include\stdlib.h(619): error C2485: '__restrict' : unrecognized extended attribute (src/backend/access/brin/brin.c) [c:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]

Presumably that is because the headers contain __declspec(restrict) on
some function declarations.

I've temporarily silenced that error by moving the stdlib.h include
before the definition of restrict, but that seems fairly fragile. I
primarily wanted to see whether there's other problems.  At least thrips
is is now happy.

I see a number of options to continue:
- only define restrict on msvc 2013+ - for some reason woodlouse didn't
  complain about this problem, but I'm very doubtful that that's
  actually reliable.
- rename restrict to pg_restrict. That's annoying because we'd have to
  copy the autoconf test.
- live with the hack of including stdlib.h early, in pg_config.h.win32.
- $better_idea

Comments?

Greetings,

Andres Freund

  4. Re: [HACKERS] Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T05:05:13Z

    On 2017-10-11 21:59:53 -0700, Andres Freund wrote:
> That fixed that problem I think. But unfortunately since then another
> problem has been reported by some other animals, all with older msvc
> versions afaict (thrips - vs 2012, bowerbird - vs 2012).

Correction, thrips is vs 2010, not 2012.

  5. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-12T15:30:00Z

    Andres Freund <andres@anarazel.de> writes:
> I've temporarily silenced that error by moving the stdlib.h include
> before the definition of restrict, but that seems fairly fragile. I
> primarily wanted to see whether there's other problems.  At least thrips
> is is now happy.

> I see a number of options to continue:
> - only define restrict on msvc 2013+ - for some reason woodlouse didn't
>   complain about this problem, but I'm very doubtful that that's
>   actually reliable.
> - rename restrict to pg_restrict. That's annoying because we'd have to
>   copy the autoconf test.
> - live with the hack of including stdlib.h early, in pg_config.h.win32.
> - $better_idea

I don't actually see why you need a #define at all --- "restrict" is the
standard spelling of the keyword no?

I really do not like the stdlib.h hack: that amounts to assuming that
only stdlib.h does or ever will contain declspec(restrict).  Maybe
you could get away with that if you were applying it only to long-dead
MSVC versions, but doing it "#if (_MSC_VER >= 1500)" is clearly going
to break someday.

			regards, tom lane

  6. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T17:45:44Z

    On 2017-10-12 11:30:00 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I've temporarily silenced that error by moving the stdlib.h include
> > before the definition of restrict, but that seems fairly fragile. I
> > primarily wanted to see whether there's other problems.  At least thrips
> > is is now happy.
> 
> > I see a number of options to continue:
> > - only define restrict on msvc 2013+ - for some reason woodlouse didn't
> >   complain about this problem, but I'm very doubtful that that's
> >   actually reliable.
> > - rename restrict to pg_restrict. That's annoying because we'd have to
> >   copy the autoconf test.
> > - live with the hack of including stdlib.h early, in pg_config.h.win32.
> > - $better_idea
> 
> I don't actually see why you need a #define at all --- "restrict" is the
> standard spelling of the keyword no?

It is, but a lot of compilers name it differently, e.g. __restrict in
the case of msvc.


> I really do not like the stdlib.h hack: that amounts to assuming that
> only stdlib.h does or ever will contain declspec(restrict).  Maybe
> you could get away with that if you were applying it only to long-dead
> MSVC versions, but doing it "#if (_MSC_VER >= 1500)" is clearly going
> to break someday.

Yea, I dislike it quite a bit too. Unfortunately not even defining
restrict to empty as if it were unsupported looks viable to me :(

Greetings,

Andres Freund

  7. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-12T17:55:07Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-10-12 11:30:00 -0400, Tom Lane wrote:
>> I don't actually see why you need a #define at all --- "restrict" is the
>> standard spelling of the keyword no?

> It is, but a lot of compilers name it differently, e.g. __restrict in
> the case of msvc.

It's 2017 and they're still not C99 compliant?  Oh well.

TBH, I really doubt that restrict buys us enough performance to justify
dealing with this.  I'd just revert that change altogether.

Or, if you insist on having it, we're going to have to go the pg_restrict
route.  I don't see why that means duplicating any configure logic: on
non-Windows we can use the autoconf probe and then write
"#define pg_restrict restrict".

			regards, tom lane

  8. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T18:03:34Z

    Hi,


On 2017-10-12 13:55:07 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-10-12 11:30:00 -0400, Tom Lane wrote:
> >> I don't actually see why you need a #define at all --- "restrict" is the
> >> standard spelling of the keyword no?
> 
> > It is, but a lot of compilers name it differently, e.g. __restrict in
> > the case of msvc.
> 
> It's 2017 and they're still not C99 compliant?  Oh well.

...


> TBH, I really doubt that restrict buys us enough performance to justify
> dealing with this.  I'd just revert that change altogether.

It's quite noticeable, and there's plenty of other case where it seems
likely to be beneficial.


> Or, if you insist on having it, we're going to have to go the pg_restrict
> route.  I don't see why that means duplicating any configure logic: on
> non-Windows we can use the autoconf probe and then write
> "#define pg_restrict restrict".

Yea, that should work. I'll try to come up with a patch.

Greetings,

Andres Freund

  9. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Andres Freund <andres@anarazel.de> — 2017-10-12T22:35:11Z

    Hi,

On 2017-10-12 11:03:34 -0700, Andres Freund wrote:
> On 2017-10-12 13:55:07 -0400, Tom Lane wrote:
> > Or, if you insist on having it, we're going to have to go the pg_restrict
> > route.  I don't see why that means duplicating any configure logic: on
> > non-Windows we can use the autoconf probe and then write
> > "#define pg_restrict restrict".
> 
> Yea, that should work. I'll try to come up with a patch.

We can't do so unconditionally in c.h or such, because that'd again
cause conflicts with __declspec(restrict) on MSVC versions that don't
support restrict, because it'd require restrict to be defined empty.

But it's easy to do so in configure, and then have a separate definition
in pg_config.h.win32. Done so in the attached commit. It's slightly ugly
to have two definitions of restrict in pg_config.h.in, but whatever.

Greetings,

Andres Freund

  10. Re: pgsql: Add configure infrastructure to detect support for C99's restric

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-12T23:14:17Z

    Andres Freund <andres@anarazel.de> writes:
> But it's easy to do so in configure, and then have a separate definition
> in pg_config.h.win32. Done so in the attached commit. It's slightly ugly
> to have two definitions of restrict in pg_config.h.in, but whatever.

WFM.

			regards, tom lane