On Tue, Oct 31, 2017 at 3:10 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Valentin Gatien-Baron <vgatien-baron@janestreet.com>
> > Date: Tue, 31 Oct 2017 10:52:44 -0400
> > Cc: Eli Zaretskii <eliz@gnu.org>,
> >       29066@debbugs.gnu.org,
> >       Mark Shinwell <mshinwell@janestreet.com>
> >
> >  > That was my first attempt, but various macros like SYMBOL_BLV and
> >  > SET_SYMBOL_BLV insist on val.blv being non-NULL.
> >
> >  SET_SYMBOL_BLV doesn't.  And calling SYMBOL_BLV with a freed symbol is a
> >  bug anyway.
> >
> > ​SET_SYMBOL_BLV insists that the new value is not NULL, even if it
> asserts nothing about the current value.
> >
> > We do call SYMBOL_BLV after freeing, when we re-sweep the symbol, which
> is fine because free does
> > nothing when given NULL, but triggers the assertion​.
> >
> > I would do this, to avoid the assertion failure:
> >
> > diff --git a/src/alloc.c b/src/alloc.c
> > index da0c3ad4b3..72550e812b 100644
> > --- a/src/alloc.c
> > +++ b/src/alloc.c
> > @@ -7030,8 +7030,10 @@ sweep_symbols (void)
> >          {
> >            if (!sym->s.gcmarkbit)
> >              {
> > -
> ​​
> if (sym->s.redirect == SYMBOL_LOCALIZED)
> > +              if (sym->s.redirect == SYMBOL_LOCALIZED &&
> sym->s.val.blv) {
> >                  xfree (SYMBOL_BLV (
> ​​
> &sym->s));
> > +                sym->s.val.blv = NULL;
> > +              }
> >                sym->s.next = symbol_free_list;
> >                symbol_free_list = &sym->s;
> >                symbol_free_list->function = Vdead;
>
> Thanks, but it makes little sense to me to work around our own
> assertions this way.  Why do we have these macros if we sometimes
> don't use them?  And why do we have the assertions if they sometimes
> get in the way?


> > Or changing the redirect type:
> >
> > diff --git a/src/alloc.c b/src/alloc.c
> > index da0c3ad4b3..6966d96c6d 100644
> > --- a/src/alloc.c
> > +++ b/src/alloc.c
> > @@ -7030,8 +7030,11 @@ sweep_symbols (void)
> >          {
> >            if (!sym->s.gcmarkbit)
> >              {
> > -              if (sym->s.redirect == SYMBOL_LOCALIZED)
> > +              if (sym->s.redirect == SYMBOL_LOCALIZED) {
> >                  xfree (SYMBOL_BLV (&sym->s));
> > +                sym->s.redirect = SYMBOL_PLAINVAL;
> > +                SET_SYMBOL_VAL (&sym->s, Qunbound);
> > +              }
>
> We could do several things, but I still didn't hear even a single
> reason why my suggestion isn't OK.  IMO, it's simpler, and the test
> for Vdead is already in at least one other place.
>
> So I went ahead and pushed my change.
>
> Is there anything else we need to do before closing this bug?
>

​The slight downside of your fix is that there are dangling pointers that
point to valid-looking things in the debugger. It's runtime behavior​ looks
fine otherwise.
​But I am not going to insist. There's nothing else to do, you can close
the bug. Thanks!



>
> Thanks.
>