Date: Sat, 10 Oct 2020 22:10:43 -0700
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
Message-ID: <20201011051043.GA1724101@rfd.leadboat.com>
References: <E1iLfOg-0005hK-98@gemulon.postgresql.org>
 <20201009092825.GD889580@msg.df7cb.de>
 <20201009100117.GA1550390@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="/04w6evG8XlLl3ft"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20201009100117.GA1550390@rfd.leadboat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--/04w6evG8XlLl3ft
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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.

--/04w6evG8XlLl3ft
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="compare_exchange-ppc-immediate-v1.patch"

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;

--/04w6evG8XlLl3ft
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="atomics-ppc64-gcc-v1.patch"

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

--/04w6evG8XlLl3ft--