Subject: Re: bug#78812: feature/igc: [PATCH] Make lface_id_to_name an exact root

From: Helmut Eller <[email-masked]>
Date: Wed, 18 Jun 2025 10:45:41 +0200

On Tue, Jun 17 2025, Pip Cet wrote:

> "Helmut Eller" <[email-masked]> writes:
>
>> This is a proposal to make the variable lface_id_to_name an exact root.
>
> Thank you!
>
> I'll apply this as it is, but I'd suggest one change, which I'd like to
> quickly run by you:
>
> Some of the code in igc_xpalloc_lisp_objs_exact handles item_size !=
> word_size, and some doesn't.  Would you prefer to remove the parameter
> or change the code to handle multi-word items, which might be useful for
> arrays of arrays of Lisp_Object?

Let's remove it.  The item_size is redundant here.  See the amended
patch.  I also added the missing xfree :-).

> I think we should add label arguments to all igc*alloc* functions.  With
> your patch, we don't see the lface-id-to-name root until the first time
> it's been resized, and it would make more sense to see it right away.

I added that as a separate patch.

> There's the general problem of memcpy atomicity, but I don't have a
> solution for that; the complicated code in xpalloc_exact is still
> insufficient, so we might as well remove it.
>
> While the ordinary memcpy function in glibc will always copy large
> chunks, some debug builds use byte-based replacements, and then we have
> a potential problem.  This is particularly annoying because
> stress-testing such builds by triggering collections from a background
> thread might see false positive segfaults.  I haven't seen one in
> practice, but I use such tools very rarely.

Fixing all problematic uses of memcpy would be quite tricky.  Two
obvious cases are larger_vector and copy_hash_table.  I suppose that
memcpy is also a reason why software write barriers would be a lot of
work.

>From e54c26eacf2978243339d7622a74d54430d9367f Mon Sep 17 00:00:00 2001
From: Helmut Eller <[email-masked]>
Date: Tue, 17 Jun 2025 09:29:07 +0200
Subject: [PATCH 1/2] Make lface_id_to_name an exact root

* src/igc.h (igc_xpalloc_lisp_objs_exact): New.
* src/igc.c (igc_xpalloc_lisp_objs_exact): Implementation.
* src/xface.c (init_xfaces): Use igc_xalloc_lisp_objs_exact instead of
igc_xzalloc_ambig.
(Finternal_make_lisp_face): Use igc_xpalloc_lisp_objs_exact instead of
igc_xpalloc_ambig.
---
 src/igc.c    | 20 ++++++++++++++++++++
 src/igc.h    |  5 +++++
 src/xfaces.c |  9 +++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index eb845ff7ae0..5d9bc9774be 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -3503,6 +3503,26 @@ igc_xnrealloc_ambig (void *old_pa, ptrdiff_t nitems, ptrdiff_t item_size)
   return new_pa;
 }
 
+Lisp_Object *
+igc_xpalloc_lisp_objs_exact (Lisp_Object *pa, ptrdiff_t *nitems,
+			     ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
+			     const char *label)
+{
+  ptrdiff_t nitems_old = *nitems;
+  ptrdiff_t nitems_new = nitems_old;
+  ptrdiff_t nbytes
+    = xpalloc_nbytes (pa, &nitems_new, nitems_incr_min, nitems_max, word_size);
+  Lisp_Object *old = pa;
+  Lisp_Object *new = xzalloc (nbytes);
+  root_create_exact (global_igc, new, new + nitems_new, scan_exact, label);
+  for (ptrdiff_t i = 0; i < nitems_old; i++)
+    new[i] = old[i];
+  igc_destroy_root_with_start (old);
+  xfree (old);
+  *nitems = nitems_new;
+  return new;
+}
+
 static void
 finalize_bignum (struct Lisp_Bignum *n)
 {
diff --git a/src/igc.h b/src/igc.h
index 5e24c7c338a..d9471b0ee62 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -96,6 +96,11 @@ #define EMACS_IGC_H
 			 ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
 			 ptrdiff_t item_size);
 
+Lisp_Object *igc_xpalloc_lisp_objs_exact (Lisp_Object *pa, ptrdiff_t *nitems,
+					  ptrdiff_t nitems_incr_min,
+					  ptrdiff_t nitems_max,
+					  const char *label);
+
 typedef int igc_scan_result_t; /* zero means success */
 struct igc_ss;
 typedef igc_scan_result_t (*igc_scan_area_t) (struct igc_ss *ss, void *start,
diff --git a/src/xfaces.c b/src/xfaces.c
index 2ca68193876..41903a6c3d5 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2990,8 +2990,10 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	{
 #ifdef HAVE_MPS
 	  lface_id_to_name
-	    = igc_xpalloc_ambig (lface_id_to_name, &lface_id_to_name_size,
-				 1, MAX_FACE_ID, sizeof *lface_id_to_name);
+	    = igc_xpalloc_lisp_objs_exact (lface_id_to_name,
+					   &lface_id_to_name_size,
+					   1, MAX_FACE_ID,
+					   "lface-id-to-name");
 #else
 	  lface_id_to_name = xpalloc (lface_id_to_name, &lface_id_to_name_size,
 				      1, MAX_FACE_ID, sizeof *lface_id_to_name);
@@ -7361,8 +7363,7 @@ init_xfaces (void)
 	  /* Allocate the lface_id_to_name[] array.  */
 	  lface_id_to_name_size = next_lface_id = nfaces;
 #ifdef HAVE_MPS
-	  lface_id_to_name
-	    = igc_xzalloc_ambig (next_lface_id * sizeof *lface_id_to_name);
+	  lface_id_to_name = igc_xalloc_lisp_objs_exact (next_lface_id);
 #else
 	  lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
 #endif
-- 
2.39.5

>From 93f9a7b55c6c73ed5467993538a68013a7c1535e Mon Sep 17 00:00:00 2001
From: Helmut Eller <[email-masked]>
Date: Wed, 18 Jun 2025 10:25:30 +0200
Subject: [PATCH 2/2] Require an explicit label for igc_xalloc_lisp_objs_exact

This makes igc-roots-stats a bit more useful.

* src/igc.h (igc_xalloc_lisp_objs_exact): Add label argument.
* src/igc.c (igc_xalloc_lisp_objs_exact): Pass the label along.
* src/xfaces.c (init_xfaces): Specify a label.
* src/json.c (json_make_object_workspace_for_slow_path): Specify a label.
---
 src/igc.c    | 5 ++---
 src/igc.h    | 2 +-
 src/json.c   | 6 ++++--
 src/xfaces.c | 3 ++-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 5d9bc9774be..26ffa462281 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -3334,12 +3334,11 @@ igc_grow_rdstack (struct read_stack *rs)
 }
 
 Lisp_Object *
-igc_xalloc_lisp_objs_exact (size_t n)
+igc_xalloc_lisp_objs_exact (size_t n, const char *label)
 {
   size_t size = n * sizeof (Lisp_Object);
   void *p = xzalloc (size);
-  root_create_exact (global_igc, p, (char *) p + size, scan_exact,
-		     "xalloc-exact");
+  root_create_exact (global_igc, p, (char *) p + size, scan_exact, label);
   return p;
 }
 
diff --git a/src/igc.h b/src/igc.h
index d9471b0ee62..178dbf20238 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -89,7 +89,7 @@ #define EMACS_IGC_H
 #ifdef ENABLE_CHECKING
 void igc_check_freeable (void *p);
 #endif
-Lisp_Object *igc_xalloc_lisp_objs_exact (size_t n);
+Lisp_Object *igc_xalloc_lisp_objs_exact (size_t n, const char *label);
 void * igc_xalloc_raw_exact (size_t n);
 
 void *igc_xpalloc_ambig (void *pa, ptrdiff_t *nitems,
diff --git a/src/json.c b/src/json.c
index 1cb72f26860..d7e7c740126 100644
--- a/src/json.c
+++ b/src/json.c
@@ -842,7 +842,8 @@ json_make_object_workspace_for_slow_path (struct json_parser *parser,
 	= xnmalloc (new_workspace_size, sizeof (Lisp_Object));
 #else
       new_workspace_ptr
-	= igc_xalloc_lisp_objs_exact (new_workspace_size);
+	= igc_xalloc_lisp_objs_exact (new_workspace_size,
+				      "json-parser-object-workspace");
 #endif
       memcpy (new_workspace_ptr, parser->object_workspace,
 	      (sizeof (Lisp_Object)
@@ -856,7 +857,8 @@ json_make_object_workspace_for_slow_path (struct json_parser *parser,
 		     sizeof (Lisp_Object));
 #else
       new_workspace_ptr
-	= igc_xalloc_lisp_objs_exact (new_workspace_size);
+	= igc_xalloc_lisp_objs_exact (new_workspace_size,
+				      "json-parser-object-workspace");
       memcpy (new_workspace_ptr, parser->object_workspace,
 	      (sizeof (Lisp_Object)
 	       * parser->object_workspace_current));
diff --git a/src/xfaces.c b/src/xfaces.c
index 41903a6c3d5..396d2d9b16c 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -7363,7 +7363,8 @@ init_xfaces (void)
 	  /* Allocate the lface_id_to_name[] array.  */
 	  lface_id_to_name_size = next_lface_id = nfaces;
 #ifdef HAVE_MPS
-	  lface_id_to_name = igc_xalloc_lisp_objs_exact (next_lface_id);
+	  lface_id_to_name = igc_xalloc_lisp_objs_exact (next_lface_id,
+							 "lface-id-to-name");
 #else
 	  lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
 #endif
-- 
2.39.5