Re: [PATCH 9/9] Remove a useless Guile finalizer

[ludo@gnu.org (Ludovic Courtès)]

Doug Evans <xdje42@gmail.com> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Doug Evans <xdje42@gmail.com> skribis:
>>
>>> Andy Wingo <wingo@igalia.com> writes:
>>>
>>>> * gdb/guile/scm-symtab.c (stscm_free_sal_smob): Remove useless free
>>>>   function.  (This was the only useless free function.)
>>>> ---
>>>>  gdb/guile/scm-symtab.c | 14 --------------
>>>>  1 file changed, 14 deletions(-)
>>>>
>>>> diff --git a/gdb/guile/scm-symtab.c b/gdb/guile/scm-symtab.c
>>>> index 8910973..876bf64 100644
>>>> --- a/gdb/guile/scm-symtab.c
>>>> +++ b/gdb/guile/scm-symtab.c
>>>> @@ -386,19 +386,6 @@ gdbscm_symtab_static_block (SCM self)
>>>>  \f
>>>>  /* Administrivia for sal (symtab-and-line) smobs.  */
>>>>  
>>>> -/* The smob "free" function for <gdb:sal>.  */
>>>> -
>>>> -static size_t
>>>> -stscm_free_sal_smob (SCM self)
>>>> -{
>>>> -  sal_smob *s_smob = (sal_smob *) SCM_SMOB_DATA (self);
>>>> -
>>>> -  /* Not necessary, done to catch bugs.  */
>>>> -  s_smob->symtab_scm = SCM_UNDEFINED;
>>>> -
>>>> -  return 0;
>>>> -}
>>>> -
>>>>  /* The smob "print" function for <gdb:sal>.  */
>>>>  
>>>>  static int
>>>> @@ -692,7 +679,6 @@ gdbscm_initialize_symtabs (void)
>>>>    scm_set_smob_print (symtab_smob_tag, stscm_print_symtab_smob);
>>>>  
>>>>    sal_smob_tag = gdbscm_make_smob_type (sal_smob_name, sizeof (sal_smob));
>>>> -  scm_set_smob_free (sal_smob_tag, stscm_free_sal_smob);
>>>>    scm_set_smob_print (sal_smob_tag, stscm_print_sal_smob);
>>>>  
>>>>    gdbscm_define_functions (symtab_functions, 1);
>>>
>>> How useful is valgrind with Guile's GC?
>>
>> I think there’s a suppression file for libgc floating around, but I
>> haven’t really used it myself.
>>
>>> And given that we have this hook, it seems a shame to just throw out
>>> such useful protections against use-after-free (I'm pretty sure early on
>>> I found one bug with them), especially given the subtleties with GC,
>>> and gdb's extensive need to have references to SCM objects from outside
>>> GC-controlled code.
>>
>> IMO it doesn’t help much to have finalizers (free functions) like this.
>> For debugging, you could always use a guardian, which has the same effect.
>
> Ah.  I probably didn't make myself clear.  Sorry 'bout that.
> For smobs that are only ever accessed from Scheme then sure,
> no disagreement there.
>
> For smobs accessed from gdb ... that's the context in which my
> comments were made.  Alas, any such object probably requires a free
> function anyway.

Why do you say it “requires” a free function?  The comment says “Not
necessary, done to catch bugs.”

>>> If we're going to have a rule that such code is disallowed,
>>> there is more such code that needs to be removed besides the above
>>> (grep for "catch bugs").
>>>
>>> But is this something we want?
>>> At this stage in gdb+guile's development, I like the protection,
>>> and the cost is within epsilon of zero (to me anyway).
>>
>> Again, I don’t think it provides any significant protection or debugging
>> aid.
>
> I'm going to keep my NULL'ing out of smob elements in free functions
> (I see I'm missing a few), but only for such smobs that can be accessed
> from outside Scheme.

Can a ‘sal_smob’ be referred to by GDB objects once the corresponding
SMOB has been reclaimed?  My impression is that the answer is “no.”
Thus I’m not sure what you mean by “accessed from outside Scheme.”

> It's an open question, I guess, whether modifying the smob like this in
> a finalizer could confuse gc.

No problem with that.

Thanks,
Ludo’.