[Gc] Patch resubmission: GC_do_blocking for Win32 and
anti-GC_do_blocking - part 1/2
hans.boehm at hp.com
Mon Dec 7 16:17:46 PST 2009
I was very belatedly looking at this, and I have some minor concerns:
1) As far as I can tell, when the code says "activation frame", it really means "traced stack section". I found that confusing, when I started reading the code without the patch documentation, since a struct GC_activation_frame_s isn't really a single frame. I think it would be good to consistently rename this, e.g. to traced_section.
2) Shouldn't GC_call_with_gc_active conspicuously fail on DARWIN? It looks like
GC_do_blocking() currently works there? If so, I think that ignoring GC_call_with_gc_active is unsafe on DARWIN, but unlikely to break during testing,
which is not a good thing.
3) Did you think about what happens to callee-save registers, and at the boundaries between regions here? I don't immediately see anything wrong, but it seems a bit tricky. There have been bugs here related to callee-save registers getting saved in the untraced region of the stack. But I think the hard case there is GC_do_blocking, not the inverse.
It would be good to add a test for this if you haven't already.
> -----Original Message-----
> From: gc-bounces at napali.hpl.hp.com
> [mailto:gc-bounces at napali.hpl.hp.com] On Behalf Of Ivan Maidanski
> Sent: Friday, September 11, 2009 1:38 PM
> To: gc at napali.hpl.hp.com
> Subject: [Gc] Patch resubmission: GC_do_blocking for Win32
> and anti-GC_do_blocking - part 1/2
> This suggested patch (ivmai130a.diff, ivmai130b.diff),
> superseding diff44 [Nov 13] and diff69a/b [Apr 14], does:
> - add API declaration for GC_do_blocking() (which is
> implemented for pthreads at present);
> - implement GC_do_blocking() for Win32;
> - introduce new API function GC_call_with_gc_active() which
> does the opposite to GC_do_blocking();
> - exclude the stack regions between each nested
> GC_do_blocking() and GC_call_with_gc_active() calls from scanning;
> - implement GC_do_blocking() and GC_call_with_gc_active() for
> th single-threaded case (for the reason just above and/or for
> more precise stack base recording).
> For more info (and rationale), see:
> Note: thie patch should be tested on IA-64.
> ChangeLog entries:
> * include/gc.h (GC_do_blocking, GC_call_with_gc_active): New
> function prototype.
> * include/private/gc_priv.h (STOP_WORLD): Replace a
> no-op (for the
> single-threaded case) with an assertion check for the
> state to be
> not a "do-blocking" one.
> * include/private/gc_priv.h (blocking_data): Move the structure
> definition from pthread_support.c; change "fn" return
> type to void
> * include/private/gc_priv.h (GC_activation_frame_s):
> New structure
> * include/private/gc_priv.h (GC_push_all_stack_frames): New
> function declaration (only if THREADS).
> * include/private/gc_priv.h (GC_world_stopped): Don't declare
> unless THREADS.
> * include/private/gc_priv.h (GC_blocked_sp,
> GC_activation_frame_s): New declaration (only if not THREADS).
> * include/private/gc_priv.h (GC_push_all_register_frames): New
> function declaration (only for IA-64).
> * include/private/gc_priv.h (NURSERY, GC_push_proc): Remove
> obsolete (unused) symbols.
> * include/private/gc_priv.h (GC_push_all_stack_partially_eager):
> Remove declaration (since it is static now).
> * mark_rts.c (GC_push_all_stack_partially_eager): Move
> from mark.c
> (for code locality) and make STATIC.
> * mark_rts.c (GC_push_all_register_frames): New
> function (only for
> * mark_rts.c (GC_push_all_stack_frames): New function (only if
> * mark_rts.c (GC_add_trace_entry): New function
> prototype (used by
> GC_push_all_stack_partially_eager(), only if TRACE_BUF).
> * mark_rts.c (GC_push_all_stack_part_eager_frames): New
> * mar_rts.c (GC_save_regs_ret_val): Move the
> declaration out of a
> function body (only for IA-64).
> * mark_rts.c (GC_push_current_stack): Call
> GC_push_all_stack_part_eager_frames() instead of
> * mark_rts.c (GC_push_current_stack): Call
> GC_push_all_register_frames() instead of GC_push_all_eager() for
> IA-64 backing store.
> * misc.c (GC_do_blocking_inner): Declare function (if THREADS
> * misc.c (GC_blocked_sp, GC_blocked_register_sp,
> GC_activation_frame): New global variables (only if not
> * misc.c (GC_call_with_gc_active, GC_do_blocking_inner): New API
> function (only if not THREADS).
> * misc.c (GC_do_blocking): Move the function from
> * include/private/pthread_support.h (GC_Thread_Rep): Add
> "activation_frame" field.
> * pthread_stop_world.c (GC_push_all_stacks): Call
> GC_push_all_stack_frames() and
> GC_push_all_register_frames instead
> of GC_push_all_stack() and/or GC_push_all_eager();
> don't check for
> STACK_GROWS_UP here.
> * pthread_support.c (GC_do_blocking_inner): Remove
> "static"; store
> "fn" result back to "client_data" field.
> * pthread_support.c (GC_call_with_gc_active): New API function.
> * win32_threads.c (GC_call_with_gc_active): Ditto.
> * win32_threads.c (GC_Thread_Rep): Add "thread_blocked_sp" and
> "activation_frame" fields.
> * win32_threads.c (GC_new_thread): Add assertion checking for
> thread_blocked_sp is NULL.
> * win32_threads.c (GC_do_blocking_inner): New function.
> * win32_threads.c (GC_stop_world): Don't suspend a thread if its
> thread_blocked_sp is non-NULL.
> * win32_threads.c (GC_push_stack_for): Use thread
> "activation_frame" (if non-NULL); use "thread_blocked_sp" if
> non-NULL (instead of calling GetThreadContext());
> "UNPROTECT" the
> thread before modifying its last_stack_min; call
> GC_push_all_stack_frames() instead of
> GC_push_all_stack(); update
> the comments.
More information about the Gc