Message ID | 1397060028-18158-10-git-send-email-wingo@igalia.com |
---|---|
State | Changes Requested, archived |
Headers |
Return-Path: <x14314964@homiemail-mx22.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx22.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 0A88036005B for <siddhesh@wilcox.dreamhost.com>; Wed, 9 Apr 2014 09:15:18 -0700 (PDT) Received: by homiemail-mx22.g.dreamhost.com (Postfix, from userid 14314964) id C058147F32B7; Wed, 9 Apr 2014 09:15:17 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx22.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx22.g.dreamhost.com (Postfix) with ESMTPS id 9E91147F32AE for <gdb@patchwork.siddhesh.in>; Wed, 9 Apr 2014 09:15:17 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id:in-reply-to :references; q=dns; s=default; b=HvBQYoM7S4xHlI37QE5z239se5RVX4w tO/gez9MEMDi9DodoqEdw713rkPohO4f3t5rzqETZgUXxez7ySi1KArZ6SIufBXj nfoQ51X7ysM6Xknw3C+Ye/Xcgq8AOei7eqshQ5F54u8KGmSJd7XSmluVkJTB0oui uyXtUEPFYZyk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id:in-reply-to :references; s=default; bh=B32Q48Qu8JamWWZjC+uT9NCVK9A=; b=ygCGP ncc6KcbrRxW4UIKcYTb+DtNlTc+LSjgU0yoLHlRyLW4YTQHPyLcok1hAR2FzohpL /ZUgW7IVOn+STA8G2d/8Wpn2x6S0ds2K67vmmdxcCsucNOaIgWgDL6KlSySP04eF gsCOQ8DXPagtbyIUnBohpvRQf6DliBzHXZRvDU= Received: (qmail 7929 invoked by alias); 9 Apr 2014 16:14:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-gdb=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 7856 invoked by uid 89); 9 Apr 2014 16:14:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_NEUTRAL autolearn=no version=3.3.2 X-HELO: sasl.smtp.pobox.com Received: from a-pb-sasl-quonix.pobox.com (HELO sasl.smtp.pobox.com) (208.72.237.25) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Apr 2014 16:14:27 +0000 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTP id E495A11067; Wed, 9 Apr 2014 12:14:25 -0400 (EDT) Received: from a-pb-sasl-quonix.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTP id DAE0611065; Wed, 9 Apr 2014 12:14:25 -0400 (EDT) Received: from localhost.localdomain (unknown [88.160.190.192]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTPSA id 7DB3011062; Wed, 9 Apr 2014 12:14:23 -0400 (EDT) From: Andy Wingo <wingo@igalia.com> To: gdb-patches@sourceware.org Cc: Andy Wingo <wingo@igalia.com> Subject: [PATCH 9/9] Remove a useless Guile finalizer Date: Wed, 9 Apr 2014 18:13:48 +0200 Message-Id: <1397060028-18158-10-git-send-email-wingo@igalia.com> In-Reply-To: <1397060028-18158-1-git-send-email-wingo@igalia.com> References: <1397060028-18158-1-git-send-email-wingo@igalia.com> X-Pobox-Relay-ID: 0466A2DC-C002-11E3-84CF-873F0E5B5709-02397024!a-pb-sasl-quonix.pobox.com X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
Andy Wingo
April 9, 2014, 4:13 p.m. UTC
* 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(-)
Comments
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
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);