common-lisp/sbcl.sbcl
1
0
Fork 0
mirror of git://git.code.sf.net/p/sbcl/sbcl synced 2026-09-10 07:26:40 -04:00
Code Issues Actions 8 Packages Projects Releases Wiki Activity

Add defensive GC validation for concurrent hash table operations on ARM64

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
SANO,Masatoshi 2026-02-12 13:32:11 +09:00 committed by Stas Boukarev
parent 1984d3675f
commit 66e8662c13
2 changed files with 123 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

94
src/runtime/gc-common.c
View file

@ -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,9 +139,34 @@ 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
else if (page < 0 && immobile_space_p(object)) {
@ -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,

32
src/runtime/gencgc.c
View file

@ -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)