From ca1d45a4e688c212d4ddb3964968650eff73717e Mon Sep 17 00:00:00 2001
From: David Chisnall <theraven@FreeBSD.org>
Date: Sat, 14 Apr 2018 09:04:40 +0100
Subject: [PATCH 1/5] Fix dangling reference in weak ref code.

Another test added as a result of coverage analysis.
---
 Test/ARCTest_arc.m  | 35 +++++++++++++++++++++++++++++++++++
 Test/CMakeLists.txt |  1 +
 arc.m               |  1 +
 3 files changed, 37 insertions(+)
 create mode 100644 Test/ARCTest_arc.m

diff --git a/Test/ARCTest_arc.m b/Test/ARCTest_arc.m
new file mode 100644
index 0000000..3271cc6
--- /dev/null
+++ b/Test/ARCTest_arc.m
@@ -0,0 +1,35 @@
+#import "Test.h"
+
+id __weak var;
+
+@interface ARC : Test @end
+@implementation ARC
+- (id)loadWeak
+{
+	return var;
+}
+- (void)setWeakFromWeak: (id __weak)anObject
+{
+	var = anObject;
+}
+- (void)setWeak: (id)anObject
+{
+	var = anObject;
+}
+@end
+
+
+int main(void)
+{
+	ARC *obj = [ARC new];
+	{
+		id o1 = [Test new];
+		id o2 = [Test new];
+		[obj setWeak: o1];
+		assert([obj loadWeak] == o1);
+		[obj setWeakFromWeak: o2];
+		assert([obj loadWeak] == o2);
+	}
+	assert([obj loadWeak] == nil);
+	return 0;
+}
diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt
index 4165d29..fcc07e4 100644
--- a/Test/CMakeLists.txt
+++ b/Test/CMakeLists.txt
@@ -7,6 +7,7 @@
 set(TESTS
 	alignTest.m
 	AllocatePair.m
+	ARCTest_arc.m
 	AssociatedObject.m
 	AssociatedObject2.m
 	BlockImpTest.m
diff --git a/arc.m b/arc.m
index 0540391..0cc0e64 100644
--- a/arc.m
+++ b/arc.m
@@ -671,6 +671,7 @@ static inline BOOL weakRefRelease(WeakRef *ref)
 	ref->weak_count--;
 	if (ref->weak_count == 0)
 	{
+		weak_ref_remove(weakRefs, ref->obj);
 		free(ref);
 		return YES;
 	}

From 0828b111250be7f8906fd976ed4f370ae98d317e Mon Sep 17 00:00:00 2001
From: David Chisnall <theraven@FreeBSD.org>
Date: Sat, 14 Apr 2018 14:18:51 +0100
Subject: [PATCH 2/5] 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.

From f2b59885964ae97412214cd8a58ca08787e42c25 Mon Sep 17 00:00:00 2001
From: David Chisnall <theraven@FreeBSD.org>
Date: Sun, 1 Apr 2018 10:25:12 +0100
Subject: [PATCH 3/5] Add constant string test.

---
 Test/CMakeLists.txt   |  1 +
 Test/ConstantString.m | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 Test/ConstantString.m

diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt
index fcc07e4..f360b0d 100644
--- a/Test/CMakeLists.txt
+++ b/Test/CMakeLists.txt
@@ -12,6 +12,7 @@ set(TESTS
 	AssociatedObject2.m
 	BlockImpTest.m
 	BlockTest_arc.m
+	ConstantString.m
 	BoxedForeignException.m
 	ExceptionTest.m
 	ForeignException.m
diff --git a/Test/ConstantString.m b/Test/ConstantString.m
new file mode 100644
index 0000000..8d3811c
--- /dev/null
+++ b/Test/ConstantString.m
@@ -0,0 +1,30 @@
+#import "Test.h"
+#include <string.h>
+
+@interface NSConstantString : Test
+{
+	const char * const str;
+	const unsigned int len;
+}
+- (unsigned int)length;
+- (const char*)cString;
+@end
+
+@implementation NSConstantString
+- (unsigned int)length
+{
+	return len;
+}
+- (const char*)cString
+{
+	return str;
+}
+@end
+
+
+int main(void)
+{
+	assert([@"1234567890" length] == 10);
+	assert(strcmp([@"123456789" cString], "123456789") == 0);
+	return 0;
+}

From 554c6623dc049ae800a6686bdff3ebdea57bb3c9 Mon Sep 17 00:00:00 2001
From: David Chisnall <theraven@FreeBSD.org>
Date: Fri, 20 Apr 2018 09:42:37 +0100
Subject: [PATCH 4/5] Back-port constant string test from newabi branch.

---
 Test/ConstantString.m | 11 +----------
 Test/Test.h           | 21 +++++++++++++++++++++
 Test/Test.m           |  2 ++
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/Test/ConstantString.m b/Test/ConstantString.m
index 8d3811c..4e7f720 100644
--- a/Test/ConstantString.m
+++ b/Test/ConstantString.m
@@ -1,19 +1,10 @@
 #import "Test.h"
 #include <string.h>
 
-@interface NSConstantString : Test
-{
-	const char * const str;
-	const unsigned int len;
-}
-- (unsigned int)length;
-- (const char*)cString;
-@end
-
 @implementation NSConstantString
 - (unsigned int)length
 {
-	return len;
+	return length;
 }
 - (const char*)cString
 {
diff --git a/Test/Test.h b/Test/Test.h
index e455aaa..36a5a2b 100644
--- a/Test/Test.h
+++ b/Test/Test.h
@@ -23,6 +23,27 @@ __attribute__((objc_root_class))
 #endif
 @end
 
+#ifdef __OBJC_GNUSTEP_RUNTIME_ABI__
+#	if __OBJC_GNUSTEP_RUNTIME_ABI__ >= 20
+#		define NEW_ABI
+#	endif
+#endif
+
+@interface NSConstantString : Test
+{
+#ifdef NEW_ABI
+	uint32_t flags;
+	uint32_t length;
+	uint32_t size;
+	uint32_t hash;
+	const char * const str;
+#else
+	const char * const str;
+	const unsigned int length;
+#endif
+}
+@end
+
 @interface NSAutoreleasePool : Test
 @end
 
diff --git a/Test/Test.m b/Test/Test.m
index 87bfcae..9edf209 100644
--- a/Test/Test.m
+++ b/Test/Test.m
@@ -8,3 +8,5 @@
 #define SINGLE_FILE_TEST 1
 #endif
 #include "Test.h"
+
+@implementation NSConstantString @end

From a5a0f70a03fbd182e526955a237085e81ef4d5ef Mon Sep 17 00:00:00 2001
From: David Chisnall <theraven@FreeBSD.org>
Date: Sat, 14 Apr 2018 08:35:08 +0100
Subject: [PATCH 5/5] Unregister classes when deleting them.

Coverage checking of the test suite showed that objc_disposeClassPair
wasn't tested at all, which then led to discovering that it didn't
unregister the class.
---
 Test/AllocatePair.m | 5 +++++
 class.h             | 7 +++++++
 class_table.c       | 6 ++++++
 runtime.c           | 1 +
 4 files changed, 19 insertions(+)

diff --git a/Test/AllocatePair.m b/Test/AllocatePair.m
index 9a4cf47..1a3f2f3 100644
--- a/Test/AllocatePair.m
+++ b/Test/AllocatePair.m
@@ -31,6 +31,11 @@ int main()
 	e = objc_allocateClassPair(d, "E", 0);
 	objc_registerClassPair(e);
 	assert(loaded == 0);
+	assert(objc_getClass("C") == c);
+	assert(objc_getClass("D") == d);
+	assert(objc_getClass("E") == e);
+	objc_disposeClassPair(e);
+	assert(objc_getClass("E") == nil);
 
 	return 0;
 }
diff --git a/class.h b/class.h
index e7989db..d4baa46 100644
--- a/class.h
+++ b/class.h
@@ -272,11 +272,18 @@ static inline BOOL objc_test_class_flag(struct objc_class *aClass,
 	return (aClass->info & (unsigned long)flag) == (unsigned long)flag;
 }
 
+
 /**
  * Adds a class to the class table.
  */
 void class_table_insert(Class class);
 
+/**
+ * Removes a class from the class table.  Must be called with the runtime lock
+ * held!
+ */
+void class_table_remove(Class cls);
+
 /**
  * Array of classes used for small objects.  Small objects are embedded in
  * their pointer.  In 32-bit mode, we have one small object class (typically
diff --git a/class_table.c b/class_table.c
index 66d95b1..4344641 100644
--- a/class_table.c
+++ b/class_table.c
@@ -457,6 +457,12 @@ BOOL objc_registerSmallObjectClass_np(Class class, uintptr_t mask)
 	return YES;
 }
 
+PRIVATE void class_table_remove(Class cls)
+{
+	assert(objc_test_class_flag(cls, objc_class_flag_user_created));
+	class_table_internal_remove(class_table, (void*)cls->name);
+}
+
 
 ////////////////////////////////////////////////////////////////////////////////
 // Public API
diff --git a/runtime.c b/runtime.c
index b3b734f..4b81805 100644
--- a/runtime.c
+++ b/runtime.c
@@ -719,6 +719,7 @@ void objc_disposeClassPair(Class cls)
 		LOCK_RUNTIME_FOR_SCOPE();
 		safe_remove_from_subclass_list(meta);
 		safe_remove_from_subclass_list(cls);
+		class_table_remove(cls);
 	}
 
 	// Free the method and ivar lists.