pgsql: For all ppc compilers, implement compare_exchange and fetch_add

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: /messages/by-id/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

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:

--- 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__(

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.

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

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

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

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.