From f264fd159d5d1624d822612ec4f7f05cb01df117 Mon Sep 17 00:00:00 2001 From: Niels Grewe Date: Mon, 8 Feb 2016 20:42:57 +0100 Subject: [PATCH 01/18] Fix a segfault on (s|g)etting ivars. When we were trying to get or set an ivar on a class that had no ivars defined, we'd dereference a NULL ivar list while determining the ownership qualifier. --- Test/ivar_arc.m | 31 ++++++++++++++++++++++++++++--- ivar.c | 3 ++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/Test/ivar_arc.m b/Test/ivar_arc.m index d45775d..0f719e5 100644 --- a/Test/ivar_arc.m +++ b/Test/ivar_arc.m @@ -20,14 +20,29 @@ int dealloc = 0; } @end +@interface EmptySubFoo : Foo +@end + +@implementation EmptySubFoo +@end + +@interface NonEmptySubFoo : Foo +{ + __strong id ignored; +} +@end + +@implementation NonEmptySubFoo +@end + void setIvar(id obj, const char * name, id val) { object_setIvar(obj, class_getInstanceVariable(object_getClass(obj), name), val); } -int main(void) +void testIvarsOn(Foo* f) { - Foo *f = [Foo new]; + dealloc = 0; Dealloc *d = [Dealloc new]; __unsafe_unretained Dealloc *dead; setIvar(f, "w", d); @@ -48,5 +63,15 @@ int main(void) setIvar(f, "s", nil); assert(dealloc == 1); assert(f->s == nil); - return 0; +} + +int main (void) +{ + /* Test for ivars in the same class */ + testIvarsOn([Foo new]); + /* Test for ivars in the superclass (receiver ivar list empty) */ + testIvarsOn([EmptySubFoo new]); + /* Test for ivars in the superclass (receiver ivar list non-empty) */ + testIvarsOn([NonEmptySubFoo new]); + return 0; } diff --git a/ivar.c b/ivar.c index 8504dae..e1053e7 100644 --- a/ivar.c +++ b/ivar.c @@ -154,7 +154,8 @@ typedef enum { ownership ownershipForIvar(Class cls, Ivar ivar) { struct objc_ivar_list *list = cls->ivars; - if ((ivar < list->ivar_list) || (ivar >= &list->ivar_list[list->count])) + if ((list == NULL) || (ivar < list->ivar_list) + || (ivar >= &list->ivar_list[list->count])) { // Try the superclass if (cls->super_class) From 6de70135e823d71e470efdd69fcb255ef78bc7b1 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 29 Apr 2016 10:24:31 +0000 Subject: [PATCH 02/18] Don't abort if NSAutoreleasePool is not found. Fixes #17 --- arc.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arc.m b/arc.m index b8d177f..b7a3b3b 100644 --- a/arc.m +++ b/arc.m @@ -220,7 +220,7 @@ static inline void initAutorelease(void) { if (Nil == AutoreleasePool) { - AutoreleasePool = objc_getRequiredClass("NSAutoreleasePool"); + AutoreleasePool = objc_getClass("NSAutoreleasePool"); if (Nil == AutoreleasePool) { useARCAutoreleasePool = YES; From 0d9deaa76f3c7db71b0999d9651ed66544f4bfa6 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Wed, 13 Jul 2016 16:47:56 +0100 Subject: [PATCH 03/18] Add (hopefully) Travis CI badge to the README. --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 1ddb22d..57e158f 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,8 @@ GNUstep Objective-C Runtime =========================== +[![Build Status](https://travis-ci.org/gnustep/libobjc2.svg?branch=master)](https://travis-ci.org/gnustep/libobjc2) + The GNUstep Objective-C runtime is designed as a drop-in replacement for the GCC runtime. It supports both a legacy and a modern ABI, allowing code compiled with old versions of GCC to be supported without requiring From 79cade0a685f485389c2c75a07c337f5923e0099 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 11:47:08 +0100 Subject: [PATCH 04/18] Trivial change to hopefully poke Travis into working. --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 57e158f..196bf53 100644 --- a/README.md +++ b/README.md @@ -467,3 +467,4 @@ be used directly. If it does not exist, ARC will implement its own `-_ARCCompatibleAutoreleasePool` then it must call `objc_autoreleasePoolPush()` and `objc_autoreleasePoolPop()` to manage autoreleased object storage and call `objc_autorelease()` in its `-addObject:` method. + From 46ce44bbf53a21452e07736b2b90c08ddc751e98 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 12:07:10 +0100 Subject: [PATCH 05/18] Enable Travis testing on OS X. --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index c6a7857..cccd87d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,6 @@ +os: + - linux + - osx language: cpp compiler: clang script: From d692a0ede4597670aed83d00df5b5b9f354bd94f Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 12:12:21 +0100 Subject: [PATCH 06/18] Remove some definitions that hide non-standard libc functions that we use. --- CMakeLists.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9041e03..16b42a7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,8 +13,6 @@ set(libobjc_VERSION 4.6) set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fexceptions") # Build configuration add_definitions( -DGNUSTEP -D__OBJC_RUNTIME_INTERNAL__=1) -# Probably not needed anymore? -add_definitions( -D_XOPEN_SOURCE=700 -D__BSD_VISIBLE=1 -D_BSD_SOURCE=1) set(libobjc_ASM_SRCS block_trampolines.S From 81ef7a98166cdae97336c5143fceb3eb5a661136 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 15:10:47 +0100 Subject: [PATCH 07/18] =?UTF-8?q?Don=E2=80=99t=20fail=20the=20test=20if=20?= =?UTF-8?q?we=20get=20more=20attributes=20than=20we=20expect.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attribute lists are not null-terminated, so this would previously have gone off into memory that it was not meant to access. --- Test/PropertyIntrospectionTest2_arc.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Test/PropertyIntrospectionTest2_arc.m b/Test/PropertyIntrospectionTest2_arc.m index 3ec266f..506c59d 100644 --- a/Test/PropertyIntrospectionTest2_arc.m +++ b/Test/PropertyIntrospectionTest2_arc.m @@ -281,7 +281,7 @@ static BOOL testPropertyForProperty_alt(objc_property_t p, attrsList = property_copyAttributeList(p, NULL); OPT_ASSERT(0 != attrsList); objc_property_attribute_t *ra; - for (attrsCount = 0, ra = attrsList; ra->name != NULL; attrsCount++, ra++) {} + for (attrsCount = 0, ra = attrsList; (ra->name != NULL) && (attrsCount < size); attrsCount++, ra++) {} OPT_ASSERT(attrsCount == size); free(attrsList); for (unsigned int index=0; index Date: Thu, 14 Jul 2016 15:11:22 +0100 Subject: [PATCH 08/18] =?UTF-8?q?Add=20missing=20properties=20and=20don?= =?UTF-8?q?=E2=80=99t=20make=20a=20runtime-specific=20test=20conditional?= =?UTF-8?q?=20on=20OS.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Test/PropertyIntrospectionTest2_arc.m | 39 +++++++++++++++------------ 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/Test/PropertyIntrospectionTest2_arc.m b/Test/PropertyIntrospectionTest2_arc.m index 506c59d..7b00063 100644 --- a/Test/PropertyIntrospectionTest2_arc.m +++ b/Test/PropertyIntrospectionTest2_arc.m @@ -404,25 +404,24 @@ static void testAddPropertyForClass(Class testClass) ATTR("R", ""), ATTR("V", "backingIvar")))); // The only concession to MacOS X is that we reorder the attributes string -#if __APPLE__ - testPropertyForProperty(class_getProperty(testClass, "addProperty5"), + if (!testPropertyForProperty_alt(class_getProperty(testClass, "addProperty5"), "addProperty5", "T@,D,W,C,&,R,VbackingIvar", ATTRS(ATTR("T", "@"), - ATTR("R", ""), - ATTR("&", ""), - ATTR("C", ""), - ATTR("W", ""), ATTR("D", ""), - ATTR("V", "backingIvar"))); -#else - testPropertyForProperty(class_getProperty(testClass, "addProperty5"), - "addProperty5", "T@,R,&,C,W,D,VbackingIvar", ATTRS(ATTR("T", "@"), - ATTR("R", ""), - ATTR("&", ""), - ATTR("C", ""), ATTR("W", ""), - ATTR("D", ""), - ATTR("V", "backingIvar"))); -#endif + ATTR("C", ""), + ATTR("&", ""), + ATTR("R", ""), + ATTR("V", "backingIvar")), NO)) + { + testPropertyForProperty(class_getProperty(testClass, "addProperty5"), + "addProperty5", "T@,R,&,C,W,D,VbackingIvar", ATTRS(ATTR("T", "@"), + ATTR("R", ""), + ATTR("&", ""), + ATTR("C", ""), + ATTR("W", ""), + ATTR("D", ""), + ATTR("V", "backingIvar"))); + } assert(class_addProperty(testClass, "replaceProperty", ATTRS(ATTR("T", "@")))); testPropertyForProperty(class_getProperty(testClass, "replaceProperty"), @@ -546,11 +545,13 @@ int main(void) ATTR("N", ""), ATTR("V", "intNonatomic"))); testProperty("idReadonlyCopyNonatomic", "T@,R,C,N,VidReadonlyCopyNonatomic", ATTRS(ATTR("T", "@"), + ATTR("C", ""), ATTR("R", ""), ATTR("N", ""), ATTR("V", "idReadonlyCopyNonatomic"))); testProperty("idReadonlyRetainNonatomic", "T@,R,&,N,VidReadonlyRetainNonatomic", ATTRS(ATTR("T", "@"), ATTR("R", ""), + ATTR("&", ""), ATTR("N", ""), ATTR("V", "idReadonlyRetainNonatomic"))); /** @@ -562,8 +563,9 @@ int main(void) ATTR("N", ""), ATTR("V", "idReadonlyWeakNonatomic")), NO)) { - testProperty("idReadonlyWeakNonatomic", "T@,R,N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), + testProperty("idReadonlyWeakNonatomic", "T@,R,W,N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), ATTR("R", ""), + ATTR("W", ""), ATTR("N", ""), ATTR("V", "idReadonlyWeakNonatomic"))); } @@ -622,15 +624,18 @@ int main(void) ATTR("N", ""))); testPropertyForProtocol(testProto, "idReadonlyCopyNonatomic", "T@,R,C,N", ATTRS(ATTR("T", "@"), ATTR("R", ""), + ATTR("C", ""), ATTR("N", ""))); testPropertyForProtocol(testProto, "idReadonlyRetainNonatomic", "T@,R,&,N", ATTRS(ATTR("T", "@"), ATTR("R", ""), + ATTR("&", ""), ATTR("N", ""))); /* * Again, different clang versions emit slightly different property declarations. */ if (!testPropertyForProtocol_alt(testProto, "idReadonlyWeakNonatomic", "T@,R,W,N", ATTRS(ATTR("T", "@"), ATTR("R", ""), + ATTR("W", ""), ATTR("N", "")), NO)) { From 2493137996c48f38f9700ee2c1d31c9484acebbe Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 15:12:02 +0100 Subject: [PATCH 09/18] Treat the property encoding string as canonical. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is more reliable than the bitfields (and is constructed from them if the compiler doesn’t fill in the string). --- properties.m | 137 ++++++++++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 77 deletions(-) diff --git a/properties.m b/properties.m index 36930a9..84fe614 100644 --- a/properties.m +++ b/properties.m @@ -1,6 +1,7 @@ #include "objc/runtime.h" #include "objc/objc-arc.h" #include +#include #include #include #include @@ -513,64 +514,62 @@ objc_property_attribute_t *property_copyAttributeList(objc_property_t property, attrs[count].value = types; count++; } - if (checkAttribute(property->attributes, OBJC_PR_readonly)) - { - attrs[count].name = "R"; - attrs[count].value = ""; - count++; - } - if (checkAttribute(property->attributes, OBJC_PR_copy)) - { - attrs[count].name = "C"; - attrs[count].value = ""; - count++; - } - if (checkAttribute(property->attributes, OBJC_PR_retain) || - checkAttribute(property->attributes2, OBJC_PR_strong)) + // If the compiler provides a type encoding string, then it's more + // informative than the bitfields and should be treated as canonical. If + // the compiler didn't provide a type encoding string, then this will + // create a best-effort one. + const char *attributes = property_getAttributes(property); + for (int i=strlen(types)+1 ; attributes[i] != 0 ; i++) { - attrs[count].name = "&"; - attrs[count].value = ""; - count++; - } - if (checkAttribute(property->attributes2, OBJC_PR_dynamic) && - !checkAttribute(property->attributes2, OBJC_PR_synthesized)) - { - attrs[count].name = "D"; - attrs[count].value = ""; - count++; - } - if (checkAttribute(property->attributes2, OBJC_PR_weak)) - { - attrs[count].name = "W"; - attrs[count].value = ""; - count++; - } - if ((property->attributes & OBJC_PR_nonatomic) == OBJC_PR_nonatomic) - { - attrs[count].name = "N"; + assert(count<12); + if (attributes[i] == ',') + { + // Comma is never the last character in the string, so this should + // never push us past the end. + i++; + } attrs[count].value = ""; + switch (attributes[i]) + { + case 'R': + attrs[count].name = "R"; + break; + case 'C': + attrs[count].name = "C"; + break; + case '&': + attrs[count].name = "&"; + break; + case 'D': + attrs[count].name = "D"; + break; + case 'W': + attrs[count].name = "W"; + break; + case 'N': + attrs[count].name = "N"; + break; + case 'G': + attrs[count].name = "G"; + attrs[count].value = property->getter_name; + i += strlen(attrs[count].value); + break; + case 'S': + attrs[count].name = "S"; + attrs[count].value = property->setter_name; + i += strlen(attrs[count].value); + break; + case 'V': + attrs[count].name = "V"; + attrs[count].value = attributes+i+1; + i += strlen(attributes+i)-1; + break; + default: + i++; + continue; + } count++; } - if ((property->attributes & OBJC_PR_getter) == OBJC_PR_getter) - { - attrs[count].name = "G"; - attrs[count].value = property->getter_name; - count++; - } - if ((property->attributes & OBJC_PR_setter) == OBJC_PR_setter) - { - attrs[count].name = "S"; - attrs[count].value = property->setter_name; - count++; - } - const char *name = property_getIVar(property); - if (name != NULL) - { - attrs[count].name = "V"; - attrs[count].value = name; - count++; - } - objc_property_attribute_t *propAttrs = calloc(sizeof(objc_property_attribute_t), count); memcpy(propAttrs, attrs, count * sizeof(objc_property_attribute_t)); if (NULL != outCount) @@ -695,6 +694,7 @@ char *property_copyAttributeValue(objc_property_t property, const char *attributeName) { if ((NULL == property) || (NULL == attributeName)) { return NULL; } + const char *attributes = property_getAttributes(property); switch (attributeName[0]) { case 'T': @@ -703,9 +703,13 @@ char *property_copyAttributeValue(objc_property_t property, return (NULL == types) ? NULL : strdup(types); } case 'D': + case 'R': + case 'W': + case 'C': + case '&': + case 'N': { - return checkAttribute(property->attributes2, OBJC_PR_dynamic) && - !checkAttribute(property->attributes2, OBJC_PR_synthesized) ? strdup("") : 0; + return strchr(attributes, attributeName[0]) ? strdup("") : 0; } case 'V': { @@ -719,27 +723,6 @@ char *property_copyAttributeValue(objc_property_t property, { return strdup(property->getter_name); } - case 'R': - { - return checkAttribute(property->attributes, OBJC_PR_readonly) ? strdup("") : 0; - } - case 'W': - { - return checkAttribute(property->attributes2, OBJC_PR_weak) ? strdup("") : 0; - } - case 'C': - { - return checkAttribute(property->attributes, OBJC_PR_copy) ? strdup("") : 0; - } - case '&': - { - return checkAttribute(property->attributes, OBJC_PR_retain) || - checkAttribute(property->attributes2, OBJC_PR_strong) ? strdup("") : 0; - } - case 'N': - { - return checkAttribute(property->attributes, OBJC_PR_nonatomic) ? strdup("") : 0; - } } return 0; } From 9a2f43ca7d579928279b5d854a19adfbe43d06d6 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 14 Jul 2016 14:43:59 +0000 Subject: [PATCH 10/18] Work around the fact that clang 3.4 and earlier emit broken metadata. This is mostly important because Travis-CI uses an archaic version of clang for the CI runs. --- Test/PropertyIntrospectionTest2_arc.m | 40 ++++++++++++++++++--------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/Test/PropertyIntrospectionTest2_arc.m b/Test/PropertyIntrospectionTest2_arc.m index 7b00063..7f38070 100644 --- a/Test/PropertyIntrospectionTest2_arc.m +++ b/Test/PropertyIntrospectionTest2_arc.m @@ -6,6 +6,20 @@ #pragma GCC diagnostic ignored "-Wobjc-property-no-attribute" +// Clang < 3 doesn't exist usefully, so we can skip tests for it. Clang 3.5 +// adds proper metadata for weak properties, earlier ones don't, so don't fail +// the tests because of known compiler bugs. +#ifndef __clang_minor__ +#define WEAK_ATTR ATTR("W", ""), +#define WEAK_STR "W," +#elif (__clang_major__ < 4) && (__clang_minor__ < 5) +#define WEAK_ATTR +#define WEAK_STR +#else +#define WEAK_ATTR ATTR("W", ""), +#define WEAK_STR "W," +#endif + enum FooManChu { FOO, MAN, CHU }; struct YorkshireTeaStruct { int pot; signed char lady; }; typedef struct YorkshireTeaStruct YorkshireTeaStructType; @@ -384,41 +398,41 @@ static void testAddPropertyForClass(Class testClass) ATTR("R", ""), ATTR("&", ""), ATTR("C", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("D", ""), ATTR("V", "backingIvar")))); testPropertyForProperty(class_getProperty(testClass, "addProperty4"), - "addProperty4", "T@,R,&,C,W,D,VbackingIvar", ATTRS(ATTR("T", "@"), + "addProperty4", "T@,R,&,C," WEAK_STR "D,VbackingIvar", ATTRS(ATTR("T", "@"), ATTR("R", ""), ATTR("&", ""), ATTR("C", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("D", ""), ATTR("V", "backingIvar"))); assert(class_addProperty(testClass, "addProperty5", ATTRS(ATTR("T", "@"), ATTR("D", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("C", ""), ATTR("&", ""), ATTR("R", ""), ATTR("V", "backingIvar")))); // The only concession to MacOS X is that we reorder the attributes string if (!testPropertyForProperty_alt(class_getProperty(testClass, "addProperty5"), - "addProperty5", "T@,D,W,C,&,R,VbackingIvar", ATTRS(ATTR("T", "@"), + "addProperty5", "T@,D," WEAK_STR "C,&,R,VbackingIvar", ATTRS(ATTR("T", "@"), ATTR("D", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("C", ""), ATTR("&", ""), ATTR("R", ""), ATTR("V", "backingIvar")), NO)) { testPropertyForProperty(class_getProperty(testClass, "addProperty5"), - "addProperty5", "T@,R,&,C,W,D,VbackingIvar", ATTRS(ATTR("T", "@"), + "addProperty5", "T@,R,&,C," WEAK_STR "D,VbackingIvar", ATTRS(ATTR("T", "@"), ATTR("R", ""), ATTR("&", ""), ATTR("C", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("D", ""), ATTR("V", "backingIvar"))); } @@ -558,14 +572,14 @@ int main(void) * The weak attribute was not present for earlier versions of clang, so we test * for all variants that the compiler may produce. */ - if (!testProperty_alt("idReadonlyWeakNonatomic", "T@,R,W,N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), + if (!testProperty_alt("idReadonlyWeakNonatomic", "T@,R," WEAK_STR "N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), ATTR("R", ""), ATTR("N", ""), ATTR("V", "idReadonlyWeakNonatomic")), NO)) { - testProperty("idReadonlyWeakNonatomic", "T@,R,W,N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), + testProperty("idReadonlyWeakNonatomic", "T@,R," WEAK_STR "N,VidReadonlyWeakNonatomic", ATTRS(ATTR("T", "@"), ATTR("R", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("N", ""), ATTR("V", "idReadonlyWeakNonatomic"))); } @@ -633,9 +647,9 @@ int main(void) /* * Again, different clang versions emit slightly different property declarations. */ - if (!testPropertyForProtocol_alt(testProto, "idReadonlyWeakNonatomic", "T@,R,W,N", ATTRS(ATTR("T", "@"), + if (!testPropertyForProtocol_alt(testProto, "idReadonlyWeakNonatomic", "T@,R," WEAK_STR "N", ATTRS(ATTR("T", "@"), ATTR("R", ""), - ATTR("W", ""), + WEAK_ATTR ATTR("N", "")), NO)) { From 6b285936bed090d06f3bfc66d0886c12dc088753 Mon Sep 17 00:00:00 2001 From: Niels Grewe Date: Mon, 24 Oct 2016 10:49:02 +0100 Subject: [PATCH 11/18] Avoid a deadlock As described in gnustep/libobjc2#20 --- dtable.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/dtable.c b/dtable.c index b537e9c..4e39562 100644 --- a/dtable.c +++ b/dtable.c @@ -824,6 +824,13 @@ PRIVATE void objc_send_initialize(id object) objc_send_initialize((id)class->super_class); } + // Lock the runtime while we're creating dtables and before we acquire any + // other locks. This prevents a lock-order reversal when + // dtable_for_class is called from something holding the runtime lock while + // we're still holding the initialize lock. We should ensure that we never + // acquire the runtime lock after acquiring the initialize lock. + LOCK_RUNTIME(); + // Superclass +initialize might possibly send a message to this class, in // which case this method would be called again. See NSObject and // NSAutoreleasePool +initialize interaction in GNUstep. @@ -831,19 +838,17 @@ PRIVATE void objc_send_initialize(id object) { // We know that initialization has started because the flag is set. // Check that it's finished by grabbing the class lock. This will be - // released once the class has been fully initialized + // released once the class has been fully initialized. The runtime + // lock needs to be released first to prevent a deadlock between the + // runtime lock and the class-specific lock. + UNLOCK_RUNTIME(); + objc_sync_enter((id)meta); objc_sync_exit((id)meta); assert(dtable_for_class(class) != uninstalled_dtable); return; } - // Lock the runtime while we're creating dtables and before we acquire any - // other locks. This prevents a lock-order reversal when - // dtable_for_class is called from something holding the runtime lock while - // we're still holding the initialize lock. We should ensure that we never - // acquire the runtime lock after acquiring the initialize lock. - LOCK_RUNTIME(); LOCK_OBJECT_FOR_SCOPE((id)meta); LOCK(&initialize_lock); if (objc_test_class_flag(class, objc_class_flag_initialized)) From 6df23377a02f187e0df8cb359175fb274b34aae1 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Tue, 25 Oct 2016 15:43:57 +0000 Subject: [PATCH 12/18] Forcibly realign instance variables to take into account the padding from the reference count. --- Test/CMakeLists.txt | 2 +- Test/alignTest.m | 17 +++++++++++++++++ ivar.c | 25 ++++++++++--------------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index 6425136..16156ff 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -5,7 +5,7 @@ # List of single-file tests. set(TESTS - #alignTest.m + alignTest.m AllocatePair.m AssociatedObject.m AssociatedObject2.m diff --git a/Test/alignTest.m b/Test/alignTest.m index 92e2164..b0f8deb 100644 --- a/Test/alignTest.m +++ b/Test/alignTest.m @@ -36,7 +36,24 @@ typedef double __attribute__((vector_size(32))) v4d; } @end +typedef int v4si __attribute__ ((vector_size (16))); +@interface Foo : Test +{ + v4si var; +} +- (void)check; +@end +@implementation Foo +- (void)check +{ + size_t addr = (size_t)&var; + fprintf(stderr, "self: %p Addr: %p\n", self, &var); + assert(addr % 16 == 0); +} +@end + int main(void) { [[Vector alloc] permute]; + [[Foo new] check]; } diff --git a/ivar.c b/ivar.c index e1053e7..35a89bb 100644 --- a/ivar.c +++ b/ivar.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -56,30 +57,24 @@ PRIVATE void objc_compute_ivar_offsets(Class class) long ivar_size = (i+1 == class->ivars->count) ? (class_size - ivar->offset) : ivar->offset - class->ivars->ivar_list[i+1].offset; -#if 0 // We only need to do the realignment for things that are // bigger than a pointer, and we don't need to do it in GC mode // where we don't add any extra padding. if (!isGCEnabled && (ivar_size > sizeof(void*))) { - long fudge = (ivar_start +ivar->offset + sizeof(void*)) % 16; + long offset = ivar_start + ivar->offset + sizeof(intptr_t); + // For now, assume that nothing needs to be more than 16-byte aligned. + // This is not correct for AVX vectors, but we probably + // can't do anything about that for now (as malloc is only + // giving us 16-byte aligned memory) + long fudge = 16 - (offset % 16); if (fudge != 0) { - // If this is the first ivar in the class, then - // we can eat some of the padding that the compiler - // added... - if ((i == 0) && (ivar->offset > 0) && ((ivar_start + sizeof(void*) %16) == 0)) - { - ivar->offset = 0; - } - else - { - ivar_start += fudge; - class->instance_size += fudge; - } + ivar->offset += fudge; + class->instance_size += fudge; + assert((ivar_start + ivar->offset + sizeof(intptr_t)) % 16 == 0); } } -#endif ivar->offset += ivar_start; /* If we're using the new ABI then we also set up the faster ivar * offset variables. From 8c79eb836c6002203dd04aa26d53ae73abe73702 Mon Sep 17 00:00:00 2001 From: Niels Grewe Date: Tue, 8 Nov 2016 12:31:23 +0000 Subject: [PATCH 13/18] Test case for misaligned ivars. --- Test/alignTest.m | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Test/alignTest.m b/Test/alignTest.m index b0f8deb..b33f760 100644 --- a/Test/alignTest.m +++ b/Test/alignTest.m @@ -52,8 +52,32 @@ typedef int v4si __attribute__ ((vector_size (16))); } @end +#if __has_attribute(objc_root_class) +__attribute__((objc_root_class)) +#endif +@interface StringLikeTest +{ + Class isa; + char* c_string; + int len; +} +@end + +@implementation StringLikeTest ++ (Class)class +{ + return self; +} +@end + int main(void) { [[Vector alloc] permute]; [[Foo new] check]; + + Ivar v_isa = class_getInstanceVariable([StringLikeTest class], "isa"); + Ivar v_c_string = class_getInstanceVariable([StringLikeTest class], "c_string"); + Ivar v_len = class_getInstanceVariable([StringLikeTest class], "len"); + assert(ivar_getOffset(v_isa) < ivar_getOffset(v_c_string)); + assert(ivar_getOffset(v_c_string) < ivar_getOffset(v_len)); } From 165f1e83bfff3784599fa31799b7bd4b418ee62f Mon Sep 17 00:00:00 2001 From: Niels Grewe Date: Tue, 8 Nov 2016 14:40:59 +0100 Subject: [PATCH 14/18] Additional asserts Assert that the offset of `isa` is 0 and the offset of `c_string` is `sizeof(Class)` --- Test/alignTest.m | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Test/alignTest.m b/Test/alignTest.m index b33f760..aa636e1 100644 --- a/Test/alignTest.m +++ b/Test/alignTest.m @@ -78,6 +78,10 @@ int main(void) Ivar v_isa = class_getInstanceVariable([StringLikeTest class], "isa"); Ivar v_c_string = class_getInstanceVariable([StringLikeTest class], "c_string"); Ivar v_len = class_getInstanceVariable([StringLikeTest class], "len"); - assert(ivar_getOffset(v_isa) < ivar_getOffset(v_c_string)); - assert(ivar_getOffset(v_c_string) < ivar_getOffset(v_len)); + ptrdiff_t o_isa = ivar_getOffset(v_isa); + ptrdiff_t o_c_string = ivar_getOffset(v_c_string); + assert(o_isa == 0); + assert(o_c_string == sizeof(Class)); + assert(o_isa < o_c_string); + assert(o_c_string < ivar_getOffset(v_len)); } From 5725aa74ecded06dd10b1c2566d38f850543ee85 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Mon, 28 Nov 2016 14:03:44 +0000 Subject: [PATCH 15/18] Fix the computation of the size when realigning ivars. Fixes #25, #26 --- ivar.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ivar.c b/ivar.c index 35a89bb..9006b89 100644 --- a/ivar.c +++ b/ivar.c @@ -56,7 +56,8 @@ PRIVATE void objc_compute_ivar_offsets(Class class) // then we will need to ensure that we are properly aligned again. long ivar_size = (i+1 == class->ivars->count) ? (class_size - ivar->offset) - : ivar->offset - class->ivars->ivar_list[i+1].offset; + : class->ivars->ivar_list[i+1].offset - ivar->offset ; + assert(ivar_size > 0); // We only need to do the realignment for things that are // bigger than a pointer, and we don't need to do it in GC mode // where we don't add any extra padding. From 695282e5d7fde596af56a6a84f920637a62d9cb4 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Mon, 28 Nov 2016 14:05:19 +0000 Subject: [PATCH 16/18] Don't conditionally run some cheap code that becomes a no-ops in the case when we weren't running it. --- ivar.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/ivar.c b/ivar.c index 9006b89..d2670b5 100644 --- a/ivar.c +++ b/ivar.c @@ -69,12 +69,9 @@ PRIVATE void objc_compute_ivar_offsets(Class class) // can't do anything about that for now (as malloc is only // giving us 16-byte aligned memory) long fudge = 16 - (offset % 16); - if (fudge != 0) - { - ivar->offset += fudge; - class->instance_size += fudge; - assert((ivar_start + ivar->offset + sizeof(intptr_t)) % 16 == 0); - } + ivar->offset += fudge; + class->instance_size += fudge; + assert((ivar_start + ivar->offset + sizeof(intptr_t)) % 16 == 0); } ivar->offset += ivar_start; /* If we're using the new ABI then we also set up the faster ivar From 4fd27a066fea9e10aabde4c91786805a133f454d Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Mon, 12 Dec 2016 16:43:47 +0000 Subject: [PATCH 17/18] When moving ivars to increase their alignment, make sure later ivars are also moved. Fixes #27 --- Test/CMakeLists.txt | 1 + Test/IVarOverlap.m | 30 ++++++++++++++++++++++++++++++ ivar.c | 3 +++ 3 files changed, 34 insertions(+) create mode 100644 Test/IVarOverlap.m diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index 16156ff..99e0ff7 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -26,6 +26,7 @@ set(TESTS WeakBlock_arc.m WeakReferences_arc.m ivar_arc.m + IVarOverlap.m objc_msgSend.m msgInterpose.m NilException.m diff --git a/Test/IVarOverlap.m b/Test/IVarOverlap.m new file mode 100644 index 0000000..bc07de6 --- /dev/null +++ b/Test/IVarOverlap.m @@ -0,0 +1,30 @@ +#import +#import "../objc/objc.h" +#import "../objc/runtime.h" +#import "../objc/Object.h" +#include "Test.h" + +#import +#import + +@interface Dummy : Test +{ + id objOne; + struct stat statBuf; + BOOL flagOne; +} +@end + +@implementation Dummy +- (void)test +{ + assert((char*)&statBuf+sizeof(struct stat) <= (char*)&flagOne); +} +@end + + +int main(int argc, char *argv[]) +{ + [[Dummy new] test]; + return 0; +} diff --git a/ivar.c b/ivar.c index d2670b5..d6556f9 100644 --- a/ivar.c +++ b/ivar.c @@ -47,6 +47,7 @@ PRIVATE void objc_compute_ivar_offsets(Class class) */ if (class->ivars) { + long cumulative_fudge = 0; for (i = 0 ; i < class->ivars->count ; i++) { struct objc_ivar *ivar = &class->ivars->ivar_list[i]; @@ -58,6 +59,7 @@ PRIVATE void objc_compute_ivar_offsets(Class class) ? (class_size - ivar->offset) : class->ivars->ivar_list[i+1].offset - ivar->offset ; assert(ivar_size > 0); + ivar->offset += cumulative_fudge; // We only need to do the realignment for things that are // bigger than a pointer, and we don't need to do it in GC mode // where we don't add any extra padding. @@ -71,6 +73,7 @@ PRIVATE void objc_compute_ivar_offsets(Class class) long fudge = 16 - (offset % 16); ivar->offset += fudge; class->instance_size += fudge; + cumulative_fudge += fudge; assert((ivar_start + ivar->offset + sizeof(intptr_t)) % 16 == 0); } ivar->offset += ivar_start; From d1eb9ad91e45af19d16c3ef9bb742eb9df822c5a Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Tue, 10 Jan 2017 14:43:10 +0000 Subject: [PATCH 18/18] Ensure that asserts in the tests are still run, even in a release build. Fixes #29 --- Test/CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index 99e0ff7..77a8fe6 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -59,8 +59,8 @@ endfunction(addtest_flags) foreach(TEST_SOURCE ${TESTS}) get_filename_component(TEST ${TEST_SOURCE} NAME_WE) - addtest_flags(${TEST} "-O0" ${TEST_SOURCE}) - addtest_flags("${TEST}_optimised" "-O3" ${TEST_SOURCE}) + addtest_flags(${TEST} "-O0 -UNDEBUG" ${TEST_SOURCE}) + addtest_flags("${TEST}_optimised" "-O3 -UNDEBUG" ${TEST_SOURCE}) endforeach() # Tests that are more than a single file.