GSFFIInvocation: Use objc_msg_lookup in -invokeWithTarget: to avoid skipping hidden classes

[hmelder]

When observing properties via a proxy object, all messages are encapsulated into an NSInvocation object.
From the example in #472, the following messages are forwarded to the underlying object:

• addObserver:forKeyPath:options:context:
• removeObserver:forKeyPath:options:context:
• setName:

When adding a observer for the first time (assuming that there are no associated objects), a hidden subclass is created and attached (ISA changes) to the TObject instance. setName: is then swizzled and added to the hidden subclass.
The current invocation mechanism uses APIs that skip over hidden classes during method lookup, resulting in the original IMP being called instead of the swizzled one.

This is documented in NSKVOSwizzling:

libs-base/Source/NSKVOSwizzling.m

Lines 96 to 111 in 9cdb4f9

	/*
	A crucial design decision was made here: because object_getClass() skips
	autogenerated subclasses, the likes of which are used for associated objects,
	the KVO machinery (if implemented using manual subclassing) would delete all
	associated objects on an observed instance. That was deemed unacceptable.
	Likewise, this implementation is not free of issues:
	- The _np methods are nonportable.
	- Anyone using class_getMethodImplementation(object_getClass(...), ...) will
	receive the original IMP for any overridden method.
	- We have to manually load isa to get at the secret subclass (thus the use of
	ABI_ISA/ABI_SUPER.)
	- It is dependent upon a libobjc2 implementation detail:
	object_addMethod_np creates a hidden subclass for the object's class (one
	per object!)
	*/

Here is a quick micro benchmark to compare the old mechanism with objc_msg_lookup. Please note, that I have not benchmarked the legacy GCC runtime.

System

OS:  Ubuntu 24.10
Architecture: AArch64
libobjc2: 0c1a89373f137489a829c69009d7ce752ac46b85

2024-12-02T17:11:28+01:00
Running ./a.out
Run on (12 X 2000 MHz CPU s)
Load Average: 1.49, 1.20, 1.06
***WARNING*** Library was built as DEBUG. Timings may be affected.
-------------------------------------------------------------
Benchmark                   Time             CPU   Iterations
-------------------------------------------------------------
BM_objc_msg_lookup       7.86 ns         7.86 ns     89906353
BM_GSGetMethod           12.5 ns         12.4 ns     56523293

#include <benchmark/benchmark.h>
#include <Foundation/Foundation.h>

struct objc_method *
GSGetMethod(Class cls, SEL sel,
  BOOL searchInstanceMethods,
  BOOL searchSuperClasses)
{
  if (cls == 0 || sel == 0)
    {
      return 0;
    }

  if (searchSuperClasses == NO)
    {
      unsigned int	count;
      Method		method = NULL;
      Method		*methods;

      if (searchInstanceMethods == NO)
	{
	  methods = class_copyMethodList(object_getClass(cls), &count);
	}
      else
	{
	  methods = class_copyMethodList(cls, &count);
	}
      if (methods != NULL)
	{
	  unsigned int	index = 0;

	  while ((method = methods[index++]) != NULL)
	    {
	      if (sel_isEqual(sel, method_getName(method)))
		{
		  break;
		}
	    }
	  free(methods);
	}
      return method;
    }
  else
    {
      if (searchInstanceMethods == NO)
	{
	  return class_getClassMethod(cls, sel);
	}
      else
	{
	  return class_getInstanceMethod(cls, sel);
	}
    }
}

static void BM_objc_msg_lookup(benchmark::State& state) {
  NSObject *target = [NSObject new];
  SEL sel = @selector(description);
  IMP imp;

  for (auto _ : state) {
    benchmark::DoNotOptimize(imp = objc_msg_lookup(target, sel));
  }
}

static void BM_GSGetMethod(benchmark::State& state) {
  Class cls = [NSObject class];
  SEL sel = @selector(description);
  IMP imp;

  for (auto _ : state) {
    benchmark::DoNotOptimize(imp = method_getImplementation(GSGetMethod(cls, sel, YES, YES)));
  }
}

BENCHMARK(BM_objc_msg_lookup);
BENCHMARK(BM_GSGetMethod);
BENCHMARK_MAIN();

[hmelder]

Seems like legacy KVO is not happy with the new tests. Should I mark them as hopeful?

base/NSKVOSupport/proxy.m:
Failed test:     (2024-12-02 16:19:52.736 +0000) proxy.m:136 ... change notification for dependent key 'derivedName' is emitted first
Failed test:     (2024-12-02 16:19:52.740 +0000) proxy.m:119 ... Got two change notifications
Failed test:     (2024-12-02 16:19:52.740 +0000) proxy.m:122 ... Got two change notifications

[hmelder]

Assuming that #474 (comment) is refering to this PR:

libobjc2 provides a safe way to do IMP caching: You lookup the slot and check the cache version, before invoking IMP. If there is a mismatch, a new lookup is required.

https://github.com/gnustep/libobjc2/blob/0c1a89373f137489a829c69009d7ce752ac46b85/objc/slot.h#L13-L20

We can probably write a shim for the GCC runtime which defines slots.

[hmelder]

Here are the APIs (located in objc/runtime.h):

/**
 * Look up a slot, without invoking any forwarding mechanisms.  The third
 * parameter is used to return a pointer to the current value of the version
 * counter.  If this value is equal to `objc_method_cache_version` then the
 * slot is safe to reuse without performing another lookup.
 */
OBJC_PUBLIC
extern struct objc_slot2 *objc_get_slot2(Class, SEL, uint64_t*)
	OBJC_NONPORTABLE;

/**
 * Look up a slot, invoking any required forwarding mechanisms.  The third
 * parameter is used to return a pointer to the current value of the version
 * counter.  If this value is equal to `objc_method_cache_version` then the
 * slot is safe to reuse without performing another lookup.
 */
OBJC_PUBLIC
extern struct objc_slot2 *objc_slot_lookup_version(id *receiver, SEL selector, uint64_t*)
	OBJC_NONPORTABLE;

[rfm]

Yes, this all seems good to me, and yes the tests that fail under the old kvo implementation should be marked as hopeful since we aren't expecting them to pass.