[PATCH v2 libX11 2/2] config: Add --enable-checked-locks to catch missing XInitThreads calls.
Gaetan Nadon
memsize at videotron.ca
Thu Feb 3 12:21:01 PST 2011
On Thu, 2011-02-03 at 17:53 +0200, Rami Ylimäki wrote:
> This change makes it possible to detect missing XInitThreads calls in
> X clients. If a client hasn't called XInitThreads and enters a
> critical section in Xlib concurrently, the client will be aborted with
> a message informing about the missing call.
>
> Enabling thread safety by default causes a minor performance
> regression for single threaded X clients. However, the performance
> regression is only noticeable in the worst case situation. In
> realistic use cases the overhead from unnecessary locking is difficult
> to see.
>
> Relative x11perf results showing the amount of regression for worst
> case and some common operations are shown below.
>
> unlocked/locked Operation
> ----------------- ------------------------------------
> 1.001 500-pixel line
> 1.000 Copy 500x500 from window to window
> 2.609 X protocol NoOperation
>
> Signed-off-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
> Reviewed-by: Erkki Seppälä <erkki.seppala at vincit.fi>
> ---
> configure.ac | 37 +++++++++++++++++++++++++++++++++++++
> src/Makefile.am | 5 +++++
> src/locking.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 92 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 40d032d..c5b000d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -270,6 +270,38 @@ AC_ARG_ENABLE(xthreads,
> [Disable Xlib support for Multithreading]),
> [xthreads=$enableval],[xthreads=yes])
>
> +dnl Determine whether the user wants to check for unsafe usage of Xlib
> +dnl from multiple threads.
Automake recommends against using dnl in lieu of # for comments.
> +AC_ARG_ENABLE(
> + checked_locks,
> + AC_HELP_STRING(
Should be AS_HELP_STRING as AC_HELP_STRING is deprecated.
> + [--enable-checked-locks],
> + [Check if multithreaded clients have missed calling XInitThreads]),
> + [checked_locks=$enableval],
> + [checked_locks=no])
> +
> +dnl Check if XInitThreads can be called by default on library
> +dnl initialization. The following conditions must be true for that.
> +dnl
> +dnl - Option --enable-xthreads is given.
> +dnl - Option --enable-checked-locks is given.
> +dnl - Compiler is GCC.
> +dnl - GCC supports contructors.
> +if test "x$xthreads" = xyes && test "x$checked_locks" = xyes && test "x$GCC" = xyes ; then
> + dnl Set warnings as errors so that GCC fails if the attribute is
> + dnl not supported.
> + old_CFLAGS="$CFLAGS"
> + CFLAGS="-Werror"
> + dnl Check if GCC recognizes constructor attribute.
> + AC_COMPILE_IFELSE(
> + [ void __attribute__((constructor)) test(void) {}; ],
> + checked_locks=yes,
> + checked_locks=no)
> + dnl Restore flags.
> + CFLAGS="$old_CFLAGS"
> +fi
> +AM_CONDITIONAL(XTHREADS_CHECKED_LOCKS, [test "x$checked_locks" = xyes])
> +
> AC_CHECK_LIB(c, getpwuid_r, [mtsafeapi="yes"], [mtsafeapi="no"])
>
> case x$xthreads in
> @@ -279,6 +311,10 @@ xyes)
> then
> AC_DEFINE(XUSE_MTSAFE_API,1,[Whether libX11 needs to use MT safe API's])
> fi
> + if test x$checked_locks = xyes
> + then
> + AC_DEFINE(XTHREADS_CHECKED_LOCKS,1,[Whether multithreaded clients have missed calling XInitThreads])
> + fi
> ;;
> *)
> ;;
> @@ -480,6 +516,7 @@ echo " Loadable xcursor library support: "$XLIB_LOADABLE_XCURSOR
> echo " Threading support: "$xthreads
> echo " Use Threads safe API: "$mtsafeapi
> echo " Threads stubs in libX11: "$thrstubs
> +echo " Missing XInitThreads calls are detected: "$checked_locks
> echo " XCMS: "$XCMS
> echo " Internationalization support: "$XLOCALE
> echo " XF86BigFont support: "$XF86BIGFONT
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8b0953c..097496a 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -377,6 +377,11 @@ libX11_la_LIBADD = \
> $(USE_XKB_LIBS) \
> $(X11_LIBS)
>
> +if XTHREADS_CHECKED_LOCKS
> +libX11_la_CFLAGS = $(AM_CFLAGS) $(XTHREAD_FLAGS)
Should that not be XTHREAD_CFLAGS? typo perhaps
Note that libX11_la_CFLAGS introduces per-target flags as these flags
can no longer be shared with the x11_xcb.o target. All the libX11 source
files are compiled as libX11_la-*.o now. The use of per-target
compilation flags with C sources requires that the macro AM_PROG_CC_C_O
be called from configure.ac.
I would recommend moving libX11-xcb to a subdir. Having dissimilar
targets in the same makefile is always asking for trouble. The only case
where per-target flags are needed is when the same object gets compiled
twice with different options for two different libraries, like xaw6 and
xaw7 IIRC.
Should libX11_la_CFLAGS be added to the lint target?
> +libX11_la_LIBADD += $(XTHREADLIB)
> +endif XTHREADS_CHECKED_LOCKS
> +
> preprocess: $(patsubst %.c,%.ii,$(libX11_la_SOURCES))
> .c.ii:
> $(COMPILE) -E -o $@ `test -f '$<' || echo '$(srcdir)/'`$<
> diff --git a/src/locking.c b/src/locking.c
> index 4f9a40f..aa310d6 100644
> --- a/src/locking.c
> +++ b/src/locking.c
> @@ -47,7 +47,7 @@ in this Software without prior written authorization from The Open Group.
>
> #include "Xprivate.h"
> #include "locking.h"
> -#ifdef XTHREADS_WARN
> +#if defined(XTHREADS_WARN) || defined(XTHREADS_CHECKED_LOCKS)
> #include <stdio.h> /* for warn/debug stuff */
> #endif
>
> @@ -99,12 +99,43 @@ static xthread_t _Xthread_self(void)
> static LockInfoRec global_lock;
> static LockInfoRec i18n_lock;
>
> +#ifdef XTHREADS_CHECKED_LOCKS
> +/** User hasn't called XInitThreads yet and we wan't to detect whether
> + * Xlib is used incorrectly from multiple threads. */
> +static Bool check_locks = True;
> +#endif /* XTHREADS_CHECKED_LOCKS */
> +
> +/**
> + * Normally this function will just lock the parameter mutex. However,
> + * the function will abort if all of the conditions listed below hold.
> + *
> + * 1. Xlib has been configured using --enable-checked-locks option.
> + * 2. XInitThreads hasn't been called by user yet. Note that it has
> + * been called by the library from XInitLib.
> + * 3. The lock given as parameter hasn't been acquired already.
> + */
> +static int xmutex_lock_checked(xmutex_t lock)
> +{
> +#ifdef XTHREADS_CHECKED_LOCKS
> + if (!check_locks)
> +#endif /* XTHREADS_CHECKED_LOCKS */
> + return xmutex_lock(lock);
> +#ifdef XTHREADS_CHECKED_LOCKS
> + if (!xmutex_trylock(lock))
> + return 0;
> + printf("Your program has been terminated because it entered a critical section\n"
> + "in Xlib simultaneously from more than one thread. To avoid this, you\n"
> + "should call XInitThreads before doing any other Xlib calls.\n");
> + abort();
> +#endif /* XTHREADS_CHECKED_LOCKS */
> +}
> +
> static void _XLockMutex(
> LockInfoPtr lip
> XTHREADS_FILE_LINE_ARGS
> )
> {
> - xmutex_lock(lip->lock);
> + xmutex_lock_checked(lip->lock);
> }
>
> static void _XUnlockMutex(
> @@ -172,7 +203,7 @@ static void _XLockDisplayWarn(
> #endif /* XTHREADS_DEBUG */
> }
>
> - xmutex_lock(dpy->lock->mutex);
> + xmutex_lock_checked(dpy->lock->mutex);
>
> if (strcmp(file, "XlibInt.c") == 0) {
> if (!xlibint_unlock)
> @@ -456,7 +487,7 @@ static void _XLockDisplay(
> #ifdef XTHREADS_WARN
> _XLockDisplayWarn(dpy, file, line);
> #else
> - xmutex_lock(dpy->lock->mutex);
> + xmutex_lock_checked(dpy->lock->mutex);
> #endif
> if (dpy->lock->locking_level > 0)
> _XDisplayLockWait(dpy);
> @@ -477,7 +508,7 @@ static void _XInternalLockDisplay(
> #ifdef XTHREADS_WARN
> _XLockDisplayWarn(dpy, file, line);
> #else
> - xmutex_lock(dpy->lock->mutex);
> + xmutex_lock_checked(dpy->lock->mutex);
> #endif
> if (!wskip && dpy->lock->locking_level > 0)
> _XDisplayLockWait(dpy);
> @@ -570,7 +601,13 @@ xthread_t (*_x11_thr_self)() = __x11_thr_self;
> Status XInitThreads(void)
> {
> if (_Xglobal_lock)
> + {
> +#ifdef XTHREADS_CHECKED_LOCKS
> + check_locks = False;
> +#endif /* XTHREADS_CHECKED_LOCKS */
> return 1;
> + }
> +
> #ifdef __UNIXWARE__
> else {
> void *dl_handle = dlopen(NULL, RTLD_LAZY);
> @@ -615,6 +652,14 @@ Status XInitThreads(void)
> return 1;
> }
>
> +#ifdef XTHREADS_CHECKED_LOCKS
> +/** Initializes threads by default when Xlib is loaded. */
> +static void __attribute__((constructor)) XInitLib(void)
> +{
> + if (!XInitThreads())
> + abort();
> +}
> +#endif /* XTHREADS_CHECKED_LOCKS */
> #else /* XTHREADS */
> Status XInitThreads(void)
> {
> --
> 1.6.3.3
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110203/1ca11307/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110203/1ca11307/attachment.pgp>
More information about the xorg-devel
mailing list