pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Noah Misch <noah@leadboat.com>
To: Christoph Berg <myon@debian.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: 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
Date: Sat, 10 Oct 2020 22:10:43 -0700
Message-ID: <20201011051043.GA1724101@rfd.leadboat.com> (raw )
In-Reply-To: <20201009100117.GA1550390@rfd.leadboat.com >
References: <E1iLfOg-0005hK-98@gemulon.postgresql.org >
<20201009092825.GD889580@msg.df7cb.de >
<20201009100117.GA1550390@rfd.leadboat.com >
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
view thread (7+ messages) latest in thread
Message-ID: <20201011051043.GA1724101@rfd.leadboat.com>
Permalink: ../20201011051043.GA1724101@rfd.leadboat.com/
Also on: postgresql.org/message-id/20201011051043.GA1724101@rfd.leadboat.com
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, myon@debian.org, pgsql-hackers@lists.postgresql.org
Subject: 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
In-Reply-To: <20201011051043.GA1724101@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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