pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpowerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
7+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2020-10-12 88ea7a1188 5efa788e1d Choose ppc compare_exchange constant path for more operand values.
2020-10-12 f5c1167b17 d41cb63ff4 For ppc gcc, implement 64-bit compare_exchange and fetch_add with asm.
* powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
@ 2020-10-09 09:28 Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Berg @ 2020-10-09 09:28 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@lists.postgresql.org
Re: Noah Misch
> For all ppc compilers, implement compare_exchange and fetch_add with asm.
>
> This is more like how we handle s_lock.h and arch-x86.h.
>
> Reviewed by Tom Lane.
>
> Discussion: https://postgr.es/m/20191005173400.GA3979129@rfd.leadboat.com
Hi,
pg-cron on powerpc/ppc64/ppc64el is raising this warning inside the
ppc atomics:
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -std=c99 -Wall -Wextra -Werror -Wno-unknown-warning-option -Wno-unused-parameter -Wno-maybe-uninitialized -Wno-implicit-fallthrough -Iinclude -I/usr/include/postgresql -I. -I./ -I/usr/include/postgresql/13/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o src/job_metadata.o src/job_metadata.c
In file included from /usr/include/postgresql/13/server/port/atomics.h:74,
from /usr/include/postgresql/13/server/utils/dsa.h:17,
from /usr/include/postgresql/13/server/nodes/tidbitmap.h:26,
from /usr/include/postgresql/13/server/access/genam.h:19,
from src/job_metadata.c:21:
/usr/include/postgresql/13/server/port/atomics/arch-ppc.h: In function ‘pg_atomic_compare_exchange_u32_impl’:
/usr/include/postgresql/13/server/port/atomics/arch-ppc.h:97:42: error: comparison of integer expressions of different signedness: ‘uint32’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
97 | *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
| ^~
src/job_metadata.c: At top level:
cc1: note: unrecognized command-line option ‘-Wno-unknown-warning-option’ may have been intended to silence earlier diagnostics
cc1: all warnings being treated as errors
Looking at the pg_atomic_compare_exchange_u32_impl, this looks like a
genuine problem:
static inline bool
pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
uint32 *expected, uint32 newval)
...
if (__builtin_constant_p(*expected) &&
*expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
If *expected is an unsigned integer, comparing it to PG_INT16_MIN
which is a negative number doesn't make sense.
src/include/c.h:#define PG_INT16_MIN (-0x7FFF-1)
Christoph
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
@ 2020-10-09 10:01 ` Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 7+ messages in thread
From: Noah Misch @ 2020-10-09 10:01 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Oct 09, 2020 at 11:28:25AM +0200, Christoph Berg wrote:
> pg-cron on powerpc/ppc64/ppc64el is raising this warning inside the
> ppc atomics:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -std=c99 -Wall -Wextra -Werror -Wno-unknown-warning-option -Wno-unused-parameter -Wno-maybe-uninitialized -Wno-implicit-fallthrough -Iinclude -I/usr/include/postgresql -I. -I./ -I/usr/include/postgresql/13/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o src/job_metadata.o src/job_metadata.c
> In file included from /usr/include/postgresql/13/server/port/atomics.h:74,
> from /usr/include/postgresql/13/server/utils/dsa.h:17,
> from /usr/include/postgresql/13/server/nodes/tidbitmap.h:26,
> from /usr/include/postgresql/13/server/access/genam.h:19,
> from src/job_metadata.c:21:
> /usr/include/postgresql/13/server/port/atomics/arch-ppc.h: In function ‘pg_atomic_compare_exchange_u32_impl’:
> /usr/include/postgresql/13/server/port/atomics/arch-ppc.h:97:42: error: comparison of integer expressions of different signedness: ‘uint32’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
> 97 | *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
> | ^~
> src/job_metadata.c: At top level:
> cc1: note: unrecognized command-line option ‘-Wno-unknown-warning-option’ may have been intended to silence earlier diagnostics
> cc1: all warnings being treated as errors
>
> Looking at the pg_atomic_compare_exchange_u32_impl, this looks like a
> genuine problem:
>
> static inline bool
> pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
> uint32 *expected, uint32 newval)
> ...
> if (__builtin_constant_p(*expected) &&
> *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
>
> If *expected is an unsigned integer, comparing it to PG_INT16_MIN
> which is a negative number doesn't make sense.
>
> src/include/c.h:#define PG_INT16_MIN (-0x7FFF-1)
Agreed. I'll probably fix it like this:
inline patch 1: src/include/port/atomics/arch-ppc.h (+4/-2 lines) download inline-1.patch
--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -96,3 +96,4 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int32) *expected <= PG_INT16_MAX &&
+ (int32) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
@@ -185,3 +186,4 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int64) *expected <= PG_INT16_MAX &&
+ (int64) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
@ 2020-10-11 05:10 ` Noah Misch <noah@leadboat.com>
2020-10-11 17:12 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Noah Misch @ 2020-10-11 05:10 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Oct 09, 2020 at 03:01:17AM -0700, Noah Misch wrote:
> On Fri, Oct 09, 2020 at 11:28:25AM +0200, Christoph Berg wrote:
> > pg-cron on powerpc/ppc64/ppc64el is raising this warning inside the
> > ppc atomics:
> >
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -std=c99 -Wall -Wextra -Werror -Wno-unknown-warning-option -Wno-unused-parameter -Wno-maybe-uninitialized -Wno-implicit-fallthrough -Iinclude -I/usr/include/postgresql -I. -I./ -I/usr/include/postgresql/13/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o src/job_metadata.o src/job_metadata.c
> > In file included from /usr/include/postgresql/13/server/port/atomics.h:74,
> > from /usr/include/postgresql/13/server/utils/dsa.h:17,
> > from /usr/include/postgresql/13/server/nodes/tidbitmap.h:26,
> > from /usr/include/postgresql/13/server/access/genam.h:19,
> > from src/job_metadata.c:21:
> > /usr/include/postgresql/13/server/port/atomics/arch-ppc.h: In function ‘pg_atomic_compare_exchange_u32_impl’:
> > /usr/include/postgresql/13/server/port/atomics/arch-ppc.h:97:42: error: comparison of integer expressions of different signedness: ‘uint32’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
> > 97 | *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
> > | ^~
> > src/job_metadata.c: At top level:
> > cc1: note: unrecognized command-line option ‘-Wno-unknown-warning-option’ may have been intended to silence earlier diagnostics
> > cc1: all warnings being treated as errors
> >
> > Looking at the pg_atomic_compare_exchange_u32_impl, this looks like a
> > genuine problem:
> >
> > static inline bool
> > pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
> > uint32 *expected, uint32 newval)
> > ...
> > if (__builtin_constant_p(*expected) &&
> > *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
> >
> > If *expected is an unsigned integer, comparing it to PG_INT16_MIN
> > which is a negative number doesn't make sense.
> >
> > src/include/c.h:#define PG_INT16_MIN (-0x7FFF-1)
>
> Agreed. I'll probably fix it like this:
The first attachment fixes the matter you've reported. While confirming that,
I observed that gcc builds don't even use the 64-bit code in arch-ppc.h.
Oops. The second attachment fixes that. I plan not to back-patch either of
these.
Attachments: download all (2 files, .tar.gz)
compare_exchange-ppc-immediate-v1.patch (text/plain, 2.4K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Choose ppc compare_exchange constant path for more operand values.
The implementation uses smaller code when the "expected" operand is a
small constant, but the implementation needlessly defined the set of
acceptable constants more narrowly than the ABI does. Core PostgreSQL
and PGXN don't use the constant path at all, so this is future-proofing.
Reviewed by FIXME. Reported by Christoph Berg.
Discussion: https://postgr.es/m/20201009092825.GD889580@msg.df7cb.de
diff --git a/src/include/port/atomics/arch-ppc.h b/src/include/port/atomics/arch-ppc.h
index 68e6603..9b2e499 100644
--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -94,7 +94,8 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int32) *expected <= PG_INT16_MAX &&
+ (int32) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
" sync \n"
" lwarx %0,0,%5 \n"
@@ -183,7 +184,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
/* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int64) *expected <= PG_INT16_MAX &&
+ (int64) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
" sync \n"
" ldarx %0,0,%5 \n"
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 02397f2..09bc42a 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -720,6 +720,14 @@ test_atomic_uint32(void)
EXPECT_EQ_U32(pg_atomic_read_u32(&var), (uint32) INT_MAX + 1);
EXPECT_EQ_U32(pg_atomic_sub_fetch_u32(&var, INT_MAX), 1);
pg_atomic_sub_fetch_u32(&var, 1);
+ expected = PG_INT16_MAX;
+ EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
+ expected = PG_INT16_MAX + 1;
+ EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
+ expected = PG_INT16_MIN;
+ EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
+ expected = PG_INT16_MIN - 1;
+ EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
/* fail exchange because of old expected */
expected = 10;
atomics-ppc64-gcc-v1.patch (text/plain, 1.0K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
For ppc gcc, implement 64-bit compare_exchange and fetch_add with asm.
While xlc defines __64BIT__, gcc does not. Due to this oversight in
commit 30ee5d17c20dbb282a9952b3048d6ad52d56c371, gcc builds continued
implementing 64-bit atomics by way of intrinsics.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/include/port/atomics/arch-ppc.h b/src/include/port/atomics/arch-ppc.h
index fdfe0d0..68e6603 100644
--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -32,14 +32,14 @@ typedef struct pg_atomic_uint32
} pg_atomic_uint32;
/* 64bit atomics are only supported in 64bit mode */
-#ifdef __64BIT__
+#if SIZEOF_VOID_P >= 8
#define PG_HAVE_ATOMIC_U64_SUPPORT
typedef struct pg_atomic_uint64
{
volatile uint64 value pg_attribute_aligned(8);
} pg_atomic_uint64;
-#endif /* __64BIT__ */
+#endif
/*
* This mimics gcc __atomic_compare_exchange_n(..., __ATOMIC_SEQ_CST), but
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
@ 2020-10-11 17:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-11 18:35 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-12 04:46 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 7+ messages in thread
From: Tom Lane @ 2020-10-11 17:12 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Christoph Berg <myon@debian.org>; pgsql-hackers@lists.postgresql.org
Noah Misch <noah@leadboat.com> writes:
> The first attachment fixes the matter you've reported. While confirming that,
> I observed that gcc builds don't even use the 64-bit code in arch-ppc.h.
> Oops. The second attachment fixes that.
I reviewed these, and tested the first one on a nearby Apple machine.
(I lack access to 64-bit PPC, so I can't actually test the second.)
They look fine, and I confirmed by examining asm output that even
the rather-old-now gcc version that Apple last shipped for PPC does
the right thing with the conditionals.
> I plan not to back-patch either of these.
Hmm, I'd argue for a back-patch. The issue of modern compilers
warning about the incorrect code will apply to all supported branches.
Moreover, even if we don't use these code paths today, who's to say
that someone won't back-patch a bug fix that requires them? I do not
think it's unreasonable to expect these functions to work well in
all branches that have them.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 17:12 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-10-11 18:35 ` Christoph Berg <myon@debian.org>
2020-10-12 01:16 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 7+ messages in thread
From: Christoph Berg @ 2020-10-11 18:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers@lists.postgresql.org
Re: Tom Lane
> > I plan not to back-patch either of these.
>
> Hmm, I'd argue for a back-patch. The issue of modern compilers
> warning about the incorrect code will apply to all supported branches.
> Moreover, even if we don't use these code paths today, who's to say
> that someone won't back-patch a bug fix that requires them? I do not
> think it's unreasonable to expect these functions to work well in
> all branches that have them.
Or remove them. (But fixing seems better.)
Christoph
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 17:12 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Tom Lane <tgl@sss.pgh.pa.us>
2020-10-11 18:35 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
@ 2020-10-12 01:16 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2020-10-12 01:16 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Noah Misch <noah@leadboat.com>; pgsql-hackers@lists.postgresql.org
On Sun, Oct 11, 2020 at 08:35:13PM +0200, Christoph Berg wrote:
> Re: Tom Lane
>> Hmm, I'd argue for a back-patch. The issue of modern compilers
>> warning about the incorrect code will apply to all supported branches.
>> Moreover, even if we don't use these code paths today, who's to say
>> that someone won't back-patch a bug fix that requires them? I do not
>> think it's unreasonable to expect these functions to work well in
>> all branches that have them.
>
> Or remove them. (But fixing seems better.)
The patch is not that invasive, so just fixing back-branches sounds
like a good idea to me. My 2c.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Noah Misch <noah@leadboat.com>
2020-10-11 17:12 ` Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-10-12 04:46 ` Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 7+ messages in thread
From: Noah Misch @ 2020-10-12 04:46 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Christoph Berg <myon@debian.org>; pgsql-hackers@lists.postgresql.org
On Sun, Oct 11, 2020 at 01:12:40PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > The first attachment fixes the matter you've reported. While confirming that,
> > I observed that gcc builds don't even use the 64-bit code in arch-ppc.h.
> > Oops. The second attachment fixes that.
>
> I reviewed these, and tested the first one on a nearby Apple machine.
> (I lack access to 64-bit PPC, so I can't actually test the second.)
> They look fine, and I confirmed by examining asm output that even
> the rather-old-now gcc version that Apple last shipped for PPC does
> the right thing with the conditionals.
Thanks for reviewing and for mentioning that old-gcc behavior. I had a
comment asserting that gcc 7.2.0 didn't deduce constancy from those
conditionals. Checking again now, it was just $SUBJECT preventing constancy
deduction. I made the patch remove that comment.
> > I plan not to back-patch either of these.
>
> Hmm, I'd argue for a back-patch. The issue of modern compilers
> warning about the incorrect code will apply to all supported branches.
> Moreover, even if we don't use these code paths today, who's to say
> that someone won't back-patch a bug fix that requires them? I do not
> think it's unreasonable to expect these functions to work well in
> all branches that have them.
Okay, I've pushed with a back-patch. compare_exchange-ppc-immediate-v1.patch
affects on code generation are limited to regress.o, so it's quite safe to
back-patch. I just didn't think it was standard to back-patch for the purpose
of removing a -Wsign-compare warning. (Every branch is noisy under
-Wsign-compare.)
atomics-ppc64-gcc-v1.patch does change code generation, in the manner
discussed in the big arch-ppc.h comment (starts with "This mimics gcc").
Still, I've accepted the modest risk.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2020-10-12 04:46 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 09:28 powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add Christoph Berg <myon@debian.org>
2020-10-09 10:01 ` Noah Misch <noah@leadboat.com>
2020-10-11 05:10 ` Noah Misch <noah@leadboat.com>
2020-10-11 17:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-11 18:35 ` Christoph Berg <myon@debian.org>
2020-10-12 01:16 ` Michael Paquier <michael@paquier.xyz>
2020-10-12 04:46 ` Noah Misch <noah@leadboat.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox