From 2e45da40c407e2ca6703c2128ad8fa0b8b9c4612 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Sat, 20 Jan 2018 16:00:26 -0800 Subject: [PATCH 1/9] fix flipped sense on ASSERT() --- visibility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/visibility.h b/visibility.h index 86c379b..d98d052 100644 --- a/visibility.h +++ b/visibility.h @@ -17,7 +17,7 @@ # define ASSERT(x) assert(x) #else # define UNREACHABLE(x) __builtin_unreachable() -# define ASSERT(x) do { if (x) __builtin_unreachable(); } while(0) +# define ASSERT(x) do { if (!(x)) __builtin_unreachable(); } while(0) #endif #define LIKELY(x) __builtin_expect(x, 1) From a0eec52bb88f039b5f8a14603a1609caf3609691 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Fri, 17 Feb 2017 14:42:02 -0800 Subject: [PATCH 2/9] fix a mismanagement of the hash table that could lead to data loss This commit fixes a data loss bug in our hopscotch table implementation. Removing values from the table can result in other values becoming disconnected and lost. Let A, B, and C be values that all hash to cell 0. Assume the hopscotch distance factor H = 2. 0 1 2 +-----+-----+-----+ | | | | +-----+-----+-----+ After adding A 0 1 2 +-----+-----+-----+ | A | | | +-----+-----+-----+ | +-Neighbors = After adding B 0 1 2 +-----+-----+-----+ | A | B | | +-----+-----+-----+ | +-Neighbors = 1 After adding C 0 1 2 +-----+-----+-----+ | A | B | C | +-----+-----+-----+ | +-Neighbors = 1, 2 If we then remove B, 0 1 2 +-----+-----+-----+ | A | [X] | C | +-----+-----+-----+ | +-Neighbors = 1, 2 * It is replaced with a placeholder [X]. * A's neighbor table is not updated to reflect the loss. If we then remove A, 0 1 2 +-----+-----+-----+ | [X] | [X] | [C] | +-----+-----+-----+ | +-Neighbors = 2 * The table is rebalanced to promote A's lowest neighbor to the primary cell position. * C from cell 2 remains cell 0's neighbor. The bug manifests if [X] the placeholder value passes the null check set out in MAP_TABLE_VALUE_NULL; that is, the placeholder is "effectively null". Looking up the key that matches C will first evaluate its base cell, the one that collided with the key in the first place. Since that is placeholder [X], and [X] is "effectively null", the lookup stops. C is never retrieved from the hash table. --- The expedient solution to this bug is to update cell 0's neighbors when B is first removed, effectively skipping the hole: If we remove B as above, 0 1 2 +-----+-----+-----+ | A | [X] | C | +-----+-----+-----+ | +-Neighbors = 2 <<< HERE but clear the neighbor bit for cell 1, the promotion that happens when A is later removed places C in cell 0. 0 1 2 +-----+-----+-----+ | C | [X] | [X] | +-----+-----+-----+ | +-Neighbors = --- hash_table.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/hash_table.h b/hash_table.h index aaae772..efa5cf6 100644 --- a/hash_table.h +++ b/hash_table.h @@ -392,6 +392,22 @@ static void PREFIX(_remove)(PREFIX(_table) *table, void *key) MAP_LOCK(); PREFIX(_table_cell) cell = PREFIX(_table_get_cell)(table, key); if (NULL == cell) { return; } + + uint32_t hash = MAP_TABLE_HASH_KEY(key); + PREFIX(_table_cell) baseCell = PREFIX(_table_lookup)(table, hash); + if (baseCell && baseCell <= cell && cell - baseCell <= 32) + { + uint32_t jump = 1 << (cell - baseCell - 1); + if ((baseCell->secondMaps & jump)) + { + // If we are removing a cell stored adjacent to its base due to hash + // collision, we have to clear the base cell's neighbor bit. + // Otherwise, a later remove can move the new placeholder value to the head + // which will cause further chained lookups to fail. + baseCell->secondMaps &= ~jump; + } + } + // If the cell contains a value, set it to the placeholder and shuffle up // everything if (0 == cell->secondMaps) From 7d5a0ff85e71a6f2fe64e3b45eda27fe5dc4ddc6 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Tue, 7 Jun 2016 17:14:17 -0700 Subject: [PATCH 3/9] fix objc_resolve_class_links to actually rescan the unresolved list It looks like this was the initial intent of 4ea82e1, but as implemented it would still only scan the unresolved class list once. Since unresolved_class_list represents the head and classes are pushed onto the head, any classes added to the resolution list after resolution started would be skipped. --- class_table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/class_table.c b/class_table.c index fa9a876..ec81255 100644 --- a/class_table.c +++ b/class_table.c @@ -258,10 +258,10 @@ PRIVATE BOOL objc_resolve_class(Class cls) PRIVATE void objc_resolve_class_links(void) { LOCK_RUNTIME_FOR_SCOPE(); - Class class = unresolved_class_list; BOOL resolvedClass; do { + Class class = unresolved_class_list; resolvedClass = NO; while ((Nil != class)) { From 21ad183e3e0dbe55882f5fb0ea716e8f35742578 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 11 Jan 2016 17:43:25 -0800 Subject: [PATCH 4/9] Wrap some headers in extern C for C++ compat. --- objc/encoding.h | 8 ++++++++ objc/hooks.h | 8 +++++++- objc/objc-arc.h | 10 ++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/objc/encoding.h b/objc/encoding.h index 4c4ff53..ceccd59 100644 --- a/objc/encoding.h +++ b/objc/encoding.h @@ -5,6 +5,10 @@ #ifndef __LIBOBJC_ENCODING_H_INCLUDED__ #define __LIBOBJC_ENCODING_H_INCLUDED__ +#ifdef __cplusplus +extern "C" { +#endif + const char *objc_skip_type_qualifiers (const char *type); const char *objc_skip_typespec(const char *type); @@ -71,4 +75,8 @@ void objc_layout_structure_get_info (struct objc_struct_layout *layout, #define _F_ONEWAY 0x10 #define _F_GCINVISIBLE 0x20 +#ifdef __cplusplus +} +#endif + #endif // __LIBOBJC_ENCODING_H_INCLUDED__ diff --git a/objc/hooks.h b/objc/hooks.h index 42c67ed..8b7e045 100644 --- a/objc/hooks.h +++ b/objc/hooks.h @@ -2,6 +2,10 @@ #pragma clang system_header #endif +#ifdef __cplusplus +extern "C" { +#endif + /** * This file includes all of the hooks that can be used to alter the behaviour * of the runtime. @@ -102,4 +106,6 @@ typedef IMP (*objc_tracing_hook)(id, SEL, IMP, int, void*); */ int objc_registerTracingHook(SEL, objc_tracing_hook); - +#ifdef __cplusplus +} +#endif diff --git a/objc/objc-arc.h b/objc/objc-arc.h index 32ecb7a..14f7d00 100644 --- a/objc/objc-arc.h +++ b/objc/objc-arc.h @@ -4,6 +4,11 @@ #ifndef __OBJC_ARC_INCLUDED__ #define __OBJC_ARC_INCLUDED__ + +#ifdef __cplusplus +extern "C" { +#endif + /** * Autoreleases the argument. Equivalent to [obj autorelease]. */ @@ -138,5 +143,10 @@ unsigned long objc_arc_autorelease_count_np(void); * this thread. */ unsigned long objc_arc_autorelease_count_for_object_np(id); + +#ifdef __cplusplus +} +#endif + #endif // __OBJC_ARC_INCLUDED__ From e3b069cedcae09eb19d28d34f6cee97c0c4943bc Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 4 Jan 2016 10:25:03 -0800 Subject: [PATCH 5/9] Use the old association policy to determine whether to release an object This commit has been augmented to address the comments in https://github.com/Microsoft/libobjc2/commit/df7f9061670cce994730d1720150adc --- associate.m | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/associate.m b/associate.m index 37b9c26..d3ab8a9 100644 --- a/associate.m +++ b/associate.m @@ -166,12 +166,17 @@ static void setReference(struct reference_list *list, lock = lock_for_pointer(r); lock_spinlock(lock); } - r->policy = policy; - id old = r->object; - r->object = obj; - if (OBJC_ASSOCIATION_ASSIGN != r->policy) + @try { - objc_release(old); + if (OBJC_ASSOCIATION_ASSIGN != r->policy) + { + objc_release(r->object); + } + } + @finally + { + r->policy = policy; + r->object = obj; } if (needLock) { From 45f6572379deab4d8766bcedf922cfb606247f11 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Thu, 22 Feb 2018 17:51:54 -0800 Subject: [PATCH 6/9] Add a test for a0eec52 --- Test/CMakeLists.txt | 1 + Test/hash_table_delete.c | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 Test/hash_table_delete.c diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index 7db5183..97b66b9 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -33,6 +33,7 @@ set(TESTS MethodArguments.m zeroSizedIVar.m exchange.m + hash_table_delete.c ) # Function for adding a test. This takes the name of the test and the list of diff --git a/Test/hash_table_delete.c b/Test/hash_table_delete.c new file mode 100644 index 0000000..9b92461 --- /dev/null +++ b/Test/hash_table_delete.c @@ -0,0 +1,64 @@ +#include +#include + +struct test_struct { + uint32_t key; +}; + +struct test_struct null_placeholder = {0}; + +static int test_compare(uint32_t value, const struct test_struct test) { + return value == test.key; +} + +// force hash collisions +static uint32_t test_key_hash(const void *ptr) { + return ((uint32_t)ptr)>>2; +} + +static uint32_t test_value_hash(const struct test_struct test) { + return test.key>>2; +} + +static int test_is_null(const struct test_struct test) { + return test.key == 0; +} + +#define MAP_TABLE_NAME test +#define MAP_TABLE_COMPARE_FUNCTION test_compare +#define MAP_TABLE_VALUE_TYPE struct test_struct +#define MAP_TABLE_VALUE_NULL test_is_null +#define MAP_TABLE_HASH_KEY test_key_hash +#define MAP_TABLE_HASH_VALUE test_value_hash +#define MAP_TABLE_VALUE_PLACEHOLDER null_placeholder +#define MAP_TABLE_ACCESS_BY_REFERENCE 1 +#define MAP_TABLE_SINGLE_THREAD 1 +#define MAP_TABLE_NO_LOCK 1 + +#include "../hash_table.h" + +int main(int argc, char *argv[]) +{ + test_table *testTable; + test_initialize(&testTable, 128); + + struct test_struct one, two, three; + one.key = 1; + two.key = 2; + three.key = 3; + + test_insert(testTable, one); + test_insert(testTable, two); + test_insert(testTable, three); + + test_remove(testTable, 2); + test_remove(testTable, 1); + + struct test_struct *pthree = test_table_get(testTable, 3); + if (!pthree) { + fprintf(stderr, "failed to find value (key=3) inserted into hash table\n"); + return 1; + } + + return 0; +} From 7539d7e042411244194bde76d48e32de79662fc0 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 23 Feb 2018 09:22:57 +0000 Subject: [PATCH 7/9] Fix warnings in test. --- Test/hash_table_delete.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Test/hash_table_delete.c b/Test/hash_table_delete.c index 9b92461..ad7af89 100644 --- a/Test/hash_table_delete.c +++ b/Test/hash_table_delete.c @@ -2,18 +2,18 @@ #include struct test_struct { - uint32_t key; + uintptr_t key; }; struct test_struct null_placeholder = {0}; -static int test_compare(uint32_t value, const struct test_struct test) { - return value == test.key; +static int test_compare(const void *key, const struct test_struct test) { + return (uintptr_t)key == test.key; } // force hash collisions static uint32_t test_key_hash(const void *ptr) { - return ((uint32_t)ptr)>>2; + return ((uint32_t)(uintptr_t)ptr)>>2; } static uint32_t test_value_hash(const struct test_struct test) { @@ -51,10 +51,10 @@ int main(int argc, char *argv[]) test_insert(testTable, two); test_insert(testTable, three); - test_remove(testTable, 2); - test_remove(testTable, 1); + test_remove(testTable, (void*)2); + test_remove(testTable, (void*)1); - struct test_struct *pthree = test_table_get(testTable, 3); + struct test_struct *pthree = test_table_get(testTable, (void*)3); if (!pthree) { fprintf(stderr, "failed to find value (key=3) inserted into hash table\n"); return 1; From 3c036b3f4f830cebc53b4e5c4bef47f2fb1aae39 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 23 Feb 2018 09:23:05 +0000 Subject: [PATCH 8/9] Use nullptr for nil in C++11. --- objc/runtime.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/objc/runtime.h b/objc/runtime.h index 3092f72..12b49d5 100644 --- a/objc/runtime.h +++ b/objc/runtime.h @@ -198,7 +198,11 @@ typedef struct #ifdef __GNUC # define _OBJC_NULL_PTR __null #elif defined(__cplusplus) -# define _OBJC_NULL_PTR 0 +# if __has_feature(cxx_nullptr) +# define _OBJC_NULL_PTR nullptr +# else +# define _OBJC_NULL_PTR 0 +# endif #else # define _OBJC_NULL_PTR ((void*)0) #endif From 81f7b16af5f9e84d22d85e7a947a40594fd2c785 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 28 Feb 2018 17:14:19 -0800 Subject: [PATCH 9/9] Add a test for e3b069c (association policy) --- Test/AssociatedObject.m | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Test/AssociatedObject.m b/Test/AssociatedObject.m index cd9464e..e41da6f 100644 --- a/Test/AssociatedObject.m +++ b/Test/AssociatedObject.m @@ -19,8 +19,21 @@ int main(void) @autoreleasepool { Associated *object = [Associated new]; Test *holder = [[Test new] autorelease]; - objc_setAssociatedObject(object, &objc_setAssociatedObjectKey, holder, OBJC_ASSOCIATION_RETAIN); + objc_setAssociatedObject(holder, &objc_setAssociatedObjectKey, object, OBJC_ASSOCIATION_RETAIN); [object release]; - } + assert(!deallocCalled); + } + // dealloc should be called when holder is released during pool drain + assert(deallocCalled); + + deallocCalled = NO; + + Associated *object = [Associated new]; + Test *holder = [Test new]; + objc_setAssociatedObject(holder, &objc_setAssociatedObjectKey, object, OBJC_ASSOCIATION_RETAIN); + [object release]; // commuted into associated object storage + objc_setAssociatedObject(holder, &objc_setAssociatedObjectKey, nil, OBJC_ASSOCIATION_ASSIGN); + [holder release]; + assert(deallocCalled); }