Re: cpluspluscheck complains about use of register
Christoph Berg 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
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
Hi, On 2022-09-24 16:01:25 -0400, Tom Lane wrote: > Andres Freund 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
Andres Freund 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
On Sat, Sep 24, 2022 at 12:11 PM Andres Freund 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
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 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
>From 03bf971d2dc701d473705fd00891028d140dd5ae Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sat, 24 Sep 2022 12:01:06 -0700
Subject: [PATCH v1] Remove uses of register due to incompatibility with C++17
and up
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.
Discussion: https://postgr.es/m/20220308185902.ibdqmasoaunzj...@alap3.anarazel.de
---
src/include/port/atomics/arch-x86.h | 2 +-
src/include/storage/s_lock.h| 14 +++---
src/backend/regex/regexec.c | 2 +-
.cirrus.yml | 4 +---
4 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/include/port/atomics/arch-x86.h b/src/include/port/atomics/arch-x86.h
index cef1ba724c9..6c0b917f12e 100644
--- a/src/include/port/atomics/arch-x86.h
+++ b/src/include/port/atomics/arch-x86.h
@@ -140,7 +140,7 @@ pg_spin_delay_impl(void)
static inline bool
pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
{
- register char _res = 1;
+ char _res = 1;
__asm__ __volatile__(
" lock \n"
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 65aa66c5984..4225d9b7fc3 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -142,7 +142,7 @@ typedef unsigned char slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
- register slock_t _res = 1;
+ slock_t _res = 1;
/*
* Use a non-locking test before asserting the bus lock. Note that the
@@ -223,7 +223,7 @@ typedef unsigned char slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
- register slock_t _res = 1;
+ slock_t _res = 1;
__asm__ __volatile__(
" lock \n"
@@ -356,7 +356,7 @@ typedef unsigned char slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
- register slock_t _res;
+ slock_t _res;
/*
* See comment in src/backend/port/tas/sunstudio_sparc.s for why this
@@ -511,9 +511,9 @@ typedef unsigned int slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
- register volatile slock_t *_l = lock;
- register int _res;
- register int _tmp;
+ volatile slock_t *_l = lock;
+ int _res;
+ int _tmp;
__asm__ __volatile__(
" .set push \n"
@@ -574,7 +574,7 @@ static __inline__ int
tas(volatile slock_t *lock)
{
volatile int *lockword = TAS_ACTIVE_WORD(lock);
- register int lockval;
+ int lockval;
/*
* The LDCWX instruction atomically clears the target word and
diff --git a/src/backend/regex/regexec.c
Re: cpluspluscheck complains about use of register
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 = _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
Hi, On 2022-03-08 13:46:36 -0500, Tom Lane wrote: > Andres Freund 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
Andres Freund 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
