From 4d828c5eba9fc7161c5f18650f2dbe218e1db06f Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Tue, 23 Oct 2007 11:03:33 -0700 Subject: [PATCH 1/6] Don't abort() on locking assertions if LIBXCB_ALLOW_SLOPPY_LOCK is set. But do still print a full backtrace, on platforms where that's supported. This commit follows the spirit of Novell's libxcb-sloppy-lock.diff. I strongly opposed proposals like this one for a long time. Originally I had a very good reason: libX11, when compiled to use XCB, would crash soon after a locking correctness violation, so it was better to have an informative assert failure than a mystifying crash soon after. It took some time for me to realize that I'd changed the libX11 implementation (for unrelated reasons) so that it could survive most invalid locking situations, as long as it wasn't actually being used from multiple threads concurrently. The other thing that has changed is that most of the code with incorrect locking has now been fixed. The value of the assert is accordingly lower. However, remaining broken callers do need to be fixed. That's why libXCB will still noisily print a stacktrace (if possible) on each assertion failure, even when assert isn't actually invoked to abort() the program; and that's why aborting is still default. This environment variable is provided only for use as a temporary workaround for broken applications. Signed-off-by: Jamey Sharp Acked-by: Josh Triplett --- src/xcb_conn.c | 3 +++ src/xcb_xlib.c | 10 +++++----- src/xcbint.h | 1 + 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/xcb_conn.c b/src/xcb_conn.c index 3b315bc..827a12b 100644 --- a/src/xcb_conn.c +++ b/src/xcb_conn.c @@ -62,6 +62,9 @@ static int set_fd_flags(const int fd) static int _xcb_xlib_init(_xcb_xlib *xlib) { xlib->lock = 0; +#ifndef NDEBUG + xlib->sloppy_lock = (getenv("LIBXCB_ALLOW_SLOPPY_LOCK") != 0); +#endif pthread_cond_init(&xlib->cond, 0); return 1; } diff --git a/src/xcb_xlib.c b/src/xcb_xlib.c index 35ad3c3..1b573e8 100644 --- a/src/xcb_xlib.c +++ b/src/xcb_xlib.c @@ -55,9 +55,9 @@ static void xcb_xlib_printbt(void) } #ifndef NDEBUG -#define xcb_assert(x) do { if (!(x)) { xcb_xlib_printbt(); assert(x); } } while(0) +#define xcb_assert(c,x) do { if (!(x)) { xcb_xlib_printbt(); if (!(c)->xlib.sloppy_lock) assert(x); } } while(0) #else -#define xcb_assert(x) +#define xcb_assert(c,x) #endif unsigned int xcb_get_request_sent(xcb_connection_t *c) @@ -70,7 +70,7 @@ unsigned int xcb_get_request_sent(xcb_connection_t *c) void xcb_xlib_lock(xcb_connection_t *c) { _xcb_lock_io(c); - xcb_assert(!c->xlib.lock); + xcb_assert(c, !c->xlib.lock); c->xlib.lock = 1; c->xlib.thread = pthread_self(); _xcb_unlock_io(c); @@ -79,8 +79,8 @@ void xcb_xlib_lock(xcb_connection_t *c) void xcb_xlib_unlock(xcb_connection_t *c) { _xcb_lock_io(c); - xcb_assert(c->xlib.lock); - xcb_assert(pthread_equal(c->xlib.thread, pthread_self())); + xcb_assert(c, c->xlib.lock); + xcb_assert(c, pthread_equal(c->xlib.thread, pthread_self())); c->xlib.lock = 0; pthread_cond_broadcast(&c->xlib.cond); _xcb_unlock_io(c); diff --git a/src/xcbint.h b/src/xcbint.h index a8e167c..ab692ee 100644 --- a/src/xcbint.h +++ b/src/xcbint.h @@ -130,6 +130,7 @@ int _xcb_in_read_block(xcb_connection_t *c, void *buf, int nread); typedef struct _xcb_xlib { int lock; + int sloppy_lock; pthread_t thread; pthread_cond_t cond; } _xcb_xlib; From f6b75d6090dc40918196d2b902e9616d0199af42 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Sun, 28 Oct 2007 11:56:08 -0700 Subject: [PATCH 2/6] Factor pthread_cond_wait(iolock) to _xcb_wait_io. This parallels the _xcb_lock_io and _xcb_unlock_io factoring. --- src/xcb_conn.c | 7 ++++++- src/xcb_out.c | 4 ++-- src/xcbint.h | 1 + 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/xcb_conn.c b/src/xcb_conn.c index 827a12b..9a58bff 100644 --- a/src/xcb_conn.c +++ b/src/xcb_conn.c @@ -288,6 +288,11 @@ void _xcb_unlock_io(xcb_connection_t *c) pthread_mutex_unlock(&c->iolock); } +void _xcb_wait_io(xcb_connection_t *c, pthread_cond_t *cond) +{ + pthread_cond_wait(cond, &c->iolock); +} + int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vector, int *count) { int ret; @@ -296,7 +301,7 @@ int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec /* If the thing I should be doing is already being done, wait for it. */ if(count ? c->out.writing : c->in.reading) { - pthread_cond_wait(cond, &c->iolock); + _xcb_wait_io(c, cond); return 1; } diff --git a/src/xcb_out.c b/src/xcb_out.c index caf8ef5..60226e5 100644 --- a/src/xcb_out.c +++ b/src/xcb_out.c @@ -190,7 +190,7 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect _xcb_lock_io(c); /* wait for other writing threads to get out of my way. */ while(c->out.writing) - pthread_cond_wait(&c->out.cond, &c->iolock); + _xcb_wait_io(c, &c->out.cond); request = ++c->out.request; /* send GetInputFocus (sync) when 64k-2 requests have been sent without @@ -297,7 +297,7 @@ int _xcb_out_flush_to(xcb_connection_t *c, unsigned int request) return _xcb_out_send(c, &vec_ptr, &count); } while(c->out.writing) - pthread_cond_wait(&c->out.cond, &c->iolock); + _xcb_wait_io(c, &c->out.cond); assert(XCB_SEQUENCE_COMPARE(c->out.request_written, >=, request)); return 1; } diff --git a/src/xcbint.h b/src/xcbint.h index ab692ee..ab0264f 100644 --- a/src/xcbint.h +++ b/src/xcbint.h @@ -183,6 +183,7 @@ struct xcb_connection_t { }; void _xcb_conn_shutdown(xcb_connection_t *c); +void _xcb_wait_io(xcb_connection_t *c, pthread_cond_t *cond); int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vector, int *count); From a29fbc2645fabb96d02c382ffef499b48fb1514a Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Sun, 28 Oct 2007 13:28:18 -0700 Subject: [PATCH 3/6] Don't hold the xlib-xcb lock while sleeping: that allows deadlock. With this patch, `ico -threads 2` runs without deadlock. Many thanks to Christoph Pfister for pointing out the problem, providing detailed analyses, explaining it to me repeatedly until I understood what was going on, and proposing and reviewing possible solutions. Signed-off-by: Jamey Sharp Acked-by: Christoph Pfister --- src/xcb_conn.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/xcb_conn.c b/src/xcb_conn.c index 9a58bff..e7856c3 100644 --- a/src/xcb_conn.c +++ b/src/xcb_conn.c @@ -290,12 +290,25 @@ void _xcb_unlock_io(xcb_connection_t *c) void _xcb_wait_io(xcb_connection_t *c, pthread_cond_t *cond) { + int xlib_locked = c->xlib.lock; + if(xlib_locked) + { + c->xlib.lock = 0; + pthread_cond_broadcast(&c->xlib.cond); + } pthread_cond_wait(cond, &c->iolock); + if(xlib_locked) + { + while(c->xlib.lock) + pthread_cond_wait(&c->xlib.cond, &c->iolock); + c->xlib.lock = 1; + c->xlib.thread = pthread_self(); + } } int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vector, int *count) { - int ret; + int ret, xlib_locked; fd_set rfds, wfds; /* If the thing I should be doing is already being done, wait for it. */ @@ -316,6 +329,12 @@ int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec ++c->out.writing; } + xlib_locked = c->xlib.lock; + if(xlib_locked) + { + c->xlib.lock = 0; + pthread_cond_broadcast(&c->xlib.cond); + } _xcb_unlock_io(c); do { ret = select(c->fd + 1, &rfds, &wfds, 0, 0); @@ -326,6 +345,11 @@ int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec ret = 0; } _xcb_lock_io(c); + if(xlib_locked) + { + c->xlib.lock = 1; + c->xlib.thread = pthread_self(); + } if(ret) { From af50de26c10c93ccc4cd3bc61e92aff47651b961 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Sun, 4 Nov 2007 17:26:21 -0800 Subject: [PATCH 4/6] Revert "Generate error constants as XCB_BAD_*, similar to Xlib." Since several extensions named their errors like "BadFoo", this patch results in names like XCB_EXT_BAD_BAD_FOO, which is really awful. Those extensions are already kind of awful, as they produce structure names like xcb_ext_bad_foo_error_t, which is redundant. A patch that removes "Bad" from the XML extension descriptions, while maintaining API and ABI compatibility in XCB, is needed before this patch can be released. This reverts commit 158c9b6ba18b39f424bd524fceb66f3fec0d1616. --- src/c-client.xsl | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/c-client.xsl b/src/c-client.xsl index c90e94f..be6aa30 100644 --- a/src/c-client.xsl +++ b/src/c-client.xsl @@ -449,11 +449,6 @@ authorization from the authors. - - - - - From 3c6c8f127c2bce4f45bface7dd45cc719af9de0d Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Sun, 4 Nov 2007 17:29:13 -0800 Subject: [PATCH 5/6] Release libxcb 1.1 --- NEWS | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ README | 9 +++++---- configure.ac | 4 ++-- 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index 91e7348..a0260cb 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,60 @@ +Release 1.1 (2007-11-04) +======================== + +This release requires xcb-proto 1.1, due to the addition of the +extension-multiword attribute to the XML schema. + +This release contains several important bug fixes, summarized below. It +also contains a patch much like Novell's libxcb-sloppy-lock.diff. +Rationale from the commit message follows. The patch and this rationale +were authored by Jamey Sharp , with agreement from +Josh Triplett . + + I strongly opposed proposals like this one for a long time. + Originally I had a very good reason: libX11, when compiled to use + XCB, would crash soon after a locking correctness violation, so it + was better to have an informative assert failure than a mystifying + crash soon after. + + It took some time for me to realize that I'd changed the libX11 + implementation (for unrelated reasons) so that it could survive most + invalid locking situations, as long as it wasn't actually being used + from multiple threads concurrently. + + The other thing that has changed is that most of the code with + incorrect locking has now been fixed. The value of the assert is + accordingly lower. + + However, remaining broken callers do need to be fixed. That's why + libXCB will still noisily print a stacktrace (if possible) on each + assertion failure, even when assert isn't actually invoked to + abort() the program; and that's why aborting is still default. This + environment variable is provided only for use as a temporary + workaround for broken applications. + +Enhancements: +* Print a backtrace, if possible, on locking assertion failures. +* Skip abort() on locking assertions if LIBXCB_ALLOW_SLOPPY_LOCK is set. +* xcb_poll_for_event: Return already-read events before reading again. +* Output a configuration summary at the end of ./configure. + +Bug fixes: +* Don't hold the xlib-xcb lock while sleeping: that allows deadlock. +* Allow unix: style display names again. +* Bug #9119: test xcb_popcount +* Fix unit tests for FreeBSD +* NetBSD doesn't have AI_ADDRCONFIG: use it only if it's available. +* Require libXau >= 0.99.2; earlier versions have a broken .pc file +* Use substitition variables in xcb-xinerama.pc.in +* Update autogen.sh to one that does objdir != srcdir +* Add tools/* and autogen.sh to EXTRA_DIST. +* Doxygen can now be fully disabled if desired. + +Documentation improvements: +* Many fixes and updates to the tutorial. +* Iterators, requests, and replies get partial Doxygen documentation. + + Release 1.0 (2006-11-23) ======================== diff --git a/README b/README index 5629fe9..167c8ac 100644 --- a/README +++ b/README @@ -1,11 +1,12 @@ About libxcb ============ -libxcb provides an interface to the X Window System protocol, which replaces -the current Xlib interface. It has several advantages over Xlib, including: -- size: small library and lower memory footprint +libxcb provides an interface to the X Window System protocol, which +replaces the current Xlib interface. It has several advantages over +Xlib, including: +- size: small, simple library, and lower memory footprint - latency hiding: batch several requests and wait for the replies later -- direct protocol access: one-to-one mapping between interface and protocol +- direct protocol access: interface and protocol correspond exactly - proven thread support: transparently access XCB from multiple threads - easy extension implementation: interfaces auto-generated from XML-XCB diff --git a/configure.ac b/configure.ac index b446e7f..df554e6 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ(2.57) AC_INIT([libxcb], - 1.0, + 1.1, [xcb@lists.freedesktop.org]) AC_CONFIG_SRCDIR([xcb.pc.in]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) @@ -30,7 +30,7 @@ fi AC_SUBST(HTML_CHECK_RESULT) # Checks for pkg-config packages -PKG_CHECK_MODULES(XCBPROTO, xcb-proto >= 1.0) +PKG_CHECK_MODULES(XCBPROTO, xcb-proto >= 1.1) NEEDED="pthread-stubs xau >= 0.99.2" PKG_CHECK_MODULES(NEEDED, $NEEDED) From 46413cd85ee4f3d51a3a3e1d8ee13bc5fa6c2d5d Mon Sep 17 00:00:00 2001 From: Eamon Walsh Date: Fri, 16 Nov 2007 19:34:42 -0500 Subject: [PATCH 6/6] Add comment noting the requirement to free replies when finished. --- src/c-client.xsl | 2 ++ xcb-xselinux.pc.in | 11 +++++++++++ 2 files changed, 13 insertions(+) create mode 100644 xcb-xselinux.pc.in diff --git a/src/c-client.xsl b/src/c-client.xsl index be6aa30..a15d824 100644 --- a/src/c-client.xsl +++ b/src/c-client.xsl @@ -407,6 +407,8 @@ authorization from the authors. * The parameter @p e supplied to this function must be NULL if * _unchecked(). is used. * Otherwise, it stores the error if any. + * + * The returned value must be freed by the caller using free(). */ diff --git a/xcb-xselinux.pc.in b/xcb-xselinux.pc.in new file mode 100644 index 0000000..6a71f73 --- /dev/null +++ b/xcb-xselinux.pc.in @@ -0,0 +1,11 @@ +prefix=@prefix@ +exec_prefix=@exec_prefix@ +libdir=@libdir@ +includedir=@includedir@ + +Name: XCB SELinux +Description: XCB SELinux Extension +Version: @PACKAGE_VERSION@ +Requires: xcb +Libs: -L${libdir} -lxcb-xselinux +Cflags: -I${includedir}