Horton

Thread

Commits

  1. Choose ppc compare_exchange constant path for more operand values.

  2. For ppc gcc, implement 64-bit compare_exchange and fetch_add with asm.

  3. For all ppc compilers, implement compare_exchange and fetch_add with asm.

  1. pgsql: For all ppc compilers, implement compare_exchange and fetch_add

    Noah Misch <noah@leadboat.com> — 2019-10-19T03:27:18Z

    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

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/30ee5d17c20dbb282a9952b3048d6ad52d56c371

Modified Files
--------------
configure                              |  40 ++++++
configure.in                           |  20 +++
src/include/pg_config.h.in             |   3 +
src/include/port/atomics.h             |  11 +-
src/include/port/atomics/arch-ppc.h    | 231 +++++++++++++++++++++++++++++++++
src/include/port/atomics/generic-xlc.h | 142 --------------------
src/tools/pginclude/cpluspluscheck     |   1 -
src/tools/pginclude/headerscheck       |   1 -
8 files changed, 298 insertions(+), 151 deletions(-)

  2. 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-09T09:28:25Z

    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

  3. 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-09T10:01:17Z

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

  4. 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-11T05:10:43Z

    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.

  5. 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-11T17:12:40Z

    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

  6. 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-11T18:35:13Z

    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

  7. 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> — 2020-10-12T01:16:46Z

    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

  8. 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-12T04:46:40Z

    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.