Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

Pip Cet wrote:

> (To reproduce the promised segfault, add
> 
> (defvar custom-dummy (make-hash-table :test 'eq))
> (puthash custom-dummy custom-dummy custom-dummy)
> 
> to custom.el, then rebuild and run (clrhash custom-dummy) in Emacs -Q.
> Trivial to fix, but apparently not obvious enough to have been caught
> in the original changes).

Thanks for reporting that. I installed the attached patch, which should fix that 
bug and clean up a few nearby glitches. This patch is a bit less trivial but 
should be a smidge faster than the trivial patch would have been.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-clrhash-bug-when-hash-table-needs-rehashing.patch --]
[-- Type: text/x-patch; name="0001-Fix-clrhash-bug-when-hash-table-needs-rehashing.patch", Size: 4348 bytes --]

From ac71011bc1726bd767446407500c20cbbdca74a2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 18:54:08 -0700
Subject: [PATCH] Fix clrhash bug when hash table needs rehashing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
* src/fns.c (maybe_resize_hash_table): Prefer ASET to gc_aset
where either will do.  Simplify appending of Qunbound values.
Put index_size calculation closer to where it’s needed.
(hash_clear): If hash_rehash_needed_p (h), don’t clear the
nonexistent hash vector.  Use memclear to speed up clearing.
* src/lisp.h (HASH_TABLE_SIZE): Check that the size is positive,
and tell that to the compiler.
---
 src/fns.c  | 28 ++++++++++++----------------
 src/lisp.h |  9 +++++----
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index b606d6299c..8ca0953fe8 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4198,21 +4198,20 @@ maybe_resize_hash_table (struct Lisp_Hash_Table *h)
 					  new_size);
       ptrdiff_t next_size = ASIZE (next);
       for (ptrdiff_t i = old_size; i < next_size - 1; i++)
-	gc_aset (next, i, make_fixnum (i + 1));
-      gc_aset (next, next_size - 1, make_fixnum (-1));
-      ptrdiff_t index_size = hash_index_size (h, next_size);
+	ASET (next, i, make_fixnum (i + 1));
+      ASET (next, next_size - 1, make_fixnum (-1));
 
       /* Build the new&larger key_and_value vector, making sure the new
          fields are initialized to `unbound`.  */
       Lisp_Object key_and_value
 	= larger_vecalloc (h->key_and_value, 2 * (next_size - old_size),
 			   2 * next_size);
-      for (ptrdiff_t i = ASIZE (h->key_and_value);
-            i < ASIZE (key_and_value); i++)
+      for (ptrdiff_t i = 2 * old_size; i < 2 * next_size; i++)
         ASET (key_and_value, i, Qunbound);
 
       Lisp_Object hash = larger_vector (h->hash, next_size - old_size,
 					next_size);
+      ptrdiff_t index_size = hash_index_size (h, next_size);
       h->index = make_vector (index_size, make_fixnum (-1));
       h->key_and_value = key_and_value;
       h->hash = hash;
@@ -4404,17 +4403,14 @@ hash_clear (struct Lisp_Hash_Table *h)
 {
   if (h->count > 0)
     {
-      ptrdiff_t i, size = HASH_TABLE_SIZE (h);
-
-      for (i = 0; i < size; ++i)
-	{
-	  set_hash_next_slot (h, i, i < size - 1 ? i + 1 : -1);
-	  set_hash_key_slot (h, i, Qunbound);
-	  set_hash_value_slot (h, i, Qnil);
-	  set_hash_hash_slot (h, i, Qnil);
-	}
-
-      for (i = 0; i < ASIZE (h->index); ++i)
+      ptrdiff_t size = HASH_TABLE_SIZE (h);
+      if (!hash_rehash_needed_p (h))
+	memclear (XVECTOR (h->hash)->contents, size * word_size);
+      memclear (XVECTOR (h->key_and_value)->contents, size * 2 * word_size);
+      for (ptrdiff_t i = 0; i < size; i++)
+	set_hash_next_slot (h, i, i < size - 1 ? i + 1 : -1);
+
+      for (ptrdiff_t i = 0; i < ASIZE (h->index); i++)
 	ASET (h->index, i, make_fixnum (-1));
 
       h->next_free = 0;
diff --git a/src/lisp.h b/src/lisp.h
index 56ad99b8e3..ae5a81e7b5 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2307,7 +2307,7 @@ #define DEFSYM(sym, name) /* empty */
      weakness of the table.  */
   Lisp_Object weak;
 
-  /* Vector of hash codes.
+  /* Vector of hash codes, or nil if the table needs rehashing.
      If the I-th entry is unused, then hash[I] should be nil.  */
   Lisp_Object hash;
 
@@ -2327,8 +2327,7 @@ #define DEFSYM(sym, name) /* empty */
      'index' are special and are either ignored by the GC or traced in
      a special way (e.g. because of weakness).  */
 
-  /* Number of key/value entries in the table.  This number is
-     negated if the table needs rehashing.  */
+  /* Number of key/value entries in the table.  */
   ptrdiff_t count;
 
   /* Index of first free entry in free list, or -1 if none.  */
@@ -2413,7 +2412,9 @@ HASH_HASH (const struct Lisp_Hash_Table *h, ptrdiff_t idx)
 INLINE ptrdiff_t
 HASH_TABLE_SIZE (const struct Lisp_Hash_Table *h)
 {
-  return ASIZE (h->next);
+  ptrdiff_t size = ASIZE (h->next);
+  eassume (0 < size);
+  return size;
 }
 
 void hash_table_rehash (struct Lisp_Hash_Table *h);
-- 
2.17.1