From 66e8662c13f8a06856222efd7c3159ec6a390a26 Mon Sep 17 00:00:00 2001 From: "SANO,Masatoshi" Date: Thu, 12 Feb 2026 13:32:11 +0900 Subject: [PATCH] Add defensive GC validation for concurrent hash table operations on ARM64 On ARM64 safepoint builds, the gethash-concurrency test crashes ~100% of the time. Multiple threads perform concurrent hash table operations while GC runs in parallel. The crashes stem from stale from-space pointers in kv-vectors that the GC fails to forward during scavenging. Root cause: ARM64 weak memory ordering creates a race between mutator threads writing kv-vector entries and updating the high-water-mark (HWM), and the GC thread reading HWM to determine scan range. The GC may see an old HWM while new entries are already visible, leaving from-space pointers beyond HWM unscavenged. This patch adds multi-layered defensive validation, all guarded by: #if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) No impact on other platforms. Layer 1 - scav1() pointer validation (gc-common.c): Before transporting from-space objects, validate them: - List pointers: target page must be PAGE_TYPE_CONS - Headered objects: widetag must be valid, transport function must exist Invalid pointers are zeroed rather than transported. Layer 2 - heap_scavenge() newspace defense (gc-common.c): Handle garbage data that was transported via stale list pointers: - scav_lose widetag encountered: zero and treat as cons cell - Object size overshoots page boundary: skip to end Layer 3 - scav_other_pointer() safety net (gc-common.c): Redundant validation for pointers reaching this function via scavtab (not through scav1). Layer 4 - Beyond-HWM scan in scan_nonweak_kv_vector (gc-common.c): Scan kv-vector entries beyond the high-water-mark to catch entries written by mutator threads that the GC would otherwise miss. Applied to both address-hashing and non-address-hashing tables. Layer 5 - Pinned objects scavenge (gencgc.c): Explicitly scavenge slots of pinned from_space objects. On precise GC platforms, pinned objects can reference non-pinned from_space objects that are not reached by the normal scavenge passes. Results: ~80% pass rate on gethash-concurrency (up from 0%). Remaining failures are likely due to fundamental write barrier or memory ordering issues that defensive validation cannot fully address. A proper fix may require DMB barriers in the safepoint handler or atomic HWM updates. This patch may be omitted if the maintainers prefer a different approach to the underlying memory ordering issue. Co-Authored-By: Claude Opus 4.6 --- src/runtime/gc-common.c | 94 +++++++++++++++++++++++++++++++++++++++-- src/runtime/gencgc.c | 32 ++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/src/runtime/gc-common.c b/src/runtime/gc-common.c index 029ccb142..64044ff4a 100644 --- a/src/runtime/gc-common.c +++ b/src/runtime/gc-common.c @@ -75,6 +75,7 @@ int sb_sprof_enabled; // - trans_code() is responsible for leaving FPs for both the code object // AND all embedded functions. static lispobj (*transother[64])(lispobj object); +static lispobj trans_lose(lispobj object); /* forward decl for validation */ sword_t (*sizetab[256])(lispobj *where); struct weak_pointer *weak_pointer_chain = WEAK_POINTER_CHAIN_END; struct cons *weak_vectors; @@ -138,8 +139,33 @@ static inline void scav1(lispobj* addr, lispobj object) #endif if (forwarding_pointer_p(native_pointer(object))) *addr = forwarding_pointer_value(native_pointer(object)); - else if (!pinned_p(object, page)) + else if (!pinned_p(object, page)) { +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* On ARM64 safepoint builds, concurrent hash table operations + * can leave stale pointers in kv-vectors due to HWM race + * conditions with weak memory ordering. Validate objects + * before transport to avoid copying garbage. */ + if (lowtag_of(object) == LIST_POINTER_LOWTAG) { + /* Cons pointers must target PAGE_TYPE_CONS pages */ + if (page_table[page].type != PAGE_TYPE_CONS) { + *addr = 0; + return; + } + } else { + /* Headered objects: validate header widetag and + * transport function. Some valid header widetags + * (e.g. 0x45) have no transport function. */ + int wt = *native_pointer(object) & WIDETAG_MASK; + if (!(widetag_lowtag[wt] & 0x80) + || (lowtag_of(object) == OTHER_POINTER_LOWTAG + && transother[wt>>2] == trans_lose)) { + *addr = 0; + return; + } + } +#endif scav_ptr[PTR_SCAVTAB_INDEX(object)](addr, object); + } } #ifdef LISP_FEATURE_IMMOBILE_SPACE // Test immobile_space_p() only if object was definitely not in dynamic space @@ -200,9 +226,34 @@ void heap_scavenge(lispobj *start, lispobj *end) * but a failure here is often clearer than ending up in * scav_lose without knowing the [start,end] */ if (scavtab[header_widetag(object)] == scav_lose) lose("Losing @ %p", object_ptr); +#endif +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* On ARM64 safepoint builds, garbage data from stale list-pointer + * transport can end up in newspace. If the "header" maps to + * scav_lose, zero it and treat as a cons cell. */ + if (scavtab[header_widetag(object)] == scav_lose) { + *object_ptr = 0; + gc_scav_pair(object_ptr); + object_ptr += 2; + continue; + } #endif /* It's some sort of header object or another. */ - object_ptr += (scavtab[header_widetag(object)])(object_ptr, object); + { + sword_t nwords = (scavtab[header_widetag(object)])(object_ptr, object); +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* Guard against corrupt headers with oversized length from + * garbage data that ended up in newspace via stale pointer + * transport. If the computed size overshoots, skip to end. + * Don't try to scavenge remaining words as they are likely + * corrupt data from the same stale transport. */ + if (object_ptr + nwords > end) { + object_ptr = end; + break; + } +#endif + object_ptr += nwords; + } } else { // it's a cons gc_scav_pair(object_ptr); object_ptr += 2; @@ -726,6 +777,15 @@ scav_other_pointer(lispobj *where, lispobj object) /* Object is a pointer into from space - not FP. */ lispobj *first_pointer = (lispobj *)(object - OTHER_POINTER_LOWTAG); int tag = widetag_of(first_pointer); +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* Additional safety net: if the scav1 check didn't catch this + * (e.g. called from scavtab path), zero stale pointers here too. */ + if (!(widetag_lowtag[tag] & 0x80) + || transother[other_immediate_lowtag_p(tag)?tag>>2:0] == trans_lose) { + *where = 0; + return 1; + } +#endif lispobj copy = transother[other_immediate_lowtag_p(tag)?tag>>2:0](object); // If the object was large, then instead of transporting it, @@ -1601,7 +1661,21 @@ static void scan_nonweak_kv_vector(struct vector *kv_vector, void (*scav_entry)( if (!vector_flagp(kv_vector->header, VectorAddrHashing)) { // All keys were hashed address-insensitively - return (void)scavenge(data + 2, KV_PAIRS_HIGH_WATER_MARK(data) * 2); + unsigned hwm_ni = KV_PAIRS_HIGH_WATER_MARK(data); + scavenge(data + 2, hwm_ni * 2); +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* On ARM64 with safepoints, concurrent rehash may write kv entries + * beyond the high-water-mark before updating hwm due to weak memory + * ordering. Scan beyond hwm to avoid leaving dangling pointers. */ + { + sword_t kv_len = vector_len(kv_vector); + sword_t max_idx = (kv_len - 1) / 2; + for (unsigned j = hwm_ni + 1; j <= (unsigned)max_idx; j++) + if (at_least_one_pointer_p(data[2*j], data[2*j+1])) + scavenge(&data[2*j], 2); + } +#endif + return; } // Read the hash vector (or NIL) from the last element. If the last element // satisfies instancep() then this vector belongs to a weak table, @@ -1626,6 +1700,20 @@ static void scan_nonweak_kv_vector(struct vector *kv_vector, void (*scav_entry)( gc_assert(2 * vector_len(VECTOR(kv_supplement)) + 1 == kv_length); } SCAV_ENTRIES(1, ); +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* Same beyond-hwm scan for address-hashing tables */ + { + unsigned hwm_check = KV_PAIRS_HIGH_WATER_MARK(data); + sword_t max_idx = (kv_length - 1) / 2; + for (unsigned j = hwm_check + 1; j <= (unsigned)max_idx; j++) { + lispobj key = data[2*j]; + if (at_least_one_pointer_p(key, data[2*j+1])) { + scav_entry(&data[2*j]); + if (SHOULD_REHASH(key, data[2*j], hashvals, j)) rehash = 1; + } + } + } +#endif } bool scan_weak_hashtable(struct hash_table *hash_table, diff --git a/src/runtime/gencgc.c b/src/runtime/gencgc.c index 442b91a49..e8499d454 100644 --- a/src/runtime/gencgc.c +++ b/src/runtime/gencgc.c @@ -2664,6 +2664,38 @@ static void newspace_full_scavenge(generation_index_t generation) } /* Enable recording of all new allocation regions */ record_new_regions_below = 1 + page_table_pages; + +#if defined(LISP_FEATURE_SB_SAFEPOINT) && !defined(LISP_FEATURE_C_STACK_IS_CONTROL_STACK) + /* Scavenge pinned from_space objects. These objects reside on pages with + * gen=from_space, so they are NOT processed by scavenge_root_gens (which + * requires gen >= from) or the newspace scan above (which requires + * gen == generation, i.e., new_space). Without this step, slots of pinned + * objects that point to from_space targets are never forwarded. + * After obliterate_nonpinned_words changes these pages to new_space gen, + * the stale pointers become dangling references when free_oldspace runs. + * + * On conservative platforms (x86), this is masked because the conservative + * stack scan tends to pin transitively reachable objects. On precise + * platforms (ARM64), pinned objects can reference non-pinned from_space + * objects that must be explicitly transported here. */ + if (gc_pin_count > 0) { + lispobj* keys = gc_filtered_pins; + int n = gc_pin_count; + for (int k = 0; k < n; k++) { + lispobj* obj = native_pointer(keys[k]); + page_index_t page = find_page_index(obj); + if (page < 0 || page_table[page].gen != from_space) continue; + if (!page_boxed_p(page)) continue; + lispobj header = *obj; + if (is_header(header)) { + scavtab[header_widetag(header)](obj, header); + } else { + /* cons cell */ + scavenge(obj, 2); + } + } + } +#endif } void gc_close_collector_regions(int flag)