patchGNU Autoconf Archive - Patches: patch #8186, ax_pthread adds -pthread when it...

 
 

patch #8186: ax_pthread adds -pthread when it should not on OSX when compiling with clang

Submitted by:  None
Submitted on:  Sat 21 Sep 2013 01:48:31 AM UTC  
 
Priority: 5 - NormalStatus: Done
Assigned to: Peter Simons <simons>Originator Email: -unavailable-
Open/Closed: Closed

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 04 Aug 2015 04:57:04 AM UTC, comment #20:

Hi Heikki,

-D_REENTRANT and -D_THREAD_SAFE can be removed from your build scripts; AX_PTHREAD now takes care of adding these to PTHREAD_CFLAGS on certain platforms. It's possible that some platforms will then lose one/both of these flags (do note that they can be defined implicitly by -mt et al.) but this is correct behavior if the system headers do not make use of those cpp symbols.

-D_POSIX_PTHREAD_SEMANTICS is, of course, not handled by AX_PTHREAD. It appears to be applicable only to Solaris. Should I add this to the special-flags section of the macro?

The build farms logs are extraordinarily helpful! Thank you for running this draft version through there!

I see a number of Clang builds now use no pthreads option flag at all (though retaining -D flags). None of those builds broke, but please let me know if there is any negative effect from dropping the options.

smew is running a fairly old version of Clang that my draft did not handle correctly. I'm surprised that build didn't break. I've fixed the issue there.

Daniel Richard G. <iskunk>
Sun 02 Aug 2015 01:18:33 PM UTC, comment #19:

Thanks Daniel. I've pushed that version to PostgreSQL git master, and the buildfarm is happy with it. I collected the PTHREAD_CFLAGS values chosen on each platform in the buildfarm, before and after this change, and they all look good to me.

For reference, the buildfarm results can be seen at http://pgbuildfarm.org/cgi-bin/show_status.pl, and my comparison of the chosen PTHREAD_CFLAGS can be seen at: http://www.postgresql.org/message-id/55BE176F.3070901@iki.fi

Heikki Linnakangas <hlinnaka>
Thu 30 Jul 2015 06:41:45 AM UTC, comment #18:

The fix for this issue specifically is to remove the Darwin special case, but my changes go beyond that.

The Clang check is still needed to support older versions properly, but that's minimal. (I've also made a fix there, to account for the saner behavior of newer Clang releases.)

I've attached a "draft2" revision of the macro, which is my current bleeding-edge code. Diffing will show you everything that's going on in there (I will, of course, have to write up a proper changelog entry).

Please give this a try, Heikki and anyone else still interested, and let me know if you see any problems. I've tested it across the breadth of systems available to me, and everything seems to check out so far.

(file #34553)

Daniel Richard G. <iskunk>
Wed 29 Jul 2015 07:37:36 AM UTC, comment #17:

Thanks Daniel! I don't have direct access to the machine, but I can commit a modified version to our git repository, and the Postgres buildfarm will run it on many platforms.

Which fix are you planning to apply for this?

1) Remove the Darwin-special case altogether, or

2) Put "none" before "-pthread" on Darwin

And were you planning to revert the clang-specific check, or is that still required? Could you post the exact version of the script you'd like me to test, please?

Heikki Linnakangas <hlinnaka>
Wed 29 Jul 2015 07:18:08 AM UTC, comment #16:

Hello Heikki,

Another developer recently contacted me as well about this just a day before your inquiry, and I have been working on the macro lately. I have a number of changes that I've been sitting on, not least the tweak worked out with Brian here, and haven't had the wherewithal to get those committed for some time.

I have here what appears to be a Darwin 11.4.2 system that I will be testing on. Would you be willing to give the revised macro a try on your machine?

Daniel Richard G. <iskunk>
Sun 26 Jul 2015 08:50:32 AM UTC, comment #15:

Any news on this? We're seeing the same issue that Brian reported, after updating to the latest version of acx_pthread.m4, in the PostgreSQL buildfarm. It happens on a Darwin 8.11.0 system running on PowerPC, using gcc.

Heikki Linnakangas <hlinnaka>
Thu 21 Nov 2013 01:20:40 AM UTC, comment #14:

Brian and Alistair,

Thanks for checking back in. I'm currently testing the macro in my employer's stable of variegated Unix systems, and once it checks out, I'll post an update here for further testing.

Daniel Richard G. <iskunk>
Wed 20 Nov 2013 03:23:04 PM UTC, comment #13:

Daniel,

Removing the darwin special case did indeed give the correct result.

Thanks for this. I'm happy to test anything you need.

Alistair King <alistairking>
Wed 20 Nov 2013 12:30:34 PM UTC, comment #12:

Sorry, emails on this have been being caught by my spam filter.

By "cc", I was referring to the case where:

CC=cc

At which point you need to dig a bit deeper in order to determine which compiler is being used.

I have faint memory of this being an issue on a few other platforms where cc could be an alias to either gcc or some other vendor's C compiler.

Brian Aker <brianaker>
Wed 20 Nov 2013 07:19:59 AM UTC, comment #11:

Alistair,

If you remove the special case for Darwin on line 162 of the macro file, does it give the correct result?

I'm doing some heavy maintenance on this macro, and will be happy to ensure that it works right on newer MacOS X systems.

Daniel Richard G. <iskunk>
Tue 19 Nov 2013 04:28:03 PM UTC, comment #10:

I am seeing the same issue that Brian mentioned.
Using this patch on OSX 10.9, with CC set to either gcc or clang (it appears they are both llvm front-ends):

alistair@gibi:~$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/c++/4.2.1
Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin13.0.0
Thread model: posix

alistair@gibi:~$ clang --version
Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin13.0.0
Thread model: posix

When I use the patched ax_pthread macro it adds -pthread to CFLAGS.
This works just fine during the compilation phase, but when linking yields this (misleading) warning:

clang: warning: argument unused during compilation: '-pthread'

The fix suggested by Brian does indeed make this go away.
My guess as to why this fixes the problem is because while clang will accept -pthread during compilation, it is not needed; whereas during linking it throws a warning if it is used.

From man pthread(3):
INSTALLATION
The default system libraries include pthread functions. No additional libraries or CFLAGS are necessary to use these interfaces.

Hope this helps.

Alistair King <alistairking>
Sun 27 Oct 2013 10:28:47 AM UTC, comment #9:

Brian, is there still a problem with this macro on your setup? Or do you think this issue can be closed?

Peter Simons <simons>
Project AdministratorIn charge of this item.
Sat 19 Oct 2013 06:16:42 AM UTC, comment #8:

Brian,

What do you mean by "if you call it via CC?" How was the macro failing?

Also, could you explain the intent behind changing the order of the flags that are tested? What was the incorrect result it was giving before, and the correct result that your change now returns?

Daniel Richard G. <iskunk>
Fri 18 Oct 2013 06:53:19 AM UTC, comment #7:

And I did a bad copy and paste...

diff m4/ax_pthread.m4 ~/Projects/ddm4/m4/ax_pthread.m4
85c85
< #serial 21
---

> #serial 22

163c163
< ax_pthread_flags="-pthread $ax_pthread_flags"
---

> ax_pthread_flags="none -pthread $ax_pthread_flags"


Adjust the order for OSX.

Brian Aker <brianaker>
Fri 18 Oct 2013 06:41:52 AM UTC, comment #6:

Hi!

Sorry about not logging in when I left the original bug report, I was in a hurry working over someone's shoulder and just copy and pasted what I had done :)

This patch works if you call clang via "clang" but if you call it via CC it will fail. I took what you all did in trunk and applied the following:

diff --git a/m4/ax_pthread.m4 b/m4/ax_pthread.m4
index 4537934..ebea7fb 100644
--- a/m4/ax_pthread.m4
+++ b/m4/ax_pthread.m4
@@ -123,7 +123,7 @@ fi
# which indicates that we try without any flags at all, and "pthread-config"
# which is a program returning the flags for the Pth emulation library.

-ax_pthread_flags="none pthreads -Kthread -kthread lthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config"
+ax_pthread_flags="pthreads none -Kthread -kthread lthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config"

# The ordering is (sometimes) important. Some notes on the
# individual items follow:
@@ -160,7 +160,7 @@ case ${host_os} in
;;

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

Cheers,
-Brian

Brian Aker <brianaker>
Thu 03 Oct 2013 04:58:28 PM UTC, comment #5:

FYI, I have pushed Daniel's patch in revision dd94691.

The original poster may not even notice that this update is available, I'm afraid, because he or she didn't leave an e-mail address with this issue and thus cannot be notified of the new development.

Peter Simons <simons>
Project AdministratorIn charge of this item.
Fri 27 Sep 2013 05:01:50 AM UTC, comment #4:

Okay, attached is a patch against git master that adds support for Clang to AX_PTHREAD, and a copy of the patched macro. Can the original poster give this a try?

(And if anyone else here uses Clang, your input will be much appreciated...)

(file #29240, file #29241)

Daniel Richard G. <iskunk>
Sat 21 Sep 2013 04:36:10 PM UTC, comment #3:

I haven't worked with Clang yet, but from a bit of searching around, it seems that -pthread will either cause the compiler to complain harmlessly, or be recognized as a proper option in newer versions.

What's the specific failure mode that the patch intends to avoid? Is it a hard error? Warning noise? It would be helpful to see some copy-and-pasted messages from the compiler.

Daniel Richard G. <iskunk>
Sat 21 Sep 2013 07:43:11 AM UTC, comment #2:

Hey Steven, hi Daniel, do you agree with this patch?

Peter Simons <simons>
Project AdministratorIn charge of this item.
Sat 21 Sep 2013 04:42:09 AM UTC, comment #1:

Surely a better test for whether the compiler is clang is needed, for example in MacPorts clang is installed as clang-mp-3.3 etc... and with this check this compiler will not be identified as clang.

In one of my projects I use the following to see if clang is being used:

# check for clang
AS_IF([test "x$GCC" = xyes],
[AS_IF([test "`$CC -v 2>&1 | grep -c 'clang'`" != "0"],[CLANG_CC=1])],
[CLANG_CC=])
AC_SUBST(CLANG_CC)

So could something like that be used here?

Anonymous
Sat 21 Sep 2013 01:48:31 AM UTC, original submission:

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

darwin*)
- ax_pthread_flags="-pthread $ax_pthread_flags"
+ if test "$CC" = "clang"; then
+ ax_pthread_flags="$ax_pthread_flags"
+ else
+ ax_pthread_flags="-pthread $ax_pthread_flags"
+ fi
;;
esac

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #34553:  ax_pthread-draft2.m4 added by iskunk (16kB - application/x-m4)
file #29240:  ax_pthread_clang.patch added by iskunk (5kB - text/x-patch - Patch to add rudimentary Clang support to AX_PTHREAD)
file #29241:  ax_pthread-draft1.m4 added by iskunk (13kB - application/x-m4 - Patch to add rudimentary Clang support to AX_PTHREAD)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by hlinnaka (Posted a comment)
  • -unavailable- added by alistairking (Posted a comment)
  • -unavailable- added by brianaker (Posted a comment)
  • -unavailable- added by iskunk (Posted a comment)
  • -unavailable- added by simons (Posted a comment)
  • -unavailable- added by simons
  • -unavailable- added by simons
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 30 Jul 2015 06:41:45 AM UTCiskunkAttached File-=>Added ax_pthread-draft2.m4, #34553
    Fri 01 Nov 2013 03:20:31 PM UTCsimonsStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Thu 03 Oct 2013 04:58:28 PM UTCsimonsStatusNeed Info=>Ready For Test
    Fri 27 Sep 2013 05:01:50 AM UTCiskunkAttached File-=>Added ax_pthread_clang.patch, #29240
      Attached File-=>Added ax_pthread-draft1.m4, #29241
    Sat 21 Sep 2013 07:43:11 AM UTCsimonsStatusNone=>Need Info
      Assigned toNone=>simons
    Sat 21 Sep 2013 07:41:47 AM UTCsimonsCarbon-Copy-=>Added steven g. johnson
      Carbon-Copy-=>Added daniel richard g.

    Back to the top


    Powered by Savane 3.1-cleanup