[Gc] Re: Fix for crash in GC_dyld_image_add and some more adjustments

Jim Hourihan jimhourihan at earthlink.net
Thu Jul 23 09:53:32 PDT 2009


Ivan, the attached patch is against the current CVS code + your patch.  
It only involves dyn_load.c. I'm still not completely sure how this is  
supposed to work relative to the way the linux code uses the callback.  
I think this is what you're after.

Also, there are a couple of unrelated problems:

Some of the dyld functions called by dyn_load.c are deprecated in 10.5  
and its not clear what the replacements should be. The existing calls  
still work so no worries there yet.

I'm getting this on linking:

     Undefined symbols:
       "_GC_with_callee_saves_pushed", referenced from:
           _GC_push_regs_and_stack in mark_rts.o
           _GC_do_blocking in pthread_support.o

So I can't test this code in the CVS cut + your patch. However, my  
slightly older cut seems to work fine.

     -Jim

-------------- next part --------------
A non-text attachment was scrubbed...
Name: has_static_roots_darwin.patch
Type: application/octet-stream
Size: 1354 bytes
Desc: not available
Url : http://napali.hpl.hp.com/pipermail/gc/attachments/20090723/46145cea/has_static_roots_darwin.obj
-------------- next part --------------


On Jul 23, 2009, at 8:43 AM, Ivan Maidanski wrote:

> Hi!
>
> The suggested patch grows from the post by Jim Hourihan <jimhourihan at earthlink.net 
> > reporting a crash in GC_dyld_image_add() when a section size is  
> less than a ptr. Also, his question was about GC_has_static_roots  
> feature on darwin.
>
> In brief, I did the following:
> - adjust upper boundary in GC_add_roots_inner/ 
> GC_exclude_static_roots() and return in case of an empty region  
> (since, typically these funcs are used passing [base .. base+size)  
> region without ensuring size % ptr_size == 0 && size > 0);
> - add an assertion for the lowest bound in GC_add_roots_inner/ 
> GC_exclude_static_roots() (the comments in gc.h are updated  
> accordingly);
> - rename GC_exclude_static_roots() to  
> GC_exclude_static_roots_inner() and
> add exported GC_exclude_static_roots() that gain the lock and call  
> its ..._inner();
> - add typedef GC_has_static_roots_func to gc.h (for convenience);
> - define GC_register_has_static_roots_callback() (and  
> GC_has_static_roots_func) for all platforms;
> - refine the comment for GC_has_static_roots callback.
>
> Note: While GC_register_has_static_roots_callback() is now defined  
> (and exported on every platform), the callback is really used by GC  
> only on few platforms (where USE_PROC_FOR_LIBRARIES is undefined),  
> but Jim Hourihan says he's going to implement the feature for darwin.
>
>
> The patch is against the current CVS.
>
> ChangeLog entries:
>        * include/gc.h (GC_has_static_roots_func): New typedef (user  
> filter
>        callback).
>        * include/gc.h (GC_register_has_static_roots_callback): Use
>        GC_has_static_roots_func type.
>        * dyn_load.c (GC_has_static_roots,
>        GC_register_has_static_roots_callback): Ditto.
>        * dyn_load.c (GC_has_static_roots,
>        GC_register_has_static_roots_callback): Define on all  
> platforms.
>        * dyn_load.c (GC_register_dynlib_callback,
>        GC_register_dynamic_libraries, GC_init_dyld): Replace K&R-style
>        functions definition with the ANSI C one.
>        * dyn_load.c (GC_register_dynlib_callback): Use new local  
> variable
>        "callback" (initialized from GC_has_static_roots) to minimize  
> data
>        races.
>        * dyn_load.c (GC_register_dynamic_libraries_dl_iterate_phdr,
>        GC_cond_add_roots): Define as STATIC.
>        * mark_rts.c (GC_remove_roots_inner): Ditto.
>        * dyn_load.c (GC_dyld_image_add): Don't call GC_add_roots() for
>        sections smaller than pointer size (just to avoid acquiring the
>        lock unnecessarily).
>        * dyn_load.c (GC_register_has_static_roots_callback): Put
>        "callback" value to GC_has_static_roots on all platforms.
>        * dyn_load.c (GC_has_static_roots): Update the comments.
>        * include/gc.h (GC_exclude_static_roots, GC_add_roots,
>        GC_remove_roots, GC_register_has_static_roots_callback): Ditto.
>        * include/private/gc_priv.h (struct roots): Ditto.
>        * include/private/gc_priv.h (GC_remove_roots_inner): Move  
> prototype
>        to mark_rts.c and declare it as STATIC.
>        * include/private/gc_priv.h (GC_exclude_static_roots_inner):  
> New
>        prototype.
>        * dyn_load.c (GC_register_dynamic_libraries_dl_iterate_phdr):  
> Use
>        GC_exclude_static_roots_inner() instead of  
> GC_exclude_static_roots.
>        * misc.c (GC_init_inner): Ditto.
>        * mark_rts.c (GC_exclude_static_roots_inner): New function  
> (move
>        all the code from GC_exclude_static_roots(); add the comment.
>        * mark_rts.c (GC_add_roots_inner,  
> GC_exclude_static_roots_inner):
>        add alignment assertion for the lower bound; add assertion  
> for the
>        lower bound to be less than the upper one.
>        * mark_rts.c (GC_add_roots_inner, GC_exclude_static_roots):  
> Adjust
>        the upper bound (round down to be of a pointer-aligned value);
>        return in case of an empty range.
>        * mark_rts.c (GC_exclude_static_roots): Acquire the lock and  
> call
>        GC_exclude_static_roots_inner().
>        * mark_rts.c (GC_remove_roots): Quickly check the bounds and  
> return
>        in case of a do-nothing case (before acquiring the lock).
>
> Bye.
> <diff105_cvs>



More information about the Gc mailing list