bug#29066: 26.0.90; crash in gc involving buffer local symbols

[Valentin Gatien-Baron]

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.