Thanks, Peter and Keith. I'll send out a patch to fix this up. Thanks, - Andy
On Tue, Jun 12, 2012 at 05:48:30PM -0700, Peter Hutterer wrote: > On Tue, Jun 12, 2012 at 05:11:49PM -0700, Andy Ritger wrote: > > Is mi/mieq.c:mieqProcessInputEvents() expected to be reentrant? Or should > > mieqProcessInputEvents() never be called in that situation? > > > > More specifically, is there a reason that the 'event' local variable in > > mieqProcessInputEvents() is declared static? > > > > static InternalEvent event; > > I don't think so, this doesn't look right. looking at the log this was added > in xorg-server-1.5.99.1-136-ga939368 to avoid calloc but you're certainly > right, it breaks reentrancy. Following the history of that, it's not even > needed anymore, this came from a time when we had to callloc a list of > devices - now we can have just have one InternalEvent on the stack. > > see more below though. > > > Consider the following sequence: > > > > (a) User hits ctrl-alt-+ to do an old-school mode switch, and the keyboard > > event is put on the event queue. > > > > (b) mieqProcessInputEvents() pops the keyboard event from the event queue, > > storing the event data in a static local variable, and then passing a > > pointer to that variable into mieqProcessDeviceEvent(): > > > > mieqProcessInputEvents(void) > > { > > [...] > > static InternalEvent event; > > [...] > > while (miEventQueue.head != miEventQueue.tail) { > > e = &miEventQueue.events[miEventQueue.head]; > > > > event = *e->events; > > [...] > > mieqProcessDeviceEvent(dev, &event, screen); > > [...] > > } > > > > (c) The event pointer gets passed through several functions until we > > hit XkbHandleActions(): > > > > #34 0x0000000000585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370, > > event=0x85cf00) at xkbActions.c:1234 > > #35 0x00000000005865d8 in XkbProcessKeyboardEvent (event=0x85cf00, > > keybd=0x1176370) at xkbPrKeyEv.c:146 > > #36 0x000000000057ad28 in AccessXFilterPressEvent (event=0x85cf00, > > keybd=0x1176370) at xkbAccessX.c:569 > > #37 0x000000000058678e in ProcessKeyboardEvent (ev=0x85cf00, > > keybd=0x1176370) at xkbPrKeyEv.c:181 > > #38 0x00000000005aec39 in mieqProcessDeviceEvent (dev=0x1176370, > > event=0x85cf00, screen=0xaa49e0) at mieq.c:557 > > #39 0x00000000005aee75 in mieqProcessInputEvents () at mieq.c:624 > > #40 0x000000000048adb9 in ProcessInputEvents () at xf86Events.c:164 > > #41 0x0000000000430835 in Dispatch () at dispatch.c:353 > > #42 0x0000000000421a45 in main (argc=1, argv=0x7fff049880d8, > > envp=0x7fff049880e8) at main.c:288 > > > > (d) XkbHandleActions() will look at the event type to decide it is a > > keyEvent type, query the key action type, perform some operation based > > on the key action type, then pass the event into FixKeyState(): > > > > void > > XkbHandleActions(DeviceIntPtr dev, DeviceIntPtr kbd, DeviceEvent *event) > > { > > [...] > > keyEvent = ((event->type == ET_KeyPress) || (event->type == > > ET_KeyRelease)); > > [...] > > act = XkbGetKeyAction(xkbi, &xkbi->state, key); > > [...] > > switch (act.type) { > > [...] > > case XkbSA_XFree86Private: > > filter = _XkbNextFreeFilter(xkbi); > > sendEvent = _XkbFilterXF86Private(xkbi, filter, key, &act); > > break; > > } > > } > > [...] > > if (keyEvent) { > > FixKeyState(event, dev); > > } > > [...] > > } > > > > (e) FixKeyState() does this: > > > > if (event->type == ET_KeyPress) > > set_key_down(keybd, key, KEY_PROCESSED); > > else if (event->type == ET_KeyRelease) > > set_key_up(keybd, key, KEY_PROCESSED); > > else > > FatalError("Impossible keyboard event"); > > > > (that will be important later...) > > > > (f) Back in XkbHandleActions(), we called _XkbFilterXF86Private which > > passes the key event to the XFree86 layer, which decodes the key event > > as > > "+vmode", and we call through the pScrnInfo->SwitchMode wrap chain: > > > > #6 0x00000000004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0, > > flags=0) at xf86Cursor.c:253 > > #7 0x0000000000496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0) > > at xf86cmap.c:492 > > #8 0x0000000000485fb1 in xf86SwitchMode (pScreen=0x1c40c00, > > mode=0x1c54eb0) at xf86Cursor.c:232 > > #9 0x00000000004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at > > xf86Cursor.c:332 > > #10 0x000000000048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, > > arg=0x0) at xf86Events.c:191 > > #11 0x00000000004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V', > > act=0x7ffff8e60770) at xkbPrivate.c:32 > > #12 0x0000000000584746 in _XkbFilterXF86Private (xkbi=0x22ad470, > > filter=0x2265400, keycode=86, > > pAction=0x7ffff8e60770) at xkbActions.c:959 > > #13 0x0000000000585596 in XkbHandleActions (dev=0x22ac580, kbd=0x22ac580, > > event=0x85cf00) > > at xkbActions.c:1234 > > > > (g) Someone in the pScrnInfo->SwitchMode wrap chain calls RRTellChanged() > > (in my case, it is the NVIDIA X driver, but I think many > > drivers could be exposed to this through xf86SetSingleMode() -> > > xf86RandR12TellChanged()) which calls UpdateCurrentTime(). > > > > (h) UpdateCurrentTime() conditionally calls ProcessInputEvents(): > > > > void > > UpdateCurrentTime(void) > > { > > [...] > > if (*checkForInput[0] != *checkForInput[1]) > > ProcessInputEvents(); > > [...] > > } > > > > --- > > > > void > > ProcessInputEvents(void) > > { > > int x, y; > > > > mieqProcessInputEvents(); > > > > /* FIXME: This is a problem if we have multiple pointers */ > > miPointerGetPosition(inputInfo.pointer, &x, &y); > > > > xf86SetViewport(xf86Info.currentScreen, x, y); > > } > > > > (i) checkForInput[0] and [1] point to miEventQueue.{head,tail}; and tail > > potentially got updated earlier in this sequence: > > > > 0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at > > mieq.c:322 > > 322 miEventQueue.tail = (oldtail + 1) % miEventQueue.nevents; > > #0 0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at > > mieq.c:322 > > #1 0x00000000005baae7 in miPointerMove (pDev=0x2309c00, pScreen=0x21dc7f0, > > x=1600, y=600) at mipointer.c:703 > > #2 0x00000000005b9f0c in miPointerWarpCursor (pDev=0x2309c00, > > pScreen=0x21dc7f0, x=1600, y=600) at mipointer.c:371 > > #3 0x00000000004875e1 in xf86WarpCursor (pDev=0x2309c00, > > pScreen=0x21dc7f0, x=1600, y=600) at xf86Cursor.c:476 > > #4 0x0000000000486f65 in xf86SwitchMode (pScreen=0x21dc7f0, > > mode=0x21fac80) at xf86Cursor.c:283 > > #5 0x000000000048712b in xf86ZoomViewport (pScreen=0x21dc7f0, zoom=1) at > > xf86Cursor.c:333 > > #6 0x000000000048bba2 in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, > > arg=0x0) at xf86Events.c:191 > > #7 0x00000000004ebed8 in XkbDDXPrivate (dev=0x26bbc00, key=86 'V', > > act=0x7fff8e43bc70) at xkbPrivate.c:32 > > #8 0x00000000005845ae in _XkbFilterXF86Private (xkbi=0x26bcaf0, > > filter=0x233b650, keycode=86, pAction=0x7fff8e43bc70) > > at xkbActions.c:958 > > #9 0x0000000000585119 in XkbHandleActions (dev=0x26bbc00, kbd=0x26bbc00, > > event=0x85c500) at xkbActions.c:1194 > > > > So, UpdateCurrentTime() may call ProcessInputEvents(), which will > > reenter > > mieqProcessInputEvents(). > > > > (j) mieqProcessInputEvents() will overwrite the contents of the static > > 'event' variable, such that when we finally get back up to > > XkbHandleActions() and it calls FixKeyState(), the contents of > > 'event' are no longer consistent with the value of keyEvent and we > > hit the FatalError() case. > > > > Making the 'event' variable in mieqProcessInputEvents() non-static > > avoids the FatalError() in FixKeyState(), but is it problematic to > > have the re-entered instance of mieqProcessInputEvents() process events > > from miEventQueue before the first instance of mieqProcessInputEvents() > > has completed processing of its current event? > > good analysis, thanks. afaict we need a mutex in > mieqProcessInputEvents(), it's certainly not designed to be reentrant and > it's pointless anyway - if you're already processing events, why would you > need to do so again? > > most of the calls to ProcessInputEvents() from the server are intended as > "process input now instead of going to sleep". > > Cheers, > Peter _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel