[Gc] Patch resubmission: GC_do_blocking for Win32 and anti-GC_do_blocking - part 1/2

Boehm, Hans 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.

Hans

> -----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
> 
> Hi!
> 
> 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: 
> http://permalink.gmane.org/gmane.comp.programming.garbage-coll
> ection.boehmgc/2612
> 
> 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
> 	pointer.
> 	* include/private/gc_priv.h (GC_activation_frame_s): 
> New structure
> 	type.
> 	* 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
> 	IA-64).
> 	* mark_rts.c (GC_push_all_stack_frames): New function (only if
> 	THREADS).
> 	* 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 
> function.
> 	* 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
> 	GC_push_all_stack_partially_eager().
> 	* 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
> 	only).
> 	* misc.c (GC_blocked_sp, GC_blocked_register_sp,
> 	GC_activation_frame): New global variables (only if not 
> THREADS).
> 	* 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
> 	pthread_support.c.
> 	* 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.
> 
> Bye.
> 


More information about the Gc mailing list