Re: retagging

[Andy Wingo]

Hi Ludo,

Thanks for the review :)

On Mon 31 Oct 2011 14:38, address@hidden (Ludovic Courtès) writes:

> Andy Wingo <address@hidden> skribis:
>
>> I was thinking that you could tag the SCM values directly, for pairs
>> and structs, instead of requiring that tc3 bits be on the heap as
>> well.
>
> Then you may have to register new displacements, since we don’t enable
> interior pointers (and even if we did, it may not always work, depending
> on the tag value.)

Actually it's the same, because the pair tag (on `retagging') is
equivalent to the displacement of a non-immediate in the car of a pair
(on `master').  Likewise with the struct tag and the displacement of the
SCM_STRUCT_VTABLE_DATA().

>> That all worked out, and is in the wip-retagging branch.  It works fine,
>> except for some issue with guardians that I didn't figure out (nor spend
>> time on).  I didn't test smob finalization either.  I'm not sure if it
>> works or not, because the GC just sees naked pointers, so if a pointer
>> comes back to Guile after coming through the GC, then it won't have
>> those extra tag bits that are associated with the pointer and not the
>> memory.  Dunno what to do about that.
>
> Hmm that seems tricky, since it means that the finalizer would have to
> “guess” the type of the value it gets.  Or maybe the type tag could be
> passed somehow as the finalizer’s void*.

Guile only has finalizers as part of its SMOB, struct, and foreign APIs,
and there we can mask the bits on and off as appropriate.  The one other
significant finalizer usage in Guile is in the guardians code, where I
think we would indeed have to store the original SCM in the
finalizer_data.

>     * libguile/inline.h (scm_is_pair): Remove the (crufty!) GCC workaround,
>       as we no longer check the heap to see if something is a pair.
>
> Move to a different patch?

OK.

> +#define SCM_HAS_TYP3(x, tag)     (SCM_TYP3 (x) == tag)
>
> Parenthesize ‘tag’.

OK.

> +/* Checking if a SCM variable holds an immediate integer: See numbers.h for
> + * the definition of the following macros: SCM_I_FIXNUM_BIT,
>
> No star within comment (I know there are lots of them around...)

It is an old comment, but I am happy to fix it.

> +      ret = scm_words ((scm_t_bits)SCM_STRUCT_DATA (vtable), n + 1);
>
> Space after closing paren.

OK

> +  GC_REGISTER_DISPLACEMENT (2 * sizeof (scm_t_bits));
>
> Why not leave that in scm_init_struct?

Dunno, it seemed clearest to me to have all displacements registered in
one place.  (This two-word displacement is pretty unfortunate though.)

I'll post a revised patch and fix the guardian tests.

Regards,

Andy
-- 
http://wingolog.org/