After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747954 - osx: fix libcheck and "make check" builds with clang
osx: fix libcheck and "make check" builds with clang
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Mac OS
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 747730 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-04-15 22:12 UTC by Julien Isorce
Modified: 2016-04-11 17:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libcheck: fix build on osx with clang (1.14 KB, patch)
2015-04-15 22:15 UTC, Julien Isorce
none Details | Review
configure.ac: add Qunused-arguments to LDFLAGS on darwin (976 bytes, patch)
2015-07-06 23:00 UTC, Julien Isorce
none Details | Review
Diff with future ax_pthreads.m4 (24.19 KB, patch)
2016-02-11 08:40 UTC, Julien Isorce
none Details | Review
tests: add PTHREAD_CFLAGS for make check to pass on OS X (1.20 KB, patch)
2016-02-11 08:42 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2015-04-15 22:12:50 UTC
clang has -pthread option but it's only valid for the compiler. It should not be passed to the linker.

Currently libcheckinternal.la contains "inherited_linker_flags='-pthread'" which make fail anything linking with with libgstcheck-1.0 when using -Werror. This is the case for "make check" builds:

"error: argument unused during compilation: '-pthread'"

(libcheck itself does not fail to build because it does not set -Werror so the error is just a warning in that case)

$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 6.0 (clang-600.0.56) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin14.1.0
Thread model: posix
Comment 1 Julien Isorce 2015-04-15 22:15:11 UTC
Created attachment 301685 [details] [review]
libcheck: fix build on osx with clang
Comment 2 Sebastian Dröge (slomo) 2015-04-16 12:00:39 UTC
See my comment in bug #747730
Comment 3 Julien Isorce 2015-04-17 07:27:33 UTC
Some infos:

* From https://autotools.io/libtool/lafiles.html:

"inherited_linker_flags:
This variable lists selected link editor flags that are needed to ensure the correct ABI is used with the library, for instance. In most cases, all you'll be seeing in here is -pthread, as it's probably the only such flag used on modern systems."

* In libtool's git log:

commit 2f15fc7d18de0492067f4c2c782d1c8bb624194e
Author: Christoph Egger <Christoph_Egger@gmx.de>
Date:   Wed Feb 23 03:30:30 2005 +0000

    * config/ltmain.m4sh (func_mode_link): Add -pthread like flags to
    inherited_linker_flags.

--- a/config/ltmain.m4sh
+++ b/config/ltmain.m4sh
@@ -2686,6 +2686,10 @@ func_mode_link ()
        compiler_flags="$compiler_flags $arg"
        compile_command="$compile_command $arg"
        finalize_command="$finalize_command $arg"
+       case "$new_inherited_linker_flags " in
+           *" $arg "*) ;;
+           * ) new_inherited_linker_flags="$new_inherited_linker_flags $arg" ;;
+       esac
        continue
        ;;

Also:
http://git.savannah.gnu.org/cgit/libtool.git/tree/build-aux/ltmain.in#n5161
http://git.savannah.gnu.org/cgit/libtool.git/tree/build-aux/ltmain.in#n5691
Comment 4 Sebastian Dröge (slomo) 2015-04-17 11:52:10 UTC
So it's fixed in newer libtool?
Comment 5 Julien Isorce 2015-04-17 12:03:22 UTC
(In reply to Sebastian Dröge (slomo) from comment #4)
> So it's fixed in newer libtool?

Nop, sorry I should have commented these infos.

(In reply to Julien Isorce from comment #3)
> Some infos:
> 
> * From https://autotools.io/libtool/lafiles.html:
> 
> "inherited_linker_flags:
> This variable lists selected link editor flags that are needed to ensure the
> correct ABI is used with the library, for instance. In most cases, all
> you'll be seeing in here is -pthread, as it's probably the only such flag
> used on modern systems."

It is confusing because I do not know any linker that support -pthread. But at the time this sentence has been written maybe "modern" became "old" and even "obsolete".

> 
> * In libtool's git log:
> 
> commit 2f15fc7d18de0492067f4c2c782d1c8bb624194e
> Author: Christoph Egger <Christoph_Egger@gmx.de>
> Date:   Wed Feb 23 03:30:30 2005 +0000
> 
>     * config/ltmain.m4sh (func_mode_link): Add -pthread like flags to
>     inherited_linker_flags.
> 
> --- a/config/ltmain.m4sh
> +++ b/config/ltmain.m4sh
> @@ -2686,6 +2686,10 @@ func_mode_link ()
>         compiler_flags="$compiler_flags $arg"
>         compile_command="$compile_command $arg"
>         finalize_command="$finalize_command $arg"
> +       case "$new_inherited_linker_flags " in
> +           *" $arg "*) ;;
> +           * ) new_inherited_linker_flags="$new_inherited_linker_flags
> $arg" ;;
> +       esac
>         continue
>         ;;

Here it was to mention a feature that has been add in 2005. Maybe it was for older linkers.

> 
> Also:
> http://git.savannah.gnu.org/cgit/libtool.git/tree/build-aux/ltmain.in#n5161
> http://git.savannah.gnu.org/cgit/libtool.git/tree/build-aux/ltmain.in#n5691

Here it was to mention that this is still present 10 years after.
Comment 6 Sebastian Dröge (slomo) 2015-04-17 12:11:44 UTC
Well, it's a clear bug in libtool/autotools somewhere and should be fixed there. And if it's fixed there by just not doing these things when it detects clang as a compiler
Comment 7 Julien Isorce 2015-04-17 14:08:05 UTC
I totally agree let's try to fix libtool
Comment 8 Julien Isorce 2015-06-09 14:06:04 UTC
I had not time to look into libtool. Even if a fix is made in the future I think it will take time to get it merged and spread everywhere. So I suggest the following alternative to turn the error into a warning to prevent build failure which is annoying especially with gst-uninstalled automatic build.

Add -Wno-error=unused-command-line-argument to OBJCFLAGS (or CFLAGS). Maybe we can add this in configure.ac .
Comment 9 Edward Hervey 2015-06-10 08:02:59 UTC
(In reply to Julien Isorce from comment #8)
> I had not time to look into libtool. Even if a fix is made in the future I
> think it will take time to get it merged and spread everywhere. So I suggest
> the following alternative to turn the error into a warning to prevent build
> failure which is annoying especially with gst-uninstalled automatic build.
> 
> Add -Wno-error=unused-command-line-argument to OBJCFLAGS (or CFLAGS). Maybe
> we can add this in configure.ac .

  You mean s/CFLAGS/LDFLAGS/ ?

  I applied that that in cerbero for osx/ios builds.
Comment 10 Tim-Philipp Müller 2015-06-10 12:06:57 UTC
Is bug #747730 a duplicate of this?
Comment 11 Tim-Philipp Müller 2015-06-13 23:41:32 UTC
*** Bug 747730 has been marked as a duplicate of this bug. ***
Comment 12 Julien Isorce 2015-06-17 21:31:35 UTC
I had another try and my conclusion is that this is not a libtool/autotools issue.
It looks like to be a clang issue because clang behaves differently between linux and darwin. Indeed on linux the -pthread option is never forwarded to the linker. clang does not filter -pthread on darwin at link time.

Then looking at clang bug tracker it seems it has been reported in 2010: https://llvm.org/bugs/show_bug.cgi?id=7798
I have added steps to reproduce the problem (add -v to clang to actually see ld invocation)
Also I have added a link to the driver file that needs to be patched.

But in order to support all clang versions on darwin I suggest we add everywhere -Qunused-arguments to LDFLAGS when compiling gstreamer on darwin. (The difference with -Wno-error=unused-command-line-argument is that it turns it back to a warning whereas -Qunused-arguments make it invisible)
Comment 13 Julien Isorce 2015-07-06 23:00:04 UTC
Created attachment 306963 [details] [review]
configure.ac: add Qunused-arguments to LDFLAGS on darwin
Comment 14 Julien Isorce 2015-07-28 07:31:54 UTC
ping ?
Comment 15 Steve Jonghee Yun 2015-10-02 13:30:47 UTC
I met compile error on GStreamer 1.7.0 (GIT)
  "error: argument unused during compilation: '-pthread'"

Now compile error is cleared with this patch.

Thanks.
Comment 16 Julien Isorce 2015-12-11 06:04:31 UTC
(In reply to Steve Jonghee Yun from comment #15)
> I met compile error on GStreamer 1.7.0 (GIT)
>   "error: argument unused during compilation: '-pthread'"
> 
> Now compile error is cleared with this patch.
> 
> Thanks.

Great!

Anyone against attached patch ? Explanations in comment #12
Comment 17 Sebastian Dröge (slomo) 2015-12-11 08:11:06 UTC
Comment on attachment 306963 [details] [review]
configure.ac: add Qunused-arguments to LDFLAGS on darwin

Let's get it in then but add a comment there why it is added... also maybe this should instead be inside AX_PTHREAD() directly instead of having to duplicate it in every configure.ac?
Comment 18 Julien Isorce 2015-12-15 14:47:40 UTC
(In reply to Sebastian Dröge (slomo) from comment #17)
> Comment on attachment 306963 [details] [review] [review]
> configure.ac: add Qunused-arguments to LDFLAGS on darwin
> 
> Let's get it in then but add a comment there why it is added... also maybe
> this should instead be inside AX_PTHREAD() directly instead of having to
> duplicate it in every configure.ac?

Sorry for the delay I sent email to Daniel Richard G. who maintains ax_pthread.m4. 
First of all you are right in worst scenario we should have a similar change locally in our http://cgit.freedesktop.org/gstreamer/common/tree/m4/ax_pthread.m4

But Daniel told me that he will have a look next month. It seems he has other pending changes to do in ax_pthread.m4. So I suggest we wait a bit more for now.

Though if someone has an idea where to properly put LDFLAGS="$LDFLAGS -Qunused-arguments" (or something equivalent) in http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_pthread.m4 I can try a such patch. So that we will save some time to Daniel I suppose. (He will still have to accept that patch, no guaranties so far)
Comment 19 Sebastian Dröge (slomo) 2015-12-16 08:32:05 UTC
Best to ask Daniel I guess, not sure where to best add that. Question is also when exactly to add it. Best might be to check if the compiler gives an error with -Werror and -pthread, and if so use -Qunused-arguments (if compilation succeeds with that). Something along the lines of the "if compiler needs -Werror to reject unknown flags" check.
Comment 20 Julien Isorce 2016-02-11 08:40:42 UTC
Created attachment 320853 [details] [review]
Diff with future ax_pthreads.m4

Daniel made a new version ax_pthreads.m4. Still a draft but should be released soon enough. See http://savannah.gnu.org/patch/?8186#comment21 (ax_pthread-draft3.m4).
Once it is ready I can push it to gstreamer/common repo.
Comment 21 Julien Isorce 2016-02-11 08:42:18 UTC
Created attachment 320854 [details] [review]
tests: add PTHREAD_CFLAGS for make check to pass on OS X

grep -rin --include \*.am PTHREAD_CFLAGS *

libs/gst/check/libcheck/Makefile.am:56:libcheckinternal_la_CFLAGS	+= $(PTHREAD_CFLAGS) -D_GNU_SOURCE -DHAVE_PTHREAD
tests/check/Makefile.am:198:AM_CFLAGS = $(PTHREAD_CFLAGS) $(GST_OBJ_CFLAGS) -UG_DISABLE_ASSERT -UG_DISABLE_CAST_CHECKS \
tests/examples/manual/Makefile.am:136:testrtpool_CFLAGS = $(GST_OBJ_CFLAGS) $(PTHREAD_CFLAGS)
tests/examples/streams/Makefile.am:12:rtpool_test_CFLAGS = $(GST_OBJ_CFLAGS) $(PTHREAD_CFLAGS)
Comment 22 Sebastian Dröge (slomo) 2016-02-16 11:57:07 UTC
Feel free to merge this already before it is accepted there if it works on the platforms we care about.
Comment 23 Julien Isorce 2016-03-03 10:35:50 UTC
(In reply to Sebastian Dröge (slomo) from comment #22)
> Feel free to merge this already before it is accepted there if it works on
> the platforms we care about.

I was waiting that it lands, and actually there were regression on FreeBSD with draft3. It is now fixed. The new ax_pthreads.m4 is now merged in the autoconf repo: http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=history;f=m4/ax_pthread.m4 and the issue is closed: https://savannah.gnu.org/patch/?8186.
So I think it is safe to use it now.

Since 1.8 RC has been released few days ago, is it still possible to update https://cgit.freedesktop.org/gstreamer/common/tree/m4/ax_pthread.m4 with that new version ? If yes, I just need to push to "common" and it will propagate new sha to other repo ? Also can I push attached patch ?
Comment 24 Sebastian Dröge (slomo) 2016-03-03 10:37:30 UTC
After 1.8.0 now :)
Comment 25 Julien Isorce 2016-04-11 08:10:33 UTC
Official ax_pthread.m4 has been updated on March 20th (http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=history;f=m4/ax_pthread.m4) and today I tested that new version which also contains the fix for osx (like previous version). It works fine on my setup. Can I push it to "common" ?
Comment 26 Sebastian Dröge (slomo) 2016-04-11 08:45:03 UTC
Yes please
Comment 27 Julien Isorce 2016-04-11 16:58:36 UTC
commit ac2f647695e7bd4b433ea108ee1d0e23901797d4
Author: Julien Isorce <j.isorce@samsung.com>
Date:   Mon Apr 11 17:51:26 2016 +0100

    m4: update ax_pthread.m4 to latest release to fix '-pthread' arguments unused errors on osx.
    
    On osx, make check in gstreamer directory currently outputs:
    "error: argument unused during compilation: '-pthread'".
    
    This new ax_pthread.m4 adds -Qunused-arguments when needed.
    
    Source:
    http://www.gnu.org/software/autoconf-archive/ax_pthread.html
    
    History:
    http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=history;f=m4/ax_pthread.m4
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747954
Comment 28 Julien Isorce 2016-04-11 17:10:54 UTC
Comment on attachment 320854 [details] [review]
tests: add PTHREAD_CFLAGS for make check to pass on OS X

commit 7d0abcc0326d2ad949fb83a3c3f0023b43ce3715
Author: Julien Isorce <j.isorce@samsung.com>
Date:   Thu Feb 11 09:33:28 2016 +0100

    tests: add PTHREAD_CFLAGS for make check to pass on OS X
    
    Currently "make check" fails with:
    "error: argument unused during compilation: '-pthread'"
    
    PTHREAD_CFLAGS now contains -Qunused-arguments to fix that.
    Explanation here: http://savannah.gnu.org/patch/?8186#comment21
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747954