Bug in cache for `ext_globalMethodSignatureForSelector`

[claybridges]

I suspect this cache is a bug:

NSMethodSignature *ext_globalMethodSignatureForSelector (SEL aSelector) {
    // set up a simplistic cache to avoid repeatedly scouring every class in the
    // runtime
    static const size_t selectorCacheLength = 1 << 8;
    static const uintptr_t selectorCacheMask = (selectorCacheLength - 1);
    static void * volatile selectorCache[selectorCacheLength];

    const char *cachedType = selectorCache[(uintptr_t)(void *)aSelector & selectorCacheMask];
    if (cachedType) {
        return [NSMethodSignature signatureWithObjCTypes:cachedType];
    }

    //...

Looks like a hash table based on the lowest-order byte of the selector pointer. Unfortunately, there's no way to tell when there is a collision. When there is, this function will probably be returning the wrong signature.

I ran a naïve test, running through every class, every instance method, gisted here. A collision occurred after 8 methods.

xctest[64093:303] first collision with -[NSMenuItemHighlightColor setStroke] and -[NSMenuItemHighlightColor colorWithAlphaComponent:]
xctest[64093:303] tested 2834 classes, 56004 methods, 50268 collisions, first collision @ method 8

I have a minimal fix in mind, probably caching a struct of {SEL sel, char *types}, which lets you detect collisions and overwrite the old value. I'll probably fix it myself for my own purposes. I can put together a pull request, if it's not fixed already by then.

[claybridges]

I started asking myself if the same selector could have different type strings. Answer: YES!

@interface Test1 : NSObject
@end

@implementation Test1
- (void)selectorTest {}
@end

@interface Test2 : NSObject
@end

@implementation Test2
- (double)selectorTest { return 0.0; }
@end

- (void)testConflictingTypes {
    Test1 *test1 = [[Test1 alloc] init];
    Test2 *test2 = [[Test2 alloc] init];
    SEL sel = @selector(selectorTest);

    const char *types[2];
    uint i = 0;

    for (NSObject *obj in @[test1, test2]) {
        Method method = class_getInstanceMethod([obj class], sel);
        types[i] = method_getTypeEncoding(method);
        NSLog(@"-[%@ %@] has type string %s", obj, NSStringFromSelector(sel), types[i]);
        i++;
    }

    // insert your flavor of test assert here
    XCTAssert(strcmp(types[0],types[1]) == 0, @"same selector has same type");
}

yields

Test Case '-[ExtObjCCacheTest testConflictingTypes]' started.
 xctest[67686:303] -[<Test1: 0x10210e370> selectorTest] has type string v16@0:8
 xctest[67686:303] -[<Test2: 0x102110400> selectorTest] has type string d16@0:8
 <...>/ExtObjCCacheTest.m:182: error: -[ExtObjCCacheTest testConflictingTypes] : ((strcmp(types[0],types[1]) == 0) is true) failed - same selector has same type
Test Case '-[ExtObjCCacheTest testConflictingTypes]' failed (0.001 seconds).

I can still write a fix to the cache, but a global selector type string then becomes <100% right. This bums me out (with the runtime). I see the separation of {SEL, typeString} in the various runtime machinations as a serious flaw, and a barrier to a lot of shenanigans that I might like to pull.

Thoughts?

[jspahrsummers]

Ah, true. That's probably why I was okay with clobbering in the first place. (Trying to remember my thought process from that long ago is difficult.)

I don't think there's really a good solution for this in the general case. This is why there's a compiler flag to warn about selectors with different type signatures — it makes objc_msgSend and a lot of runtime machinations dangerous. :\

The only consolation, I think, is that object types and pointers will always have compatible type signatures on x86_64 and ARM. Other types are less safe.

[claybridges]

@jspahrsummers I just used my new contributor superpowers to delete your reply of (something like) "nice catch. yes, pull request, please". I thought that circle-X was a cancel at some point there. Lesson learned, doh, sorry about that.

[jspahrsummers]

Haha, no worries.

[claybridges]

This is a good read, but probably not a real issue that should be open anymore.