Closed
Bug 712281
Opened 13 years ago
Closed 13 years ago
Check for dladdr during configure
Categories
(NSPR :: NSPR, defect, P2)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.9
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
The support for dladdr is currently hardcoded in prlink.c as not being available for Android. In fact, it is available in the headers for Android >= 2.2. The way this is handled in the main configure is to use ANDROID_VERSION to decide whether dladdr is available or not, but since I'm planning to allow nspr to use dladdr even when the headers don't have it, it would be easier if it were overridable without changing the Android headers being used.
All in all, the simple solution is just to check whether the function is available or not with an AC_CHECK.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #583131 -
Flags: review?(wtc)
Assignee | ||
Updated•13 years ago
|
Assignee: wtc → mh+mozilla
Comment 2•13 years ago
|
||
Comment on attachment 583131 [details] [diff] [review]
Check for dladdr during configure
Thanks for the patch.
In nsprpub/configure.in:
>+AC_CHECK_FUNCS(dladdr)
There is an existing AC_CHECK_FUNCS in nsprpub/configure.in:
AC_CHECK_FUNCS(lchown strerror)
We can add dladdr to the existing AC_CHECK_FUNCS.
When I run the updated configure on Linux, it says:
checking for dladdr... no
I think it is because the test is performed without -ldl.
In nsprpub/pr/src/linking/prlink.c:
> PR_IMPLEMENT(char *)
> PR_GetLibraryFilePathname(const char *name, PRFuncPtr addr)
> {
>-#if defined(USE_DLFCN) && !defined(ANDROID) \
>+#if defined(USE_DLFCN) && defined(HAVE_DLADDR) \
> && (defined(SOLARIS) || defined(FREEBSD) \
> || defined(LINUX) || defined(__GNU__) || defined(__GLIBC__) \
> || defined(DARWIN))
If the AC_CHECK_FUNCS(dladdr) autoconf test works as expected, then
this whole ifdef can be simplified to be just
#if defined(USE_DLFCN) && defined(HAVE_DLADDR)
Correct?
Attachment #583131 -
Flags: review?(wtc) → review-
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Wan-Teh Chang from comment #2)
> Comment on attachment 583131 [details] [diff] [review]
> Check for dladdr during configure
>
> Thanks for the patch.
>
> In nsprpub/configure.in:
>
> >+AC_CHECK_FUNCS(dladdr)
>
> There is an existing AC_CHECK_FUNCS in nsprpub/configure.in:
>
> AC_CHECK_FUNCS(lchown strerror)
>
> We can add dladdr to the existing AC_CHECK_FUNCS.
>
> When I run the updated configure on Linux, it says:
>
> checking for dladdr... no
>
> I think it is because the test is performed without -ldl.
Oh, so I guess that the main configure in the mozilla tree is adding -ldl to the LDFLAGS.
> In nsprpub/pr/src/linking/prlink.c:
>
> > PR_IMPLEMENT(char *)
> > PR_GetLibraryFilePathname(const char *name, PRFuncPtr addr)
> > {
> >-#if defined(USE_DLFCN) && !defined(ANDROID) \
> >+#if defined(USE_DLFCN) && defined(HAVE_DLADDR) \
> > && (defined(SOLARIS) || defined(FREEBSD) \
> > || defined(LINUX) || defined(__GNU__) || defined(__GLIBC__) \
> > || defined(DARWIN))
>
> If the AC_CHECK_FUNCS(dladdr) autoconf test works as expected, then
> this whole ifdef can be simplified to be just
>
> #if defined(USE_DLFCN) && defined(HAVE_DLADDR)
>
> Correct?
In theory, yes.
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #583402 -
Flags: review?(wtc)
Assignee | ||
Updated•13 years ago
|
Attachment #583131 -
Attachment is obsolete: true
Comment 5•13 years ago
|
||
Comment on attachment 583402 [details] [diff] [review]
Check for dladdr during configure
Is there an autoconf macro that we can use to test if
the header <dlfcn.h> declares the dladdr() function?
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Wan-Teh Chang from comment #5)
> Comment on attachment 583402 [details] [diff] [review]
> Check for dladdr during configure
>
> Is there an autoconf macro that we can use to test if
> the header <dlfcn.h> declares the dladdr() function?
I don't think there is. If you want to write such a test, make sure to define _GNU_SOURCE when we define it (android and linux), because without it on linux, you don't get dladdr.
Assignee | ||
Comment 7•13 years ago
|
||
Wan-Teh, how do you want to go forward on this bug?
Assignee | ||
Comment 8•13 years ago
|
||
Comment on attachment 583402 [details] [diff] [review]
Check for dladdr during configure
I'd s/SAVE_LDFLAGS/_SAVE_LDFLAGS/ when landing, though.
Attachment #583402 -
Flags: review?(wtc) → review?(ted.mielczarek)
Comment 9•13 years ago
|
||
Comment on attachment 583402 [details] [diff] [review]
Check for dladdr during configure
Review of attachment 583402 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is reasonable.
Attachment #583402 -
Flags: review?(ted.mielczarek) → review+
Comment 10•13 years ago
|
||
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v <-- configure
new revision: 1.319; previous revision: 1.318
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v <-- configure.in
new revision: 1.321; previous revision: 1.320
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v <-- prlink.c
new revision: 3.111; previous revision: 3.110
done
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 11•13 years ago
|
||
This broke NSPR for me apparently. See bug 724243
Comment 12•13 years ago
|
||
Comment on attachment 583402 [details] [diff] [review]
Check for dladdr during configure
Review of attachment 583402 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry about the late review.
::: nsprpub/pr/src/linking/prlink.c
@@ -1362,5 @@
> {
> -#if defined(USE_DLFCN) && !defined(ANDROID) \
> - && (defined(SOLARIS) || defined(FREEBSD) \
> - || defined(LINUX) || defined(__GNU__) || defined(__GLIBC__) \
> - || defined(DARWIN))
We should verify the configure script detects dladdr
on Solaris, FreeBSD, and Mac OS X. Otherwise it'd be
safer to say
#if defined(USE_DLFCN) && (defined(HAVE_DLADDR) \
&& (defined(SOLARIS) || defined(FREEBSD) \
|| defined(DARWIN))
Based on my inspection of nsprpub/configure.in, I
believe the configure script adds -ldl to OS_LIBS
on these three platforms, so it should be able to
detect dladdr.
Updated•13 years ago
|
Priority: -- → P2
Target Milestone: --- → 4.9
You need to log in
before you can comment on or make changes to this bug.
Description
•