Created attachment 58328 [details] [review] configure: redo pthread check to check for more things It seems pthread_mutexattr_init and pthread_mutexattr_settype are in libpthread, not libc, on Linux. In principle, anything in the pthread namespace might be in the platform-specific thread library (libpthread or libpthreads or libthreads or ...) on any platform. Previously, a faulty configure check for pthread_cond_timedwait worked around this on Linux by checking for -lpthread and adding it to THREAD_LIBS if pthread_cond_timedwait *was* found in libc... but let's not rely on that. So far I've only added checks for the new symbols introduced by using recursive pthreads mutexes. If we get reports of compilation failures on weird platforms, we can check for more symbols. Also clarify the indentation, which was turning into quite a mess.
Review (re-)requested. This could break compilation on non-GNU Unixes.
Comment on attachment 58328 [details] [review] configure: redo pthread check to check for more things Review of attachment 58328 [details] [review]: ----------------------------------------------------------------- "In principle...on any platform." Be more specific. What real-world problem are you fixing? If this is just a theoretical problem, say so. ::: configure.ac @@ +947,2 @@ > save_libs="$LIBS" > LIBS="$LIBS $THREAD_LIBS" This bit no longer makes sense because we're not setting THREAD_LIBS yet. @@ +948,5 @@ > LIBS="$LIBS $THREAD_LIBS" > + > +is_missing_pthread_function="is required when compiling D-Bus on Unix platforms, but is not in your libc or libpthread. Please open a bug on https://bugs.freedesktop.org/enter_bug.cgi?product=dbus with details of your platform." > + > +if test "x$dbus_unix" = xyes; then One thing I learned recently is that you should use AS_IF() instead of "if" inside configure scripts. See https://bugzilla.gnome.org/show_bug.cgi?id=674483#c9 and following. @@ +972,5 @@ > + [AC_MSG_ERROR([pthread_mutexattr_settype $is_missing_pthread_function])], > + []) > + > + # Optional, for monotonic clocks > + AC_CHECK_FUNC([pthread_condattr_setclock]) Don't we need to have -lpthread in LIBS for this check to succeed? We so far only have it in THREAD_LIBS.
(In reply to comment #2) > Be more specific. What real-world problem are you fixing? If this is just a > theoretical problem, say so. That sentence was background information. The real-world problem is that we assumed that pthread_mutexattr_init, pthread_mutexattr_settype do not need extra libraries, which is false on Linux (they need -lpthread) However, due to a compensating bug in another check, it happened to work on platforms where pthread_cond_timedwait does *not* need extra libraries - again, like Linux. That check added -lpthread if and only if pthread_cond_timedwait is available *without* using -lpthread, which makes no sense. > > save_libs="$LIBS" > > LIBS="$LIBS $THREAD_LIBS" > > This bit no longer makes sense because we're not setting THREAD_LIBS yet. Hmm. I think it still has value if you "./configure THREAD_LIBS=-lmythreads", but if we're going to allow that, THREAD_LIBS should be an AC_ARG_VAR. > One thing I learned recently is that you should use AS_IF() instead of "if" > inside configure scripts. I'll bear that in mind in future. Patches to AS_IF()'ify the rest of configure.ac welcome (preferably with sane indentation). > > + # Optional, for monotonic clocks > > + AC_CHECK_FUNC([pthread_condattr_setclock]) > > Don't we need to have -lpthread in LIBS for this check to succeed? Yes, in principle this should be a non-fatal AC_SEARCH_LIBS (so we add -lpthread if and only if it's needed).
(In reply to comment #3) > The real-world problem is that we assumed that pthread_mutexattr_init, > pthread_mutexattr_settype do not need extra libraries, which is false on Linux > (they need -lpthread) The new patch is clearer about this, hopefully. > > > save_libs="$LIBS" > > > LIBS="$LIBS $THREAD_LIBS" > > > > This bit no longer makes sense because we're not setting THREAD_LIBS yet. > > Hmm. I think it still has value if you "./configure THREAD_LIBS=-lmythreads", > but if we're going to allow that, THREAD_LIBS should be an AC_ARG_VAR. In the new patch it's an AC_ARG_VAR, so you can "./configure THREAD_LIBS=-lmythreads" if your platform is one where -lpthread is insufficient. > > One thing I learned recently is that you should use AS_IF() instead of "if" > > inside configure scripts. > > I'll bear that in mind in future. Patches to AS_IF()'ify the rest of > configure.ac welcome (preferably with sane indentation). I wrote such a patch but it's +414, -456 lines (and doesn't replace case/esac with AS_CASE, which we should also do, for the same reason). That seems far too much churn for this bug, particularly if we fix this bug in dbus-1.6 (which is what I would suggest). I would like to fix at least this bug and Bug #38201 before rewriting a quarter of configure.ac... > > > + # Optional, for monotonic clocks > > > + AC_CHECK_FUNC([pthread_condattr_setclock]) > > > > Don't we need to have -lpthread in LIBS for this check to succeed? > > Yes, in principle this should be a non-fatal AC_SEARCH_LIBS (so we add > -lpthread if and only if it's needed). Now it is.
Created attachment 65511 [details] [review] dbus-sysdeps-pthread.c: don't fail if !HAVE_MONOTONIC_CLOCK under -Werror=unused --- I noticed while testing the new patch (specifically, a broken version that didn't ever set HAVE_MONOTONIC_CLOCK) that the build fails with a fatal warning under our "development" configuration.
Created attachment 65512 [details] [review] configure: redo pthread check to check for more things In principle, anything in the pthread namespace might either be in the platform-specific thread library (libpthread or libpthreads or libthreads or ...), or in libc. In particular, it seems that pthread_mutexattr_init and pthread_mutexattr_settype are in libpthread, not libc, on Linux. We previously didn't (intentionally) look for them in libpthread, only in libc; so this check deserved to fail. However, a faulty configure check for pthread_cond_timedwait worked around this on Linux by checking for -lpthread and adding it to THREAD_LIBS if pthread_cond_timedwait *was* found in libc (even though that behaviour makes no sense). The practical impact was that D-Bus would fail to compile on platforms where pthread_cond_timedwait is in a special threading library that is not linked by default, and at least one of (pthread_mutexattr_init, pthread_mutexattr_settype) is also in a special threading library. So far I've only added checks for the new symbols introduced by using recursive pthreads mutexes. If we get reports of compilation failures on weird platforms, we can check for more symbols. Also clarify the indentation, which was turning into quite a mess, and use AS_IF instead of if/elif/else/fi in accordance with Autoconf best-practice.
*** Bug 54416 has been marked as a duplicate of this bug. ***
Works for me with OpenBSD. It now properly detects and uses the pthreads library. checking for library containing pthread_cond_timedwait... -lpthread^M checking for library containing pthread_mutexattr_init... none required^M checking for library containing pthread_mutexattr_settype... none required^M checking for library containing pthread_condattr_setclock... none required^M checking for library containing clock_getres... none required^M checking for CLOCK_MONOTONIC... found
(In reply to comment #8) > Works for me with OpenBSD. It now properly detects and uses the pthreads > library. Did it previously fail? If so, that's a good reason to apply this. Colin or another maintainer: any chance of a re-review?
Yes, you marked my PR 54416 duplicate which was about libpthread being missing when linking dbus and my attempt at a fix.
Created attachment 69938 [details] [review] configure: redo pthread check to check for more things In principle, anything in the pthread namespace might either be in the platform-specific thread library (libpthread or libpthreads or libthreads or ...), or in libc. In particular, it seems that pthread_mutexattr_init and pthread_mutexattr_settype are in libpthread, not libc, on Linux. We previously didn't (intentionally) look for them in libpthread, only in libc; so this check deserved to fail. However, a faulty configure check for pthread_cond_timedwait worked around this on Linux by checking for -lpthread and adding it to THREAD_LIBS if pthread_cond_timedwait *was* found in libc (even though that behaviour makes no sense). The practical impact was that D-Bus would fail to compile on platforms where pthread_cond_timedwait is in a special threading library that is not linked by default, and at least one of (pthread_mutexattr_init, pthread_mutexattr_settype) is also in a special threading library. This is the case on at least OpenBSD (fd.o #54416). So far I've only added checks for the new symbols introduced by using recursive pthreads mutexes. If we get reports of compilation failures on weird platforms, we can check for more symbols. Also clarify the indentation, which was turning into quite a mess, and use AS_IF instead of if/elif/else/fi in accordance with Autoconf best-practice.
(In reply to comment #11) > configure: redo pthread check to check for more things This new version of the patch only changes the extended commit message to mention OpenBSD. The code is the same as the previous version.
Comment on attachment 69938 [details] [review] configure: redo pthread check to check for more things Review of attachment 69938 [details] [review]: ----------------------------------------------------------------- I just have one comment...feel free to address it or not (the patch is fine as is): ::: configure.ac @@ +972,5 @@ > + # In principle we might need to look in -lpthreads, -lthreads, ... > + # as well - please file a bug if your platform needs this. > + AC_SEARCH_LIBS([pthread_cond_timedwait], > + [pthread], > + [THREAD_LIBS="$LIBS"], I think this patch will function correctly, but what we're doing here seems odd to me. AC_SEARCH_LIBS will automatically mutate LIBS, so we could just drop the whole THREAD_LIBS variable.
(In reply to comment #13) > I think this patch will function correctly, but what we're doing here seems > odd to me. AC_SEARCH_LIBS will automatically mutate LIBS, so we could just > drop the whole THREAD_LIBS variable. This whole block is bracketed in: save_LIBS="$LIBS" ... things that mutate LIBS, and set THREAD_LIBS on success... LIBS="$save_LIBS" This is a semi-common idiom for making AC_SEARCH_LIBS and friends (which set automatically-used variables from which you'd have to "opt out" if you don't want to use them globally) behave more like PKG_CHECK_MODULES (which sets unique variables for which you have to "opt in" for the objects that want them). I believe the intention is: * have the thread libraries in $THREAD_LIBS so we can substitute them in the .pc file's Libs line without potentially also substituting libraries we didn't want there * link each object against the libraries it needs, and not against the libraries it doesn't (Admittedly, just about everything in the tree will pick up the thread libraries via either libdbus-1.la or libdbus-internal.la anyway.)
(In reply to comment #13) > the patch is fine as is Merged to master for 1.7.0, thanks. This is quite a bit of diffstat, so I didn't put it on the dbus-1.6 stable branch. For those whose Unix flavour is actually affected by this issue, the commits to cherry-pick are 277675a and 0b7ab6c (or you should be able to work around it by configuring with THREAD_LIBS=-lpthread, I think). (In reply to comment #5) > dbus-sysdeps-pthread.c: don't fail if !HAVE_MONOTONIC_CLOCK under > -Werror=unused I merged this to master too, since it's so trivial and I gave people plenty of chance to comment. Please revert+reopen if you object.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.