<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/3.26.0"> </HEAD> <BODY> On Thu, 2011-02-03 at 17:53 +0200, Rami Ylimäki wrote: <BLOCKQUOTE TYPE=CITE> <PRE> 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 <<A HREF="mailto:rami.ylimaki@vincit.fi">rami.ylimaki@vincit.fi</A>> Reviewed-by: Erkki Seppälä <<A HREF="mailto:erkki.seppala@vincit.fi">erkki.seppala@vincit.fi</A>> --- 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. </PRE> </BLOCKQUOTE> Automake recommends against using dnl in lieu of # for comments. <BLOCKQUOTE TYPE=CITE> <PRE> +AC_ARG_ENABLE( + checked_locks, +        AC_HELP_STRING( </PRE> </BLOCKQUOTE> Should be AS_HELP_STRING as AC_HELP_STRING is deprecated. <BLOCKQUOTE TYPE=CITE> <PRE> + [--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) </PRE> </BLOCKQUOTE> 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 <TT>AM_PROG_CC_C_O</TT> 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? <BLOCKQUOTE TYPE=CITE> <PRE> +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 _______________________________________________ <A HREF="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</A>: X.Org development Archives: <A HREF="http://lists.x.org/archives/xorg-devel">http://lists.x.org/archives/xorg-devel</A> Info: <A HREF="http://lists.x.org/mailman/listinfo/xorg-devel">http://lists.x.org/mailman/listinfo/xorg-devel</A> </PRE> </BLOCKQUOTE> </BODY> </HTML>