Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

UIEdgeInsets injection issue on arm64 devices #287

Closed
gillesguilleminpiksel opened this issue Dec 4, 2014 · 7 comments
Closed

UIEdgeInsets injection issue on arm64 devices #287

gillesguilleminpiksel opened this issue Dec 4, 2014 · 7 comments

Comments

@gillesguilleminpiksel
Copy link

Hi there,

A quite unexpected issue we started to stumble on a few days ago: we've been working on a quite massive project for several months already but until a few days ago we hadn't upgraded it to 64 bits.

After the switch, we started seeing some strange UI behaviours on our 64-bit devices only. After (quite) some digging, we've realised that when injecting [NSValue valueWithUIEdgeInsets:someUIEdgeInsets], the value actually injected was completely off the chart in some circumstances.

More precisely, if we tried to initialise a class with more than 3 parameters for the constructor, and that the 4th parameter was a UIEdgeInsets, then the injected value was wrong.

I've created a small project to illustrate this: https://github.com/gillesguillemin/UIEdgeInsetsTyphoonTestApp

When running this we get the following log results:

Test1: initWithSomeCGSize:someCGFloat:anotherCGSize:anotherCGFloat:edgeInsets:
someCGSize = {300, 79}
someCGFloat = 20.000000
anotherCGSize =  {0, 0}
anotherCGFloat = 20.000000
edgeInsets = {30, 40, 30, 40}

Test2: initWithSomeCGSize:someCGFloat:anotherCGSize:edgeInsets:anotherCGFloat:
someCGSize = {300, 79}
someCGFloat = 20.000000
anotherCGSize =  {0, 0}
edgeInsets = {20, 3.0836664839514239e-314, 2.4235229083898867e-314, 3.0837547596484977e-314}
anotherCGFloat = 0.000000

Test3: initWithSomeCGSize:someCGFloat:edgeInsets:anotherCGSize:anotherCGFloat:
someCGSize = {300, 79}
someCGFloat = 20.000000
edgeInsets = {30, 40, 30, 40}
anotherCGSize =  {0, 0}
anotherCGFloat = 20.000000

We tested the following 3 scenarios:

  • Test1: constructor has 5 parameters, UIEdgeInsets are in 3rd position: everything is fine
  • Test2: constructor has 5 parameters, UIEdgeInsets are in 4th position: edgeInsets' value is completely messed up, and so is the value for the 5th parameter
  • Test3: constructor has 5 parameters, UIEdgeInsets are in 5th position: everything is fine again

So to sum it up: put a UIEdgeInsets (or something else? and if yes what?) as the 4th, 6th, etc value of an injected constructor on arm64 and all bets are off...

I know it may sound like an edge case but I'm a tad afraid to see the same behaviour reproduced for other injected values in the most unexpected places...

So, it looks like some memory addressing issue during the invocation's creation but what are your thoughts on this guys? (and what can be done to address that?)

@jasperblues
Copy link
Member

@gillesguilleminpiksel Thanks for reporting. Given the high number of users deploying 64bit Typhoon apps, I would say this is an (unfortunate) edge case. I've assigned this issue absolute top priority.

The description of this issue reminds me of a case early in the release of Typhoon 2.0, where 64bit devices had trouble with runtime arguments.

It related to the way va_args are stored directly to registers on 64bit CPUs instead of copying to the stack, so we had to convert to use NSInvocation instead. I'm not sure why this would be happening for basic initializer injection though. NSInvocation is already being used here.

Suggested workarounds:

If a release with 64bit is urgently required, here's two suggested workarounds:

  • Wrap the UIEdgeInsets inside an object, and unwrap in the initializer, ie _insets = [MyEdgeInsets asInsets]
  • Use property injection for the insets.

@alexgarbarev
Copy link
Contributor

What version of typhoon do you use? Latest?

I'll try to run your test on my arm64 device with latest typhoon version.

@gillesguilleminpiksel
Copy link
Author

@alexgarbarev Sorry, that was implicit. We're using the latest version (2.3.1) of course.

@jasperblues We've already taken the object wrapper approach which solves the issue at hand for us but doesn't quite clear the unease about this reoccurring in the most unexpected ways elsewhere in our app!

@jasperblues
Copy link
Member

@gillesguilleminpiksel Thanks for confirming the version, that should've have been the first question I asked. While I assumed you meant the latest version, we do sometimes get requests for support from folks on earlier versions. (Usually the best approach is to upgrade).

While I can understand that you're concerned after having encountered an error:

  • Running Typhoon's test suite on 64bit device gives 100% pass. Coverage is quite comprehensive.
  • Many apps have been deployed to the store supporting 64 bit.

I would like to allocate a 64-bit device and have our build server do on-device testing for each commit. Though as a non-profit & volunteer-based project this is a fairly expensive endeavor.

In the meantime this issue has been assigned top priority.

@alexgarbarev
Copy link
Contributor

@gillesguilleminpiksel That was great catch from you!

Sorry for long response, haven't so much time to dig this problem.

Spend a lot of time researching.. Looks like it's Apple's bug in NSInvocation implementation on armv64

  1. I've checked NSInvocation created by Typhoon - it's totally correct.

  2. This bug happens only when using structure with four doubles (i.e. floating point, 8 bytes). Tried structures with three and five doubles - works fine in that case..

  3. This bug happens only when using that structure after 5 double arguments - i.e. not 4 and not 6, only 5 double arguments (structure argument is sixth float) - it’s not important if first doubles wrapped into structure or not.

I reported that bug to Apple.. Waiting for response..

Here is demo project with that issue:
https://github.com/alexgarbarev/NSInvocationBug

@alexgarbarev
Copy link
Contributor

Apple said:

We believe this issue has been addressed in the latest iOS 9 beta.

Please test with this release. If you still have issues, please provide any relevant logs or information that could help us investigate.

@etolstoy
Copy link
Contributor

etolstoy commented Feb 5, 2017

Closing due to inactivity. If you still experience this issue, feel free to reopen it.

@etolstoy etolstoy closed this as completed Feb 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants