Recent change to class_getMethodImplementation() breaks gnustep-base NSProxy (and other code)

[rfm]

The old behavior was to return a function which could be used to call a method (the method implementation), or NULL when the receiver does not implement the method.
The new behavior is (when the receiver forwards) to return the forwarding hook (__objc_msg_forward2) which, instead of calling the method, returns the method implementation.
That breaks any code which expects to be able to use the returned function to call a method.
IMO the correct behavior is the original behavior or to return a function which calls __objc_msg_forward2, then calls the function returned by __objc_msg_forward2, and returns the result.

[rfm]

Another way of looking at it:
The function is declared to return (IMP), but now it sometimes returns (IMP (*)(id, SEL)), which is wrong.

[davidchisnall]

Ah, sorry, this is indeed the wrong forwarding hook to return. I thought we had one that used libFFI or similar to do the lookup and forward. I've reverted the change for now.

It occurs to me that we do actually have such a function: objc_msgSend, which we could return for cases where the method is not present. That's not trivial though, because we'd need to decode some of the selector's type string to know whether to return the _fpret or _sret variants (and that's ABI-specific). Keeping the null result might be simpler.

[rfm]

I agree that it's non trivial.
As I understand it, the whole point of __objc_msg_forward2 is so that the forwarding code for the gnustep runtime in gnustep-base (probably written by you), sets __objc_msg_forward2 to be a function which uses libFFI to build the forwarding function matching the selector and return it, which is why having the runtime call __objc_msg_forward2 with a null first argument works. Perhaps it's worth adding a comment to that effect?

[davidchisnall]

Does GNUstep require objc_proxy_lookup to be called, or will the hook returned from __objc_msg_foward2 handle the equivalent (if the receiver needs modifying for 0forwardingTargetForSelector:)? If just returning the result of __objc_msg_foward2(nil, sel) works, that's easy to do.

[cmpwi]

Hi everyone, sorry about that. I thought I'd leave my two cents here about this as well. My train of thought for making this change was that, technically, the runtime and libs-base are functionally independent entities. Even though the most common pairing is libobjc2 + libs-base, other Foundation implementations can exist, and can thus register their own forwarding hooks. Having the runtime pass nil to __objc_msg_forward2() means that the runtime itself depends on the behaviour of the hook, which couples libobjc2 and Foundation. Apple's behaviour of returning the hook itself does, in effect, create a different kind of problem, as now every caller would have to do:

if (returnedIMP == __objc_msg_forward2)
	returnedIMP = __objc_msg_forward2(someObject, someSelector);

returnedIMP(someObject, someSelector, ...);

And this now simply creates a dependency on the symbol name itself, which is not good either.

I think that perhaps a comment in objc/hooks.h to the tune of: "class_getMethodImplementation() calls this hook with a nil receiver if its lookup fails. The hook then becomes responsible for returning something. GNUstep returns nil; another possible option is to return an IMP that calls the forwarding hook again with a nonnull receiver, then call the actual IMP and return its value." would help to deter overeager individuals like myself, while also documenting what a hook should expect to be called with (and also what libobjc2 expects).

EDIT: (Although I think having an IMP call an IMP transparently and have the arguments + return value in their proper places means that Foundation would have to basically reimplement half of objc_msgSend(). Oh boy.)

[davidchisnall]

Although I think having an IMP call an IMP transparently and have the arguments + return value in their proper places means that Foundation would have to basically reimplement half of objc_msgSend(). Oh boy

This is why the runtime doesn't do that. Apple's implementation does the forwarding entirely in their runtime, where it's written in assembly. We made a choice that almost everything (the block to IMP stuff is the exception) should work without target-specific assembly. Instead, we rely on the Foundation implementation using something like libFFI to generate the equivalent forwarding code.

There is intrinsically some coupling of Foundation and the runtime. Apple moved most of NSObject into the runtime as a result of this. We've aimed to make that coupling a bit weaker.

[cmpwi]

Right, makes sense! Do you think adding an extra comment in hooks.h is still in order? I can make a quick PR for that.

[davidchisnall]

If we make this function return the result of the hook with nil, we should document that.

[cmpwi]

Okay, that seems like a better place.

[rfm]

Looking at the code in GSFFInvocation.m I see that, while there's an implementation for objc_proxy_lookup which will return a value for any NSObject, it won't generally for NSProxy (because it's an abstract class with no concrete target). So I would say it's not required.

The implementation of __objc_msg_forward2 will use the target object to get a method signature if it's supplied, but otherwise will use the selector to get the best guess at type information and construct a method signature from that. Either way the method signature is then used to have libffi create the code to call the method. Clearly constructing the call without information from the target object is not completely reliable (because the type information found is not guaranteed to match the method of the target), but it's almost always correct.

[davidchisnall]

My question is really:

If I call __objc_msg_forward2 with nil as a receiver and @selector(foo) as the selector, and then I call that result with an NSProxy subclass that implements -forwardingTargetForSelector: to return an object that does implement -foo, does the returned object's -foo method get called?

If so (or if that's easy to add), then making class_getMethodImplementation(cls, sel) return __objc_msg_forward2(nil, sel) for missing methods makes sense and would give largely the same behaviour as Apple's implementation.

[rfm]

I don't think -forwardingTargetForSelector: is relevant, but the answer is yes.

Calling the return value of __objc_msg_forward2 with the NSProxy instance causes an NSInvocation instance to be created and passed to the -forwardInvocation: method of the proxy. That method sets the target of the invocation and invokes it, which calls -foo on that target.

Calling -forwardingTargetForSelector: would potentially let the code skip calling the proxy's -forwardInvocation: method and invoke directly, but that would break the API/contract for forwarding (because the proxy subclass should be able to do other things when using the invocation if it wants to().

[davidchisnall]

Calling -forwardingTargetForSelector: would potentially let the code skip calling the proxy's -forwardInvocation: method and invoke directly, but that would break the API/contract for forwarding (because the proxy subclass should be able to do other things when using the invocation if it wants to().

I think it's the other way around. The API contract is that -forwardingTargetForSelector: is called in before (and instead of) to -forwardInvocation: if it exists.

[rfm]

That case is bypassing the normal/original forwarding mechanism altogether though, isn't it?
With that newer API the runtime checks for the method, gets a new target, and sends the message directly to the new target. In which case I would expect there to be no call to __objc_msg_forward2

[davidchisnall]

That case is bypassing the normal/original forwarding mechanism altogether though, isn't it?

I think the Apple version of returns an assembly function that then does all of the NSObject / NSProxy forwarding things, so ideally the implementation of the hook in -base would do the same.

With that newer API the runtime checks for the method, gets a new target, and sends the message directly to the new target. In which case I would expect there to be no call to __objc_msg_forward2

When I originally implemented support for -forwardingTargetForSelector:, I think I did it in the forwarding hook without support in the runtime, but this was almost 20 years ago now, so I'm not sure.