Thread: pgsql: Replace our hacked version of ax_pthread.m4 with latest upstream

Replace our hacked version of ax_pthread.m4 with latest upstream version.

Our version was different from the upstream version in that we tried to use
all possible pthread-related flags that the compiler accepts, rather than
just the first one that works. That change was made in commit
e48322a6d6cfce1ec52ab303441df329ddbc04d1, to work-around a bug affecting GCC
versions 3.2 and below (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=8888),
although we didn't realize that it was a GCC bug at the time. We hardly care
about that old GCC versions anymore, so we no longer need that workaround.

This fixes the macro for compilers that print warnings with the chosen
flags. That's pretty annoying on its own right, but it also inconspicuously
disabled thread-safety, because we refused to use any pthread-related flags
if the compiler produced warnings. Max Filippov reported that problem when
linking with uClibc and OpenSSL. The warnings-check was added because the
workaround for the GCC bug caused warnings otherwise, so it's no longer
needed either. We can just use the upstream version as is.

If you really want to compile with GCC version 3.2 or older, you can still
work-around it manually by setting PTHREAD_CFLAGS="-pthread -lpthread"
manually on the configure command line.

Backpatch to 9.5. I don't want to unnecessarily rock the boat on stable
branches, but 9.5 seems like fair game.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/e97af6c8bfc80d084bca5bf41f036de944b63efe

Modified Files
--------------
aclocal.m4            |    2 +-
config/acx_pthread.m4 |  170 -------------------------
config/ax_pthread.m4  |  332 +++++++++++++++++++++++++++++++++++++++++++++++++
configure             |  310 ++++++++++++++++++++++++++++++++++++---------
configure.in          |    2 +-
5 files changed, 583 insertions(+), 233 deletions(-)

Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> Replace our hacked version of ax_pthread.m4 with latest upstream version.

Well, in fact that did not work.  See gharial: it appears to have picked
the wrong thread flags.

            regards, tom lane

I wrote:
> Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
>> Replace our hacked version of ax_pthread.m4 with latest upstream version.

> Well, in fact that did not work.  See gharial: it appears to have picked
> the wrong thread flags.

And even more annoying, now that I've git pull'd, I find it's also broken
the build on my RHEL6 box:

../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_ctl] Error 1
make[2]: *** [all-pg_ctl-recurse] Error 2
make[2]: *** Waiting for unfinished jobs....
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_receivexlog] Error 1
make[3]: *** Waiting for unfinished jobs....
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_basebackup] Error 1
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_recvlogical] Error 1
make[2]: *** [all-pg_basebackup-recurse] Error 2
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_dumpall] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [pg_rewind] Error 1
make[2]: *** [all-pg_rewind-recurse] Error 2
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_restore] Error 1
../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
collect2: ld returned 1 exit status
make[3]: *** [pg_dump] Error 1
make[2]: *** [all-pg_dump-recurse] Error 2
make[1]: *** [all-bin-recurse] Error 2
make: *** [all-src-recurse] Error 2

It appears the new improved macro doesn't bother with -pthread at all.
Previously I got this in Makefile.global:

PTHREAD_CFLAGS                =   -pthread  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS
PTHREAD_LIBS          =  -lpthread

Now I get

PTHREAD_CFLAGS                =  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS
PTHREAD_LIBS          =

and that simply doesn't work.  I do not know why the buildfarm isn't
entirely red, but I'm dead in the water.  Please revert this.

            regards, tom lane

On 07/08/2015 11:20 PM, Tom Lane wrote:
> I wrote:
>> Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
>>> Replace our hacked version of ax_pthread.m4 with latest upstream version.
>
>> Well, in fact that did not work.  See gharial: it appears to have picked
>> the wrong thread flags.
>
> And even more annoying, now that I've git pull'd, I find it's also broken
> the build on my RHEL6 box:
>
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_ctl] Error 1
> make[2]: *** [all-pg_ctl-recurse] Error 2
> make[2]: *** Waiting for unfinished jobs....
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_receivexlog] Error 1
> make[3]: *** Waiting for unfinished jobs....
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_basebackup] Error 1
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_recvlogical] Error 1
> make[2]: *** [all-pg_basebackup-recurse] Error 2
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_dumpall] Error 1
> make[3]: *** Waiting for unfinished jobs....
> make[3]: *** [pg_rewind] Error 1
> make[2]: *** [all-pg_rewind-recurse] Error 2
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_restore] Error 1
> ../../../src/interfaces/libpq/libpq.so: undefined reference to `pthread_sigmask'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_dump] Error 1
> make[2]: *** [all-pg_dump-recurse] Error 2
> make[1]: *** [all-bin-recurse] Error 2
> make: *** [all-src-recurse] Error 2
>
> It appears the new improved macro doesn't bother with -pthread at all.
> Previously I got this in Makefile.global:
>
> PTHREAD_CFLAGS                =   -pthread  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS
> PTHREAD_LIBS          =  -lpthread
>
> Now I get
>
> PTHREAD_CFLAGS                =  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS
> PTHREAD_LIBS          =
>
> and that simply doesn't work.  I do not know why the buildfarm isn't
> entirely red, but I'm dead in the water.  Please revert this.

I'm debugging this ATM. You're probably seeing a failure similar to
shearwater. The problem seems to be that the earlier autoconf tests add
"-lrt" to LIBS, and that somehow pulls in pthread_join() and other
functions, but not pthread_sigmask(), even when not compiling with -pthread.

I'll dig a bit deeper, but I think that could be fixed by moving the
AX_PTHREAD call in the configure script earlier, or by temporarily
clearing LIBS before calling it.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> I'm debugging this ATM. You're probably seeing a failure similar to
> shearwater. The problem seems to be that the earlier autoconf tests add
> "-lrt" to LIBS, and that somehow pulls in pthread_join() and other
> functions, but not pthread_sigmask(), even when not compiling with -pthread.

> I'll dig a bit deeper, but I think that could be fixed by moving the
> AX_PTHREAD call in the configure script earlier, or by temporarily
> clearing LIBS before calling it.

Ah.  The former sounds sensible from here.

This kind of points up that I've always thought the advice at the top of
configure.in is a tad broken:

dnl 0. Initialization and options processing
dnl 1. Programs
dnl 2. Libraries
dnl 3. Header files
dnl 4. Types
dnl 5. Structures
dnl 6. Compiler characteristics
dnl 7. Functions, global variables
dnl 8. System services

Why isn't "Compiler characteristics" a lot earlier in the list, certainly
before "Libraries"?

            regards, tom lane

On 07/08/2015 11:39 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> I'm debugging this ATM. You're probably seeing a failure similar to
>> shearwater. The problem seems to be that the earlier autoconf tests add
>> "-lrt" to LIBS, and that somehow pulls in pthread_join() and other
>> functions, but not pthread_sigmask(), even when not compiling with -pthread.
>
>> I'll dig a bit deeper, but I think that could be fixed by moving the
>> AX_PTHREAD call in the configure script earlier, or by temporarily
>> clearing LIBS before calling it.
>
> Ah.  The former sounds sensible from here.

Pushed that. Should make shearwater happy, but I'm not very sure what
the problem on gharial is. -lrt is not used there, but one of the other
libraries is having a similar effect there.

> This kind of points up that I've always thought the advice at the top of
> configure.in is a tad broken:
>
> dnl 0. Initialization and options processing
> dnl 1. Programs
> dnl 2. Libraries
> dnl 3. Header files
> dnl 4. Types
> dnl 5. Structures
> dnl 6. Compiler characteristics
> dnl 7. Functions, global variables
> dnl 8. System services
>
> Why isn't "Compiler characteristics" a lot earlier in the list, certainly
> before "Libraries"?

Hmm, yeah, although it's not clear what category the pthread check falls
into.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 07/08/2015 11:39 PM, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> I'll dig a bit deeper, but I think that could be fixed by moving the
>>> AX_PTHREAD call in the configure script earlier, or by temporarily
>>> clearing LIBS before calling it.

>> Ah.  The former sounds sensible from here.

> Pushed that. Should make shearwater happy, but I'm not very sure what
> the problem on gharial is. -lrt is not used there, but one of the other
> libraries is having a similar effect there.

While my RHEL6 box seems happy now, this change has completely broken
pademelon/gaur (which were not broken prior to the rearrangement),
and I believe it also accounts for frogmouth being unhappy.

The pademelon/gaur failure is because we are now failing to detect
any of the major standard C headers:

$ diff pg_config.h.broken pg_config.h.saved
266c266
< /* #undef HAVE_INTTYPES_H */
---
> #define HAVE_INTTYPES_H 1
351c351
< /* #undef HAVE_MEMORY_H */
---
> #define HAVE_MEMORY_H 1
466c466
< /* #undef HAVE_STDLIB_H */
---
> #define HAVE_STDLIB_H 1
475c475
< /* #undef HAVE_STRINGS_H */
---
> #define HAVE_STRINGS_H 1
478c478
< /* #undef HAVE_STRING_H */
---
> #define HAVE_STRING_H 1
568c568
< /* #undef HAVE_SYS_STAT_H */
---
> #define HAVE_SYS_STAT_H 1
577c577
< /* #undef HAVE_SYS_TYPES_H */
---
> #define HAVE_SYS_TYPES_H 1
614c614
< /* #undef HAVE_UNISTD_H */
---
> #define HAVE_UNISTD_H 1
780c780
< /* #undef STDC_HEADERS */
---
> #define STDC_HEADERS 1

Now, this is odd, because those critters build with
--disable-thread-safety and thus shouldn't be executing AX_PTHREAD
at all.  However, I can reproduce something similar to frogmouth's
symptoms on RHEL6 by configuring HEAD with --disable-thread-safety:
configure fails with "zlib version is too old".  And investigation
makes it look like something is completely broken in the major
system headers, not zlib proper, which is what frogmouth is showing.

I suspect the explanation is that physically moving the AX_PTHREAD call
before our regular header checks causes autoconf to move some of the
header checks to be before the AX_PTHREAD expansion, *without regard to
the fact that AX_PTHREAD might not get executed*.

>> This kind of points up that I've always thought the advice at the top of
>> configure.in is a tad broken:
>>
>> dnl 0. Initialization and options processing
>> dnl 1. Programs
>> dnl 2. Libraries
>> dnl 3. Header files
>> dnl 4. Types
>> dnl 5. Structures
>> dnl 6. Compiler characteristics
>> dnl 7. Functions, global variables
>> dnl 8. System services
>>
>> Why isn't "Compiler characteristics" a lot earlier in the list, certainly
>> before "Libraries"?

> Hmm, yeah, although it's not clear what category the pthread check falls
> into.

At this point it's crystal clear that that advice is dead wrong, and that
what we need to be doing is something along the lines of

Initialization and options processing
Compiler characteristics
Header files
Types
Structures
Functions, global variables
Pthreads
Other libraries
Programs
System services

Before we start such major whacking, though, I am curious to know where
Peter got the current ordering advice from.

Also, I'm wondering why we are doing this at all in the 9.5 branch,
because this is sure looking like development not stabilization.

            regards, tom lane

On 07/09/2015 06:17 AM, Tom Lane wrote:
> While my RHEL6 box seems happy now, this change has completely broken
> pademelon/gaur (which were not broken prior to the rearrangement),
> and I believe it also accounts for frogmouth being unhappy.
>
> The pademelon/gaur failure is because we are now failing to detect
> any of the major standard C headers:
>
> $ diff pg_config.h.broken pg_config.h.saved
> 266c266
> < /* #undef HAVE_INTTYPES_H */
> ---
>> #define HAVE_INTTYPES_H 1
> 351c351
> < /* #undef HAVE_MEMORY_H */
> ---
>> #define HAVE_MEMORY_H 1
> 466c466
> < /* #undef HAVE_STDLIB_H */
> ---
>> #define HAVE_STDLIB_H 1
> 475c475
> < /* #undef HAVE_STRINGS_H */
> ---
>> #define HAVE_STRINGS_H 1
> 478c478
> < /* #undef HAVE_STRING_H */
> ---
>> #define HAVE_STRING_H 1
> 568c568
> < /* #undef HAVE_SYS_STAT_H */
> ---
>> #define HAVE_SYS_STAT_H 1
> 577c577
> < /* #undef HAVE_SYS_TYPES_H */
> ---
>> #define HAVE_SYS_TYPES_H 1
> 614c614
> < /* #undef HAVE_UNISTD_H */
> ---
>> #define HAVE_UNISTD_H 1
> 780c780
> < /* #undef STDC_HEADERS */
> ---
>> #define STDC_HEADERS 1
>
> Now, this is odd, because those critters build with
> --disable-thread-safety and thus shouldn't be executing AX_PTHREAD
> at all.  However, I can reproduce something similar to frogmouth's
> symptoms on RHEL6 by configuring HEAD with --disable-thread-safety:
> configure fails with "zlib version is too old".

A-ha, I was also able to reproduce that on my x86 debian system.

> And investigation makes it look like something is completely broken
> in the major system headers, not zlib proper, which is what frogmouth
> is showing.

Yeah, googling for "mingw stdint.h error: two or more data types in
declaration specifiers" shows up discussions on similar issues. Not sure
if there's a fix in later versions.

> I suspect the explanation is that physically moving the AX_PTHREAD call
> before our regular header checks causes autoconf to move some of the
> header checks to be before the AX_PTHREAD expansion, *without regard to
> the fact that AX_PTHREAD might not get executed*.

Yep. The AC_CHECK_HEADER(pthread.h, ...) check right after AX_PTHREAD is
now the very first AC_CHECK_HEADER check in the script. Apparently the
first call to one of those triggers those additional standard-header
tests. Autoconf puts them inside the if-block too.

Found this thread that came to the same conclusion:
http://permalink.gmane.org/gmane.comp.sysutils.autoconf.general/15324.
It suggests using AS_IF, which knows about that problem, instead of
plain if. I guess we could do that, but I'm not too excited about
replacing every if-call in the script with AS_IF. We could do it just
for the pthread-check, I guess.

>>> This kind of points up that I've always thought the advice at the top of
>>> configure.in is a tad broken:
>>>
>>> dnl 0. Initialization and options processing
>>> dnl 1. Programs
>>> dnl 2. Libraries
>>> dnl 3. Header files
>>> dnl 4. Types
>>> dnl 5. Structures
>>> dnl 6. Compiler characteristics
>>> dnl 7. Functions, global variables
>>> dnl 8. System services
>>>
>>> Why isn't "Compiler characteristics" a lot earlier in the list, certainly
>>> before "Libraries"?
>
>> Hmm, yeah, although it's not clear what category the pthread check falls
>> into.
>
> At this point it's crystal clear that that advice is dead wrong, and that
> what we need to be doing is something along the lines of
>
> Initialization and options processing
> Compiler characteristics
> Header files
> Types
> Structures
> Functions, global variables
> Pthreads
> Other libraries
> Programs
> System services
>
> Before we start such major whacking, though, I am curious to know where
> Peter got the current ordering advice from.

 From the autoconf manual, see
https://www.gnu.org/software/autoconf/manual/autoconf.html#Autoconf-Input-Layout.
I'm not eager to change that, even though I agree it feels wrong.

We could move the pthread-related AC_CHECK_HEADER and AC_CHECK_FUNCS
tests away from the if-block around AX_PTHREAD, and sprinkle them into
the correct sections, "Header files" and "Functions, global variables"
sections. I'm not sure if that would be an improvement, though, it's
nice to have all the pthread-related stuff in one place.

I'll change the pthread-block to use AS_IF, I think that's the
least-ugly option.

> Also, I'm wondering why we are doing this at all in the 9.5 branch,
> because this is sure looking like development not stabilization.

Yeah, I didn't think it would be when I started :-(. I'll revert all
this in 9.5.

Once the dust settles, I plan to go through the configure logs from the
buildfarm to verify that all platforms choose the same pthread-related
flags as before, except for the spurious additional flags that really
are not needed.

- Heikki

On 7/8/15 4:39 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> I'm debugging this ATM. You're probably seeing a failure similar to
>> shearwater. The problem seems to be that the earlier autoconf tests add
>> "-lrt" to LIBS, and that somehow pulls in pthread_join() and other
>> functions, but not pthread_sigmask(), even when not compiling with -pthread.
>
>> I'll dig a bit deeper, but I think that could be fixed by moving the
>> AX_PTHREAD call in the configure script earlier, or by temporarily
>> clearing LIBS before calling it.
>
> Ah.  The former sounds sensible from here.
>
> This kind of points up that I've always thought the advice at the top of
> configure.in is a tad broken:
>
> dnl 0. Initialization and options processing
> dnl 1. Programs
> dnl 2. Libraries
> dnl 3. Header files
> dnl 4. Types
> dnl 5. Structures
> dnl 6. Compiler characteristics
> dnl 7. Functions, global variables
> dnl 8. System services
>
> Why isn't "Compiler characteristics" a lot earlier in the list, certainly
> before "Libraries"?

Compiler characteristics are things like sizeof and alignof, for which
you need header files and types, for which you might need libraries.

There will surely be some complicated cases, but I think as a guideline
the order is sound.

I've located another problem in the new improved ax_pthread.m4 script:
prairiedog is now concluding that it should use -pthread, which leads
to a whole bunch of build warnings along the lines of

powerpc-apple-darwin8-gcc-4.0.1: unrecognized option '-pthread'

Previously it concluded that it didn't need any special switch.

As near as I can tell, the problem is that this bit of ax_pthread.m4
is backwards:

# Clang doesn't consider unrecognized options an error unless we specify
# -Werror. We throw in some extra Clang-specific options to ensure that
# this doesn't happen for GCC, which also accepts -Werror.

AC_MSG_CHECKING([if compiler needs -Werror to reject unknown flags])
save_CFLAGS="$CFLAGS"
ax_pthread_extra_flags="-Werror"
CFLAGS="$CFLAGS $ax_pthread_extra_flags -Wunknown-warning-option -Wsizeof-array-argument"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([int foo(void);],[foo()])],
                  [AC_MSG_RESULT([yes])],
                  [ax_pthread_extra_flags=
                   AC_MSG_RESULT([no])])
CFLAGS="$save_CFLAGS"

AFAICS, the only way that ax_pthread_extra_flags ends up containing -Werror
is if this compile attempt *succeeds*, which surely ought not happen ever
with any compiler; it certainly won't on any of mine.  Thus, we never
actually apply -Werror and so the subsequent thread-switch testing
completely fails to notice if the compiler merely emits warnings.

Moreover, this check is logically unsound even if it weren't accidentally
backwards, because it cannot distinguish whether the error (if there was
one) required -Werror to happen or would have happened anyway.

And on top of that, at least with the versions of gcc I have laying about,
an unknown or misspelled -p option does not result in nonzero exit status,
whether or not you say -Werror:

$ touch foo.c
$ gcc -pthread -c foo.c
$ gcc -pzthread -c foo.c
gcc: unrecognized option '-pzthread'
gcc: unrecognized option '-pzthread'
$ echo $?
0
$ gcc -Werror -pzthread -c foo.c
gcc: unrecognized option '-pzthread'
gcc: unrecognized option '-pzthread'
$ echo $?
0

(same results on gcc 4.0.1 or 4.4.7)

Therefore, adding -Werror would fail to produce the desired results in the
subsequent thread-flag checks even if the check about whether to add it
had been done correctly.

I realize this is verbatim from upstream autoconf, but that doesn't mean
it's not utterly broken.

            regards, tom lane

On 07/26/2015 12:19 AM, Tom Lane wrote:
> I've located another problem in the new improved ax_pthread.m4 script:
> prairiedog is now concluding that it should use -pthread, which leads
> to a whole bunch of build warnings along the lines of
>
> powerpc-apple-darwin8-gcc-4.0.1: unrecognized option '-pthread'
>
> Previously it concluded that it didn't need any special switch.
>
> As near as I can tell, the problem is that this bit of ax_pthread.m4
> is backwards:
>
> # Clang doesn't consider unrecognized options an error unless we specify
> # -Werror. We throw in some extra Clang-specific options to ensure that
> # this doesn't happen for GCC, which also accepts -Werror.
>
> AC_MSG_CHECKING([if compiler needs -Werror to reject unknown flags])
> save_CFLAGS="$CFLAGS"
> ax_pthread_extra_flags="-Werror"
> CFLAGS="$CFLAGS $ax_pthread_extra_flags -Wunknown-warning-option -Wsizeof-array-argument"
> AC_COMPILE_IFELSE([AC_LANG_PROGRAM([int foo(void);],[foo()])],
>                    [AC_MSG_RESULT([yes])],
>                    [ax_pthread_extra_flags=
>                     AC_MSG_RESULT([no])])
> CFLAGS="$save_CFLAGS"
>
> AFAICS, the only way that ax_pthread_extra_flags ends up containing -Werror
> is if this compile attempt *succeeds*, which surely ought not happen ever
> with any compiler; it certainly won't on any of mine.  Thus, we never
> actually apply -Werror and so the subsequent thread-switch testing
> completely fails to notice if the compiler merely emits warnings.

Hmm. I don't think that's backwards. It's a strange way of testing "does
the compiler understand -Werror, and is it not GCC?". It succeeds on
clang, as the comment says:

~$ clang  a.c -c -Werror -Wunknown-warning-option -Wsizeof-array-argument
[success]

The problem it's trying to work-around is that allegedly clang does not
throw an error if -pthread is given on command-line on a platform where
it's not required, merely a warning. That workaround is too narrow, as
we have the exact same problem on prairiedog, with gcc. (But as you
noted, -Werror wouldn't help with older gcc versions anyway)

Googling around, I found a ticket in the autoconf bug tracker for this:
https://savannah.gnu.org/patch/?8186. The discussion died out in 2013,
but it seems that the latest idea there was to change the order the
flags are tested on Darwin, so that "none" is tested before "-pthread".
That should fix the issue, and hopefully not create other problems on
other Darwin variants. So I suggest that we do:

@@ -160,7 +165,7 @@ case ${host_os} in
          ;;

          darwin*)
-        ax_pthread_flags="-pthread $ax_pthread_flags"
+        ax_pthread_flags="none -pthread $ax_pthread_flags"
          ;;
  esac

It's sad that we have to hack this script again, I hoped we could use
the upstream version as is. I'll comment on the upstream ticket, let's
see if we can get the upstream version fixed too.

> And on top of that, at least with the versions of gcc I have laying about,
> an unknown or misspelled -p option does not result in nonzero exit status,
> whether or not you say -Werror:
>
> $ touch foo.c
> $ gcc -pthread -c foo.c
> $ gcc -pzthread -c foo.c
> gcc: unrecognized option '-pzthread'
> gcc: unrecognized option '-pzthread'
> $ echo $?
> 0
> $ gcc -Werror -pzthread -c foo.c
> gcc: unrecognized option '-pzthread'
> gcc: unrecognized option '-pzthread'
> $ echo $?
> 0
>
> (same results on gcc 4.0.1 or 4.4.7)
>
> Therefore, adding -Werror would fail to produce the desired results in the
> subsequent thread-flag checks even if the check about whether to add it
> had been done correctly.

This was fixed somewhere between 4.4 and 4.6. But yeah, it means that we
cannot rely on it.

- Heikki

On 07/26/2015 10:49 AM, Heikki Linnakangas wrote:
> On 07/26/2015 12:19 AM, Tom Lane wrote:
>> I've located another problem in the new improved ax_pthread.m4 script:
>> prairiedog is now concluding that it should use -pthread, which leads
>> to a whole bunch of build warnings along the lines of
>>
>> powerpc-apple-darwin8-gcc-4.0.1: unrecognized option '-pthread'
>>
>> Previously it concluded that it didn't need any special switch.
>>
>> ...
>
> Googling around, I found a ticket in the autoconf bug tracker for this:
> https://savannah.gnu.org/patch/?8186. The discussion died out in 2013,
> but it seems that the latest idea there was to change the order the
> flags are tested on Darwin, so that "none" is tested before "-pthread".
> That should fix the issue, and hopefully not create other problems on
> other Darwin variants. So I suggest that we do:
>
> @@ -160,7 +165,7 @@ case ${host_os} in
>            ;;
>
>            darwin*)
> -        ax_pthread_flags="-pthread $ax_pthread_flags"
> +        ax_pthread_flags="none -pthread $ax_pthread_flags"
>            ;;
>    esac
>
> It's sad that we have to hack this script again, I hoped we could use
> the upstream version as is. I'll comment on the upstream ticket, let's
> see if we can get the upstream version fixed too.

I commented on that autoconf bug tracker ticket, and Daniel Richard G
posted a new draft version there that fixes this problem. I pushed that
to master a few days ago, and the buildfarm results are now in. Seems to
have fixed the problem.


Andrew kindly let me run queries on the buildfarm database, and I was
able to extract the PTHREAD_CFLAGS chosen by the configure script, from
each buildfarm animal, with the three versions of the script that we've
had in the repository:

0 = Old hacked acx_pthread.m4 script, same as in 9.5 and below
1 = latest stable ax_pthread.m4 from upstream
2 = draft ax_pthread.m4, to be pushed upstream

I went through the results, and I believe the script is working the way
we want now. Attached is a trimmed result set, where I have removed all
the animals where there was no change in the PTHREAD_CFLAGS value chosen
between the three versions (configlogs-trimmed). I've also attached the
raw results and the query I used, for the sake of the archives
(configlogs-raw)


On some platforms, however, we are now getting duplicate -D options,
e.g. on dromedary:

> PTHREAD_CFLAGS='-D_REENTRANT  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS'

That's harmless, but it would be nice to not clutter the cc command
line. It's happening because we are doing this in configure.in:

> # Some platforms use these, so just define them.  They can't hurt if they
> # are not supported.  For example, on Solaris -D_POSIX_PTHREAD_SEMANTICS
> # enables 5-arg getpwuid_r, among other things.
> PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS"

I'm tempted to just remove -D_REENTRAND and -D_THREAD_SAFE, because
ax_pthread.m4 knows about those, and will add them on platforms where
they are needed. Any objections?

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> I went through the results, and I believe the script is working the way
> we want now. Attached is a trimmed result set, where I have removed all
> the animals where there was no change in the PTHREAD_CFLAGS value chosen
> between the three versions (configlogs-trimmed). I've also attached the
> raw results and the query I used, for the sake of the archives
> (configlogs-raw)

Cool, thanks for doing the legwork on this!

> On some platforms, however, we are now getting duplicate -D options,
> e.g. on dromedary:

>> PTHREAD_CFLAGS='-D_REENTRANT  -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS'

> That's harmless, but it would be nice to not clutter the cc command
> line. It's happening because we are doing this in configure.in:

>> # Some platforms use these, so just define them.  They can't hurt if they
>> # are not supported.  For example, on Solaris -D_POSIX_PTHREAD_SEMANTICS
>> # enables 5-arg getpwuid_r, among other things.
>> PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS"

> I'm tempted to just remove -D_REENTRAND and -D_THREAD_SAFE, because
> ax_pthread.m4 knows about those, and will add them on platforms where
> they are needed. Any objections?

Seems worth a try anyway.

            regards, tom lane

Hi Heikki,

On 2015-07-08 17:42:24 +0000, Heikki Linnakangas wrote:
> Replace our hacked version of ax_pthread.m4 with latest upstream version.
> Modified Files
> --------------
> aclocal.m4            |    2 +-
> config/acx_pthread.m4 |  170 -------------------------
> config/ax_pthread.m4  |  332 +++++++++++++++++++++++++++++++++++++++++++++++++
> configure             |  310 ++++++++++++++++++++++++++++++++++++---------
> configure.in          |    2 +-
> 5 files changed, 583 insertions(+), 233 deletions(-)

Since this commit I get additional diffs to pg_config.h.in when running
autoconf. Namely

+/* Define if you have POSIX threads libraries and header files. */
+#undef HAVE_PTHREAD
+

+/* Have PTHREAD_PRIO_INHERIT. */
+#undef HAVE_PTHREAD_PRIO_INHERIT
+

+/* Define to necessary symbol if this constant uses a non-standard name on
+   your system. */
+#undef PTHREAD_CREATE_JOINABLE
+

afaics they're newly emitted by the updated scripts. Was there any
reason not to commit them? If not I'll push those.

- Andres

On 08/12/2015 05:48 PM, Andres Freund wrote:
> On 2015-07-08 17:42:24 +0000, Heikki Linnakangas wrote:
>> Replace our hacked version of ax_pthread.m4 with latest upstream version.
>> Modified Files
>> --------------
>> aclocal.m4            |    2 +-
>> config/acx_pthread.m4 |  170 -------------------------
>> config/ax_pthread.m4  |  332 +++++++++++++++++++++++++++++++++++++++++++++++++
>> configure             |  310 ++++++++++++++++++++++++++++++++++++---------
>> configure.in          |    2 +-
>> 5 files changed, 583 insertions(+), 233 deletions(-)
>
> Since this commit I get additional diffs to pg_config.h.in when running
> autoconf. Namely
>
> +/* Define if you have POSIX threads libraries and header files. */
> +#undef HAVE_PTHREAD
> +
>
> +/* Have PTHREAD_PRIO_INHERIT. */
> +#undef HAVE_PTHREAD_PRIO_INHERIT
> +
>
> +/* Define to necessary symbol if this constant uses a non-standard name on
> +   your system. */
> +#undef PTHREAD_CREATE_JOINABLE
> +
>
> afaics they're newly emitted by the updated scripts. Was there any
> reason not to commit them? If not I'll push those.

No reason, just an oversight. Fixed, thanks!

- Heikki