bug#68690: Segmentation fault building with native-comp

["Mattias Engdegård" <mattias.engdegard@gmail.com>]

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

> The original DOHASH's comment indeed said it didn't support that
> operation, yet the code used DOHASH to implement `maphash`, which *does*
> support such operations, and it used DOHASH in places which perform such
> operations, so I think it's clear we do want to support `puthash` there.

Sorry, my fault -- indeed maphash 'supports' irregular mutation in the sense that it shouldn't crash or corrupt Emacs if the rules are violated. I can't reproduce the reported crash(es) on my platform but is my understanding correct that no other uses of DOHASH caused any trouble?

This patch reverts my last change to Fmaphash and yours to DOHASH. It's perfectly fine to forego DOHASH in Fmaphash, it's chums with the hash-table implementation. Assuming that the problems were confined to Fmaphash, this should be safe to apply.

What I certainly would accept is an assertion in DOHASH that verifies the assumptions but doesn't result in any code at all with checking disabled. I'll add that if you think it's warranted (and maybe even if you don't).



[-- Attachment #2: 0001-Revert-to-fast-and-simple-DOHASH-keeping-Fmaphash-ro.patch --]
[-- Type: application/octet-stream, Size: 4139 bytes --]

From 1b44dc419c55cba7e41e8fd8376ebfbae12f04e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 25 Jan 2024 18:56:03 +0100
Subject: [PATCH] Revert to fast and simple DOHASH keeping Fmaphash robust
 (bug#68690)

`maphash` mustn't crash if the supplied function does odd things but
that doesn't mean we have to make DOHASH more expensive.
This change essentially reverts ad004f10f3 and the Fmaphash part of
fec87a4b36.

* src/lisp.h (DOHASH): Go back to fast design with more rules.
* src/fns.c (Fmaphash): Ditch DOHASH in favour of explicit loop.
---
 src/fns.c  | 11 +++++++++--
 src/lisp.h | 38 ++++++++++++++------------------------
 2 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 859df6748f7..519d85df288 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -5662,8 +5662,15 @@ DEFUN ("maphash", Fmaphash, Smaphash, 2, 2, 0,
   (Lisp_Object function, Lisp_Object table)
 {
   struct Lisp_Hash_Table *h = check_hash_table (table);
-  DOHASH (h, k, v)
-    call2 (function, k, v);
+  /* We can't use DOHASH here since FUNCTION may violate the rules and
+     we shouldn't crash as a result (although the effects are
+     unpredictable).  */
+  for (ptrdiff_t i = 0; i < HASH_TABLE_SIZE (h); i++)
+    {
+      Lisp_Object k = HASH_KEY (h, i);
+      if (!hash_unused_entry_key_p (k))
+        call2 (function, k, HASH_VALUE (h, i));
+    }
   return Qnil;
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index d07d9d14e2f..f822417ffb1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2604,30 +2604,20 @@ hash_from_key (struct Lisp_Hash_Table *h, Lisp_Object key)
 }
 
 /* Iterate K and V as key and value of valid entries in hash table H.
-   The body may mutate the hash-table.  */
-#define DOHASH(h, k, v)							 \
-  for (Lisp_Object *dohash_##k##_##v##_base = (h)->key_and_value,	 \
-                   *dohash_##k##_##v##_kv   = dohash_##k##_##v##_base,	 \
-                   *dohash_##k##_##v##_end  = dohash_##k##_##v##_base	 \
-                                              + 2 * HASH_TABLE_SIZE (h), \
-                   k, v;						 \
-       dohash_##k##_##v##_kv < dohash_##k##_##v##_end			 \
-       && (dohash_##k##_##v##_base == (h)->key_and_value                 \
-           /* The `key_and_value` table has been reallocated!  */        \
-           || (dohash_##k##_##v##_kv                                     \
-                  = (dohash_##k##_##v##_kv - dohash_##k##_##v##_base)	 \
-                    + (h)->key_and_value,                                \
-               dohash_##k##_##v##_base = (h)->key_and_value,             \
-               dohash_##k##_##v##_end  = dohash_##k##_##v##_base	 \
-                                         + 2 * HASH_TABLE_SIZE (h),      \
-               /* Check again, in case the table has shrunk.  */         \
-               dohash_##k##_##v##_kv < dohash_##k##_##v##_end))          \
-       && (k = dohash_##k##_##v##_kv[0],                                 \
-           v = dohash_##k##_##v##_kv[1], /*maybe unused*/ (void)v,       \
-           true);			                                 \
-        dohash_##k##_##v##_kv += 2)				         \
-    if (hash_unused_entry_key_p (k))				         \
-      ;								         \
+   The body may remove the current entry or alter its value slot, but not
+   mutate TABLE in any other way.  */
+#define DOHASH(h, k, v)							\
+  for (Lisp_Object *dohash_##k##_##v##_kv = (h)->key_and_value,		\
+                   *dohash_##k##_##v##_end = dohash_##k##_##v##_kv	\
+                                             + 2 * HASH_TABLE_SIZE (h),	\
+                   k, v;						\
+       dohash_##k##_##v##_kv < dohash_##k##_##v##_end			\
+       && (k = dohash_##k##_##v##_kv[0],				\
+           v = dohash_##k##_##v##_kv[1], /*maybe unsed*/ (void)v,       \
+           true);			                                \
+        dohash_##k##_##v##_kv += 2)					\
+    if (hash_unused_entry_key_p (k))					\
+      ;									\
     else
 
 
-- 
2.32.0 (Apple Git-132)