| Message ID | 20181128010817.6191-1-programmingkidx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | (no subject) | expand |
On Wed, 28 Nov 2018 at 01:12, John Arbuckle <programmingkidx@gmail.com> wrote: > > From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001 > From: John Arbuckle <programmingkidx@gmail.com> > Date: Tue, 27 Nov 2018 20:01:20 -0500 > Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14 Something seems to have got the formatting of this patch email wrong -- it's got all this in the body and the actual Subject line of the email is blank. > Mac OS 10.14 only wants UI code to be called from the main thread. The > cocoa_refresh() function is called on another thread and this causes a > crash to take place. To fix this problem the cocoa_refresh() code is > called from the main thread only. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > --- > ui/cocoa.m | 59 ++++++++++++++++++++++++++++++++++------------------------- > 1 file changed, 34 insertions(+), 25 deletions(-) I get a compile warning with this patch: /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method '-cocoa_refresh' not found (return type defaults to 'id') [-Wobjc-method-access] [[NSApp delegate] cocoa_refresh]; ^~~~~~~~~~~~~ To be honest, I'm still confused about what is causing the problems on Mojave. The refresh method should be being called on the main thread even with the code on master -- it's just that that is the iothread and it's running the event pumping code in the refresh callback rather than a standard OSX event loop. I'm hoping that the backtrace with symbols of the Mojave assertion failure will help there, since I can't currently see where refresh gets called from some non-main thread. I also think that this patch doesn't address the problems of locking that I mention on the discussion thread for Berkus' patch. It also doesn't handle any of the other callbacks from QEMU to the cocoa UI -- surely we need to handle all of them if there is a problem here? (I have some prototype patches which I've been working on which address the locking problem and also make all the QEMU callbacks run their work on the main thread. I may be able get those into shape to post those next week.) thanks -- PMM
> On Nov 28, 2018, at 2:39 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 28 Nov 2018 at 01:12, John Arbuckle <programmingkidx@gmail.com> wrote: >> >> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001 >> From: John Arbuckle <programmingkidx@gmail.com> >> Date: Tue, 27 Nov 2018 20:01:20 -0500 >> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14 > > Something seems to have got the formatting of this patch email > wrong -- it's got all this in the body and the actual Subject > line of the email is blank. I don't know what happened. > >> Mac OS 10.14 only wants UI code to be called from the main thread. The >> cocoa_refresh() function is called on another thread and this causes a >> crash to take place. To fix this problem the cocoa_refresh() code is >> called from the main thread only. >> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >> --- >> ui/cocoa.m | 59 ++++++++++++++++++++++++++++++++++------------------------- >> 1 file changed, 34 insertions(+), 25 deletions(-) > > I get a compile warning with this patch: > /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method > '-cocoa_refresh' not found (return type defaults to 'id') > [-Wobjc-method-access] > [[NSApp delegate] cocoa_refresh]; > ^~~~~~~~~~~~~ This will fix the problem: static void cocoa_refresh(DisplayChangeListener *dcl) { QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp delegate]; [controller cocoa_refresh]; } > To be honest, I'm still confused about what is causing the > problems on Mojave. The refresh method should be being called > on the main thread even with the code on master -- it's just > that that is the iothread and it's running the event pumping > code in the refresh callback rather than a standard OSX > event loop. I'm hoping that the backtrace with symbols of > the Mojave assertion failure will help there, since I > can't currently see where refresh gets called from some > non-main thread. This might be a Mojave issue and not a QEMU issue. I'm on Mac OS 10.12 so I can't confirm anything. > I also think that this patch doesn't address the problems > of locking that I mention on the discussion thread for > Berkus' patch. It also doesn't handle any of the other > callbacks from QEMU to the cocoa UI -- surely we need to > handle all of them if there is a problem here? To answer this question I would have to know how my patch does on Mac OS 10.14. Does it stop the crashing issue? If this patch does fix that problem then I think sticking to a simple solution may be the answer. The use of locks may not be needed. > (I have some prototype patches which I've been working > on which address the locking problem and also make all > the QEMU callbacks run their work on the main thread. > I may be able get those into shape to post those next week.) Please CC me when do release it. I will test it on Mac OS 10.12 and Mac OS 10.6.
I suspect the main problem is the blocking call to qemu_main from the UI thread in the app delegate didFinishLoadingWithOptions if i’m not mistaken and everything else grows from there. Going to build and run it now, since I woke up in the middle of the night anyway for reasons unexplainable) On Thu, 29 Nov 2018 at 02:21, Programmingkid <programmingkidx@gmail.com> wrote: > > > On Nov 28, 2018, at 2:39 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: > > > > On Wed, 28 Nov 2018 at 01:12, John Arbuckle <programmingkidx@gmail.com> > wrote: > >> > >> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001 > >> From: John Arbuckle <programmingkidx@gmail.com> > >> Date: Tue, 27 Nov 2018 20:01:20 -0500 > >> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS > 10.14 > > > > Something seems to have got the formatting of this patch email > > wrong -- it's got all this in the body and the actual Subject > > line of the email is blank. > > I don't know what happened. > > > > >> Mac OS 10.14 only wants UI code to be called from the main thread. The > >> cocoa_refresh() function is called on another thread and this causes a > >> crash to take place. To fix this problem the cocoa_refresh() code is > >> called from the main thread only. > >> > >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > >> --- > >> ui/cocoa.m | 59 > ++++++++++++++++++++++++++++++++++------------------------- > >> 1 file changed, 34 insertions(+), 25 deletions(-) > > > > I get a compile warning with this patch: > > /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method > > '-cocoa_refresh' not found (return type defaults to 'id') > > [-Wobjc-method-access] > > [[NSApp delegate] cocoa_refresh]; > > ^~~~~~~~~~~~~ > > This will fix the problem: > > static void cocoa_refresh(DisplayChangeListener *dcl) > { > QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp > delegate]; > [controller cocoa_refresh]; > } > > > To be honest, I'm still confused about what is causing the > > problems on Mojave. The refresh method should be being called > > on the main thread even with the code on master -- it's just > > that that is the iothread and it's running the event pumping > > code in the refresh callback rather than a standard OSX > > event loop. I'm hoping that the backtrace with symbols of > > the Mojave assertion failure will help there, since I > > can't currently see where refresh gets called from some > > non-main thread. > > This might be a Mojave issue and not a QEMU issue. > I'm on Mac OS 10.12 so I can't confirm anything. > > > I also think that this patch doesn't address the problems > > of locking that I mention on the discussion thread for > > Berkus' patch. It also doesn't handle any of the other > > callbacks from QEMU to the cocoa UI -- surely we need to > > handle all of them if there is a problem here? > > To answer this question I would have to know how my patch > does on Mac OS 10.14. Does it stop the crashing issue? If > this patch does fix that problem then I think sticking to > a simple solution may be the answer. The use of locks may > not be needed. > > > (I have some prototype patches which I've been working > > on which address the locking problem and also make all > > the QEMU callbacks run their work on the main thread. > > I may be able get those into shape to post those next week.) > > Please CC me when do release it. I will test it on Mac OS 10.12 > and Mac OS 10.6.
On Thu, 29 Nov 2018 at 02:11, berkus infinitus <berkus@gmail.com> wrote: > > I suspect the main problem is the blocking call to qemu_main > from the UI thread in the app delegate didFinishLoadingWithOptions > if i’m not mistaken and everything else grows from there. Yes; if there's no way that Mojave will allow us to run qemu_main directly on the main thread, then we have to create a 2nd thread to run qemu_main on (which then becomes what QEMU thinks of as the "main loop thread"), and then we run into the need to make all the UI calls be forwarded from the main loop thread to the main thread, and to get QEMU locks when making calls into qemu from the main thread. I think the code we have in git currently will already do all the UI calls on the main thread -- it just does it by doing a blocking call into qemu_main which later does event processing itself. (It's a shame OSX doesn't document what you need to do to write code that way, it's a fairly common paradigm for other GUIs.) For High Sierra there is apparently a "main thread checker" utility: https://developer.apple.com/documentation/code_diagnostics/main_thread_checker so running DYLD_INSERT_LIBRARIES=/Applications/Xcode.app/Contents/Developer/usr/lib/libMainThreadChecker.dylib qemu-system-x86_64 args... will let us check for violations of the "do things on main thread" principle even without Mohave. For me with current QEMU it doesn't report any issues. thanks -- PMM
diff --git a/ui/cocoa.m b/ui/cocoa.m index ecf12bfc2e..17c168d08f 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -972,6 +972,8 @@ - (void)openDocumentation:(NSString *)filename; - (IBAction) do_about_menu_item: (id) sender; - (void)make_about_window; - (void)adjustSpeed:(id)sender; +- (void) cocoa_refresh; +- (void) cocoa_refresh_internal: (id) dummy; @end @implementation QemuCocoaAppController @@ -1406,6 +1408,37 @@ - (void)adjustSpeed:(id)sender COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), '%'); } +- (void) cocoa_refresh +{ + [self performSelectorOnMainThread: @selector(cocoa_refresh_internal:) withObject: nil waitUntilDone: YES]; +} + +- (void) cocoa_refresh_internal: (id) dummy +{ + COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n"); + graphic_hw_update(NULL); + + if (qemu_input_is_absolute()) { + if (![cocoaView isAbsoluteEnabled]) { + if ([cocoaView isMouseGrabbed]) { + [cocoaView ungrabMouse]; + } + } + [cocoaView setAbsoluteEnabled:YES]; + } + + NSDate *distantPast; + NSEvent *event; + distantPast = [NSDate distantPast]; + do { + event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast + inMode: NSDefaultRunLoopMode dequeue:YES]; + if (event != nil) { + [cocoaView handleEvent:event]; + } + } while(event != nil); +} + @end @@ -1579,31 +1612,7 @@ static void cocoa_switch(DisplayChangeListener *dcl, static void cocoa_refresh(DisplayChangeListener *dcl) { - NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; - - COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n"); - graphic_hw_update(NULL); - - if (qemu_input_is_absolute()) { - if (![cocoaView isAbsoluteEnabled]) { - if ([cocoaView isMouseGrabbed]) { - [cocoaView ungrabMouse]; - } - } - [cocoaView setAbsoluteEnabled:YES]; - } - - NSDate *distantPast; - NSEvent *event; - distantPast = [NSDate distantPast]; - do { - event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast - inMode: NSDefaultRunLoopMode dequeue:YES]; - if (event != nil) { - [cocoaView handleEvent:event]; - } - } while(event != nil); - [pool release]; + [[NSApp delegate] cocoa_refresh]; } static void cocoa_cleanup(void)