From a7d1f46b3ae8fe14e417c3440c9dccb55241ab83 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 24 Mar 2023 10:43:09 +0100 Subject: [PATCH 1/9] Remove WeakMap entries whose key is only reacheable through the entry value --- Zend/tests/weakrefs/gh10043-001.phpt | 41 +++++++ Zend/tests/weakrefs/gh10043-002.phpt | 44 +++++++ Zend/tests/weakrefs/gh10043-003.phpt | 50 ++++++++ Zend/tests/weakrefs/gh10043-004.phpt | 50 ++++++++ Zend/tests/weakrefs/gh10043-005.phpt | 49 ++++++++ Zend/tests/weakrefs/gh10043-006.phpt | 41 +++++++ Zend/tests/weakrefs/gh10043-007.phpt | 33 +++++ Zend/tests/weakrefs/gh10043-008.phpt | 30 +++++ Zend/zend_gc.c | 173 +++++++++++++++++++++++++-- Zend/zend_gc.h | 13 ++ Zend/zend_refcounted.h | 1 + Zend/zend_types.h | 1 + Zend/zend_weakrefs.c | 73 ++++++++++- Zend/zend_weakrefs.h | 3 + 14 files changed, 588 insertions(+), 14 deletions(-) create mode 100644 Zend/tests/weakrefs/gh10043-001.phpt create mode 100644 Zend/tests/weakrefs/gh10043-002.phpt create mode 100644 Zend/tests/weakrefs/gh10043-003.phpt create mode 100644 Zend/tests/weakrefs/gh10043-004.phpt create mode 100644 Zend/tests/weakrefs/gh10043-005.phpt create mode 100644 Zend/tests/weakrefs/gh10043-006.phpt create mode 100644 Zend/tests/weakrefs/gh10043-007.phpt create mode 100644 Zend/tests/weakrefs/gh10043-008.phpt diff --git a/Zend/tests/weakrefs/gh10043-001.phpt b/Zend/tests/weakrefs/gh10043-001.phpt new file mode 100644 index 0000000000000..a9caaa96239fe --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-001.phpt @@ -0,0 +1,41 @@ +--TEST-- +Self-referencing map entry GC - 001 +--FILE-- + + array(2) { + ["key"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + } +} +object(WeakMap)#%d (0) { +} diff --git a/Zend/tests/weakrefs/gh10043-002.phpt b/Zend/tests/weakrefs/gh10043-002.phpt new file mode 100644 index 0000000000000..bf6067bc5ea68 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-002.phpt @@ -0,0 +1,44 @@ +--TEST-- +Self-referencing map entry GC - 002 +--FILE-- + + array(2) { + ["key"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + array(1) { + [0]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + } + } +} +object(WeakMap)#1 (0) { +} diff --git a/Zend/tests/weakrefs/gh10043-003.phpt b/Zend/tests/weakrefs/gh10043-003.phpt new file mode 100644 index 0000000000000..017b91fe1fc15 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-003.phpt @@ -0,0 +1,50 @@ +--TEST-- +Self-referencing map entry GC - 003 +--FILE-- +get()); + +gc_collect_cycles(); + +// $obj is first in the root buffer +$obj = null; +$map = null; +gc_collect_cycles(); + +var_dump($ref->get()); + +--EXPECTF-- +object(WeakMap)#%d (1) { + [0]=> + array(2) { + ["key"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + array(2) { + [0]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + [1]=> + *RECURSION* + } + } +} +NULL diff --git a/Zend/tests/weakrefs/gh10043-004.phpt b/Zend/tests/weakrefs/gh10043-004.phpt new file mode 100644 index 0000000000000..9f3f264ad6437 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-004.phpt @@ -0,0 +1,50 @@ +--TEST-- +Self-referencing map entry GC - 004 +--FILE-- +get()); + +gc_collect_cycles(); + +// $map is first in the root buffer +$map = null; +$obj = null; +gc_collect_cycles(); + +var_dump($ref->get()); + +--EXPECTF-- +object(WeakMap)#%d (1) { + [0]=> + array(2) { + ["key"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + array(2) { + [0]=> + *RECURSION* + [1]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + } + } +} +NULL diff --git a/Zend/tests/weakrefs/gh10043-005.phpt b/Zend/tests/weakrefs/gh10043-005.phpt new file mode 100644 index 0000000000000..d495502625181 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-005.phpt @@ -0,0 +1,49 @@ +--TEST-- +Self-referencing map entry GC - 005 +--FILE-- + + array(2) { + ["key"]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + array(1) { + [0]=> + object(Value)#%d (1) { + ["value"]=> + string(1) "a" + } + } + } +} +object(WeakMap)#1 (0) { +} diff --git a/Zend/tests/weakrefs/gh10043-006.phpt b/Zend/tests/weakrefs/gh10043-006.phpt new file mode 100644 index 0000000000000..ea91a7d9076d9 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-006.phpt @@ -0,0 +1,41 @@ +--TEST-- +Self-referencing map entry GC - 006 +--FILE-- + + array(2) { + ["key"]=> + object(Value)#2 (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + object(Value)#2 (1) { + ["value"]=> + string(1) "a" + } + } +} diff --git a/Zend/tests/weakrefs/gh10043-007.phpt b/Zend/tests/weakrefs/gh10043-007.phpt new file mode 100644 index 0000000000000..ca25a73e2c4e0 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-007.phpt @@ -0,0 +1,33 @@ +--TEST-- +Self-referencing map entry GC - 007 +--FILE-- +name."\n"; + } +} + +$container = new Canary('container'); +$canary = new Canary('canary'); +$container->canary = $canary; + +$map = new \WeakMap(); +$map[$canary] = $container; + +echo 1; +unset($container, $canary); +gc_collect_cycles(); +echo 2; + +--EXPECT-- +1container +canary +2 diff --git a/Zend/tests/weakrefs/gh10043-008.phpt b/Zend/tests/weakrefs/gh10043-008.phpt new file mode 100644 index 0000000000000..ffbf1fbe44085 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-008.phpt @@ -0,0 +1,30 @@ +--TEST-- +Self-referencing map entry GC - 008 +--FILE-- +name."\n"; + } +} + +$canary = new Canary('canary'); + +$map = new \WeakMap(); +$map[$canary] = $canary; + +echo 1; +unset($canary); +gc_collect_cycles(); +echo 2; + +--EXPECT-- +1canary +2 diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index 6f00786b66928..0c5fcf883c86e 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -69,6 +69,7 @@ #include "zend.h" #include "zend_API.h" #include "zend_fibers.h" +#include "zend_weakrefs.h" #ifndef GC_BENCH # define GC_BENCH 0 @@ -186,6 +187,16 @@ /* GC flags */ #define GC_HAS_DESTRUCTORS (1<<0) +/* Weak maps */ +#define GC_SET_REFERENCE_SCANNED(zv) do { \ + zval *_z = (zv); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT); \ +} while (0) +#define GC_UNSET_REFERENCE_SCANNED(zv) do { \ + zval *_z = (zv); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT); \ +} while (0) + /* unused buffers */ #define GC_HAS_UNUSED() \ (GC_G(unused) != GC_INVALID) @@ -268,6 +279,13 @@ struct _gc_stack { zend_refcounted *data[GC_STACK_SEGMENT_SIZE]; }; +typedef struct _gc_stack_handle gc_stack_handle; + +struct _gc_stack_handle { + gc_stack *stack; + size_t top; +}; + #define GC_STACK_DCL(init) \ gc_stack *_stack = init; \ size_t _top = 0; @@ -689,7 +707,7 @@ ZEND_API void ZEND_FASTCALL gc_remove_from_buffer(zend_refcounted *ref) gc_remove_from_roots(root); } -static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) +static void gc_scan_black(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) { HashTable *ht; Bucket *p; @@ -705,6 +723,26 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) zval *table; int len; + if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { + zend_weakmap_get_object_values_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *value = (zval*) Z_PTR_P(zv); + if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + ref = Z_COUNTED_P(value); + GC_ADDREF(ref); + if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { + GC_REF_SET_BLACK(ref); + GC_STACK_PUSH(ref); + } + gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*)value); + } + zv++; + } + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -829,6 +867,32 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) zval *table; int len; + if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { + zend_weakmap_get_object_entries_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n-=2) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *value = (zval*) Z_PTR_P(zv); + zval *weakmap = zv+1; + ZEND_ASSERT(Z_REFCOUNTED_P(weakmap)); + /* Skip reference if already followed through weakmap */ + if (!GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_GREY)) { + if (Z_REFCOUNTED_P(value)) { + ZEND_ASSERT(!GC_REFERENCE_SCANNED(value)); + GC_SET_REFERENCE_SCANNED(value); + ref = Z_COUNTED_P(value); + GC_DELREF(ref); + if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { + GC_REF_SET_COLOR(ref, GC_GREY); + GC_STACK_PUSH(ref); + } + } + } + zv+=2; + } + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -990,7 +1054,7 @@ static void gc_mark_roots(gc_stack *stack) } } -static void gc_scan(zend_refcounted *ref, gc_stack *stack) +static void gc_scan(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) { HashTable *ht; Bucket *p; @@ -1011,7 +1075,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) } /* Split stack and reuse the tail */ _stack->next->prev = NULL; - gc_scan_black(ref, _stack->next); + gc_scan_black(ref, _stack->next, defer_stack_handle); _stack->next->prev = _stack; } goto next; @@ -1023,6 +1087,24 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) zval *table; int len; + if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { + zend_weakmap_get_object_values_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *value = (zval*) Z_PTR_P(zv); + if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + ref = Z_COUNTED_P(value); + if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { + GC_REF_SET_COLOR(ref, GC_WHITE); + GC_STACK_PUSH(ref); + } + } + zv++; + } + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -1070,7 +1152,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) } else if (GC_TYPE(ref) == IS_ARRAY) { ht = (HashTable *)ref; ZEND_ASSERT(ht != &EG(symbol_table)); - + handle_ht: n = ht->nNumUsed; if (HT_IS_PACKED(ht)) { @@ -1125,20 +1207,31 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) } } -static void gc_scan_roots(gc_stack *stack) +static void gc_scan_roots(gc_stack *stack, gc_stack *defer_stack) { gc_root_buffer *current = GC_IDX2PTR(GC_FIRST_ROOT); gc_root_buffer *last = GC_IDX2PTR(GC_G(first_unused)); + gc_stack_handle defer_stack_handle = { + .stack = defer_stack, + }; while (current != last) { if (GC_IS_ROOT(current->ref)) { if (GC_REF_CHECK_COLOR(current->ref, GC_GREY)) { GC_REF_SET_COLOR(current->ref, GC_WHITE); - gc_scan(current->ref, stack); + gc_scan(current->ref, stack, &defer_stack_handle); } } current++; } + + while (1) { + zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); + if (!value) { + break; + } + GC_UNSET_REFERENCE_SCANNED(value); + } } static void gc_add_garbage(zend_refcounted *ref) @@ -1166,7 +1259,7 @@ static void gc_add_garbage(zend_refcounted *ref) GC_G(num_roots)++; } -static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *stack) +static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *stack, gc_stack_handle *defer_stack_handle) { int count = 0; HashTable *ht; @@ -1197,6 +1290,27 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta || obj->ce->destructor != NULL)) { *flags |= GC_HAS_DESTRUCTORS; } + + if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { + zend_weakmap_get_object_values_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *value = (zval*) Z_PTR_P(zv); + if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + ref = Z_COUNTED_P(value); + GC_ADDREF(ref); + if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { + GC_REF_SET_BLACK(ref); + GC_STACK_PUSH(ref); + } + gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*)value); + } + zv++; + } + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -1219,7 +1333,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta } } -handle_zvals: +handle_zvals: for (; n != 0; n--) { if (Z_REFCOUNTED_P(zv)) { ref = Z_COUNTED_P(zv); @@ -1309,13 +1423,16 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta return count; } -static int gc_collect_roots(uint32_t *flags, gc_stack *stack) +static int gc_collect_roots(uint32_t *flags, gc_stack *stack, gc_stack *defer_stack) { uint32_t idx, end; zend_refcounted *ref; int count = 0; gc_root_buffer *current = GC_IDX2PTR(GC_FIRST_ROOT); gc_root_buffer *last = GC_IDX2PTR(GC_G(first_unused)); + gc_stack_handle defer_stack_handle = { + .stack = defer_stack, + }; /* remove non-garbage from the list */ while (current != last) { @@ -1341,11 +1458,19 @@ static int gc_collect_roots(uint32_t *flags, gc_stack *stack) current->ref = GC_MAKE_GARBAGE(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { GC_REF_SET_BLACK(ref); - count += gc_collect_white(ref, flags, stack); + count += gc_collect_white(ref, flags, stack, &defer_stack_handle); } idx++; } + while (1) { + zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); + if (!value) { + break; + } + GC_UNSET_REFERENCE_SCANNED(value); + } + return count; } @@ -1384,6 +1509,21 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe int len; zval *table; + if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { + zend_weakmap_get_object_values_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *value = (zval*) Z_PTR_P(zv); + if (Z_OPT_REFCOUNTED_P(value)) { + ref = Z_COUNTED_P(value); + GC_STACK_PUSH(ref); + } + zv++; + } + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -1460,7 +1600,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe if (ref) { goto tail_call; } - + return count; } @@ -1481,10 +1621,15 @@ ZEND_API int zend_gc_collect_cycles(void) uint32_t gc_flags = 0; uint32_t idx, end; gc_stack stack; + gc_stack defer_stack; stack.prev = NULL; stack.next = NULL; + defer_stack.prev = NULL; + defer_stack.next = NULL; + + if (GC_G(gc_active)) { return 0; } @@ -1496,15 +1641,16 @@ ZEND_API int zend_gc_collect_cycles(void) GC_TRACE("Marking roots"); gc_mark_roots(&stack); GC_TRACE("Scanning roots"); - gc_scan_roots(&stack); + gc_scan_roots(&stack, &defer_stack); GC_TRACE("Collecting roots"); - count = gc_collect_roots(&gc_flags, &stack); + count = gc_collect_roots(&gc_flags, &stack, &defer_stack); if (!GC_G(num_roots)) { /* nothing to free */ GC_TRACE("Nothing to free"); gc_stack_free(&stack); + gc_stack_free(&defer_stack); GC_G(gc_active) = 0; goto finish; } @@ -1596,6 +1742,7 @@ ZEND_API int zend_gc_collect_cycles(void) } gc_stack_free(&stack); + gc_stack_free(&defer_stack); /* Destroy zvals. The root buffer may be reallocated. */ GC_TRACE("Destroying zvals"); diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h index 762f3180637e3..f15b264472535 100644 --- a/Zend/zend_gc.h +++ b/Zend/zend_gc.h @@ -74,6 +74,10 @@ size_t zend_gc_globals_size(void); ((GC_TYPE_INFO(ref) & \ (GC_INFO_MASK | (GC_NOT_COLLECTABLE << GC_FLAGS_SHIFT))) == 0) +#define Z_REFERENCE_SCANNED (1<<0) + +#define GC_REFERENCE_SCANNED(zv) (Z_TYPE_INFO_P((zv)) & (Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT)) + static zend_always_inline void gc_check_possible_root(zend_refcounted *ref) { if (EXPECTED(GC_TYPE_INFO(ref) == GC_REFERENCE)) { @@ -122,6 +126,15 @@ static zend_always_inline void zend_get_gc_buffer_add_obj( gc_buffer->cur++; } +static zend_always_inline void zend_get_gc_buffer_add_ptr( + zend_get_gc_buffer *gc_buffer, void *ptr) { + if (UNEXPECTED(gc_buffer->cur == gc_buffer->end)) { + zend_get_gc_buffer_grow(gc_buffer); + } + ZVAL_PTR(gc_buffer->cur, ptr); + gc_buffer->cur++; +} + static zend_always_inline void zend_get_gc_buffer_use( zend_get_gc_buffer *gc_buffer, zval **table, int *n) { *table = gc_buffer->start; diff --git a/Zend/zend_refcounted.h b/Zend/zend_refcounted.h index e4ffc99568530..5e9c5adaeb0e1 100644 --- a/Zend/zend_refcounted.h +++ b/Zend/zend_refcounted.h @@ -70,6 +70,7 @@ typedef struct _zend_refcounted_h { union { uint32_t type_info; } u; + // bool followed; } zend_refcounted_h; typedef struct _zend_refcounted { diff --git a/Zend/zend_types.h b/Zend/zend_types.h index faed815401948..d1c4f08488560 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -567,6 +567,7 @@ static zend_always_inline uint8_t zval_get_type(const zval* pz) { #define Z_TYPE_FLAGS_MASK 0xff00 #define Z_TYPE_FLAGS_SHIFT 8 +#define Z_TYPE_INFO_EXTRA_SHIFT 16 #define GC_TYPE_MASK 0x0000000f #define GC_FLAGS_MASK 0x000003f0 diff --git a/Zend/zend_weakrefs.c b/Zend/zend_weakrefs.c index db95b13a7254a..346c2f353c757 100644 --- a/Zend/zend_weakrefs.c +++ b/Zend/zend_weakrefs.c @@ -17,6 +17,7 @@ #include "zend.h" #include "zend_interfaces.h" #include "zend_objects_API.h" +#include "zend_types.h" #include "zend_weakrefs.h" #include "zend_weakrefs_arginfo.h" @@ -466,12 +467,82 @@ static HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n) zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); zval *val; ZEND_HASH_MAP_FOREACH_VAL(&wm->ht, val) { - zend_get_gc_buffer_add_zval(gc_buffer, val); + if (!GC_REFERENCE_SCANNED(val)) { + zend_get_gc_buffer_add_zval(gc_buffer, val); + } } ZEND_HASH_FOREACH_END(); zend_get_gc_buffer_use(gc_buffer, table, n); return NULL; } +HashTable *zend_weakmap_get_object_entries_gc(zend_object *object, zval **table, int *n) +{ + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + const zend_ulong obj_key = zend_object_to_weakref_key(object); + void *tagged_ptr = zend_hash_index_find_ptr(&EG(weakrefs), obj_key); +#if ZEND_DEBUG + ZEND_ASSERT(tagged_ptr && "Tracking of the IS_OBJ_WEAKLY_REFERENCE flag should be precise"); +#endif + void *ptr = ZEND_WEAKREF_GET_PTR(tagged_ptr); + uintptr_t tag = ZEND_WEAKREF_GET_TAG(tagged_ptr); + + if (tag == ZEND_WEAKREF_TAG_HT) { + HashTable *ht = ptr; + ZEND_HASH_MAP_FOREACH_PTR(ht, tagged_ptr) { + if (ZEND_WEAKREF_GET_TAG(tagged_ptr) == ZEND_WEAKREF_TAG_MAP) { + zend_weakmap *wm = (zend_weakmap*) ZEND_WEAKREF_GET_PTR(tagged_ptr); + zval *zv = zend_hash_index_find(&wm->ht, obj_key); + ZEND_ASSERT(zv); + zend_get_gc_buffer_add_ptr(gc_buffer, zv); + zend_get_gc_buffer_add_obj(gc_buffer, &wm->std); + } + } ZEND_HASH_FOREACH_END(); + } else if (tag == ZEND_WEAKREF_TAG_MAP) { + zend_weakmap *wm = (zend_weakmap*) ptr; + zval *zv = zend_hash_index_find(&wm->ht, obj_key); + ZEND_ASSERT(zv); + zend_get_gc_buffer_add_ptr(gc_buffer, zv); + zend_get_gc_buffer_add_obj(gc_buffer, &wm->std); + } + + zend_get_gc_buffer_use(gc_buffer, table, n); + + return NULL; +} + +HashTable *zend_weakmap_get_object_values_gc(zend_object *object, zval **table, int *n) +{ + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + const zend_ulong obj_key = zend_object_to_weakref_key(object); + void *tagged_ptr = zend_hash_index_find_ptr(&EG(weakrefs), obj_key); +#if ZEND_DEBUG + ZEND_ASSERT(tagged_ptr && "Tracking of the IS_OBJ_WEAKLY_REFERENCE flag should be precise"); +#endif + void *ptr = ZEND_WEAKREF_GET_PTR(tagged_ptr); + uintptr_t tag = ZEND_WEAKREF_GET_TAG(tagged_ptr); + + if (tag == ZEND_WEAKREF_TAG_HT) { + HashTable *ht = ptr; + ZEND_HASH_MAP_FOREACH_PTR(ht, tagged_ptr) { + if (ZEND_WEAKREF_GET_TAG(tagged_ptr) == ZEND_WEAKREF_TAG_MAP) { + zend_weakmap *wm = (zend_weakmap*) ZEND_WEAKREF_GET_PTR(tagged_ptr); + zval *zv = zend_hash_index_find(&wm->ht, obj_key); + ZEND_ASSERT(zv); + zend_get_gc_buffer_add_ptr(gc_buffer, zv); + } + } ZEND_HASH_FOREACH_END(); + } else if (tag == ZEND_WEAKREF_TAG_MAP) { + zend_weakmap *wm = (zend_weakmap*) ptr; + zval *zv = zend_hash_index_find(&wm->ht, obj_key); + ZEND_ASSERT(zv); + zend_get_gc_buffer_add_ptr(gc_buffer, zv); + } + + zend_get_gc_buffer_use(gc_buffer, table, n); + + return NULL; +} + static zend_object *zend_weakmap_clone_obj(zend_object *old_object) { zend_object *new_object = zend_weakmap_create_object(zend_ce_weakmap); diff --git a/Zend/zend_weakrefs.h b/Zend/zend_weakrefs.h index 506e2e9d40c5c..84c179f60e3db 100644 --- a/Zend/zend_weakrefs.h +++ b/Zend/zend_weakrefs.h @@ -62,6 +62,9 @@ static zend_always_inline zend_object *zend_weakref_key_to_object(zend_ulong key return (zend_object *) (((uintptr_t) key) << ZEND_MM_ALIGNMENT_LOG2); } +HashTable *zend_weakmap_get_object_entries_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_object_values_gc(zend_object *object, zval **table, int *n); + END_EXTERN_C() #endif From 4f3620b8d3f590f7a5d2e178b4e532e077782e79 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sun, 26 Mar 2023 22:32:50 +0200 Subject: [PATCH 2/9] Fixes --- Zend/tests/weakrefs/gh10043-009.phpt | 28 ++++++++++++ Zend/tests/weakrefs/gh10043-010.phpt | 32 ++++++++++++++ Zend/zend_gc.c | 66 ++++++++++++---------------- 3 files changed, 87 insertions(+), 39 deletions(-) create mode 100644 Zend/tests/weakrefs/gh10043-009.phpt create mode 100644 Zend/tests/weakrefs/gh10043-010.phpt diff --git a/Zend/tests/weakrefs/gh10043-009.phpt b/Zend/tests/weakrefs/gh10043-009.phpt new file mode 100644 index 0000000000000..6218d189806f7 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-009.phpt @@ -0,0 +1,28 @@ +--TEST-- +Self-referencing map entry GC - 009 +--FILE-- +get()); +?> +--EXPECT-- +NULL diff --git a/Zend/tests/weakrefs/gh10043-010.phpt b/Zend/tests/weakrefs/gh10043-010.phpt new file mode 100644 index 0000000000000..d532ca71f16e0 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-010.phpt @@ -0,0 +1,32 @@ +--TEST-- +Self-referencing map entry GC - 011 +--FILE-- + +==DONE== +--EXPECT-- +==DONE== diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index 0c5fcf883c86e..790f5e9eef0e7 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -707,7 +707,7 @@ ZEND_API void ZEND_FASTCALL gc_remove_from_buffer(zend_refcounted *ref) gc_remove_from_roots(root); } -static void gc_scan_black(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) +static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) { HashTable *ht; Bucket *p; @@ -737,7 +737,6 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack, gc_stack_handle GC_REF_SET_BLACK(ref); GC_STACK_PUSH(ref); } - gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*)value); } zv++; } @@ -849,7 +848,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack, gc_stack_handle } } -static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) +static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) { HashTable *ht; Bucket *p; @@ -881,6 +880,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) if (Z_REFCOUNTED_P(value)) { ZEND_ASSERT(!GC_REFERENCE_SCANNED(value)); GC_SET_REFERENCE_SCANNED(value); + gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*) value); ref = Z_COUNTED_P(value); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1035,7 +1035,7 @@ static void gc_compact(void) } } -static void gc_mark_roots(gc_stack *stack) +static void gc_mark_roots(gc_stack *stack, gc_stack_handle *defer_stack_handle) { gc_root_buffer *current, *last; @@ -1047,14 +1047,14 @@ static void gc_mark_roots(gc_stack *stack) if (GC_IS_ROOT(current->ref)) { if (GC_REF_CHECK_COLOR(current->ref, GC_PURPLE)) { GC_REF_SET_COLOR(current->ref, GC_GREY); - gc_mark_grey(current->ref, stack); + gc_mark_grey(current->ref, stack, defer_stack_handle); } } current++; } } -static void gc_scan(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) +static void gc_scan(zend_refcounted *ref, gc_stack *stack) { HashTable *ht; Bucket *p; @@ -1075,7 +1075,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defe } /* Split stack and reuse the tail */ _stack->next->prev = NULL; - gc_scan_black(ref, _stack->next, defer_stack_handle); + gc_scan_black(ref, _stack->next); _stack->next->prev = _stack; } goto next; @@ -1207,31 +1207,20 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defe } } -static void gc_scan_roots(gc_stack *stack, gc_stack *defer_stack) +static void gc_scan_roots(gc_stack *stack) { gc_root_buffer *current = GC_IDX2PTR(GC_FIRST_ROOT); gc_root_buffer *last = GC_IDX2PTR(GC_G(first_unused)); - gc_stack_handle defer_stack_handle = { - .stack = defer_stack, - }; while (current != last) { if (GC_IS_ROOT(current->ref)) { if (GC_REF_CHECK_COLOR(current->ref, GC_GREY)) { GC_REF_SET_COLOR(current->ref, GC_WHITE); - gc_scan(current->ref, stack, &defer_stack_handle); + gc_scan(current->ref, stack); } } current++; } - - while (1) { - zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); - if (!value) { - break; - } - GC_UNSET_REFERENCE_SCANNED(value); - } } static void gc_add_garbage(zend_refcounted *ref) @@ -1259,7 +1248,7 @@ static void gc_add_garbage(zend_refcounted *ref) GC_G(num_roots)++; } -static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *stack, gc_stack_handle *defer_stack_handle) +static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *stack) { int count = 0; HashTable *ht; @@ -1298,6 +1287,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *value = (zval*) Z_PTR_P(zv); + // if (!GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_BLACK)) { if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { ref = Z_COUNTED_P(value); GC_ADDREF(ref); @@ -1305,7 +1295,6 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta GC_REF_SET_BLACK(ref); GC_STACK_PUSH(ref); } - gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*)value); } zv++; } @@ -1423,16 +1412,13 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta return count; } -static int gc_collect_roots(uint32_t *flags, gc_stack *stack, gc_stack *defer_stack) +static int gc_collect_roots(uint32_t *flags, gc_stack *stack) { uint32_t idx, end; zend_refcounted *ref; int count = 0; gc_root_buffer *current = GC_IDX2PTR(GC_FIRST_ROOT); gc_root_buffer *last = GC_IDX2PTR(GC_G(first_unused)); - gc_stack_handle defer_stack_handle = { - .stack = defer_stack, - }; /* remove non-garbage from the list */ while (current != last) { @@ -1458,19 +1444,11 @@ static int gc_collect_roots(uint32_t *flags, gc_stack *stack, gc_stack *defer_st current->ref = GC_MAKE_GARBAGE(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { GC_REF_SET_BLACK(ref); - count += gc_collect_white(ref, flags, stack, &defer_stack_handle); + count += gc_collect_white(ref, flags, stack); } idx++; } - while (1) { - zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); - if (!value) { - break; - } - GC_UNSET_REFERENCE_SCANNED(value); - } - return count; } @@ -1622,6 +1600,9 @@ ZEND_API int zend_gc_collect_cycles(void) uint32_t idx, end; gc_stack stack; gc_stack defer_stack; + gc_stack_handle defer_stack_handle = { + .stack = &defer_stack, + }; stack.prev = NULL; stack.next = NULL; @@ -1629,7 +1610,6 @@ ZEND_API int zend_gc_collect_cycles(void) defer_stack.prev = NULL; defer_stack.next = NULL; - if (GC_G(gc_active)) { return 0; } @@ -1639,12 +1619,20 @@ ZEND_API int zend_gc_collect_cycles(void) GC_G(gc_active) = 1; GC_TRACE("Marking roots"); - gc_mark_roots(&stack); + gc_mark_roots(&stack, &defer_stack_handle); GC_TRACE("Scanning roots"); - gc_scan_roots(&stack, &defer_stack); + gc_scan_roots(&stack); GC_TRACE("Collecting roots"); - count = gc_collect_roots(&gc_flags, &stack, &defer_stack); + count = gc_collect_roots(&gc_flags, &stack); + + while (1) { + zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); + if (!value) { + break; + } + GC_UNSET_REFERENCE_SCANNED(value); + } if (!GC_G(num_roots)) { /* nothing to free */ From 62a7f4d65cf4ed73c040e09eec8d1496f775bcdd Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 31 Mar 2023 11:01:37 +0200 Subject: [PATCH 3/9] Commented code --- Zend/zend_gc.c | 1 - Zend/zend_refcounted.h | 1 - 2 files changed, 2 deletions(-) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index 790f5e9eef0e7..90e46995399f3 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -1287,7 +1287,6 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *value = (zval*) Z_PTR_P(zv); - // if (!GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_BLACK)) { if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { ref = Z_COUNTED_P(value); GC_ADDREF(ref); diff --git a/Zend/zend_refcounted.h b/Zend/zend_refcounted.h index 5e9c5adaeb0e1..e4ffc99568530 100644 --- a/Zend/zend_refcounted.h +++ b/Zend/zend_refcounted.h @@ -70,7 +70,6 @@ typedef struct _zend_refcounted_h { union { uint32_t type_info; } u; - // bool followed; } zend_refcounted_h; typedef struct _zend_refcounted { From da378dc080766d86790cb06d52f40ee33e0fa93c Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 31 Mar 2023 12:13:28 +0200 Subject: [PATCH 4/9] Naming --- Zend/zend_gc.c | 20 ++++++++++---------- Zend/zend_gc.h | 4 ++-- Zend/zend_weakrefs.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index 90e46995399f3..93802442eccea 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -188,13 +188,13 @@ #define GC_HAS_DESTRUCTORS (1<<0) /* Weak maps */ -#define GC_SET_REFERENCE_SCANNED(zv) do { \ +#define GC_SET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ - Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) -#define GC_UNSET_REFERENCE_SCANNED(zv) do { \ +#define GC_UNSET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ - Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) /* unused buffers */ @@ -730,7 +730,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *value = (zval*) Z_PTR_P(zv); - if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { ref = Z_COUNTED_P(value); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -878,8 +878,8 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack, gc_stack_handle /* Skip reference if already followed through weakmap */ if (!GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_GREY)) { if (Z_REFCOUNTED_P(value)) { - ZEND_ASSERT(!GC_REFERENCE_SCANNED(value)); - GC_SET_REFERENCE_SCANNED(value); + ZEND_ASSERT(!GC_FROM_WEAKMAP_KEY(value)); + GC_SET_FROM_WEAKMAP_KEY(value); gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*) value); ref = Z_COUNTED_P(value); GC_DELREF(ref); @@ -1094,7 +1094,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *value = (zval*) Z_PTR_P(zv); - if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { ref = Z_COUNTED_P(value); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1287,7 +1287,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *value = (zval*) Z_PTR_P(zv); - if (GC_REFERENCE_SCANNED(value) && Z_OPT_REFCOUNTED_P(value)) { + if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { ref = Z_COUNTED_P(value); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1630,7 +1630,7 @@ ZEND_API int zend_gc_collect_cycles(void) if (!value) { break; } - GC_UNSET_REFERENCE_SCANNED(value); + GC_UNSET_FROM_WEAKMAP_KEY(value); } if (!GC_G(num_roots)) { diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h index f15b264472535..c1ba9848113a9 100644 --- a/Zend/zend_gc.h +++ b/Zend/zend_gc.h @@ -74,9 +74,9 @@ size_t zend_gc_globals_size(void); ((GC_TYPE_INFO(ref) & \ (GC_INFO_MASK | (GC_NOT_COLLECTABLE << GC_FLAGS_SHIFT))) == 0) -#define Z_REFERENCE_SCANNED (1<<0) +#define Z_FROM_WEAKMAP_KEY (1<<0) -#define GC_REFERENCE_SCANNED(zv) (Z_TYPE_INFO_P((zv)) & (Z_REFERENCE_SCANNED << Z_TYPE_INFO_EXTRA_SHIFT)) +#define GC_FROM_WEAKMAP_KEY(zv) (Z_TYPE_INFO_P((zv)) & (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT)) static zend_always_inline void gc_check_possible_root(zend_refcounted *ref) { diff --git a/Zend/zend_weakrefs.c b/Zend/zend_weakrefs.c index 346c2f353c757..b19c887f897ad 100644 --- a/Zend/zend_weakrefs.c +++ b/Zend/zend_weakrefs.c @@ -467,7 +467,7 @@ static HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n) zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); zval *val; ZEND_HASH_MAP_FOREACH_VAL(&wm->ht, val) { - if (!GC_REFERENCE_SCANNED(val)) { + if (!GC_FROM_WEAKMAP_KEY(val)) { zend_get_gc_buffer_add_zval(gc_buffer, val); } } ZEND_HASH_FOREACH_END(); From 9e04035a51328775daa1702383579bc09a1a39f1 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 19 May 2023 13:42:51 +0200 Subject: [PATCH 5/9] Comment --- Zend/zend_gc.c | 7 +++++-- Zend/zend_gc.h | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index 93802442eccea..ac307b04e7dc7 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -188,11 +188,14 @@ #define GC_HAS_DESTRUCTORS (1<<0) /* Weak maps */ -#define GC_SET_FROM_WEAKMAP_KEY(zv) do { \ +/* The WeakMap entry zv is first reachable by following the virtual reference + * from the a WeakMap key to the entry, and should not be scanned again through + * WeakMap entry enumeration */ +#define GC_SET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) -#define GC_UNSET_FROM_WEAKMAP_KEY(zv) do { \ +#define GC_UNSET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h index c1ba9848113a9..0fde35db97350 100644 --- a/Zend/zend_gc.h +++ b/Zend/zend_gc.h @@ -76,6 +76,9 @@ size_t zend_gc_globals_size(void); #define Z_FROM_WEAKMAP_KEY (1<<0) +/* The WeakMap entry zv is first reachable by following the virtual reference + * from the a WeakMap key to the entry, and should not be scanned again through + * WeakMap entry enumeration */ #define GC_FROM_WEAKMAP_KEY(zv) (Z_TYPE_INFO_P((zv)) & (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT)) static zend_always_inline void gc_check_possible_root(zend_refcounted *ref) From 3e95e1530e6fb2301d64c961da76f68e1f0c1152 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sun, 18 Jun 2023 13:31:26 +0200 Subject: [PATCH 6/9] Fix flaw --- Zend/tests/weakrefs/gh10043-010.phpt | 2 +- Zend/tests/weakrefs/gh10043-011.phpt | 24 +++ Zend/tests/weakrefs/gh10043-012.phpt | 33 +++ Zend/tests/weakrefs/gh10043-014.phpt | 41 ++++ Zend/tests/weakrefs/gh10043-015.phpt | 24 +++ Zend/tests/weakrefs/gh10043-016.phpt | 45 +++++ Zend/zend_gc.c | 288 +++++++++++++++++++++------ Zend/zend_gc.h | 7 - Zend/zend_weakrefs.c | 35 +++- Zend/zend_weakrefs.h | 9 +- 10 files changed, 429 insertions(+), 79 deletions(-) create mode 100644 Zend/tests/weakrefs/gh10043-011.phpt create mode 100644 Zend/tests/weakrefs/gh10043-012.phpt create mode 100644 Zend/tests/weakrefs/gh10043-014.phpt create mode 100644 Zend/tests/weakrefs/gh10043-015.phpt create mode 100644 Zend/tests/weakrefs/gh10043-016.phpt diff --git a/Zend/tests/weakrefs/gh10043-010.phpt b/Zend/tests/weakrefs/gh10043-010.phpt index d532ca71f16e0..03b4b76cd72f5 100644 --- a/Zend/tests/weakrefs/gh10043-010.phpt +++ b/Zend/tests/weakrefs/gh10043-010.phpt @@ -1,5 +1,5 @@ --TEST-- -Self-referencing map entry GC - 011 +Self-referencing map entry GC - 010 --FILE-- k = $k; +$m[$k] = $v; + +$m2 = $m; +unset($m2, $k, $v); + +gc_collect_cycles(); + +var_dump($m); + +--EXPECT-- +object(WeakMap)#1 (0) { +} diff --git a/Zend/tests/weakrefs/gh10043-012.phpt b/Zend/tests/weakrefs/gh10043-012.phpt new file mode 100644 index 0000000000000..951b609a27064 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-012.phpt @@ -0,0 +1,33 @@ +--TEST-- +Self-referencing map entry GC - 012 +--FILE-- +get()); +?> +--EXPECT-- +NULL diff --git a/Zend/tests/weakrefs/gh10043-014.phpt b/Zend/tests/weakrefs/gh10043-014.phpt new file mode 100644 index 0000000000000..4099427d42add --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-014.phpt @@ -0,0 +1,41 @@ +--TEST-- +Self-referencing map entry GC - 014 +--FILE-- + +--EXPECT-- +object(WeakMap)#1 (1) { + [0]=> + array(2) { + ["key"]=> + object(Value)#2 (1) { + ["value"]=> + string(1) "a" + } + ["value"]=> + object(Value)#2 (1) { + ["value"]=> + string(1) "a" + } + } +} diff --git a/Zend/tests/weakrefs/gh10043-015.phpt b/Zend/tests/weakrefs/gh10043-015.phpt new file mode 100644 index 0000000000000..9a1acc6533628 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-015.phpt @@ -0,0 +1,24 @@ +--TEST-- +Self-referencing map entry GC - 015 +--FILE-- +get()); + +?> +--EXPECT-- +NULL diff --git a/Zend/tests/weakrefs/gh10043-016.phpt b/Zend/tests/weakrefs/gh10043-016.phpt new file mode 100644 index 0000000000000..88e934da47e20 --- /dev/null +++ b/Zend/tests/weakrefs/gh10043-016.phpt @@ -0,0 +1,45 @@ +--TEST-- +Self-referencing map entry GC - 016 +--FILE-- + +--EXPECT-- +object(WeakMap)#1 (2) { + [0]=> + array(2) { + ["key"]=> + object(K1)#2 (0) { + } + ["value"]=> + array(2) { + [0]=> + object(K1)#2 (0) { + } + [1]=> + *RECURSION* + } + } + [1]=> + array(2) { + ["key"]=> + object(K2)#3 (0) { + } + ["value"]=> + object(K2)#3 (0) { + } + } +} diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index ac307b04e7dc7..ba61b0be652c9 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -188,18 +188,39 @@ #define GC_HAS_DESTRUCTORS (1<<0) /* Weak maps */ -/* The WeakMap entry zv is first reachable by following the virtual reference - * from the a WeakMap key to the entry, and should not be scanned again through - * WeakMap entry enumeration */ +#define Z_FROM_WEAKMAP_KEY (1<<0) +#define Z_FROM_WEAKMAP (1<<1) + +/* The WeakMap entry zv is reachable from roots by following the virtual + * reference from the a WeakMap key to the entry */ +#define GC_FROM_WEAKMAP_KEY(zv) \ + (Z_TYPE_INFO_P((zv)) & (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT)) + #define GC_SET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) + #define GC_UNSET_FROM_WEAKMAP_KEY(zv) do { \ zval *_z = (zv); \ Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT); \ } while (0) +/* The WeakMap entry zv is reachable from roots by following the reference from + * the WeakMap */ +#define GC_FROM_WEAKMAP(zv) \ + (Z_TYPE_INFO_P((zv)) & (Z_FROM_WEAKMAP << Z_TYPE_INFO_EXTRA_SHIFT)) + +#define GC_SET_FROM_WEAKMAP(zv) do { \ + zval *_z = (zv); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) | (Z_FROM_WEAKMAP << Z_TYPE_INFO_EXTRA_SHIFT); \ +} while (0) + +#define GC_UNSET_FROM_WEAKMAP(zv) do { \ + zval *_z = (zv); \ + Z_TYPE_INFO_P(_z) = Z_TYPE_INFO_P(_z) & ~(Z_FROM_WEAKMAP << Z_TYPE_INFO_EXTRA_SHIFT); \ +} while (0) + /* unused buffers */ #define GC_HAS_UNUSED() \ (GC_G(unused) != GC_INVALID) @@ -681,6 +702,39 @@ ZEND_API void ZEND_FASTCALL gc_possible_root(zend_refcounted *ref) GC_BENCH_PEAK(root_buf_peak, root_buf_length); } +static void ZEND_FASTCALL gc_extra_root(zend_refcounted *ref) +{ + uint32_t idx; + gc_root_buffer *newRoot; + + if (EXPECTED(GC_HAS_UNUSED())) { + idx = GC_FETCH_UNUSED(); + } else if (EXPECTED(GC_HAS_NEXT_UNUSED_UNDER_THRESHOLD())) { + idx = GC_FETCH_NEXT_UNUSED(); + } else { + gc_grow_root_buffer(); + if (UNEXPECTED(!GC_HAS_NEXT_UNUSED())) { + /* TODO: can this really happen? */ + return; + } + idx = GC_FETCH_NEXT_UNUSED(); + } + + ZEND_ASSERT(GC_TYPE(ref) == IS_ARRAY || GC_TYPE(ref) == IS_OBJECT); + ZEND_ASSERT(GC_REF_ADDRESS(ref) == 0); + + newRoot = GC_IDX2PTR(idx); + newRoot->ref = ref; /* GC_ROOT tag is 0 */ + + idx = gc_compress(idx); + GC_REF_SET_INFO(ref, idx | GC_REF_COLOR(ref)); + GC_G(num_roots)++; + + GC_BENCH_INC(zval_buffered); + GC_BENCH_INC(root_buf_length); + GC_BENCH_PEAK(root_buf_peak, root_buf_length); +} + static zend_never_inline void ZEND_FASTCALL gc_remove_compressed(zend_refcounted *ref, uint32_t idx) { gc_root_buffer *root = gc_decompress(ref, idx); @@ -727,22 +781,79 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) int len; if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { - zend_weakmap_get_object_values_gc(obj, &table, &len); + zend_weakmap_get_object_key_entry_gc(obj, &table, &len); n = len; zv = table; - for (; n != 0; n--) { + for (; n != 0; n-=2) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); - zval *value = (zval*) Z_PTR_P(zv); - if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { - ref = Z_COUNTED_P(value); - GC_ADDREF(ref); - if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { - GC_REF_SET_BLACK(ref); - GC_STACK_PUSH(ref); + zval *entry = (zval*) Z_PTR_P(zv); + zval *weakmap = zv+1; + ZEND_ASSERT(Z_REFCOUNTED_P(weakmap)); + if (Z_OPT_REFCOUNTED_P(entry)) { + GC_UNSET_FROM_WEAKMAP_KEY(entry); + if (GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_GREY)) { + /* Weakmap was scanned in gc_mark_roots, we must + * ensure that it's eventually scanned in + * gc_scan_roots as well. */ + if (!GC_REF_ADDRESS(Z_COUNTED_P(weakmap))) { + gc_extra_root(Z_COUNTED_P(weakmap)); + } + } else if (/* GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_BLACK) && */ !GC_FROM_WEAKMAP(entry)) { + /* Both the entry weakmap and key are BLACK, so we + * can mark the entry BLACK as well. + * !GC_FROM_WEAKMAP(entry) means that the weakmap + * was already scanned black (or will not be + * scanned), so it's our responsibility to mark the + * entry */ + ZEND_ASSERT(GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_BLACK)); + ref = Z_COUNTED_P(entry); + GC_ADDREF(ref); + if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { + GC_REF_SET_BLACK(ref); + GC_STACK_PUSH(ref); + } } } - zv++; + zv+=2; + } + } + + if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + zend_weakmap_get_key_entry_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n-=2) { + ZEND_ASSERT(Z_TYPE_P(zv+1) == IS_PTR); + zval *key = zv; + zval *entry = (zval*) Z_PTR_P(zv+1); + if (Z_OPT_REFCOUNTED_P(entry)) { + GC_UNSET_FROM_WEAKMAP(entry); + if (GC_REF_CHECK_COLOR(Z_COUNTED_P(key), GC_GREY)) { + /* Key was scanned in gc_mark_roots, we must + * ensure that it's eventually scanned in + * gc_scan_roots as well. */ + if (!GC_REF_ADDRESS(Z_COUNTED_P(key))) { + gc_extra_root(Z_COUNTED_P(key)); + } + } else if (/* GC_REF_CHECK_COLOR(Z_COUNTED_P(key), GC_BLACK) && */ !GC_FROM_WEAKMAP_KEY(entry)) { + /* Both the entry weakmap and key are BLACK, so we + * can mark the entry BLACK as well. + * !GC_FROM_WEAKMAP_KEY(entry) means that the key + * was already scanned black (or will not be + * scanned), so it's our responsibility to mark the + * entry */ + ZEND_ASSERT(GC_REF_CHECK_COLOR(Z_COUNTED_P(key), GC_BLACK)); + ref = Z_COUNTED_P(entry); + GC_ADDREF(ref); + if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { + GC_REF_SET_BLACK(ref); + GC_STACK_PUSH(ref); + } + } + } + zv += 2; } + goto next; } ht = obj->handlers->get_gc(obj, &table, &len); @@ -845,13 +956,14 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) } } +next: ref = GC_STACK_POP(); if (ref) { goto tail_call; } } -static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack, gc_stack_handle *defer_stack_handle) +static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) { HashTable *ht; Bucket *p; @@ -870,32 +982,56 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack, gc_stack_handle int len; if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { - zend_weakmap_get_object_entries_gc(obj, &table, &len); + zend_weakmap_get_object_key_entry_gc(obj, &table, &len); n = len; zv = table; for (; n != 0; n-=2) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); - zval *value = (zval*) Z_PTR_P(zv); + zval *entry = (zval*) Z_PTR_P(zv); zval *weakmap = zv+1; ZEND_ASSERT(Z_REFCOUNTED_P(weakmap)); - /* Skip reference if already followed through weakmap */ - if (!GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_GREY)) { - if (Z_REFCOUNTED_P(value)) { - ZEND_ASSERT(!GC_FROM_WEAKMAP_KEY(value)); - GC_SET_FROM_WEAKMAP_KEY(value); - gc_stack_push(&defer_stack_handle->stack, &defer_stack_handle->top, (void*) value); - ref = Z_COUNTED_P(value); + if (Z_REFCOUNTED_P(entry)) { + GC_SET_FROM_WEAKMAP_KEY(entry); + ref = Z_COUNTED_P(entry); + /* Only DELREF if the contribution from the weakmap has + * not been cancelled yet */ + if (!GC_FROM_WEAKMAP(entry)) { GC_DELREF(ref); - if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { - GC_REF_SET_COLOR(ref, GC_GREY); - GC_STACK_PUSH(ref); - } + } + if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { + GC_REF_SET_COLOR(ref, GC_GREY); + GC_STACK_PUSH(ref); } } zv+=2; } } + if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + zend_weakmap_get_entry_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *entry = (zval*) Z_PTR_P(zv); + if (Z_REFCOUNTED_P(entry)) { + GC_SET_FROM_WEAKMAP(entry); + ref = Z_COUNTED_P(entry); + /* Only DELREF if the contribution from the weakmap key + * has not been cancelled yet */ + if (!GC_FROM_WEAKMAP_KEY(entry)) { + GC_DELREF(ref); + } + if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { + GC_REF_SET_COLOR(ref, GC_GREY); + GC_STACK_PUSH(ref); + } + } + zv++; + } + goto next; + } + ht = obj->handlers->get_gc(obj, &table, &len); n = len; zv = table; @@ -995,6 +1131,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack, gc_stack_handle } } +next: ref = GC_STACK_POP(); if (ref) { goto tail_call; @@ -1038,7 +1175,7 @@ static void gc_compact(void) } } -static void gc_mark_roots(gc_stack *stack, gc_stack_handle *defer_stack_handle) +static void gc_mark_roots(gc_stack *stack) { gc_root_buffer *current, *last; @@ -1050,7 +1187,7 @@ static void gc_mark_roots(gc_stack *stack, gc_stack_handle *defer_stack_handle) if (GC_IS_ROOT(current->ref)) { if (GC_REF_CHECK_COLOR(current->ref, GC_PURPLE)) { GC_REF_SET_COLOR(current->ref, GC_GREY); - gc_mark_grey(current->ref, stack, defer_stack_handle); + gc_mark_grey(current->ref, stack); } } current++; @@ -1091,14 +1228,14 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) int len; if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { - zend_weakmap_get_object_values_gc(obj, &table, &len); + zend_weakmap_get_object_entry_gc(obj, &table, &len); n = len; zv = table; for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); - zval *value = (zval*) Z_PTR_P(zv); - if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { - ref = Z_COUNTED_P(value); + zval *entry = (zval*) Z_PTR_P(zv); + if (Z_OPT_REFCOUNTED_P(entry)) { + ref = Z_COUNTED_P(entry); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); GC_STACK_PUSH(ref); @@ -1212,17 +1349,34 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) static void gc_scan_roots(gc_stack *stack) { - gc_root_buffer *current = GC_IDX2PTR(GC_FIRST_ROOT); - gc_root_buffer *last = GC_IDX2PTR(GC_G(first_unused)); + uint32_t idx, end; + gc_root_buffer *current; - while (current != last) { + /* Root buffer might be reallocated during gc_scan, + * make sure to reload pointers. */ + idx = GC_FIRST_ROOT; + end = GC_G(first_unused); + while (idx != end) { + current = GC_IDX2PTR(idx); if (GC_IS_ROOT(current->ref)) { if (GC_REF_CHECK_COLOR(current->ref, GC_GREY)) { GC_REF_SET_COLOR(current->ref, GC_WHITE); gc_scan(current->ref, stack); } } - current++; + idx++; + } + + /* Scan extra roots added during gc_scan */ + while (idx != GC_G(first_unused)) { + current = GC_IDX2PTR(idx); + if (GC_IS_ROOT(current->ref)) { + if (GC_REF_CHECK_COLOR(current->ref, GC_GREY)) { + GC_REF_SET_COLOR(current->ref, GC_WHITE); + gc_scan(current->ref, stack); + } + } + idx++; } } @@ -1284,14 +1438,37 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta } if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { - zend_weakmap_get_object_values_gc(obj, &table, &len); + zend_weakmap_get_object_entry_gc(obj, &table, &len); + n = len; + zv = table; + for (; n != 0; n--) { + ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); + zval *entry = (zval*) Z_PTR_P(zv); + if (Z_REFCOUNTED_P(entry) && GC_FROM_WEAKMAP_KEY(entry)) { + GC_UNSET_FROM_WEAKMAP_KEY(entry); + GC_UNSET_FROM_WEAKMAP(entry); + ref = Z_COUNTED_P(entry); + GC_ADDREF(ref); + if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { + GC_REF_SET_BLACK(ref); + GC_STACK_PUSH(ref); + } + } + zv++; + } + } + + if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + zend_weakmap_get_entry_gc(obj, &table, &len); n = len; zv = table; for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); - zval *value = (zval*) Z_PTR_P(zv); - if (GC_FROM_WEAKMAP_KEY(value) && Z_OPT_REFCOUNTED_P(value)) { - ref = Z_COUNTED_P(value); + zval *entry = (zval*) Z_PTR_P(zv); + if (Z_REFCOUNTED_P(entry) && GC_FROM_WEAKMAP(entry)) { + GC_UNSET_FROM_WEAKMAP_KEY(entry); + GC_UNSET_FROM_WEAKMAP(entry); + ref = Z_COUNTED_P(entry); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { GC_REF_SET_BLACK(ref); @@ -1300,6 +1477,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta } zv++; } + goto next; } ht = obj->handlers->get_gc(obj, &table, &len); @@ -1406,6 +1584,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta } } +next: ref = GC_STACK_POP(); if (ref) { goto tail_call; @@ -1490,14 +1669,14 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe zval *table; if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) { - zend_weakmap_get_object_values_gc(obj, &table, &len); + zend_weakmap_get_object_entry_gc(obj, &table, &len); n = len; zv = table; for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); - zval *value = (zval*) Z_PTR_P(zv); - if (Z_OPT_REFCOUNTED_P(value)) { - ref = Z_COUNTED_P(value); + zval *entry = (zval*) Z_PTR_P(zv); + if (Z_OPT_REFCOUNTED_P(entry)) { + ref = Z_COUNTED_P(entry); GC_STACK_PUSH(ref); } zv++; @@ -1601,17 +1780,10 @@ ZEND_API int zend_gc_collect_cycles(void) uint32_t gc_flags = 0; uint32_t idx, end; gc_stack stack; - gc_stack defer_stack; - gc_stack_handle defer_stack_handle = { - .stack = &defer_stack, - }; stack.prev = NULL; stack.next = NULL; - defer_stack.prev = NULL; - defer_stack.next = NULL; - if (GC_G(gc_active)) { return 0; } @@ -1621,26 +1793,17 @@ ZEND_API int zend_gc_collect_cycles(void) GC_G(gc_active) = 1; GC_TRACE("Marking roots"); - gc_mark_roots(&stack, &defer_stack_handle); + gc_mark_roots(&stack); GC_TRACE("Scanning roots"); gc_scan_roots(&stack); GC_TRACE("Collecting roots"); count = gc_collect_roots(&gc_flags, &stack); - while (1) { - zval *value = (zval*) gc_stack_pop(&defer_stack_handle.stack, &defer_stack_handle.top); - if (!value) { - break; - } - GC_UNSET_FROM_WEAKMAP_KEY(value); - } - if (!GC_G(num_roots)) { /* nothing to free */ GC_TRACE("Nothing to free"); gc_stack_free(&stack); - gc_stack_free(&defer_stack); GC_G(gc_active) = 0; goto finish; } @@ -1732,7 +1895,6 @@ ZEND_API int zend_gc_collect_cycles(void) } gc_stack_free(&stack); - gc_stack_free(&defer_stack); /* Destroy zvals. The root buffer may be reallocated. */ GC_TRACE("Destroying zvals"); diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h index 0fde35db97350..93e4f5a464f64 100644 --- a/Zend/zend_gc.h +++ b/Zend/zend_gc.h @@ -74,13 +74,6 @@ size_t zend_gc_globals_size(void); ((GC_TYPE_INFO(ref) & \ (GC_INFO_MASK | (GC_NOT_COLLECTABLE << GC_FLAGS_SHIFT))) == 0) -#define Z_FROM_WEAKMAP_KEY (1<<0) - -/* The WeakMap entry zv is first reachable by following the virtual reference - * from the a WeakMap key to the entry, and should not be scanned again through - * WeakMap entry enumeration */ -#define GC_FROM_WEAKMAP_KEY(zv) (Z_TYPE_INFO_P((zv)) & (Z_FROM_WEAKMAP_KEY << Z_TYPE_INFO_EXTRA_SHIFT)) - static zend_always_inline void gc_check_possible_root(zend_refcounted *ref) { if (EXPECTED(GC_TYPE_INFO(ref) == GC_REFERENCE)) { diff --git a/Zend/zend_weakrefs.c b/Zend/zend_weakrefs.c index b19c887f897ad..33487db874cb1 100644 --- a/Zend/zend_weakrefs.c +++ b/Zend/zend_weakrefs.c @@ -467,15 +467,40 @@ static HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n) zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); zval *val; ZEND_HASH_MAP_FOREACH_VAL(&wm->ht, val) { - if (!GC_FROM_WEAKMAP_KEY(val)) { - zend_get_gc_buffer_add_zval(gc_buffer, val); - } + zend_get_gc_buffer_add_zval(gc_buffer, val); + } ZEND_HASH_FOREACH_END(); + zend_get_gc_buffer_use(gc_buffer, table, n); + return NULL; +} + +HashTable *zend_weakmap_get_key_entry_gc(zend_object *object, zval **table, int *n) +{ + zend_weakmap *wm = zend_weakmap_from(object); + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + zend_ulong h; + zval *val; + ZEND_HASH_MAP_FOREACH_NUM_KEY_VAL(&wm->ht, h, val) { + zend_object *key = zend_weakref_key_to_object(h); + zend_get_gc_buffer_add_obj(gc_buffer, key); + zend_get_gc_buffer_add_ptr(gc_buffer, val); + } ZEND_HASH_FOREACH_END(); + zend_get_gc_buffer_use(gc_buffer, table, n); + return NULL; +} + +HashTable *zend_weakmap_get_entry_gc(zend_object *object, zval **table, int *n) +{ + zend_weakmap *wm = zend_weakmap_from(object); + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + zval *val; + ZEND_HASH_MAP_FOREACH_VAL(&wm->ht, val) { + zend_get_gc_buffer_add_ptr(gc_buffer, val); } ZEND_HASH_FOREACH_END(); zend_get_gc_buffer_use(gc_buffer, table, n); return NULL; } -HashTable *zend_weakmap_get_object_entries_gc(zend_object *object, zval **table, int *n) +HashTable *zend_weakmap_get_object_key_entry_gc(zend_object *object, zval **table, int *n) { zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); const zend_ulong obj_key = zend_object_to_weakref_key(object); @@ -510,7 +535,7 @@ HashTable *zend_weakmap_get_object_entries_gc(zend_object *object, zval **table, return NULL; } -HashTable *zend_weakmap_get_object_values_gc(zend_object *object, zval **table, int *n) +HashTable *zend_weakmap_get_object_entry_gc(zend_object *object, zval **table, int *n) { zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); const zend_ulong obj_key = zend_object_to_weakref_key(object); diff --git a/Zend/zend_weakrefs.h b/Zend/zend_weakrefs.h index 84c179f60e3db..7eb18b30eb353 100644 --- a/Zend/zend_weakrefs.h +++ b/Zend/zend_weakrefs.h @@ -21,7 +21,13 @@ BEGIN_EXTERN_C() +HashTable *zend_weakmap_get_key_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_object_key_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_object_entry_gc(zend_object *object, zval **table, int *n); + extern ZEND_API zend_class_entry *zend_ce_weakref; +extern ZEND_API zend_class_entry *zend_ce_weakmap; void zend_register_weakref_ce(void); @@ -62,9 +68,6 @@ static zend_always_inline zend_object *zend_weakref_key_to_object(zend_ulong key return (zend_object *) (((uintptr_t) key) << ZEND_MM_ALIGNMENT_LOG2); } -HashTable *zend_weakmap_get_object_entries_gc(zend_object *object, zval **table, int *n); -HashTable *zend_weakmap_get_object_values_gc(zend_object *object, zval **table, int *n); - END_EXTERN_C() #endif From c769eb96792412ff8497a2f3efbce3889967b6af Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 30 Jun 2023 16:44:55 +0200 Subject: [PATCH 7/9] Reduce overhead The normal code path will fetch `obj->handlers->get_gc`, so it's much cheaper to identify weakmaps by looking at `obj->handlers->get_gc`. --- Zend/zend_gc.c | 6 +++--- Zend/zend_weakrefs.c | 2 +- Zend/zend_weakrefs.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index ba61b0be652c9..a84893d354c03 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -818,7 +818,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) } } - if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + if (UNEXPECTED(obj->handlers->get_gc == zend_weakmap_get_gc)) { zend_weakmap_get_key_entry_gc(obj, &table, &len); n = len; zv = table; @@ -1007,7 +1007,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) } } - if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + if (UNEXPECTED(obj->handlers->get_gc == zend_weakmap_get_gc)) { zend_weakmap_get_entry_gc(obj, &table, &len); n = len; zv = table; @@ -1458,7 +1458,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta } } - if (UNEXPECTED(obj->ce == zend_ce_weakmap)) { + if (UNEXPECTED(obj->handlers->get_gc == zend_weakmap_get_gc)) { zend_weakmap_get_entry_gc(obj, &table, &len); n = len; zv = table; diff --git a/Zend/zend_weakrefs.c b/Zend/zend_weakrefs.c index 33487db874cb1..442b19fb62adb 100644 --- a/Zend/zend_weakrefs.c +++ b/Zend/zend_weakrefs.c @@ -461,7 +461,7 @@ static HashTable *zend_weakmap_get_properties_for(zend_object *object, zend_prop return ht; } -static HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n) +HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n) { zend_weakmap *wm = zend_weakmap_from(object); zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); diff --git a/Zend/zend_weakrefs.h b/Zend/zend_weakrefs.h index 7eb18b30eb353..72059d6f99f06 100644 --- a/Zend/zend_weakrefs.h +++ b/Zend/zend_weakrefs.h @@ -21,13 +21,13 @@ BEGIN_EXTERN_C() +HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n); HashTable *zend_weakmap_get_key_entry_gc(zend_object *object, zval **table, int *n); HashTable *zend_weakmap_get_entry_gc(zend_object *object, zval **table, int *n); HashTable *zend_weakmap_get_object_key_entry_gc(zend_object *object, zval **table, int *n); HashTable *zend_weakmap_get_object_entry_gc(zend_object *object, zval **table, int *n); extern ZEND_API zend_class_entry *zend_ce_weakref; -extern ZEND_API zend_class_entry *zend_ce_weakmap; void zend_register_weakref_ce(void); From 962acddcedc751f6688f73c0ec31add7c11126b4 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 30 Jun 2023 16:52:57 +0200 Subject: [PATCH 8/9] Cleanup --- Zend/zend_gc.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index a84893d354c03..9752a1420dabd 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -303,13 +303,6 @@ struct _gc_stack { zend_refcounted *data[GC_STACK_SEGMENT_SIZE]; }; -typedef struct _gc_stack_handle gc_stack_handle; - -struct _gc_stack_handle { - gc_stack *stack; - size_t top; -}; - #define GC_STACK_DCL(init) \ gc_stack *_stack = init; \ size_t _top = 0; From 47633e344a8165fda1edf9aa814b19f211cce810 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sun, 16 Jul 2023 12:43:09 +0200 Subject: [PATCH 9/9] Move internal APIs to the bottom --- Zend/zend_weakrefs.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Zend/zend_weakrefs.h b/Zend/zend_weakrefs.h index 72059d6f99f06..5b1c8ea1c325d 100644 --- a/Zend/zend_weakrefs.h +++ b/Zend/zend_weakrefs.h @@ -21,12 +21,6 @@ BEGIN_EXTERN_C() -HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n); -HashTable *zend_weakmap_get_key_entry_gc(zend_object *object, zval **table, int *n); -HashTable *zend_weakmap_get_entry_gc(zend_object *object, zval **table, int *n); -HashTable *zend_weakmap_get_object_key_entry_gc(zend_object *object, zval **table, int *n); -HashTable *zend_weakmap_get_object_entry_gc(zend_object *object, zval **table, int *n); - extern ZEND_API zend_class_entry *zend_ce_weakref; void zend_register_weakref_ce(void); @@ -68,6 +62,12 @@ static zend_always_inline zend_object *zend_weakref_key_to_object(zend_ulong key return (zend_object *) (((uintptr_t) key) << ZEND_MM_ALIGNMENT_LOG2); } +HashTable *zend_weakmap_get_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_key_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_object_key_entry_gc(zend_object *object, zval **table, int *n); +HashTable *zend_weakmap_get_object_entry_gc(zend_object *object, zval **table, int *n); + END_EXTERN_C() #endif