299661 - Don't explicitly link against -lpthread on Mac OS X

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Mentovai]

It should be possible to set DYLD_IMAGE_SUFFIX=_debug at run time to make dyld
prefer libraries with a _debug suffix, for example, libSystem_debug.dylib.  This
could ease debugging calls into and callbacks out of system libraries.  Right
now, it's impossible because the application dies due to assertions in
libSystem_debug.dylib.

Comment 1

[Mark Mentovai]

Attachment: Stack backtrace

It's crashing in pthread_mutex.c, which is
http://www.opensource.apple.com/darwinsource/10.4.1/Libc-391/pthreads/pthread_mutex.c
(requires ADC), line 369.  During the crash, mutex->type =
PTHREAD_MUTEX_RECURSIVE.

The problem looks like it may be Apple's bug.  NSAddImage() may not work in
libSystem_debug.dylib.	In that case, I'll INVALIDate this and file with them.

Comment 2

[Mark Mentovai]

Things went sour when the application attempted loading libpthread.dylib, which
is a link to libSystem.dylib.  Having already loaded libSystem_debug.dylib, this
became a problem.  Think about static data.  Big ouch.

libpthread.dylib was being loaded because it was present in compreg.dat. 
Workaround: remove compreg.dat.  Static builds may luck out here, but who debugs
with static builds if they can help it?  Eew.

libpthread.dylib was in compreg.dat, in turn, because various things link
against -lpthread.  This is unnecessary on Mac OS X, because all pthreads
functionality is included in libSystem.dylib and libpthread.dylib is merely a
link.  I'd like to remove the explicit dependencies on libpthread.dylib as an
independent library on Mac OS X.  Ordinarily, I'd achieve this by making the
first test for pthreads check the standard system library with no added -l, but
I suspect there may be some obscure platforms out there with dysfunctional or
incomplete pthreads implementations in libc that need to be avoided.

Anyone who can confirm this is welcome to chime in: does anyone envision a
problem with checking for pthreads without any special -l linkage before trying
-lpthread, -lpthreads, etc.?  If so, I wouldn't mind special-casing Mac OS X. 
There's already a good deal of platform-specific special-casing going on here
anyway.

This affects both the top-level mozilla and NSPR.  Repurposing bug.

Comment 3

[Mark Mentovai]

wtchang, seawood, any ideas?  Is it safe to look for pthreads without any -l
before trying -lpthread, -lpthreads, etc., or should OS X be special-cased here?

Comment 4

[Wan-Teh Chang]

We've wanted to make this change in NSPR (bug 212708 comment 9)
and cls already proposed a patch.  Our solution is to make Mac
OS X a special case.

Comment 5

[Mark Mentovai]

Should also avoid linking against libm for the same reason.

Comment 6

[cls]

Attachment: v1

Comment 7

[Wan-Teh Chang]

Comment on attachment 190352 [details] [diff] [review]
v1

r=wtc.	I suggest making two changes when you check in
this patch.

>-AC_CHECK_LIB(m, atan)
> 
> dnl We don't want to link with libdl even if it's present on OS X, since
> dnl it's not used and not part of the default installation.
>+dnl We don't want to link against libm on OSX either (bug 299661)

Rather than citing bug 299661, we should explain why we don't want
to link against libm, especially the reason is different from the
reason why we don't want to link with libdl.

I suggest a commment like this:

+dnl We don't want to link against libm on OS X either because the
+dnl math functions are in libSystem.

> case $target in
> *-darwin*)
> 	;;
> *)
>+    AC_CHECK_LIB(m, atan)
> 	AC_CHECK_LIB(dl, dlopen,
>         AC_CHECK_HEADER(dlfcn.h, 
>             LIBS="-ldl $LIBS"

The indentation of AC_CHECK_LIB(m, atan) looks wrong.

>+case "$target_os" in
>+darwin*)
>+    USE_PTHREADS=1
>+    ;;
>+*)
> MOZ_CHECK_PTHREADS(pthreads,
>     USE_PTHREADS=1 _PTHREAD_LDFLAGS="-lpthreads",
>     MOZ_CHECK_PTHREADS(pthread,
>@@ -2545,6 +2551,8 @@
>         )
>     )
> )
>+    ;;
>+esac

I suggest indenting the original MOZ_CHECK_PTHREADS code.
Might want to add a comment that says "The pthread functions
are in libSystem."

Comment 8

[cls]

The patch has been checked in with updated comments.