bug#36447: 27.0.50; New "Unknown keyword" errors

[Pip Cet]

On Fri, Jul 5, 2019 at 6:00 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > A naïve question: wouldn't the problem go away if we modified purecopy
> > not to do the above, i.e. not to merge the next vector of a hash table
> > with that of another?
>
> It might do the trick, yes (the patch below does that, for example, tho
> I think Pip's patch is a better approach).
> Note that it would mean we still modify data in the purespace, which
> ideally shouldn't happen.
>
> The problem fundamentally is that `purecopy` assumes that the argument
> passed to it will never again be modified (it doesn't have to be
> immutable before, but it should be afterwards) but if we rehash
> afterwards then we break this promise.

Surely you mean you're allowed to modify the argument to purecopy,
just not the return value of it?

> His patch can/should be made slightly more efficient by only doing the
> Fcopy_sequence on those hash-tables that are in purespace.

How do we test for that? If I'm reading the pdumper code correctly,
it'll put all "unstable" hash tables at the end of the dump, far away
from the actually pure objects.

I'm not sure how difficult it would be, but we could dump the ->index,
->next, ->hash vectors as Qnil (so not include the actual vectors in
the dump), which would make the dump slightly smaller and give us a
better test than h->count < 0:

INLINE bool
hash_rehash_needed_p (const struct Lisp_Hash_Table *h)
{
  return NILP (h->index);
}

(I used h->index because it's in the same cache line as h->count).

But that's hard to do without writing a shrink_hash_table function to
be used before dumping. I think hash tables should be shrinkable, but
that's beyond the scope of this bug.

> diff --git a/src/fns.c b/src/fns.c
> index 2fc000a7f4..cc4fd7f2d7 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -4218,6 +4218,11 @@ maybe_resize_hash_table (struct Lisp_Hash_Table *h)
>      }
>  }
>
> +/* Recompute the hashes (and hence also the "next" pointers).

And the "index" pointers, if we're listing all of them.

> +   Normally there's never a need to recompute hashes
> +   This is done only on first-access to a hash-table loaded from
> +   the "pdump", because the object's addresses may have changed, thus
> +   affecting their hash.  */
>  void
>  hash_table_rehash (struct Lisp_Hash_Table *h)
>  {
> diff --git a/src/alloc.c b/src/alloc.c
> index 64aaa8acdf..b0114a9459 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -5348,9 +5348,24 @@ purecopy_hash_table (struct Lisp_Hash_Table *table)
>
>    pure->header = table->header;
>    pure->weak = purecopy (Qnil);
> +  /* After reloading the pdumped data, objects may ehave changed location
> +     in memory, so their hash may have changed.  So the `hash`, `next`, and
> +     `index` vectors are not immutable and can't safely be hash-cons'ed.  */
> +  if (HASH_TABLE_P (Vpurify_flag))
> +    {
> +      Fremhash (table->hash, Vpurify_flag);
> +      Fremhash (table->next, Vpurify_flag);
> +      Fremhash (table->index, Vpurify_flag);
> +    }
>    pure->hash = purecopy (table->hash);
>    pure->next = purecopy (table->next);

Slightly contrived problem here: if the single key in a single-entry
EQ hash is -7, or an expression that happens to have hash value -1,
pure->hash and pure->next would be EQ after these two lines, and
updating the hash would corrupt the hash table...

>    pure->index = purecopy (table->index);

Same for ->index and ->hash if both are equal to [0].


> +  if (HASH_TABLE_P (Vpurify_flag))
> +    {
> +      Fremhash (table->hash, Vpurify_flag);
> +      Fremhash (table->next, Vpurify_flag);
> +      Fremhash (table->index, Vpurify_flag);
> +    }