Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Remove uses of register due to incompatibility with C++17 and up
- 03bf971d2dc7 16.0 landed

cpluspluscheck complains about use of register

Andres Freund <andres@anarazel.de> — 2022-03-08T18:18:37Z

Hi,

When running cpluspluscheck I get many many complaints like

In file included from /tmp/pg-test-repo/src/include/port/atomics.h:70,
                 from /tmp/pg-test-repo/src/include/utils/dsa.h:17,
                 from /tmp/pg-test-repo/src/include/nodes/tidbitmap.h:26,
                 from /tmp/pg-test-repo/src/include/nodes/execnodes.h:24,
                 from /tmp/pg-test-repo/src/include/commands/trigger.h:17,
                 from /tmp/pg-test-repo/src/pl/plpgsql/src/plpgsql.h:21,
                 from /tmp/cpluspluscheck.qOi18T/test.cpp:3:
/tmp/pg-test-repo/src/include/port/atomics/arch-x86.h: In function ‘bool pg_atomic_test_set_flag_impl(volatile pg_atomic_flag*)’:
/tmp/pg-test-repo/src/include/port/atomics/arch-x86.h:143:23: warning: ISO C++17 does not allow ‘register’ storage class specifier [-Wregister]
  143 |         register char _res = 1;
      |                       ^~~~

It seems we should just remove the use of register? It's currently only used
in
src/include/storage/s_lock.h
src/include/port/atomics/arch-x86.h

From what I understand compilers essentially have been ignoring it for quite a
while...

Greetings,

Andres Freund

Re: cpluspluscheck complains about use of register

Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-08T18:46:36Z

Andres Freund <andres@anarazel.de> writes:
> When running cpluspluscheck I get many many complaints like
> /tmp/pg-test-repo/src/include/port/atomics/arch-x86.h:143:23: warning: ISO C++17 does not allow ‘register’ storage class specifier [-Wregister]

Interesting, I don't see that here.

> It seems we should just remove the use of register?

I have a vague idea that it was once important to say "register" if
you are going to use the variable in an asm snippet that requires it
to be in a register.  That might be wrong, or it might be obsolete
even if once true.  We could try taking these out and seeing if the
buildfarm complains.  (If so, maybe -Wno-register would help?)

			regards, tom lane

Re: cpluspluscheck complains about use of register

Andres Freund <andres@anarazel.de> — 2022-03-08T18:59:02Z

Hi,

On 2022-03-08 13:46:36 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > When running cpluspluscheck I get many many complaints like
> > /tmp/pg-test-repo/src/include/port/atomics/arch-x86.h:143:23: warning: ISO C++17 does not allow ‘register’ storage class specifier [-Wregister]
>
> Interesting, I don't see that here.

Probably a question of the gcc version. I think starting with 11 g++ defaults
to C++ 17.

> > It seems we should just remove the use of register?
>
> I have a vague idea that it was once important to say "register" if
> you are going to use the variable in an asm snippet that requires it
> to be in a register.  That might be wrong, or it might be obsolete
> even if once true.  We could try taking these out and seeing if the
> buildfarm complains.

We have several inline asm statements not using register despite using
variables in a register (e.g. pg_atomic_compare_exchange_u32_impl()), so I
wouldn't expect a problem with compilers we support.

Should we make configure test for -Wregister? There's at least one additional
use of register that we'd have to change (pg_regexec).

> (If so, maybe -Wno-register would help?)

That's what I did to work around the flood of warnings locally, so it'd
work.

Greetings,

Andres Freund

Re: cpluspluscheck complains about use of register

Fabien COELHO <coelho@cri.ensmp.fr> — 2022-03-09T10:08:57Z

>>> It seems we should just remove the use of register?
>>
>> I have a vague idea that it was once important to say "register" if
>> you are going to use the variable in an asm snippet that requires it
>> to be in a register.  That might be wrong, or it might be obsolete
>> even if once true.  We could try taking these out and seeing if the
>> buildfarm complains.
>
> We have several inline asm statements not using register despite using
> variables in a register (e.g. pg_atomic_compare_exchange_u32_impl()), so I
> wouldn't expect a problem with compilers we support.
>
> Should we make configure test for -Wregister? There's at least one additional
> use of register that we'd have to change (pg_regexec).

From a compilation perspective, "register" tells the compiler that you 
cannot have a pointer on a variable, i.e. it generates an error if someone 
adds something like:

    void * p = &register_variable;

Removing the "register" declaration means that such protection would be 
removed, and creating such a pointer could reduce drastically compiler 
optimization opportunities.

-- 
Fabien.

Re: cpluspluscheck complains about use of register

Andres Freund <andres@anarazel.de> — 2022-09-24T19:11:40Z

Hi,

On 2022-03-08 10:59:02 -0800, Andres Freund wrote:
> On 2022-03-08 13:46:36 -0500, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > When running cpluspluscheck I get many many complaints like
> > > /tmp/pg-test-repo/src/include/port/atomics/arch-x86.h:143:23: warning: ISO C++17 does not allow ‘register’ storage class specifier [-Wregister]
> >
> > Interesting, I don't see that here.
> 
> Probably a question of the gcc version. I think starting with 11 g++ defaults
> to C++ 17.
> 
> 
> > > It seems we should just remove the use of register?
> >
> > I have a vague idea that it was once important to say "register" if
> > you are going to use the variable in an asm snippet that requires it
> > to be in a register.  That might be wrong, or it might be obsolete
> > even if once true.  We could try taking these out and seeing if the
> > buildfarm complains.
> 
> We have several inline asm statements not using register despite using
> variables in a register (e.g. pg_atomic_compare_exchange_u32_impl()), so I
> wouldn't expect a problem with compilers we support.
> 
> Should we make configure test for -Wregister? There's at least one additional
> use of register that we'd have to change (pg_regexec).
> 
> 
> > (If so, maybe -Wno-register would help?)
> 
> That's what I did to work around the flood of warnings locally, so it'd
> work.

I hit this again while porting cplupluscheck to be invoked by meson as
well. ISTM that we should just remove the uses of register. Yes, some very old
compilers might generate worse code without register, but I don't think we
need to care about peak efficiency with neolithic compilers.

Fabien raised the concern that removing register might lead to accidentally
adding pointers to such variables - I don't find that convincing, because a)
such code is typically inside a helper inline anyway b) we don't use register
widely enough to ensure this.

Attached is a patch removing uses of register. The use in regexec.c could
remain, since we only try to keep headers C++ clean. But there really doesn't
seem to be a good reason to use register in that spot.

I tried to use -Wregister to keep us honest going forward, but unfortunately
it only works with a C++ compiler...

I tested this by redefining register to something else, and I grepped for
non-comment uses of register. Entirely possible that I missed something.

Greetings,

Andres Freund

Re: cpluspluscheck complains about use of register

Peter Geoghegan <pg@bowt.ie> — 2022-09-24T19:59:30Z

On Sat, Sep 24, 2022 at 12:11 PM Andres Freund <andres@anarazel.de> wrote:
> I hit this again while porting cplupluscheck to be invoked by meson as
> well. ISTM that we should just remove the uses of register. Yes, some very old
> compilers might generate worse code without register, but I don't think we
> need to care about peak efficiency with neolithic compilers.

+1. I seem to recall reading that the register keyword was basically
useless as long as 15 years ago.

-- 
Peter Geoghegan

Re: cpluspluscheck complains about use of register

Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-24T20:01:25Z

Andres Freund <andres@anarazel.de> writes:
> I hit this again while porting cplupluscheck to be invoked by meson as
> well. ISTM that we should just remove the uses of register.

OK by me.

> I tried to use -Wregister to keep us honest going forward, but unfortunately
> it only works with a C++ compiler...

I think we only really care about stuff that cpluspluscheck would spot,
so I don't feel a need to mess with the standard compilation flags.

			regards, tom lane

Re: cpluspluscheck complains about use of register

Andres Freund <andres@anarazel.de> — 2022-09-24T22:13:00Z

Hi,

On 2022-09-24 16:01:25 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I hit this again while porting cplupluscheck to be invoked by meson as
> > well. ISTM that we should just remove the uses of register.
> 
> OK by me.

Done. Thanks Tom, Peter.

Re: cpluspluscheck complains about use of register

Christoph Berg <myon@debian.org> — 2024-02-12T11:03:01Z

Re: Tom Lane
> > I hit this again while porting cplupluscheck to be invoked by meson as
> > well. ISTM that we should just remove the uses of register.
> 
> OK by me.
> 
> > I tried to use -Wregister to keep us honest going forward, but unfortunately
> > it only works with a C++ compiler...
> 
> I think we only really care about stuff that cpluspluscheck would spot,
> so I don't feel a need to mess with the standard compilation flags.

This has started to hurt: postgresql-debversion (a Debian version number
data type written in C++) failed to build against Postgresql <= 15 on
Ubuntu's next LTS release (24.04):

In file included from /usr/include/postgresql/15/server/port/atomics.h:70:
/usr/include/postgresql/15/server/port/atomics/arch-x86.h:143:2: error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
  143 |         register char _res = 1;

I managed to work around it by putting `#define register` before
including the PG headers.

Should the removal of "register" be backported to support that better?

Christoph

Re: cpluspluscheck complains about use of register

Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-12T16:08:47Z

Christoph Berg <myon@debian.org> writes:
> Should the removal of "register" be backported to support that better?

Perhaps.  It's early days yet, but nobody has complained that that
broke anything in v16, so I'm guessing it'd be fine.

			regards, tom lane

Re: cpluspluscheck complains about use of register

Tristan Partin <tristan@partin.io> — 2024-10-25T20:07:10Z

On Fri Oct 25, 2024 at 3:04 PM CDT, Tom Lane wrote:
> Christoph Berg <myon@debian.org> writes:
>> Should the removal of "register" be backported to support that better?
>
> Perhaps.  It's early days yet, but nobody has complained that that
> broke anything in v16, so I'm guessing it'd be fine.

FWIW, I ran into this compiling an extension written in C++ for v15, so 
I'll throw my support for backpatching this if that is still on the 
table. Understandable if not, though.

-- 
Tristan Partin
Neon (https://neon.tech)

Re: cpluspluscheck complains about use of register

Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-25T20:19:25Z

"Tristan Partin" <tristan@partin.io> writes:
> FWIW, I ran into this compiling an extension written in C++ for v15, so 
> I'll throw my support for backpatching this if that is still on the 
> table. Understandable if not, though.

I'm inclined to think "too late".  Even if we back-patched to v15
and earlier now, your extension would probably still want to be
compilable against earlier minor releases, so the back-patch
would not help you a lot.  Christoph's workaround of
"#define register" is probably the best answer for old PG versions,
or you can compile with "-Wno-register".

			regards, tom lane

Re: cpluspluscheck complains about use of register

Tristan Partin <tristan@partin.io> — 2024-10-25T20:24:46Z

On Fri Oct 25, 2024 at 3:19 PM CDT, Tom Lane wrote:
> "Tristan Partin" <tristan@partin.io> writes:
>> FWIW, I ran into this compiling an extension written in C++ for v15, so 
>> I'll throw my support for backpatching this if that is still on the 
>> table. Understandable if not, though.
>
> I'm inclined to think "too late".  Even if we back-patched to v15
> and earlier now, your extension would probably still want to be
> compilable against earlier minor releases, so the back-patch
> would not help you a lot.  Christoph's workaround of
> "#define register" is probably the best answer for old PG versions,
> or you can compile with "-Wno-register".

For my purposes, I only need to support the latest minor releases of PG 
versions, so a backpatch would be useful come December (?). I do 
understand the "too late" argument though.

We did indeed fix the problem with "-Wno-register," though it's always 
nice to not have the manual fix. But hey, Postgres is a C project, so 
it's all good 😄. Thanks for getting back to me, Tom.

-- 
Tristan Partin
Neon (https://neon.tech)