From 2fb1194cec49fd7af883c918f733bd237f1cb318 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Wed, 21 Aug 2019 15:04:16 +0100 Subject: [PATCH 1/9] Add test case for weak ref issue. This test now fails deterministically, which should make fixing it easier. --- Test/CMakeLists.txt | 1 + Test/WeakRefLoad.m | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 Test/WeakRefLoad.m diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index 9e292a7..09e5902 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -37,6 +37,7 @@ set(TESTS ResurrectInDealloc_arc.m RuntimeTest.m WeakBlock_arc.m + WeakRefLoad.m WeakReferences_arc.m WeakImportClass.m ivar_arc.m diff --git a/Test/WeakRefLoad.m b/Test/WeakRefLoad.m new file mode 100644 index 0000000..c851638 --- /dev/null +++ b/Test/WeakRefLoad.m @@ -0,0 +1,22 @@ +#include "Test.h" + +#define SIZE 5000 + +int main(int argc, const char * argv[]) +{ + id t = [Test new]; + id w1; + id w2; + objc_initWeak(&w1, t); + objc_initWeak(&w2, t); + [t release]; + assert(objc_loadWeakRetained(&w1) == nil); + assert(objc_loadWeakRetained(&w2) == nil); + assert(w1 == nil); + assert(w2 == nil); + assert(objc_loadWeakRetained(&w1) == nil); + assert(objc_loadWeakRetained(&w2) == nil); + assert(w1 == nil); + assert(w2 == nil); + return 0; +} From 4482919e09d23dbd663bb5bc7249ed90b896a697 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Wed, 21 Aug 2019 15:06:24 +0100 Subject: [PATCH 2/9] Fix an issue with `WeakRef`s being over-released As an optimisation, on load of a weak reference we check if the object has already been deallocated and, if so, decrement the weak reference count and zero the pointer to the weak reference structure so that the next check is faster and doesn't need to hold locks for as long. Unfortunately, the prior implementation of this instead decremented the weak reference count and then only zeroed the pointer if the reference count reached zero. This meant that loading the same __weak pointer twice after the pointed-to object had been deallocated would decrement the reference count twice. --- arc.m | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arc.m b/arc.m index 719ed9d..321e532 100644 --- a/arc.m +++ b/arc.m @@ -856,8 +856,9 @@ OBJC_PUBLIC id objc_loadWeakRetained(id* addr) if (obj == nil) { // If we've destroyed this weak ref, then make sure that we also deallocate the object. - if (weakRefRelease(ref)) + if (ref != NULL) { + weakRefRelease(ref); *addr = nil; } return nil; From 469d616a7779d2ba981dde408246fd7fdf8f724e Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Wed, 21 Aug 2019 16:38:42 +0100 Subject: [PATCH 3/9] [NFC] Fix comment. --- arc.m | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arc.m b/arc.m index 321e532..b73276c 100644 --- a/arc.m +++ b/arc.m @@ -855,7 +855,8 @@ OBJC_PUBLIC id objc_loadWeakRetained(id* addr) // will acquire the lock before attempting to deallocate) if (obj == nil) { - // If we've destroyed this weak ref, then make sure that we also deallocate the object. + // If the object is destroyed, drop this reference to the WeakRef + // struct. if (ref != NULL) { weakRefRelease(ref); From 60a657fbc693f869d7c52e78d47ca99e17e2e36c Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 22 Aug 2019 15:06:01 +0100 Subject: [PATCH 4/9] Fix CFA calculation in AArch64 objc_msgSend. Prior to this, throwing an exception from a +initialize method would leave the stack pointer 16 bytes offset from its correct location. --- objc_msgSend.aarch64.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/objc_msgSend.aarch64.S b/objc_msgSend.aarch64.S index a571bba..31b6b52 100644 --- a/objc_msgSend.aarch64.S +++ b/objc_msgSend.aarch64.S @@ -54,9 +54,9 @@ stp fp, lr, [sp, #192] add fp, sp, 192 stp \receiver, x8, [sp, #-16]! - .cfi_def_cfa fp, 0 - .cfi_offset fp, 0 - .cfi_offset lr, 8 + .cfi_def_cfa fp, 16 + .cfi_offset fp, -16 + .cfi_offset lr, -8 // We now have all argument registers, the link // register and the receiver spilled on the // stack, with sp containing From c1a3d8f4705c50c88ad018e64d1b332e4104dd14 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 22 Aug 2019 15:12:52 +0100 Subject: [PATCH 5/9] Fix stack resetting for objc_msgSend on ARM. If we called into C to find the IMP (e.g. for forwarding), we were then reloading all of the arguments but failing to adjust the stack pointer by the correct amount, leaving it around 192 bytes offset from its correct location. This, unsurprisingly, led to crashing and other exciting behaviour. --- objc_msgSend.aarch64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/objc_msgSend.aarch64.S b/objc_msgSend.aarch64.S index 31b6b52..6d0b327 100644 --- a/objc_msgSend.aarch64.S +++ b/objc_msgSend.aarch64.S @@ -76,7 +76,7 @@ ldp q4, q5, [sp, #144] ldp q6, q7, [sp, #176] ldp fp, lr, [sp, #208] - ldp \receiver, x8, [sp], #16 + ldp \receiver, x8, [sp], #(ARGUMENT_SPILL_SIZE + 16) br x9 6: adr x10, SmallObjectClasses From b1964451e123050c9062eb605894d712a195cb27 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 22 Aug 2019 15:14:27 +0100 Subject: [PATCH 6/9] Fix the AArch64 small object class lookup. This was generating a relocation that didn't do the right thing and didn't raise linker errors. Now it is using GOT-relative addressing. In combination with the last two commits, this now makes all of the objc_msgSend tests pass on AArch64. Fixes #105 --- objc_msgSend.aarch64.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/objc_msgSend.aarch64.S b/objc_msgSend.aarch64.S index 6d0b327..fd14d26 100644 --- a/objc_msgSend.aarch64.S +++ b/objc_msgSend.aarch64.S @@ -79,7 +79,8 @@ ldp \receiver, x8, [sp], #(ARGUMENT_SPILL_SIZE + 16) br x9 6: - adr x10, SmallObjectClasses + adrp x10, :got:SmallObjectClasses + ldr x10, [x10, :got_lo12:SmallObjectClasses] ldr x9, [x10, x9, lsl #3] b 1b .cfi_endproc From 7166a41999bbdb25aa7c7d33e4b4c38389934b15 Mon Sep 17 00:00:00 2001 From: Frederik Seiffert Date: Wed, 16 Oct 2019 09:19:23 +0200 Subject: [PATCH 7/9] Fixed usage of properties linked list. --- properties.m | 2 +- protocol.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/properties.m b/properties.m index c2216e9..7c1d9a8 100644 --- a/properties.m +++ b/properties.m @@ -302,7 +302,7 @@ objc_property_t* class_copyPropertyList(Class cls, unsigned int *outCount) unsigned int out = 0; for (struct objc_property_list *l=properties ; NULL!=l ; l=l->next) { - for (int i=0 ; icount ; i++) + for (int i=0 ; icount ; i++) { list[out++] = property_at_index(l, i); } diff --git a/protocol.c b/protocol.c index dd1061e..e0144e2 100644 --- a/protocol.c +++ b/protocol.c @@ -421,7 +421,7 @@ objc_property_t *protocol_copyPropertyList2(Protocol *p, unsigned int *outCount, unsigned int count = 0; for (struct objc_property_list *l=properties ; l!=NULL ; l=l->next) { - count += properties->count; + count += l->count; } if (0 == count) { From 710b2368cca8039523cdfef217626b3345f17e65 Mon Sep 17 00:00:00 2001 From: Rupert Daniel Date: Thu, 3 Oct 2019 09:07:33 +0100 Subject: [PATCH 8/9] Fixed crash when using self in imp_implementationWithBlock on armv7a --- Test/BlockImpTest.m | 2 ++ block_trampolines.S | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Test/BlockImpTest.m b/Test/BlockImpTest.m index ce9931a..4ccd05b 100644 --- a/Test/BlockImpTest.m +++ b/Test/BlockImpTest.m @@ -26,6 +26,7 @@ int main(void) { __block int b = 0; void* blk = ^(id self, int a) { + assert([self class] == [Foo class]); b += a; return b; }; blk = Block_copy(blk); @@ -45,6 +46,7 @@ int main(void) assert(imp_getBlock(imp) != (blk)); blk = ^(id self) { + assert([self class] == [Foo class]); struct big b = {1, 2, 3, 4, 5}; return b; }; diff --git a/block_trampolines.S b/block_trampolines.S index 70e321d..6107e7a 100644 --- a/block_trampolines.S +++ b/block_trampolines.S @@ -126,7 +126,7 @@ .thumb .macro trampoline arg0, arg1 sub r12, pc, #4095 - mov \arg0, \arg1 // Move self over _cmd + mov \arg1, \arg0 // Move self over _cmd ldr \arg0, [r12, #-5] // Load the block pointer over self ldr r12, [r12, #-1] // Jump to the block function bx r12 @@ -134,7 +134,7 @@ # else .macro trampoline arg0, arg1 sub r12, pc, #4096 - mov \arg0, \arg1 // Move self over _cmd + mov \arg1, \arg0 // Move self over _cmd ldr \arg0, [r12, #-8] // Load the block pointer over self ldr pc, [r12, #-4] // Jump to the block function .endm From 108d4e19c0ed5443419279ab9998bc93da7d3f19 Mon Sep 17 00:00:00 2001 From: Rupert Daniel Date: Thu, 3 Oct 2019 11:51:11 +0100 Subject: [PATCH 9/9] Updated BlockImpTest asserts. Fixes #126 assert now directly compares block self with expected class, avoiding objc_msgSend. --- Test/BlockImpTest.m | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Test/BlockImpTest.m b/Test/BlockImpTest.m index 4ccd05b..aa07499 100644 --- a/Test/BlockImpTest.m +++ b/Test/BlockImpTest.m @@ -24,9 +24,10 @@ __attribute__((objc_root_class)) int main(void) { + __block Class cls = objc_getClass("Foo"); __block int b = 0; void* blk = ^(id self, int a) { - assert([self class] == [Foo class]); + assert(self == cls); b += a; return b; }; blk = Block_copy(blk); @@ -34,7 +35,6 @@ int main(void) char *type = block_copyIMPTypeEncoding_np(blk); assert(NULL != type); class_addMethod((objc_getMetaClass("Foo")), @selector(count:), imp, type); - Class cls = objc_getClass("Foo"); assert(2 == ((int(*)(id,SEL,int))imp)(cls, @selector(count:), 2)); free(type); assert(4 == [Foo count: 2]); @@ -46,7 +46,7 @@ int main(void) assert(imp_getBlock(imp) != (blk)); blk = ^(id self) { - assert([self class] == [Foo class]); + assert(self == cls); struct big b = {1, 2, 3, 4, 5}; return b; };