From e51cf4bf654889cb06f579795ea98d3f092d684c Mon Sep 17 00:00:00 2001 From: Andy Wingo Date: Wed, 2 Jul 2025 15:11:03 +0200 Subject: [PATCH] Precise tracing works with heap-conservative-mmc * libguile/ports.c (scm_make_port_type): Pin the ptob. * libguile/threads.c (scm_trace_thread_roots): No need to trace dynstack roots; we will do so within scm_trace_thread. * libguile/trace.h (scm_trace_struct): Fix arg order to logbit?. (scm_trace_ephemeron_table): Indentation fix. (scm_trace_pointer): Actually trace the protected objects. (scm_trace_vm): (scm_trace_active_dynamic_state): (scm_trace_thread): Fix to trace missing references. (scm_trace_port): Trace the ptob and write_buf_aux. (scm_trace_random_state): Add comment about rng being static. * libguile/whippet-embedder.h (gc_is_valid_conservative_ref_displacement): Don't hide behind an ifdef. --- libguile/dynstack.c | 28 ----------------- libguile/ports.c | 10 +++--- libguile/threads.c | 3 -- libguile/trace.h | 61 ++++++++++++++++++++++++------------- libguile/whippet-embedder.h | 15 ++++----- 5 files changed, 54 insertions(+), 63 deletions(-) diff --git a/libguile/dynstack.c b/libguile/dynstack.c index 7f9238f51..6c557c6a0 100644 --- a/libguile/dynstack.c +++ b/libguile/dynstack.c @@ -193,34 +193,6 @@ scm_trace_dynstack (struct scm_dynstack *dynstack, } } -struct trace_pinned_trampoline -{ - void (*trace_pinned) (struct gc_ref ref, - struct gc_heap *heap, - void *trace_data); - void *trace_data; -}; - -static void -trace_pinned_trampoline (struct gc_edge edge, - struct gc_heap *heap, - void *trace_data) -{ - struct trace_pinned_trampoline *data = trace_data; - return data->trace_pinned (gc_edge_ref (edge), heap, data->trace_data); -} - -void -scm_trace_dynstack_roots (struct scm_dynstack *dynstack, - void (*trace_pinned) (struct gc_ref ref, - struct gc_heap *heap, - void *trace_data), - struct gc_heap *heap, void *trace_data) -{ - struct trace_pinned_trampoline data = { trace_pinned, trace_data }; - return scm_trace_dynstack (dynstack, trace_pinned_trampoline, heap, &data); -} - static inline scm_t_bits * push_dynstack_entry_unchecked (scm_t_dynstack *dynstack, scm_t_dynstack_item_type type, diff --git a/libguile/ports.c b/libguile/ports.c index 5058e73c3..e83c5601b 100644 --- a/libguile/ports.c +++ b/libguile/ports.c @@ -248,10 +248,8 @@ scm_make_port_type (char *name, size_t (*write) (SCM port, SCM src, size_t start, size_t count)) { - scm_t_port_type *desc; - - desc = scm_allocate_tagged (SCM_I_CURRENT_THREAD, sizeof (*desc)); - memset (desc, 0, sizeof (*desc)); + struct scm_thread *thr = SCM_I_CURRENT_THREAD; + scm_t_port_type *desc = scm_allocate_tagged (thr, sizeof (*desc)); desc->tag = scm_tc7_port_type; desc->name = name; @@ -266,6 +264,10 @@ scm_make_port_type (char *name, desc->stream_mode = SCM_PORT_STREAM_CONSERVATIVE; scm_make_port_classes (desc); + // Since we have to reach into the ptob to get stream mode when + // collecting ports, let's pin the ptob. + scm_gc_pin_object (thr, SCM_PACK_POINTER (desc)); + return desc; } diff --git a/libguile/threads.c b/libguile/threads.c index 45e016625..85ccc7a57 100644 --- a/libguile/threads.c +++ b/libguile/threads.c @@ -107,9 +107,6 @@ scm_trace_thread_roots (struct scm_thread *thread, struct gc_heap *heap, void *trace_data) { trace_pinned (gc_ref_from_heap_object (thread), heap, trace_data); -#if GC_CONSERVATIVE_TRACE - scm_trace_dynstack_roots (&thread->dynstack, trace_pinned, heap, trace_data); -#endif /* FIXME: Trace is not a tagged allocation. */ scm_trace_vm_roots (&thread->vm, trace_pinned, trace_ambiguous, heap, trace_data); diff --git a/libguile/trace.h b/libguile/trace.h index 99cefbd76..b80bf09bc 100644 --- a/libguile/trace.h +++ b/libguile/trace.h @@ -116,14 +116,6 @@ scm_trace_loader_roots (void (*trace_ambiguous) (uintptr_t lo, struct gc_heap *heap, void *trace_data); -SCM_INTERNAL void -scm_trace_dynstack_roots (struct scm_dynstack *dynstack, - void (*trace_pinned) (struct gc_ref ref, - struct gc_heap *heap, - void *trace_data), - struct gc_heap *heap, - void *trace_data); - #define TRACE_PARAMS \ void (*trace) (struct gc_edge, struct gc_heap *, void *), \ struct gc_heap *heap, \ @@ -204,6 +196,8 @@ scm_trace_struct (struct scm_struct *s, TRACE_PARAMS) if (trace) { + // Because it's pinned, the source of the vtable edge doesn't matter. + TRACE (&vtable); // unboxed_fields is either an inum or a bignum. If it's a // bignum, it's pinned. SCM unboxed_fields = scm_i_vtable_unboxed_fields (vtable); @@ -212,7 +206,7 @@ scm_trace_struct (struct scm_struct *s, TRACE_PARAMS) TRACE_SLOT (s->slots, i); else for (size_t i = 0; i < nfields; i++) - if (scm_is_false (scm_logbit_p (unboxed_fields, SCM_I_MAKINUM (i)))) + if (scm_is_false (scm_logbit_p (SCM_I_MAKINUM (i), unboxed_fields))) TRACE_SLOT (s->slots, i); } @@ -253,10 +247,8 @@ static inline size_t scm_trace_ephemeron_table (struct scm_ephemeron_table *et, TRACE_PARAMS) { if (trace) - { - for (size_t idx = 0; idx < et->size; idx++) - TRACE_SLOT (et->contents, idx); - } + for (size_t idx = 0; idx < et->size; idx++) + TRACE_SLOT (et->contents, idx); return SIZEOF_WITH_TAIL (et, contents, et->size); } @@ -312,7 +304,7 @@ scm_trace_pointer (struct scm_pointer *p, TRACE_PARAMS) if (trace) for (size_t i = 0; i < extra_words; i++) - TRACE_MEMBER (p, gc_objects); + TRACE_SLOT (p->gc_objects, i); return SIZEOF_WITH_TAIL (p, gc_objects, extra_words); } @@ -451,18 +443,41 @@ scm_trace_bytevector (struct scm_bytevector *x, TRACE_PARAMS) return sizeof (*x); } +static inline void +scm_trace_vm (struct scm_vm *x, TRACE_PARAMS) +{ + TRACE_MEMBER (x, apply_hook); + TRACE_MEMBER (x, return_hook); + TRACE_MEMBER (x, next_hook); + TRACE_MEMBER (x, abort_hook); + TRACE_MEMBER (x, overflow_handler_stack); +} + +static inline void +scm_trace_active_dynamic_state (struct scm_dynamic_state *x, TRACE_PARAMS) +{ + TRACE_MEMBER (x, thread_local_values); + TRACE_MEMBER (x, values); + for (size_t i = 0; i < SCM_CACHE_SIZE; i++) + { + TRACE (&x->cache.entries[i].key); + TRACE (&x->cache.entries[i].value); + } +} + static inline size_t scm_trace_thread (struct scm_thread *x, TRACE_PARAMS) { if (trace) { TRACE_MEMBER (x, next_thread); + scm_trace_vm (&x->vm, TRACE_ARGS); TRACE_MEMBER (x, pending_asyncs); + scm_trace_active_dynamic_state (&x->dynamic_state, TRACE_ARGS); TRACE_MEMBER (x, continuation_root); TRACE_MEMBER (x, join_cond); TRACE_MEMBER (x, join_lock); TRACE_MEMBER (x, join_results); - scm_trace_dynstack (&x->dynstack, TRACE_ARGS); } @@ -525,18 +540,22 @@ scm_trace_port (struct scm_t_port *x, TRACE_PARAMS) { if (trace) { + TRACE_MEMBER (x, ptob); + + // ptob is pinned. + if (x->ptob->stream_mode != SCM_PORT_STREAM_UNMANAGED) + TRACE_MEMBER (x, stream); + TRACE_MEMBER (x, file_name); TRACE_MEMBER (x, position); TRACE_MEMBER (x, read_buf); TRACE_MEMBER (x, write_buf); + TRACE_MEMBER (x, write_buf_aux); TRACE_MEMBER (x, encoding); TRACE_MEMBER (x, conversion_strategy); TRACE_MEMBER (x, precise_encoding); TRACE_MEMBER (x, close_handle); TRACE_MEMBER (x, alist); - - if (x->ptob->stream_mode != SCM_PORT_STREAM_UNMANAGED) - TRACE_MEMBER (x, stream); } return sizeof (*x); @@ -577,11 +596,10 @@ scm_trace_continuation (struct scm_continuation *c, TRACE_PARAMS) { if (trace) { - TRACE_MEMBER (c, root); - TRACE_MEMBER (c, vm_cont); - scm_trace_range_conservatively (&c->jmpbuf, sizeof (c->jmpbuf), 1, TRACE_ARGS); + TRACE_MEMBER (c, root); + TRACE_MEMBER (c, vm_cont); scm_trace_range_conservatively (c->stack, c->num_stack_items * sizeof (SCM_STACKITEM), 1, @@ -613,6 +631,7 @@ scm_trace_syntax_transformer (struct scm_syntax_transformer *tx, TRACE_PARAMS) static inline size_t scm_trace_random_state (struct scm_t_rstate *s, TRACE_PARAMS) { + // rng is off-heap. return s->rng->rstate_size; } diff --git a/libguile/whippet-embedder.h b/libguile/whippet-embedder.h index b7e733416..bd6c9f934 100644 --- a/libguile/whippet-embedder.h +++ b/libguile/whippet-embedder.h @@ -49,15 +49,16 @@ static inline size_t gc_finalizer_priority_count (void) { return 2; } static inline int gc_is_valid_conservative_ref_displacement (uintptr_t displacement) { -#if GC_CONSERVATIVE_ROOTS || GC_CONSERVATIVE_TRACE - if (displacement == 0) return 1; - if (displacement == scm_tc3_cons) return 1; - if (displacement == scm_tc3_struct) return 1; - return 0; -#else + if (GC_CONSERVATIVE_ROOTS || GC_CONSERVATIVE_TRACE) + { + if (displacement == 0) return 1; + if (displacement == scm_tc3_cons) return 1; + if (displacement == scm_tc3_struct) return 1; + return 0; + } + // Shouldn't get here. GC_CRASH (); -#endif } // FIXME: Here add tracing for SCM literals in .go files or .data