925217 - ldap c-sdk libprldap needs to link with -lnspr4 in all cases (FreeBSD)

Status: RESOLVED WONTFIX

(Directory :: LDAP C SDK, defect)

Description

[Mikhail T.]

Attachment: patch-ldap-sdks-c-sdk-ldap-libraries-libprldap-Makefile.in

Although this problem is not always obvious -- because other pieces of the thunderbird executable do link with nspr, the libprldap60.so should always link with $(NSPRLINK).

Currently ldap/sdks/c-sdk/ldap/libraries/libprldap/Makefile.in adds the library to the EXTRA_LIBS for some platforms -- it should be for all. Because, for just one example, the library calls PR_GetCurrentThread().

This can be seen on FreeBSD, for example, when Thunderbird (or Seamonkey) is compiled with clang, rather than gcc (but only on i386 -- FreeBSD's 64-bit platforms don't manifest this problem):

http://www.freebsd.org/cgi/query-pr.cgi?pr=180473

   % thunderbird 
   XPCOMGlueLoad error for file /opt/lib/thunderbird/libprldap60.so:
   /opt/lib/thunderbird/libprldap60.so: Undefined symbol "PR_GetCurrentThread"
   Couldn't load XPCOM.

I'm not sure, why the choice of compiler makes a difference, but it does not really matter -- if a symbol is needed, the library providing it should be linked-in... Unfortunately, ${NSPRLINK} can not be used verbatim, because it is simply the output of "nspr-config --libs" and may contain other flags (such as -pthread), that are meant for compiler, rather than linker (ld).

The attached patch is a modified version of what's already included in FreeBSD's mail/thunderbird port -- the second EXTRA_LIBS line adds the filtered NSPRLINK values.

It is meant for illustration -- I'm not sure, how to make this properly generic. Maybe, the libraries ought to be linked with compiler (${CXX} -shared?) rather than linker (ld) -- in that case, the output of ${NSPRLINK} can be used without filtering...

Comment 1

[Landry Breuil (:gaston)]

Comment on attachment 815199 [details] [diff] [review]
patch-ldap-sdks-c-sdk-ldap-libraries-libprldap-Makefile.in

From what i understand Makefile.in already does this by default in http://mxr.mozilla.org/comm-central/source/ldap/sdks/c-sdk/ldap/libraries/libprldap/Makefile.in#159

so is it only the case of a missing -lpthread ? Here on OpenBSD nspr-config --libs returns -pthread.. and i dont see any issue at runtime with clang or gcc on amd64 or i386.

Comment 2

[Philip Chee]

Mikhail T: Question for you:

(In reply to Landry Breuil (:gaston) from comment #1)
> Comment on attachment 815199 [details] [diff] [review]
> patch-ldap-sdks-c-sdk-ldap-libraries-libprldap-Makefile.in
> 
> From what i understand Makefile.in already does this by default in
> http://mxr.mozilla.org/comm-central/source/ldap/sdks/c-sdk/ldap/libraries/
> libprldap/Makefile.in#159
> 
> so is it only the case of a missing -lpthread ? Here on OpenBSD nspr-config
> --libs returns -pthread.. and i don't see any issue at runtime with clang or
> gcc on amd64 or i386.

Comment 3

[Mikhail T.]

(In reply to Landry Breuil (:gaston) from comment #1)
> so is it only the case of a missing -lpthread ?

If it were -lpthread, the missing symbol reported at runtime would've been something like pthread_*, wouldn't it have? Instead, the missing symbol is "PR_GetCurrentThread", which is supposed to come from -lnspr.

Comment 4

[Mikhail T.]

Do you still need anything from me?

Comment 5

[Landry Breuil (:gaston)]

I still fail to see the usefulness of this patch, to me its not needed. nspr libs should be added by that block (which is the fallback for bsd/linux/etc):

159 ifndef CUSTOM_LIBS
160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
161 EXTRA_LIBS += $(NSPRLINK)
162 endif

So unless you have other patches in the freebsd port that change the default behaviour.. oh and btw, my comm-central buildslave on freebsd/amd64 builds without patches, (see http://buildbot.rhaalovely.net/builders/comm-central-freebsd-amd64) and the packaging step exercises the runtime.. 

I also see that the fix in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=180473 was to force the use of gcc, so maybe it was a clang issue at the time, which got fixed since then ?

Comment 6

[Mikhail T.]

(In reply to Landry Breuil (:gaston) from comment #5)
> I still fail to see the usefulness of this patch, to me its not needed.

First of all, let's agree, that a shared library, which uses nspr-functions, needs to link with libnspr. Now we can discuss, how to achieve that.

> nspr libs should be added by that block (which is the fallback for bsd/linux/etc):
> 
> 159 ifndef CUSTOM_LIBS
> 160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
> 161 EXTRA_LIBS += $(NSPRLINK)
> 162 endif

I think, the above was being skipped because libprldap/Makefile.in did define CUSTOM_LIBS. Of course, neither ldap/ nor libprldap/ are even found in today's firefox tarball so the proposed patch may no longer be needed indeed...

> So unless you have other patches in the freebsd port that change the default
> behaviour...

FreeBSD port www/firefox (as well as devel/nspr and security/nss, that it relies upon) have dozens of patches... Most of them have been submitted to Mozilla, of course.

http://freshports.org/www/firefox

> oh and btw, my comm-central buildslave on freebsd/amd64 builds
> without patches, (see
> http://buildbot.rhaalovely.net/builders/comm-central-freebsd-amd64) and the
> packaging step exercises the runtime.. 

That's because it is built with --disable-ldap. Trying to enable this option by running "make config" in the www/seamonkey port results in the following error-message from the port:
seamonkey-2.31 is marked as broken: XPCOMGlueLoad error for file /opt/lib/seamonkey/libxul.so:  Cannot open "../../ldap/sdks/c-sdk/ldap/libraries/libldap/libldap60.so".

I was not aware of this until now (I thought, seamonkey -- like firefox -- no longer even has the ldap-subdirectory), so I'll try to fix our seamonkey port now by adding this patch to it. But the bug should, of course, be fixed by the "upstream" (that is -- you).

> I also see that the fix in
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=180473 was to force the
> use of gcc, so maybe it was a clang issue at the time, which got fixed since
> then ?

Clang was doing the right thing. Gcc was a little bit more cavalier, which masked the problem. It really is very simple: if libldap60.so calls PR_*-functions, it needs to link with nspr. We started with that...

Comment 7

[Landry Breuil (:gaston)]

(In reply to Mikhail T. from comment #6)
> (In reply to Landry Breuil (:gaston) from comment #5)
> > I still fail to see the usefulness of this patch, to me its not needed.
> 
> First of all, let's agree, that a shared library, which uses nspr-functions,
> needs to link with libnspr. Now we can discuss, how to achieve that.

I wont debate that, we agree here :)

> > nspr libs should be added by that block (which is the fallback for bsd/linux/etc):
> > 
> > 159 ifndef CUSTOM_LIBS
> > 160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
> > 161 EXTRA_LIBS += $(NSPRLINK)
> > 162 endif

I dont need to do anything, and on OpenBSD libprldap60.so is linked against nspr/has the correct dependency registered:

/usr/obj/c-c/ldap/sdks/c-sdk/ldap/libraries/ $objdump -p libprldap/libprldap60.so.1.0 | grep NEED
  NEEDED      libldap60.so.1.0
  NEEDED      libplds4.so.1.0
  NEEDED      libplc4.so.1.0
  NEEDED      libnspr4.so.1.0
 
> I think, the above was being skipped because libprldap/Makefile.in did
> define CUSTOM_LIBS. Of course, neither ldap/ nor libprldap/ are even found
> in today's firefox tarball so the proposed patch may no longer be needed
> indeed...
> 
> > So unless you have other patches in the freebsd port that change the default
> > behaviour...
> 
> FreeBSD port www/firefox (as well as devel/nspr and security/nss, that it
> relies upon) have dozens of patches... Most of them have been submitted to
> Mozilla, of course.
> 
> http://freshports.org/www/firefox

I know that, jan works a lot to upstream those patches. Here i'm just trying to make sense of it. Jan, any hint ?

> > oh and btw, my comm-central buildslave on freebsd/amd64 builds
> > without patches, (see
> > http://buildbot.rhaalovely.net/builders/comm-central-freebsd-amd64) and the
> > packaging step exercises the runtime.. 
> 
> That's because it is built with --disable-ldap. Trying to enable this option
> by running "make config" in the www/seamonkey port results in the following
> error-message from the port:

I said my comm-central builder - it builds thunderbird trunk which builds ldap. It is *not* a build of the freebsd port. If you look at http://buildbot.rhaalovely.net/builders/comm-central-freebsd-amd64/builds/440/steps/configure/logs/stdio it runs configure within ldap.

Comment 8

[Mikhail T.]

(In reply to Landry Breuil (:gaston) from comment #7)
> I dont need to do anything, and on OpenBSD libprldap60.so is linked against
> nspr/has the correct dependency registered

That is probably because pkgsrc (which OpenBSD and NetBSD use) already addresses this problem differently -- by patching ldap/sdks/c-sdk/build.mk to contain ${OS_LDFLAGS}. Or it may be because they do not share the same nspr (and nss) between all Mozilla applications as FreeBSD does. Note sure.

> I said my comm-central builder - it builds thunderbird trunk which builds
> ldap. It is *not* a build of the freebsd port. If you look at
> http://buildbot.rhaalovely.net/builders/comm-central-freebsd-amd64/builds/
> 440/steps/configure/logs/stdio it runs configure within ldap.

Our mail/thunderbird port contains exactly the patch I'm proposing here (its current version covers DragonFlyBSD in addition to FreeBSD, though). Look in files/patch-ldap-sdks-c-sdk-ldap-libraries-libprldap-Makefile.in

(Actually, the www/seamonkey port has the same patch. That it currently refuses to build with LDAP is for a different reason...)

Comment 9

[Landry Breuil (:gaston)]

(In reply to Mikhail T. from comment #8)
> (In reply to Landry Breuil (:gaston) from comment #7)
> > I dont need to do anything, and on OpenBSD libprldap60.so is linked against
> > nspr/has the correct dependency registered
> 
> That is probably because pkgsrc (which OpenBSD and NetBSD use) already
> addresses this problem differently -- by patching ldap/sdks/c-sdk/build.mk
> to contain ${OS_LDFLAGS}. Or it may be because they do not share the same
> nspr (and nss) between all Mozilla applications as FreeBSD does. Note sure.

No, no, no and no. OpenBSD doesnt use pkgsrc, and here i'm only talking about unpatched trunk builds, not "packaging builds". And both (packaging and trunk) builds against systemwide nspr or bundled nspr have no issues.

Comment 10

[Mikhail T.]

(In reply to Landry Breuil (:gaston) from comment #9)
Ok, I give up. I don't know, why builds on OpenBSD succeed without the patch. They do fail on FreeBSD, however.

I can give you an account on my FreeBSD-server, if you wish to see for yourself...

Comment 11

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #7)
>> > nspr libs should be added by that block (which is the fallback for
>> > bsd/linux/etc):
>> > 
>> > 159 ifndef CUSTOM_LIBS
>> > 160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
>> > 161 EXTRA_LIBS += $(NSPRLINK)
>> > 162 endif
>
> I dont need to do anything, and on OpenBSD libprldap60.so is linked against
> nspr/has the correct dependency registered:

Do you have -pthread or -lpthread in |nspr-config --libs| output?

Either EXTRA_LIBS is ignored by ld(1) if it contains invalid flags or
FreeBSD needs to link against libpthread explicitly in order to override
pthread_* stubs in libc and not break dlopen(3).

FYI, -pthread in NSPR_LIBS is a legacy from having multiple threading
implementation during FreeBSD 5.x days (-lc_r 1, -lthr 1:1, -lkse M:N).
The special case in configure can be removed letting FreeBSD fallback to
-lpthread like on Linux.

Comment 12

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #11)
> (In reply to Landry Breuil (:gaston) from comment #7)
> >> > nspr libs should be added by that block (which is the fallback for
> >> > bsd/linux/etc):
> >> > 
> >> > 159 ifndef CUSTOM_LIBS
> >> > 160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
> >> > 161 EXTRA_LIBS += $(NSPRLINK)
> >> > 162 endif
> >
> > I dont need to do anything, and on OpenBSD libprldap60.so is linked against
> > nspr/has the correct dependency registered:
> 
> Do you have -pthread or -lpthread in |nspr-config --libs| output?

As i said in comment 1, on the systemwide nspr, yes:

$nspr-config --libs
-L/usr/local/lib -lplds4 -lplc4 -lnspr4 -pthread -lc

> Either EXTRA_LIBS is ignored by ld(1) if it contains invalid flags or
> FreeBSD needs to link against libpthread explicitly in order to override
> pthread_* stubs in libc and not break dlopen(3).
> 
> FYI, -pthread in NSPR_LIBS is a legacy from having multiple threading
> implementation during FreeBSD 5.x days (-lc_r 1, -lthr 1:1, -lkse M:N).
> The special case in configure can be removed letting FreeBSD fallback to
> -lpthread like on Linux.

Fwiw, i have no opinion on the patch itself, i'm not even the reviewer for anything, i'm just trying to understand it.

Joshua, since you have plans for ldap build-system, any hint/opinion here ?

Comment 13

[Jan Beich]

To be addressed as part of bug 1112904 by fixing -pthread case.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Landry Breuil (:gaston) from comment #12)
> (In reply to Jan Beich from comment #11)
> > (In reply to Landry Breuil (:gaston) from comment #7)
> > >> > nspr libs should be added by that block (which is the fallback for
> > >> > bsd/linux/etc):
> > >> > 
> > >> > 159 ifndef CUSTOM_LIBS
> > >> > 160 EXTRA_LIBS = -L$(dist_libdir) -l$(LDAP_LIBNAME)
> > >> > 161 EXTRA_LIBS += $(NSPRLINK)
> > >> > 162 endif
> > >
> > > I dont need to do anything, and on OpenBSD libprldap60.so is linked against
> > > nspr/has the correct dependency registered:
> > 
> > Do you have -pthread or -lpthread in |nspr-config --libs| output?
> 
> As i said in comment 1, on the systemwide nspr, yes:
> 
> $nspr-config --libs
> -L/usr/local/lib -lplds4 -lplc4 -lnspr4 -pthread -lc
> 
> > Either EXTRA_LIBS is ignored by ld(1) if it contains invalid flags or
> > FreeBSD needs to link against libpthread explicitly in order to override
> > pthread_* stubs in libc and not break dlopen(3).
> > 
> > FYI, -pthread in NSPR_LIBS is a legacy from having multiple threading
> > implementation during FreeBSD 5.x days (-lc_r 1, -lthr 1:1, -lkse M:N).
> > The special case in configure can be removed letting FreeBSD fallback to
> > -lpthread like on Linux.
> 
> Fwiw, i have no opinion on the patch itself, i'm not even the reviewer for
> anything, i'm just trying to understand it.
> 
> Joshua, since you have plans for ldap build-system, any hint/opinion here ?

I don't have plans, only some wild ideas, so not really.

Comment 15

[Ping Chen [:rnons] (away)]

LDAP c-sdk has been removed in bug 1729862.