Re: xquartz dereferencing a NULL pointer (patch 2)
Matthieu Herrb wrote: > Simon Thum wrote: >> Tiago Vignatti wrote: >>> http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html >>> >>> Does we have this kind of thing in C libraries? It would be useful. >> If I got it right, that's posix + pthreads functionality specialized for >> an amtel platform. So in general, yes. > > Sorry, but no. These macros are not standard at all, and without > explicit using of some kind of lock, there no portable way to handle > atomicity in pthreads afaik. Actually, I tried to argue that the functionality is covered by posix+pthreads, as far as is needed at least. So 'yes' meant yes, we've got that kind of thing covered, in some sense. I didn't intend to argue for those macros in X at all. X should stick to pthreads of course. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
Simon Thum wrote: > Tiago Vignatti wrote: >> http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html >> >> Does we have this kind of thing in C libraries? It would be useful. > If I got it right, that's posix + pthreads functionality specialized for > an amtel platform. So in general, yes. Sorry, but no. These macros are not standard at all, and without explicit using of some kind of lock, there no portable way to handle atomicity in pthreads afaik. -- Matthieu Herrb ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
Tiago Vignatti wrote: > http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html > > Does we have this kind of thing in C libraries? It would be useful. If I got it right, that's posix + pthreads functionality specialized for an amtel platform. So in general, yes. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
Simon Thum wrote: > > newtail = (oldtail + 1) % QUEUE_SIZE; > > miEventQueue.tail = newtail; > > > > becoming > > > > miEventQueue.tail++; > > miEventQueue.tail |= QUEUE_SIZE - 1; Er, shouldn't this be "&=" ? > I don't think a compiler should be doing this to a non-local store. It > could probably be considered a bug. C doesn't really have a memory model > but few rules likely to forbid this. I didn't check, but I'd be highly > surprised by this being legal. Unless an lvalue is declared "volatile", the compiler is free to generate code which modifies it as and when it sees fit, so long as it has the correct value "in the end". In the meantime, it can freely store arbitrary data there if it wishes. This is why the "volatile" keyword was added to the ANSI C standard: so that the compiler could optimise memory access as it saw fit, while providing an opt-out for the cases where "intermediate" values are significant (signals, interrupts, memory-mapped I/O, threads, etc). -- Glynn Clements <[EMAIL PROTECTED]> ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
Simon Thum <[EMAIL PROTECTED]> writes: >> newtail = (oldtail + 1) % QUEUE_SIZE; >> miEventQueue.tail = newtail; >> >> becoming >> >> miEventQueue.tail++; >> miEventQueue.tail |= QUEUE_SIZE - 1; > I don't think a compiler should be doing this to a non-local store. It > could probably be considered a bug. C doesn't really have a memory model > but few rules likely to forbid this. I didn't check, but I'd be highly > surprised by this being legal. Do you have a case where it happens? On the contrary, a c compiler is allowed to assume that the code is running single-threaded. Since the two code snippets above will yield the exact same result (under the obvious assumptions), it is a valid transformation for the compiler. Posix only guarantees correct behaviour if you use a mutex, I believe. There are other constructs that are likely to behave as partial memory barriers, but they aren't guaranteed to work. And note that as soon as you have more than one physical core, you may need to force cache coherence as well. Of course, there is quite a bit of code written by now that really depends on a non-optimizing compiler for its correctness in the face of multiple threads. And it is reasonable to expect that many compilers try to avoid breaking this kind of code. But there are no guarantees. For some discussion on these problems (and some horrible examples), see "Threads cannot be implemented as a library": http://www.hpl.hp.com/techreports/2004/HPL-2004-209.pdf eirik ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
Simon Thum escreveu: >> newtail = (oldtail + 1) % QUEUE_SIZE; >> miEventQueue.tail = newtail; >> >> becoming >> >> miEventQueue.tail++; >> miEventQueue.tail |= QUEUE_SIZE - 1; > I don't think a compiler should be doing this to a non-local store. It > could probably be considered a bug. C doesn't really have a memory model > but few rules likely to forbid this. I didn't check, but I'd be highly > surprised by this being legal. Do you have a case where it happens? http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html Does we have this kind of thing in C libraries? It would be useful. Cheers, -- Tiago Vignatti C3SL - Centro de Computação Científica e Software Livre www.c3sl.ufpr.br ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
> newtail = (oldtail + 1) % QUEUE_SIZE; > miEventQueue.tail = newtail; > > becoming > > miEventQueue.tail++; > miEventQueue.tail |= QUEUE_SIZE - 1; I don't think a compiler should be doing this to a non-local store. It could probably be considered a bug. C doesn't really have a memory model but few rules likely to forbid this. I didn't check, but I'd be highly surprised by this being legal. Do you have a case where it happens? Cheers, Simon ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
On Nov 5, 2008, at 11:37, George Peter Staplin wrote:
On Nov 5, 2008, at 10:35 AM, Jeremy Huddleston wrote:
I see two possibilities for handling this:
1) Lock inside mieqProcessInputEvents
I think the pthread locking is actually safe in xquartz. I can't
find any places calling mieqEnqueue from a signal handler, so that
comment I made earlier about signals and deadlocks probably doesn't
apply to XQuartz.
2) Disable this optimization when we have multiple threads playing
with mieqEventQueue and just push a second MotionNotify event.
Hmm, I don't fully understand that. Can you explain more?
1) Lock inside mieqProcessInputEvents (this is based off of xorg-
server-1.4-apple... but the concept applies to master after Peter
pushes the changes discussed over the past couple days)
diff --git a/mi/mieq.c b/mi/mieq.c
index e93d24f..a634bf8 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -235,6 +236,10 @@ mieqProcessInputEvents(void)
int x = 0, y = 0;
DeviceIntPtr dev = NULL;
+#ifdef INPUT_THREAD
+pthread_mutex_lock(&miEventQueueMutex);
+#endif
+
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
SaveScreens (SCREEN_SAVER_OFF, ScreenSaverReset);
@@ -249,6 +254,10 @@ mieqProcessInputEvents(void)
memcpy(&e, &miEventQueue.events[miEventQueue.head],
sizeof(EventRec));
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(&miEventQueueMutex);
+#endif
+
if (miEventQueue.handlers[e.event[0].u.u.type]) {
/* If someone's registered a custom event handler, let
them
* steal it. */
@@ -283,5 +292,14 @@ mieqProcessInputEvents(void)
dev->public.processInputProc(e.event, dev, e.nevents);
}
+
+#ifdef INPUT_THREAD
+pthread_mutex_lock(&miEventQueueMutex);
+#endif
}
+
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(&miEventQueueMutex);
+#endif
+
}
2) Avoid the "replace the last motion-event with this one"
optimization while using an input thread. I think this is ugly, but
I'm mentioning it for completeness.
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..25136b2 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -122,7 +122,8 @@ mieqResizeEvents(int min_size)
void
mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
{
-unsigned int oldtail = miEventQueue.tail, newtail;
+unsigned int oldtail = miEventQueue.tail;
+unsigned int newtail = oldtail;
EventListPtr evt;
intisMotion = 0;
intevlen;
@@ -164,11 +165,16 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
return;
}
+#ifndef INPUT_THREAD
if (isMotion && isMotion == miEventQueue.lastMotion &&
oldtail != miEventQueue.head) {
oldtail = (oldtail - 1) % QUEUE_SIZE;
+
+ miEventQueue.tail = oldtail;
}
-else {
+else
+#endif
+{
static int stuck = 0;
newtail = (oldtail + 1) % QUEUE_SIZE;
/* Toss events which come in late. Usually this means your server's
@@ -184,7 +190,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
return;
}
stuck = 0;
- miEventQueue.tail = newtail;
}
evlen = sizeof(xEvent);
@@ -218,6 +223,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
miEventQueue.events[oldtail].pDev = pDev;
miEventQueue.lastMotion = isMotion;
+miEventQueue.tail = newtail;
}
void
smime.p7s
Description: S/MIME cryptographic signature
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: xquartz dereferencing a NULL pointer (patch 2)
On Nov 5, 2008, at 07:34, George Peter Staplin wrote:
On Nov 5, 2008, at 12:33 AM, Jeremy Huddleston wrote:
The locks inside mieqProcessInputEvents() should do nothing.
mieqProcessInputEvents is the only thread that modifies
miEventQueue.head, and we don't care if miEventQueue.tail changes
out from under us so long as miEventQueue.tail is changed AFTER the
data has been pushed to it (which might actually be the problem).
Your patch seems to fix the problem too. Are we relying on certain
operations being atomic?
That's a good point. Let's take a look at this in a bit more detail,
so hopefully I can convince you that we don't actually care about the
atomicity of this opperation.
mieqProcessInputEvents does a check to see if head != tail in order to
determine if there are more events to process. If "tail = newtail"
and "head != tail" are atomic, this should work fine. I argue that we
don't care about "head != tail" itself being atomic, but we do care
that the value of tail copied into register be valid. We don't care
if it's "old" or "new". If it's "old", then the last event won't be
processed until the next time mieqProcessInputEvents is called (not
really a problem). If it's "new" then we know the event is fully in
the queue and ready to be processed. So miEventQueue.tail needs to be
valid. Without optimization "miEventQueue.tail = newtail" should be a
single store-word or copy-word type instruction. With optimization,
that instruction would possibly be ignored (for the case when
(isMotion && isMotion == miEventQueue.lastMotion && oldtail !=
miEventQueue.head) [see below as this also has a different problem I
just realized]) or would be atomic. I guess we run the risk that it
is optimized as:
newtail = (oldtail + 1) % QUEUE_SIZE;
miEventQueue.tail = newtail;
becoming
miEventQueue.tail++;
miEventQueue.tail |= QUEUE_SIZE - 1;
with the intermediate value actually stored in memory for some
reason... in that case, the != would still succeed and
mieqProcessInputEvents would behave correctly EXCEPT if the new
miEventQueue.tail was now QUEUE_SIZE in which case it would process
events forever until the other thread got context back to finish the
mod/store...
You're starting to convince me a little that we need a lock with this
argument, but it really relies on a compiler being utterly stupid...
this next case, however, is more persuasive...
So for the case when (isMotion && isMotion == miEventQueue.lastMotion
&& oldtail != miEventQueue.head), we essentially modify the last item
already in the queue... but this is an event that the server thread
could already be processing (I think this code might be what caused
that "must be reentrant with ProcessInputEvents" comment that I
dismissed earlier). This is essentially an optimization (combining a
delta-x1 and delta-x2 mouse motion into a single delta-(x1 + x2) mouse
motion). At first glance, we could do something like this:
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..ac2eedb 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -167,6 +168,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
if (isMotion && isMotion == miEventQueue.lastMotion &&
oldtail != miEventQueue.head) {
oldtail = (oldtail - 1) % QUEUE_SIZE;
+
+ miEventQueue.tail = oldtail;
}
else {
static int stuck = 0;
but that only prevents the mieqProcessInputEvents thread from
processing the event if it hasn't already done the check. Since we
don't have a mutex, we need to assume a context switch could happen
while the server thread is already in the middle of processing this
event... I see two possibilities for handling this:
1) Lock inside mieqProcessInputEvents
2) Disable this optimization when we have multiple threads playing
with mieqEventQueue and just push a second MotionNotify event.
Thoughts?
--George
Try this:
diff --git a/mi/mieq.c b/mi/mieq.c
index e93d24f..7902608 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -113,7 +113,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
#ifdef XQUARTZ
pthread_mutex_lock(&miEventQueueMutex);
#endif
-unsigned int oldtail = miEventQueue.tail, newtail;
+unsigned int oldtail = miEventQueue.tail
+unsigned int newtail = oldtail;
intisMotion = 0;
deviceValuator *v = (deviceValuator *) e;
EventPtr laste = &miEventQueue.events[(oldtail -
1) %
@@ -173,7 +174,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
#endif
return;
}
- miEventQueue.tail = newtail;
}
memcpy(&(miEventQueue.events[oldtail].event[0]), e,
sizeof(xEvent));
@@ -192,6 +192,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
miEventQueue.events[oldtail].pDev = pDev;
miEventQueue.lastMotion = isMotion;
+ miEventQueue.tail = newtail;
#ifdef XQUARTZ
pthread_mutex_unlock(&miEventQueueMutex);
#endif
--Jeremy
On
