[9/9] Remove a useless Guile finalizer

  * 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(-)
  

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)
>  
>  /* 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?
My experience is that the S/N ratio is just too low, but maybe there's
a canonical suppression file that works well enough.
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.

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).
[The debug version of the GC may have similar support, but the
benefit of this is that it's "always on" for negligible cost.]
I'm not opposed to removing the code at some later point after things
are really stable and after there's data that they've stopped being
useful.
  

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)
>>  
>>  /* 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.

> 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.

Thanks,
Ludo’.
  

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)
>>>  
>>>  /* 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.

>> 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.
It's an open question, I guess, whether modifying the smob like this in
a finalizer could confuse gc.
  

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)
>>>>  
>>>>  /* 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’.
  

On Tue, Apr 15, 2014 at 2:28 AM, Ludovic Courtès <ludo@gnu.org> wrote:
>> 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.”

I was *not* referring to that particular function.
This function can indeed go.
Again, sorry for the confusion.
  

On Sat 12 Apr 2014 22:18, Doug Evans <xdje42@gmail.com> writes:

> 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.)
>
> How useful is valgrind with Guile's GC?

It seems to work fine now, and in my recollection, but I haven't used
memcheck in anger recently.  I usually use callgrind ;).  The GC heap
doesn't present any problems for it.  Conservative root scanning on the
stack and in the static data sections is an issue; you have to add some
things to the suppressions for sanity.

> 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.

Thing is, you don't control very much about the environment of the
finalizer.  In particular it could be called from some other thread, and
in general a finalizer runs concurrently with arbitrary other parts of
your program; see
http://wingolog.org/archives/2012/02/16/unexpected-concurrency for some
discussion.  Avoiding finalizers can actually improve correctness in
this regard.

> 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").

Sure.  I think I looked for finalizers that _only_ had this kind of
body, and this was the only one.  It's harmless otherwise.  Finalizers
do impose a perf penalty on the GC, but it's much less than mark
functions.

Andy