From 0828b111250be7f8906fd976ed4f370ae98d317e Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Sat, 14 Apr 2018 14:18:51 +0100 Subject: [PATCH] Improve ARC test and fix bugs it uncovered. This cleans up handling of objects that are not reference counted and makes their interactions with ARC more consistent. We should probably generalise this somewhat - it currently special cases NSConstantString and NSGlobalBlock, but it would be nice to have an API for constant objects. --- NSBlocks.m | 3 ++ Test/ARCTest_arc.m | 88 ++++++++++++++++++++++++++++++++++++++++++++-- arc.m | 43 +++++++++++++--------- class.h | 8 ++++- class_table.c | 6 ++++ dtable.c | 6 ++++ 6 files changed, 135 insertions(+), 19 deletions(-) diff --git a/NSBlocks.m b/NSBlocks.m index 32870da..409dbf4 100644 --- a/NSBlocks.m +++ b/NSBlocks.m @@ -48,5 +48,8 @@ BOOL objc_create_block_classes_as_subclasses_of(Class super) NEW_CLASS(&_NSBlock, _NSConcreteStackBlock); NEW_CLASS(&_NSBlock, _NSConcreteGlobalBlock); NEW_CLASS(&_NSBlock, _NSConcreteMallocBlock); + // Global blocks never need refcount manipulation. + objc_set_class_flag(&_NSConcreteGlobalBlock, + objc_class_flag_permanent_instances); return YES; } diff --git a/Test/ARCTest_arc.m b/Test/ARCTest_arc.m index 3271cc6..af26898 100644 --- a/Test/ARCTest_arc.m +++ b/Test/ARCTest_arc.m @@ -1,9 +1,32 @@ #import "Test.h" +// Checks using non-portable APIs +#ifdef GNUSTEP_RUNTIME +void check_retain_count(id obj, size_t rc) +{ + assert(object_getRetainCount_np(obj) == rc); +} +void check_autorelease_count(id obj, size_t rc) +{ + assert(objc_arc_autorelease_count_for_object_np(obj) == rc); +} +#else +void check_retain_count(id obj, size_t rc) +{ +} +void check_autorelease_count(id obj, size_t rc) +{ +} +#endif + id __weak var; @interface ARC : Test @end @implementation ARC +- (id __autoreleasing)loadWeakAutoreleasing +{ + return var; +} - (id)loadWeak { return var; @@ -11,6 +34,7 @@ id __weak var; - (void)setWeakFromWeak: (id __weak)anObject { var = anObject; + anObject = nil; } - (void)setWeak: (id)anObject { @@ -18,18 +42,78 @@ id __weak var; } @end +@interface CheckDealloc : Test +@end +@implementation CheckDealloc +{ + BOOL *flag; +} +- (id)initWithFlag: (BOOL*)aFlag +{ + flag = aFlag; + *flag = NO; + return self; +} +- (void)dealloc +{ + *flag = YES; +} +@end + int main(void) { ARC *obj = [ARC new]; + BOOL f1; + BOOL f2; + // Check that storing weak references works. { - id o1 = [Test new]; - id o2 = [Test new]; + id o1 = [[CheckDealloc new] initWithFlag: &f1]; + id o2 = [[CheckDealloc new] initWithFlag: &f2]; [obj setWeak: o1]; assert([obj loadWeak] == o1); [obj setWeakFromWeak: o2]; assert([obj loadWeak] == o2); + @autoreleasepool + { + id __autoreleasing o2a = [obj loadWeakAutoreleasing]; + assert(o2a == o2); + } } + assert(f1); + assert(f2); assert([obj loadWeak] == nil); + @autoreleasepool + { + id __autoreleasing o1a; + { + id o1 = [[CheckDealloc new] initWithFlag: &f1]; + assert(!f1); + [obj setWeak: o1]; + assert([obj loadWeak] == o1); + o1a = [obj loadWeakAutoreleasing]; + assert(o1a == o1); + check_autorelease_count(o1a, 1); + check_retain_count(o1a, 1); + } + assert(o1a == [obj loadWeak]); + } + assert(f1); + assert([obj loadWeak] == nil); + // Try to trigger an objc_moveWeak call + { + id o1 = [Test new]; + { + id __weak x = o1; + var = x; + } + } + assert([obj loadWeak] == nil); + // Now check what happens with a constant string in a weak variable. + { + id x = @"foo"; + [obj setWeak: x]; + } + assert([obj loadWeak] != nil); return 0; } diff --git a/arc.m b/arc.m index 0cc0e64..82c73be 100644 --- a/arc.m +++ b/arc.m @@ -258,9 +258,27 @@ id objc_retain_fast_np(id obj) return obj; } +__attribute__((always_inline)) +static inline BOOL isPersistentObject(id obj) +{ + // No reference count manipulations on nil objects. + if (obj == nil) + { + return YES; + } + // Small objects are never accessibly by reference + if (isSmallObject(obj)) + { + return YES; + } + // Persistent objects are persistent. Safe to access isa directly here + // because we've already handled the small object case separately. + return objc_test_class_flag(obj->isa, objc_class_flag_permanent_instances); +} + static inline id retain(id obj) { - if (isSmallObject(obj)) { return obj; } + if (isPersistentObject(obj)) { return obj; } Class cls = obj->isa; if ((Class)&_NSConcreteMallocBlock == cls || (Class)&_NSConcreteStackBlock == cls) @@ -322,15 +340,14 @@ void objc_release_fast_np(id obj) static inline void release(id obj) { - if (isSmallObject(obj)) { return; } + if (isPersistentObject(obj)) { return; } Class cls = obj->isa; if (cls == &_NSConcreteMallocBlock) { _Block_release(obj); return; } - if ((cls == &_NSConcreteStackBlock) || - (cls == &_NSConcreteGlobalBlock)) + if (cls == &_NSConcreteStackBlock) { return; } @@ -691,18 +708,8 @@ id objc_storeWeak(id *addr, id obj) { return obj; } - BOOL isGlobalObject = (obj == nil) || isSmallObject(obj); - Class cls = Nil; - if (!isGlobalObject) - { - cls = classForObject(obj); - // TODO: We probably also want to do the same for constant strings and - // classes. - if (cls == &_NSConcreteGlobalBlock) - { - isGlobalObject = YES; - } - } + BOOL isGlobalObject = isPersistentObject(obj); + Class cls = isGlobalObject ? Nil : obj->isa; if (obj && cls && objc_test_class_flag(cls, objc_class_flag_fast_arc)) { uintptr_t *refCount = ((uintptr_t*)obj) - 1; @@ -839,6 +846,10 @@ id objc_loadWeakRetained(id* addr) { obj = block_load_weak(obj); } + else if (objc_test_class_flag(cls, objc_class_flag_permanent_instances)) + { + return obj; + } else if (!objc_test_class_flag(cls, objc_class_flag_fast_arc)) { obj = _objc_weak_load(obj); diff --git a/class.h b/class.h index e90db48..e7989db 100644 --- a/class.h +++ b/class.h @@ -238,7 +238,13 @@ enum objc_class_flags /** * This class is a hidden class used to store associated values. */ - objc_class_flag_assoc_class = (1<<8) + objc_class_flag_assoc_class = (1<<8), + /** + * This class has instances that are never deallocated and are therefore + * safe to store directly into weak variables and to skip all reference + * count manipulations. + */ + objc_class_flag_permanent_instances = (1<<14) }; /** diff --git a/class_table.c b/class_table.c index ec81255..66d95b1 100644 --- a/class_table.c +++ b/class_table.c @@ -413,6 +413,12 @@ PRIVATE void objc_load_class(struct objc_class *class) class->dtable = uninstalled_dtable; class->isa->dtable = uninstalled_dtable; + // Mark constant string instances as never needing refcount manipulation. + if (strcmp(class->name, "NSConstantString") == 0) + { + objc_set_class_flag(class, objc_class_flag_permanent_instances); + } + // If this is a root class, make the class into the metaclass's superclass. // This means that all instance methods will be available to the class. if (NULL == superclassName) diff --git a/dtable.c b/dtable.c index 6f5f8a9..799e883 100644 --- a/dtable.c +++ b/dtable.c @@ -936,6 +936,12 @@ PRIVATE void objc_send_initialize(id object) return; } BOOL skipMeta = objc_test_class_flag(meta, objc_class_flag_initialized); + // Mark metaclasses as never needing refcount manipulation for their + // instances (classes). + if (!skipMeta) + { + objc_set_class_flag(meta, objc_class_flag_permanent_instances); + } // Set the initialized flag on both this class and its metaclass, to make // sure that +initialize is only ever sent once.