Skip to content

Fix partial mock override implementation when returning structs #41

Merged
merged 1 commit into from Aug 8, 2013

2 participants

@carllindberg

Fix partial mock override implementation when returning structs. The current implementation uses a trick to get a handle to the objc_msgForward function (or whatever the runtime is using to forward normal messages) and installs that as the "implementation" for any stubbed methods; however depending on the struct and the architecture
it may be necessary to override with objc_msgForward_stret instead since that is how the compiler is going to set
up the frame for the call. When using the wrong one, the compiler will set up the frame differently, and
objc_msgForward will then assume its arguments are in places they are not, leading to crashes very quickly.

In particular, the frame in the objc_msgSend_stret situations typically has an extra argument on the stack
(the memory address for the returned structure memory), and the normal objc_msgFoward implementation
will take the value at that frame location and assume it is "self", and try to obtain the class for that to
start the forwarding process (create NSInvocation etc.). If that happens to be nil, you will see some error
logs like

*** NSForwarding: warning: object 0x679e0 of class 'nil' does not implement methodSignatureForSelector: -- trouble ahead

before crashing. If it is some other non-zero value, it will probably crash earlier.

Problem mentioned at http://stackoverflow.com/questions/16559191/returning-cgrect-from-mocked-method-crashes .

The solution is to determine which forwarder function needs to be used (I used class_getMethodImplementation_stret() to get the right one), and use that in the override instead.
Unfortunately, determining whether it is needed or not is very architecture dependent, and the rules
can get mind-numbingly complex particularly for the 64-bit architectures. I found a horrid backhanded way
where NSMethodSignature can sort of tell us, and went with that. You probably want to find a better way
if at all possible.

With this fix in, the test case mentioned in the stackoverflow article passes (test case added here).

Carl Lindberg Fix partial mock override implementation when returning structs; depe…
…nding on the struct and the architecture

it may be necessary to override with objc_msgForward_stret since that is how the compiler is going to set
up the frame for the call.  When using the wrong one, the compiler will set up the frame differently, and
objc_msgForward will then assume its arguments are in places they are not, leading to crashes very quickly.
Problem mentioned at http://stackoverflow.com/questions/16559191/returning-cgrect-from-mocked-method-crashes .
28578f6
@carllindberg

In looking, OCClassMockObject has a similar issue in its setupForwarderForClassMethodSelector method.

@erikdoe
Owner
erikdoe commented Jul 31, 2013

Thanks! This is super helpful. I had briefly stepped through the assembler code but couldn't understand why things were the way they were. It didn't occur to me that the wrong forward function was being used... Out of curiosity, where is the ABI description that we'd have to follow to support x86_64 properly?

By the way, I checked the GNUstep source code but that wasn't helpful. I'll also check whether NSMethodSignature has a non-public method that might help us. I'd be happy to call that (with a test guarding it). After all I can't see how anyone would need OCMock code in the App Store.

@erikdoe
Owner
erikdoe commented Jul 31, 2013

Ok, so, there is a method named _frameDescriptor, and its return type is ^{?=^{NSMethodFrameArgInfo}^{NSMethodFrameArgInfo}II}. Of course, NSMethodFrameArgInfo is not in the public headers and after searching for it on the web I'm anything but excited about the prospect of using it...

@carllindberg

For documentation links, there are:

A good overview of the situation:
http://sealiesoftware.com/blog/archive/2008/10/30/objc_explain_objc_msgSend_stret.html

Apple's low-level ABI documentation (has details for ppc, i386, and ppc64, with an external reference for x86-64):
http://developer.apple.com/documentation/DeveloperTools/Conceptual/LowLevelABI

The document for x86_64 (archive.org link since I can't get to main site at the moment):
http://web.archive.org/web/20120417163521/http://www.x86-64.org/documentation/abi.pdf

ARM information:
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042e/IHI0042E_aapcs.pdf

It's possible that libffi might have the code to determine this stuff, but I don't know enough about it.

@carllindberg

Yeah, I looked at class-dump to see if I could find any useful private methods, but nothing was evident. Class-dump does show that method, and the structure it returns (and also expands NSMethodFrameArgInfo, which is several more fields plus about 10-15 bitfield flags, and there are two of those in whatever is returned from _frameDescriptor).

Reading the GNUStep NSMethodSignature.m, there may be an extra '+' character in the type string for the return type to indicate a needed struct return, not sure. But that may be a GNU runtime thing only. It would be very nice to find better API to figure it out, to be sure. The current hack is technically not private API, but... it might be fragile. Not sure how often Apple changes the output of that method.

@carllindberg

Does look like libffi has the logic...

https://github.com/atgreen/libffi/blob/master/src/x86/ffi64.c
(the classify_argument and merge_classes functions)

Also, in theory, all the above would hold true for unions as well, but it appears that NSMethodSignature throws an exception if you try to initialize with a type string that uses them. Thus, nothing that uses NSInvocation would appear to be able to support union returns. It works if you just implement forwardingTargetForSelector:, which apparently avoids NSMethodSignature/NSInvocation creation, but once you need NSInvocation it's a problem.

@erikdoe
Owner
erikdoe commented Aug 8, 2013

In the long-term we should have a "real" implementation. Something based on the libffi Code. For now, to get the bug fixed soon, it looks like we have to base it off the debug output of NSMethodSignature as you suggest, Carl.

@erikdoe erikdoe merged commit 51d69a8 into erikdoe:master Aug 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.