Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix symbolication in Yosemite and Xcode6 #597

Merged
merged 1 commit into from
Apr 30, 2015
Merged

Fix symbolication in Yosemite and Xcode6 #597

merged 1 commit into from
Apr 30, 2015

Conversation

jerrymarino
Copy link
Contributor

While this fixes the dead lock bug Yosemite, I still think this feature could be improved:

"atos in 10.0 with a process id no longer works for the simulator so use the
executable path instead.

This patch introduces KWBackgroundTask to remove use of the dangerous
-[NSTask waitUntilExit]; and encapsulate use of NSTask.

  • breakout callsite creation for symbolicated addresses from KWExample by adding
    a factory for KWCallSite."

@jerrymarino jerrymarino changed the title (RFC) Fix symbolication in Yosemite and Xcode6 Fix symbolication in Yosemite and Xcode6 Feb 19, 2015
atos in 10.0 with a process id no longer works for the simulator so use the
executable path instead.

This patch introduces KWBackgroundTask to remove use of the dangerous
`-[NSTask waitUntilExit];` and encapsulate use of NSTask.

+ breakout callsite creation for symbolicated addresses from KWExample by adding
a factory for KWCallSite.
@ecaselles
Copy link
Member

👍 I would love to see this fix merged in, since it is a pitty not being able to run the tests from Xcode 🙏

#import <objc/runtime.h>
#import <libunwind.h>
#import <pthread.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import doesn't appear to be used anywhere in this file.

@sharplet
Copy link
Contributor

Thanks @jerrymarino, I've tested this out on my machine and it appears to work just fine! 👍

I'm a little bit nervous about this code:

  • Will this still work fine on other versions of OS X?
  • Anything involving dyld.h is currently, unfortunately, beyond my understanding.

So to the end of making this as clear as possible for future maintainers, what do you think about these suggestions?

  • Move KWBackgroundTask into its own source file. This should hopefully ensure that all of the async code involving CFRunLoop is fully encapsulated.
  • Document the actual symbolication stuff in some way. I'm thinking developer-focused documentation here, whether it's comments in the implementation providing a bit more context, or maybe a separate file, or something. I think even just having some more method/function doc comments could be really valuable here, and even potentially splitting out some smaller methods/functions if it makes sense.
  • Or any other ideas you have! 😁

Ultimately I'm happy to merge this pretty soon, but if there's anything we can do to improve the overall clarity of this beforehand I think that would be a win for everybody!

@sharplet sharplet merged commit 1b92d7c into kiwi-bdd:master Apr 30, 2015
@sharplet
Copy link
Contributor

Thanks again @jerrymarino! I decided to go ahead and extract KWBackgroundTask into its own file. Great to have the tests running again! 👏

@jerrymarino
Copy link
Contributor Author

Hey @sharplet glad I could help out on this one.

I tested it out on both 10.9 and 10.10 and it worked great. I believe atos supported the updated arguments in 10.8 too. I hope atos functionality is stabilized in future releases. If it stops working again the error handling we've got now should make it obvious 🍻

It'd be helpful to have developer documentation for this. Inline comments could be sufficient and potentially add a page/section in the wiki for higher level design documentation. Do you think it is appropriate to put design docs in the wiki some where?

I'll can submit a follow up soon 😄

@sharplet
Copy link
Contributor

sharplet commented May 1, 2015

@jerrymarino I think I'd prefer to keep to user-facing docs in the wiki. So something in the source code should be most appropriate for developer documentation! I'd be happy for you to create a new Documentation/ directory for the higher-level stuff.

@sharplet sharplet mentioned this pull request May 1, 2015
4 tasks
@lyahdav
Copy link

lyahdav commented Aug 12, 2015

I just ran into this issue on Kiwi 2.3.1 (since it's the latest Kiwi version on cocoapods). Will there be an official Kiwi release with this fix anytime soon so we don't have to use master?

@sharplet
Copy link
Contributor

@ecaselles
Copy link
Member

@sharplet 👏🎉🎊🎆✨💥🎋🎏

@lyahdav
Copy link

lyahdav commented Sep 30, 2015

I'm still getting this deadlock. OS X 10.10.5, Xcode 7.0, iOS 9.0, Kiwi 2.4.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants