> I think we don't need to mark this 'volatile sig_atomic_t',
> but could use a simple 'bool', because both applicationDidFinishLaunching()
> and handleEvent() are called from the same thread (the OSX run loop
> thread).
Oh, I didn't notice that.
I replaced 'volatile sig_atomic_t' with 'static bool' because it only
requires a file scope, and confirmed the allow_events is referenced by
the same thread.
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
frame #0: 0x00000001004be802 qemu-system-x86_64`-[QemuCocoaView
handleEvent:](self=0x0000000111e46650, _cmd="handleEvent:",
event=0x0000000111ebdc30) at cocoa.m:733:8
730
731 - (bool) handleEvent:(NSEvent *)event
732 {
-> 733 if(!allow_events) {
734 /*
735 * Just let OSX have all events that arrive before
736 * applicationDidFinishLaunching.
(lldb) c
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
frame #0: 0x00000001004c01e4
qemu-system-x86_64`-[QemuCocoaAppController
applicationDidFinishLaunching:](self=0x0000000111e46540,
_cmd="applicationDidFinishLaunching:",
note=@"NSApplicationDidFinishLaunchingNotification") at
cocoa.m:1170:18
1167 - (void)applicationDidFinishLaunching: (NSNotification *) note
1168 {
1169 COCOA_DEBUG("QemuCocoaAppController:
applicationDidFinishLaunching\n");
-> 1170 allow_events = true;
1171 /* Tell cocoa_display_init to proceed */
1172 qemu_sem_post(&app_started_sem);
1173 }
I resent the patch v3. Thanks!
Hikaru Nishida
2019年10月15日(火) 2:16 Peter Maydell <peter.mayd...@linaro.org>:
>
> On Mon, 14 Oct 2019 at 15:16, <hikaru...@gmail.com> wrote:
> >
> > From: Hikaru Nishida <hikaru...@gmail.com>
> >
> > macOS API documentation says that before applicationDidFinishLaunching
> > is called, any events will not be processed. However, some events are
> > fired before it is called in macOS Catalina. This causes deadlock of
> > iothread_lock in handleEvent while it will be released after the
> > app_started_sem is posted.
> > This patch avoids processing events before the app_started_sem is
> > posted to prevent this deadlock.
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
> > Signed-off-by: Hikaru Nishida <hikaru...@gmail.com>
> > ---
> > ui/cocoa.m | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index f12e21df6e..bccd861d16 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -134,6 +134,7 @@
> >
> > static QemuSemaphore display_init_sem;
> > static QemuSemaphore app_started_sem;
> > +volatile sig_atomic_t allow_events;
>
> Sorry, I failed to spot this on version 1 of the patch...
> I think we don't need to mark this 'volatile sig_atomic_t',
> but could use a simple 'bool', because both applicationDidFinishLaunching()
> and handleEvent() are called from the same thread (the OSX run loop
> thread). Could you test that it still works with plain 'bool',
> please?
>
> (If we did need to handle multiple thread accesses we should
> probably prefer one of the QEMU atomic primitives described
> in docs/devel/atomics.txt, since 'volatile' isn't really sufficient.)
>
> thanks
> -- PMM
