GNU bug report logs - #79667
31.0.50; igc Xawd3d EMACS crashes when the buffer scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)

Previous Next

To reply to this bug, email your comments to 79667 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Josh P." <zcheeze0 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; igc Xawd3d EMACS crashes when the buffer scrollbar is used:
 Emacs fatal error: assertion failed: WINDOWP (a)
Date: Tue, 21 Oct 2025 05:41:19 -0600

[Message part 1 (text/plain, inline)]

Hello, there is a fatal error in the igc branch that, when scrolling
under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:

emacs -Q

C-x i igc.c RET (it happens when there is quite a bit to scroll in a
buffer)

scroll "violently" downwards and upwards with the scrollwheel, then do
it "violently" with with the pointer being used for the scrollbar.

SIGABRT.

The symptoms are, that when the buffer is long, and you attempt to
scroll it, at times, will crash. It is the latest checkout of
origin/feature/igc.

[stacktrace-and-data.txt (text/plain, attachment)]

Message #8 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: "Josh P." <zcheeze0 <at> gmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 79667 <at> debbugs.gnu.org, Pip Cet <pipcet <at> protonmail.com>
Subject: Re: bug#79667: 31.0.50; igc Xawd3d EMACS crashes when the buffer
 scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)
Date: Thu, 23 Oct 2025 16:19:52 +0200

[Message part 1 (text/plain, inline)]

On Tue, Oct 21 2025, Josh P. wrote:

> Hello, there is a fatal error in the igc branch that, when scrolling
> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>
> emacs -Q
>
> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
> buffer)
>
> scroll "violently" downwards and upwards with the scrollwheel, then do
> it "violently" with with the pointer being used for the scrollbar.
>
> SIGABRT.
>
> The symptoms are, that when the buffer is long, and you attempt to
> scroll it, at times, will crash. It is the latest checkout of
> origin/feature/igc.
>
> Hello, there is a fatal error in the igc branch that, when scrolling
> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>
> emacs -Q
>
> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
> buffer)
>
> scroll "violently" downwards and upwards with the scrollwheel, then do
> it "violently" with with the pointer being used for the scrollbar.
>
> SIGABRT.
>
> The symptoms are, that when the buffer is long, and you attempt to
> scroll it, at times, will crash. It is the latest checkout of
> origin/feature/igc.

Pip and Gerd, what would you think of introducing a GC-handle type as
in the attached patch?

I imagine GC handles to be used mostly for GUI toolkit callbacks, like
the one in this bug.  Perhaps GC handles could also be used in some
way for module_global_references.

The patch implements GC handles that support reference counting; it
was easy to do but was not needed for the bug.

JDK and Oilpan also use GC handles (though in a much more
sophisticated way). See:

  https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/handles.hpp
  https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#Persistent_WeakPersistent

[0001-Introduce-a-GC-handle-type.patch (text/x-diff, inline)]

From bc6d01243ea894d9a53fee3d308ca5120e9ae052 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut <at> gmail.com>
Date: Thu, 23 Oct 2025 15:48:43 +0200
Subject: [PATCH] Introduce a GC handle type

---
 src/Makefile.in  |   2 +-
 src/emacs.c      |   2 +
 src/gc-handles.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++
 src/gc-handles.h |  17 +++++
 src/xterm.c      |  27 ++++++--
 5 files changed, 208 insertions(+), 5 deletions(-)
 create mode 100644 src/gc-handles.c
 create mode 100644 src/gc-handles.h

diff --git a/src/Makefile.in b/src/Makefile.in
index 9b1aee0aa40..8101d4406da 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -467,7 +467,7 @@ base_obj =
 	$(XWIDGETS_OBJ) 						       \
 	profiler.o decompress.o $(IGCOBJ) 				       \
 	thread.o systhread.o sqlite.o  treesit.o			       \
-	itree.o json.o 							       \
+	itree.o json.o gc-handles.o					       \
 	$(MSDOS_OBJ) $(MSDOS_X_OBJ) $(NS_OBJ) $(CYGWIN_OBJ) $(FONT_OBJ)        \
 	$(W32_OBJ) $(WINDOW_SYSTEM_OBJ) $(XGSELOBJ)			       \
 	$(HAIKU_OBJ) $(PGTK_OBJ) $(ANDROID_OBJ)
diff --git a/src/emacs.c b/src/emacs.c
index effd60561e2..5951c6c8c82 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -117,6 +117,7 @@ #define MAIN_PROGRAM
 
 #include "getpagesize.h"
 #include "gnutls.h"
+#include "gc-handles.h"
 
 #ifdef HAVE_HAIKU
 #include <kernel/OS.h>
@@ -2498,6 +2499,7 @@ android_emacs_init (int argc, char **argv, char *dump_file)
       syms_of_profiler ();
       syms_of_pdumper ();
       syms_of_json ();
+      syms_of_gc_handles ();
 
       keys_of_keyboard ();
 
diff --git a/src/gc-handles.c b/src/gc-handles.c
new file mode 100644
index 00000000000..79a011e7324
--- /dev/null
+++ b/src/gc-handles.c
@@ -0,0 +1,165 @@
+/* GC-handles can be used to retain Lisp_Objects. A GC-handle keeps
+   its Lisp_Object alive until the GC-handle is freed.  */
+
+/* Deficiencies of the current implementation:
+   - no thread safety
+   - handles are not dumpable
+*/
+
+#include "gc-handles.h"
+
+struct gc_handle
+{
+  size_t index;
+  size_t refcount;
+};
+
+struct handle_table
+{
+  struct gc_handle **handles;
+  Lisp_Object objects;
+  size_t length;
+};
+
+static struct handle_table _global_handle_table;
+
+#define INITIAL_TABLE_SIZE 0
+
+void
+syms_of_gc_handles (void)
+{
+  _global_handle_table.objects = Qnil;
+  staticpro (&_global_handle_table.objects);
+}
+
+static size_t
+handle_table_size (struct handle_table *tab)
+{
+  return ASIZE (tab->objects);
+}
+
+static void
+init_handle_table (struct handle_table *tab)
+{
+  size_t size = INITIAL_TABLE_SIZE;
+  tab->handles = xzalloc (size * sizeof *tab->handles);
+  tab->objects = make_vector (size, Qnil);
+  tab->length = 0;
+}
+
+static struct handle_table *
+global_handle_table (void)
+{
+  struct handle_table *tab = &_global_handle_table;
+  if (tab->handles == 0)
+    init_handle_table (tab);
+  return tab;
+}
+
+static void
+grow_handle_table (struct handle_table *tab)
+{
+  size_t old_size = handle_table_size (tab);
+  size_t new_size = old_size * 133 / 100 + 1;
+  Lisp_Object v = make_vector (new_size, Qnil);
+  struct gc_handle **old = tab->handles;
+  struct gc_handle **new = xzalloc (new_size * sizeof *new);
+  for (size_t i = 0; i < old_size; i++)
+    {
+      ASET (v, i, AREF (tab->objects, i));
+      new[i] = old[i];
+    }
+  tab->objects = v;
+  tab->handles = new;
+  xfree (old);
+  eassert (handle_table_size (tab) == new_size);
+}
+
+static size_t
+alloc_index (struct handle_table *tab)
+{
+  size_t idx = tab->length;
+  if (idx == handle_table_size (tab))
+    grow_handle_table (tab);
+  tab->length++;
+  return idx;
+}
+
+struct gc_handle *
+alloc_gc_handle (Lisp_Object obj)
+{
+  if (NILP (obj))
+    return NULL;
+  struct handle_table *tab = global_handle_table ();
+  size_t idx = alloc_index (tab);
+  ASET (tab->objects, idx, obj);
+  struct gc_handle *h = xmalloc (sizeof *h);
+  h->index = idx;
+  h->refcount = 1;
+  tab->handles[idx] = h;
+  return h;
+}
+
+struct gc_handle *
+clone_gc_handle (struct gc_handle *h)
+{
+  if (h == NULL)
+    return NULL;
+  h->refcount++;
+  return h;
+}
+
+static void
+maybe_shrink_handle_table (struct handle_table *tab)
+{
+  /* Not yet implemented */
+}
+
+static void
+swap_remove (struct handle_table *tab, size_t idx)
+{
+  eassert (idx < tab->length);
+  size_t last = tab->length - 1;
+  if (idx != last)
+    {
+      ASET (tab->objects, idx, AREF (tab->objects, last));
+      struct gc_handle *h = tab->handles[last];
+      tab->handles[idx] = h;
+      h->index = idx;
+    }
+  ASET (tab->objects, last, Qnil);
+  tab->handles[last] = NULL;
+  tab->length--;
+  maybe_shrink_handle_table (tab);
+}
+
+void
+free_gc_handle (struct gc_handle *h)
+{
+  if (h == NULL)
+    return;
+  h->refcount--;
+  if (h->refcount != 0)
+    return;
+  size_t idx = h->index;
+  struct handle_table *tab = global_handle_table ();
+  swap_remove (tab, idx);
+  h->index = ~idx; /* invalidate index */
+  xfree (h);
+}
+
+Lisp_Object
+gc_handle_ref (struct gc_handle *h)
+{
+  if (h == NULL)
+    return Qnil;
+  struct handle_table *tab = global_handle_table ();
+  return AREF (tab->objects, h->index);
+}
+
+struct gc_handle *
+alloc_gc_handle_for_pvec (struct vectorlike_header *h)
+{
+  Lisp_Object obj = make_lisp_ptr (h, Lisp_Vectorlike);
+  return alloc_gc_handle (obj);
+}
diff --git a/src/gc-handles.h b/src/gc-handles.h
new file mode 100644
index 00000000000..a4b62a1f6d5
--- /dev/null
+++ b/src/gc-handles.h
@@ -0,0 +1,17 @@
+#ifndef EMACS_GC_HANDLES_H
+#define EMACS_GC_HANDLES_H
+
+#include "config.h"
+#include "lisp.h"
+
+struct gc_handle;
+
+struct gc_handle *alloc_gc_handle (Lisp_Object);
+struct gc_handle *alloc_gc_handle_for_pvec (struct vectorlike_header *);
+struct gc_handle *clone_gc_handle (struct gc_handle *);
+void free_gc_handle (struct gc_handle *);
+Lisp_Object gc_handle_ref (struct gc_handle *);
+
+void syms_of_gc_handles (void);
+
+#endif
diff --git a/src/xterm.c b/src/xterm.c
index 3798439d03f..684943e7afe 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -719,6 +719,7 @@ Copyright (C) 1989, 1993-2025 Free Software Foundation, Inc.
 #include "sysselect.h"
 #include "menu.h"
 #include "pdumper.h"
+#include "gc-handles.h"
 
 #ifdef USE_X_TOOLKIT
 #include <X11/Shell.h>
@@ -15928,7 +15929,9 @@ xg_end_scroll_callback (GtkWidget *widget,
 static void
 xaw_jump_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 {
-  struct scroll_bar *bar = client_data;
+  struct gc_handle *bar_handle = client_data;
+  Lisp_Object bar_obj = gc_handle_ref (bar_handle);
+  struct scroll_bar *bar = XSCROLL_BAR (bar_obj);
   float *top_addr = call_data;
   float top = *top_addr;
   float shown;
@@ -15995,7 +15998,9 @@ xaw_jump_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 static void
 xaw_scroll_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 {
-  struct scroll_bar *bar = client_data;
+  struct gc_handle *bar_handle = client_data;
+  Lisp_Object bar_obj = gc_handle_ref (bar_handle);
+  struct scroll_bar *bar = XSCROLL_BAR (bar_obj);
   /* The position really is stored cast to a pointer.  */
   int position = (intptr_t) call_data;
   Dimension height, width;
@@ -16050,6 +16055,14 @@ xaw_scroll_callback (Widget widget, XtPointer client_data, XtPointer call_data)
     }
 }
 
+static void
+xaw_destroy_scrollbar_callback (Widget widget, XtPointer client_data,
+				XtPointer call_data)
+{
+  struct gc_handle *handle = client_data;
+  free_gc_handle (handle);
+}
+
 #endif /* not USE_GTK and not USE_MOTIF */
 
 #define SCROLL_BAR_NAME "verticalScrollBar"
@@ -16268,10 +16281,16 @@ x_create_toolkit_scroll_bar (struct frame *f, struct scroll_bar *bar)
       }
   }
 
+  struct gc_handle *bar_handle
+    = alloc_gc_handle_for_pvec (&bar->header);
   /* Define callbacks.  */
-  XtAddCallback (widget, XtNjumpProc, xaw_jump_callback, (XtPointer) bar);
+  XtAddCallback (widget, XtNjumpProc, xaw_jump_callback,
+		 (XtPointer) bar_handle);
   XtAddCallback (widget, XtNscrollProc, xaw_scroll_callback,
-		 (XtPointer) bar);
+		 (XtPointer) bar_handle);
+  XtAddCallback (widget, XtNdestroyCallback,
+		 xaw_destroy_scrollbar_callback,
+		 (XtPointer) bar_handle);
 
   /* Realize the widget.  Only after that is the X window created.  */
   XtRealizeWidget (widget);
-- 
2.47.3

Message #11 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 79667 <at> debbugs.gnu.org, Pip Cet <pipcet <at> protonmail.com>,
 "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50; igc Xawd3d EMACS crashes when the buffer
 scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)
Date: Thu, 23 Oct 2025 16:46:53 +0200

Helmut Eller <eller.helmut <at> gmail.com> writes:

> Pip and Gerd, what would you think of introducing a GC-handle type as
> in the attached patch?
>
> I imagine GC handles to be used mostly for GUI toolkit callbacks, like
> the one in this bug.  Perhaps GC handles could also be used in some
> way for module_global_references.
>
> The patch implements GC handles that support reference counting; it
> was easy to do but was not needed for the bug.
>
> JDK and Oilpan also use GC handles (though in a much more
> sophisticated way). See:
>
>   https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/handles.hpp
>   https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#Persistent_WeakPersistent

Hallo Helmut.

I could imagine they are useful, as a general concept, potentially
unifying different bespoke solutions.

It's probably not so useful for ns or mac because Cocoa works a bit
differently in this regard, not using callbacks, but
classes/objects/delegates implementing methods. Insofar I can't add much
from practical experience.

Message #14 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 79667 <at> debbugs.gnu.org, "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50;
 igc Xawd3d EMACS crashes when the buffer scrollbar is used: Emacs
 fatal error: assertion failed: WINDOWP (a)
Date: Thu, 23 Oct 2025 19:10:17 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> On Tue, Oct 21 2025, Josh P. wrote:
>
>> Hello, there is a fatal error in the igc branch that, when scrolling
>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>
>> emacs -Q
>>
>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>> buffer)
>>
>> scroll "violently" downwards and upwards with the scrollwheel, then do
>> it "violently" with with the pointer being used for the scrollbar.
>>
>> SIGABRT.
>>
>> The symptoms are, that when the buffer is long, and you attempt to
>> scroll it, at times, will crash. It is the latest checkout of
>> origin/feature/igc.
>>
>> Hello, there is a fatal error in the igc branch that, when scrolling
>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>
>> emacs -Q
>>
>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>> buffer)
>>
>> scroll "violently" downwards and upwards with the scrollwheel, then do
>> it "violently" with with the pointer being used for the scrollbar.
>>
>> SIGABRT.
>>
>> The symptoms are, that when the buffer is long, and you attempt to
>> scroll it, at times, will crash. It is the latest checkout of
>> origin/feature/igc.
>
> Pip and Gerd, what would you think of introducing a GC-handle type

I think we're definitely going to have to introduce some sort of
combined handle mechanism for turning a void * into a "constant"
(traced) Lisp_Object.

Which way they are implemented should be, ideally, configurable until we
figure out what's best:

1. Pointer to a Lisp_Object
1a. ... which is ambiguously rooted (glib_user_data)
1b. ... which we ensure is fixed in some other way
2. Index into a table, cast to void *
2a. ... the table is a vector
2b. ... the table is an ambiguous root
2c. ... the table is fixed in some other way
3. Index into a hash table, cast to void *
3a. ... the hash table is a hash table
3b. ... the hash table is some custom structure
4. Pointer to an auxiliary structure containing one of the above

All of these can be combined with reference counting in various ways.

You've gone for 4. I chose 1a (for glib_user_data), and put the refcount
in the igc_root instead of the auxiliary structure (on my branch).

Another option would be to use Lisp_Objects directly, using the unused
tag to mark the GC handle type. Just like symbols-with-positions get
resolved to bare symbols, GC handles would get resolved to other
objects. That would allow us to special-case lispsym entries and
fixnums, which are common values that don't need extra handling.

> as in the attached patch?

Some comments below.

> I imagine GC handles to be used mostly for GUI toolkit callbacks, like
> the one in this bug.  Perhaps GC handles could also be used in some

That's why glib_user_data is defined in gtkutil.h, yes :-)

> way for module_global_references.

Maybe, but the complexity requirements are different for those.

> The patch implements GC handles that support reference counting; it
> was easy to do but was not needed for the bug.

The reference counting is not needed, right?

> diff --git a/src/emacs.c b/src/emacs.c
> index effd60561e2..5951c6c8c82 100644
> --- a/src/emacs.c
> +++ b/src/emacs.c
> @@ -117,6 +117,7 @@ #define MAIN_PROGRAM
>  
>  #include "getpagesize.h"
>  #include "gnutls.h"
> +#include "gc-handles.h"
>  
>  #ifdef HAVE_HAIKU
>  #include <kernel/OS.h>
> @@ -2498,6 +2499,7 @@ android_emacs_init (int argc, char **argv, char *dump_file)
>        syms_of_profiler ();
>        syms_of_pdumper ();
>        syms_of_json ();
> +      syms_of_gc_handles ();
>  
>        keys_of_keyboard ();
>  

Should this be earlier, maybe?

> diff --git a/src/gc-handles.c b/src/gc-handles.c
> new file mode 100644
> index 00000000000..79a011e7324
> --- /dev/null
> +++ b/src/gc-handles.c
> @@ -0,0 +1,165 @@
> +/* GC-handles can be used to retain Lisp_Objects. A GC-handle keeps
> +   its Lisp_Object alive until the GC-handle is freed.  */
> +
> +/* Deficiencies of the current implementation:
> +   - no thread safety
> +   - handles are not dumpable
> +*/
> +
> +#include "gc-handles.h"
> +
> +struct gc_handle
> +{
> +  size_t index;
> +  size_t refcount;
> +};
> +
> +struct handle_table
> +{
> +  struct gc_handle **handles;
> +  Lisp_Object objects;
> +  size_t length;
> +};
> +
> +static struct handle_table _global_handle_table;

I don't think there's a convention to add leading underscores to Emacs
symbols.

> +
> +#define INITIAL_TABLE_SIZE 0
> +
> +void
> +syms_of_gc_handles (void)
> +{
> +  _global_handle_table.objects = Qnil;

Why not just initialize to the empty vector?

> +  staticpro (&_global_handle_table.objects);
> +}
> +
> +static size_t
> +handle_table_size (struct handle_table *tab)
> +{
> +  return ASIZE (tab->objects);
> +}
> +
> +static void
> +init_handle_table (struct handle_table *tab)
> +{
> +  size_t size = INITIAL_TABLE_SIZE;
> +  tab->handles = xzalloc (size * sizeof *tab->handles);

No need to allocate a zero-entry vector, xfree (NULL) works fine.

> +  tab->objects = make_vector (size, Qnil);
> +  tab->length = 0;

I'd prefer "count", if this is meant to be the number of entries that
are in use.

> +}
> +
> +static struct handle_table *
> +global_handle_table (void)
> +{
> +  struct handle_table *tab = &_global_handle_table;
> +  if (tab->handles == 0)
> +    init_handle_table (tab);
> +  return tab;
> +}

No need for this function if you just initialize to empty_zero_vector,
AFAICT.

> +
> +static void
> +grow_handle_table (struct handle_table *tab)
> +{
> +  size_t old_size = handle_table_size (tab);
> +  size_t new_size = old_size * 133 / 100 + 1;

That will grow very slowly at small sizes. It might be a good idea to
start with a reasonably large number.

> +  Lisp_Object v = make_vector (new_size, Qnil);
> +  struct gc_handle **old = tab->handles;
> +  struct gc_handle **new = xzalloc (new_size * sizeof *new);
> +  for (size_t i = 0; i < old_size; i++)
> +    {
> +      ASET (v, i, AREF (tab->objects, i));
> +      new[i] = old[i];
> +    }
> +  tab->objects = v;
> +  tab->handles = new;
> +  xfree (old);
> +  eassert (handle_table_size (tab) == new_size);
> +}
> +
> +static size_t
> +alloc_index (struct handle_table *tab)
> +{
> +  size_t idx = tab->length;
> +  if (idx == handle_table_size (tab))
> +    grow_handle_table (tab);
> +  tab->length++;
> +  return idx;
> +}
> +
> +struct gc_handle *
> +alloc_gc_handle (Lisp_Object obj)
> +{
> +  if (NILP (obj))
> +    return NULL;

Why?  I think it's better to return non-NULL values on success (module
functions need a special failure value IIUC).

> +  struct handle_table *tab = global_handle_table ();
> +  size_t idx = alloc_index (tab);
> +  ASET (tab->objects, idx, obj);
> +  struct gc_handle *h = xmalloc (sizeof *h);
> +  h->index = idx;
> +  h->refcount = 1;
> +  tab->handles[idx] = h;
> +  return h;
> +}
> +
> +struct gc_handle *
> +clone_gc_handle (struct gc_handle *h)
> +{
> +  if (h == NULL)
> +    return NULL;
> +  h->refcount++;
> +  return h;
> +}
> +
> +static void
> +maybe_shrink_handle_table (struct handle_table *tab)
> +{
> +  /* Not yet implemented */
> +}
> +
> +static void
> +swap_remove (struct handle_table *tab, size_t idx)
> +{
> +  eassert (idx < tab->length);
> +  size_t last = tab->length - 1;
> +  if (idx != last)
> +    {
> +      ASET (tab->objects, idx, AREF (tab->objects, last));
> +      struct gc_handle *h = tab->handles[last];
> +      tab->handles[idx] = h;
> +      h->index = idx;
> +    }
> +  ASET (tab->objects, last, Qnil);
> +  tab->handles[last] = NULL;
> +  tab->length--;
> +  maybe_shrink_handle_table (tab);
> +}

This seems complicated. If you must reuse index values, why not just use
a free list?

> +void
> +free_gc_handle (struct gc_handle *h)
> +{
> +  if (h == NULL)
> +    return;
> +  h->refcount--;
> +  if (h->refcount != 0)
> +    return;
> +  size_t idx = h->index;
> +  struct handle_table *tab = global_handle_table ();
> +  swap_remove (tab, idx);
> +  h->index = ~idx; /* invalidate index */
> +  xfree (h);

I'd leave the poisoning of xfreed values to the malloc implementation.

> +Lisp_Object
> +gc_handle_ref (struct gc_handle *h)
> +{
> +  if (h == NULL)
> +    return Qnil;
> +  struct handle_table *tab = global_handle_table ();
> +  return AREF (tab->objects, h->index);
> +}

This, for example, could simply be AREF (global_handle_table.objects, h->index)

> +struct gc_handle *
> +alloc_gc_handle_for_pvec (struct vectorlike_header *h)

This covers vectors, too, right?

> diff --git a/src/xterm.c b/src/xterm.c
> index 3798439d03f..684943e7afe 100644
> --- a/src/xterm.c
> +++ b/src/xterm.c

I think it'd be better to use glib_user_data here (not because it's
better, but because it already exists and we shouldn't add what is now
the third solution to this problem).

Pip

Message #17 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 79667 <at> debbugs.gnu.org, "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50; igc Xawd3d EMACS crashes when the buffer
 scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)
Date: Fri, 24 Oct 2025 09:40:41 +0200

On Thu, Oct 23 2025, Pip Cet wrote:

> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>
>> On Tue, Oct 21 2025, Josh P. wrote:
>>
>>> Hello, there is a fatal error in the igc branch that, when scrolling
>>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>>
>>> emacs -Q
>>>
>>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>>> buffer)
>>>
>>> scroll "violently" downwards and upwards with the scrollwheel, then do
>>> it "violently" with with the pointer being used for the scrollbar.
>>>
>>> SIGABRT.
>>>
>>> The symptoms are, that when the buffer is long, and you attempt to
>>> scroll it, at times, will crash. It is the latest checkout of
>>> origin/feature/igc.
>>>
>>> Hello, there is a fatal error in the igc branch that, when scrolling
>>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>>
>>> emacs -Q
>>>
>>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>>> buffer)
>>>
>>> scroll "violently" downwards and upwards with the scrollwheel, then do
>>> it "violently" with with the pointer being used for the scrollbar.
>>>
>>> SIGABRT.
>>>
>>> The symptoms are, that when the buffer is long, and you attempt to
>>> scroll it, at times, will crash. It is the latest checkout of
>>> origin/feature/igc.
>>
>> Pip and Gerd, what would you think of introducing a GC-handle type
>
> I think we're definitely going to have to introduce some sort of
> combined handle mechanism for turning a void * into a "constant"
> (traced) Lisp_Object.
>
> Which way they are implemented should be, ideally, configurable until we
> figure out what's best:
>
> 1. Pointer to a Lisp_Object
> 1a. ... which is ambiguously rooted (glib_user_data)
> 1b. ... which we ensure is fixed in some other way
> 2. Index into a table, cast to void *
> 2a. ... the table is a vector
> 2b. ... the table is an ambiguous root
> 2c. ... the table is fixed in some other way
> 3. Index into a hash table, cast to void *
> 3a. ... the hash table is a hash table
> 3b. ... the hash table is some custom structure
> 4. Pointer to an auxiliary structure containing one of the above
>
> All of these can be combined with reference counting in various ways.
>
> You've gone for 4. I chose 1a (for glib_user_data), and put the refcount
> in the igc_root instead of the auxiliary structure (on my branch).
>
> Another option would be to use Lisp_Objects directly, using the unused
> tag to mark the GC handle type. Just like symbols-with-positions get
> resolved to bare symbols, GC handles would get resolved to other
> objects. That would allow us to special-case lispsym entries and
> fixnums, which are common values that don't need extra handling.
>
>> as in the attached patch?
>
> Some comments below.
>
>> I imagine GC handles to be used mostly for GUI toolkit callbacks, like
>> the one in this bug.  Perhaps GC handles could also be used in some
>
> That's why glib_user_data is defined in gtkutil.h, yes :-)
>
>> way for module_global_references.
>
> Maybe, but the complexity requirements are different for those.
>
>> The patch implements GC handles that support reference counting; it
>> was easy to do but was not needed for the bug.
>
> The reference counting is not needed, right?

Yes.

>> diff --git a/src/emacs.c b/src/emacs.c
>> index effd60561e2..5951c6c8c82 100644
>> --- a/src/emacs.c
>> +++ b/src/emacs.c
>> @@ -117,6 +117,7 @@ #define MAIN_PROGRAM
>>  
>>  #include "getpagesize.h"
>>  #include "gnutls.h"
>> +#include "gc-handles.h"
>>  
>>  #ifdef HAVE_HAIKU
>>  #include <kernel/OS.h>
>> @@ -2498,6 +2499,7 @@ android_emacs_init (int argc, char **argv, char *dump_file)
>>        syms_of_profiler ();
>>        syms_of_pdumper ();
>>        syms_of_json ();
>> +      syms_of_gc_handles ();
>>  
>>        keys_of_keyboard ();
>>  
>
> Should this be earlier, maybe?

That was not needed.

>> diff --git a/src/gc-handles.c b/src/gc-handles.c
>> new file mode 100644
>> index 00000000000..79a011e7324
>> --- /dev/null
>> +++ b/src/gc-handles.c
>> @@ -0,0 +1,165 @@
>> +/* GC-handles can be used to retain Lisp_Objects. A GC-handle keeps
>> +   its Lisp_Object alive until the GC-handle is freed.  */
>> +
>> +/* Deficiencies of the current implementation:
>> +   - no thread safety
>> +   - handles are not dumpable
>> +*/
>> +
>> +#include "gc-handles.h"
>> +
>> +struct gc_handle
>> +{
>> +  size_t index;
>> +  size_t refcount;
>> +};
>> +
>> +struct handle_table
>> +{
>> +  struct gc_handle **handles;
>> +  Lisp_Object objects;
>> +  size_t length;
>> +};
>> +
>> +static struct handle_table _global_handle_table;
>
> I don't think there's a convention to add leading underscores to Emacs
> symbols.

Is there a convention to avoid leading underscores?

>> +
>> +#define INITIAL_TABLE_SIZE 0
>> +
>> +void
>> +syms_of_gc_handles (void)
>> +{
>> +  _global_handle_table.objects = Qnil;
>
> Why not just initialize to the empty vector?

Because then an empty vector would end up in the dump.

>> +  staticpro (&_global_handle_table.objects);
>> +}
>> +
>> +static size_t
>> +handle_table_size (struct handle_table *tab)
>> +{
>> +  return ASIZE (tab->objects);
>> +}
>> +
>> +static void
>> +init_handle_table (struct handle_table *tab)
>> +{
>> +  size_t size = INITIAL_TABLE_SIZE;
>> +  tab->handles = xzalloc (size * sizeof *tab->handles);
>
> No need to allocate a zero-entry vector, xfree (NULL) works fine.

INITIAL_TABLE_SIZE is not necessarily 0.

>> +  tab->objects = make_vector (size, Qnil);
>> +  tab->length = 0;
>
> I'd prefer "count", if this is meant to be the number of entries that
> are in use.

It's the length of sub-vectors that contain valid entries.

>> +}
>> +
>> +static struct handle_table *
>> +global_handle_table (void)
>> +{
>> +  struct handle_table *tab = &_global_handle_table;
>> +  if (tab->handles == 0)
>> +    init_handle_table (tab);
>> +  return tab;
>> +}
>
> No need for this function if you just initialize to empty_zero_vector,
> AFAICT.
>> +
>> +static void
>> +grow_handle_table (struct handle_table *tab)
>> +{
>> +  size_t old_size = handle_table_size (tab);
>> +  size_t new_size = old_size * 133 / 100 + 1;
>
> That will grow very slowly at small sizes. It might be a good idea to
> start with a reasonably large number.
>
>> +  Lisp_Object v = make_vector (new_size, Qnil);
>> +  struct gc_handle **old = tab->handles;
>> +  struct gc_handle **new = xzalloc (new_size * sizeof *new);
>> +  for (size_t i = 0; i < old_size; i++)
>> +    {
>> +      ASET (v, i, AREF (tab->objects, i));
>> +      new[i] = old[i];
>> +    }
>> +  tab->objects = v;
>> +  tab->handles = new;
>> +  xfree (old);
>> +  eassert (handle_table_size (tab) == new_size);
>> +}
>> +
>> +static size_t
>> +alloc_index (struct handle_table *tab)
>> +{
>> +  size_t idx = tab->length;
>> +  if (idx == handle_table_size (tab))
>> +    grow_handle_table (tab);
>> +  tab->length++;
>> +  return idx;
>> +}
>> +
>> +struct gc_handle *
>> +alloc_gc_handle (Lisp_Object obj)
>> +{
>> +  if (NILP (obj))
>> +    return NULL;
>
> Why?

A minor optimization for a possibly common value.

> I think it's better to return non-NULL values on success (module
> functions need a special failure value IIUC).

It seems that alloc_gc_handle only fails if make_vector or xzalloc
signal an error.

>> +  struct handle_table *tab = global_handle_table ();
>> +  size_t idx = alloc_index (tab);
>> +  ASET (tab->objects, idx, obj);
>> +  struct gc_handle *h = xmalloc (sizeof *h);
>> +  h->index = idx;
>> +  h->refcount = 1;
>> +  tab->handles[idx] = h;
>> +  return h;
>> +}
>> +
>> +struct gc_handle *
>> +clone_gc_handle (struct gc_handle *h)
>> +{
>> +  if (h == NULL)
>> +    return NULL;
>> +  h->refcount++;
>> +  return h;
>> +}
>> +
>> +static void
>> +maybe_shrink_handle_table (struct handle_table *tab)
>> +{
>> +  /* Not yet implemented */
>> +}
>> +
>> +static void
>> +swap_remove (struct handle_table *tab, size_t idx)
>> +{
>> +  eassert (idx < tab->length);
>> +  size_t last = tab->length - 1;
>> +  if (idx != last)
>> +    {
>> +      ASET (tab->objects, idx, AREF (tab->objects, last));
>> +      struct gc_handle *h = tab->handles[last];
>> +      tab->handles[idx] = h;
>> +      h->index = idx;
>> +    }
>> +  ASET (tab->objects, last, Qnil);
>> +  tab->handles[last] = NULL;
>> +  tab->length--;
>> +  maybe_shrink_handle_table (tab);
>> +}
>
> This seems complicated. If you must reuse index values, why not just use
> a free list?

A freelist would be more complicated.

>> +void
>> +free_gc_handle (struct gc_handle *h)
>> +{
>> +  if (h == NULL)
>> +    return;
>> +  h->refcount--;
>> +  if (h->refcount != 0)
>> +    return;
>> +  size_t idx = h->index;
>> +  struct handle_table *tab = global_handle_table ();
>> +  swap_remove (tab, idx);
>> +  h->index = ~idx; /* invalidate index */
>> +  xfree (h);
>
> I'd leave the poisoning of xfreed values to the malloc implementation.
>
>> +Lisp_Object
>> +gc_handle_ref (struct gc_handle *h)
>> +{
>> +  if (h == NULL)
>> +    return Qnil;
>> +  struct handle_table *tab = global_handle_table ();
>> +  return AREF (tab->objects, h->index);
>> +}
>
> This, for example, could simply be AREF (global_handle_table.objects, h->index)
>
>> +struct gc_handle *
>> +alloc_gc_handle_for_pvec (struct vectorlike_header *h)
>
> This covers vectors, too, right?

If you mean Lisp_Vector, then yes.

>> diff --git a/src/xterm.c b/src/xterm.c
>> index 3798439d03f..684943e7afe 100644
>> --- a/src/xterm.c
>> +++ b/src/xterm.c
>
> I think it'd be better to use glib_user_data here (not because it's
> better, but because it already exists and we shouldn't add what is now
> the third solution to this problem).

glib_user_data creates an ambiguous root.  Seems undesirable to me and
should be avoided.

Helmut

Message #20 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 79667 <at> debbugs.gnu.org, Pip Cet <pipcet <at> protonmail.com>,
 "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50; igc Xawd3d EMACS crashes when the buffer
 scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)
Date: Fri, 24 Oct 2025 10:08:37 +0200

Helmut Eller <eller.helmut <at> gmail.com> writes:

>> I don't think there's a convention to add leading underscores to Emacs
>> symbols.
>
> Is there a convention to avoid leading underscores?

The C and C++ standards reserve some forms.

AFAIR, C++ reserves all names with a leading underscore at file scope.
Don't remember what C does. I've always avoided leading underscore, so I
don't have to remember :-).

Message #23 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: 79667 <at> debbugs.gnu.org, pipcet <at> protonmail.com, eller.helmut <at> gmail.com,
 zcheeze0 <at> gmail.com
Subject: Re: bug#79667: 31.0.50;
 igc Xawd3d EMACS crashes when the buffer scrollbar is used: Emacs
 fatal error: assertion failed: WINDOWP (a)
Date: Fri, 24 Oct 2025 13:36:53 +0300

> Cc: 79667 <at> debbugs.gnu.org, Pip Cet <pipcet <at> protonmail.com>,
>  "Josh P." <zcheeze0 <at> gmail.com>
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Fri, 24 Oct 2025 10:08:37 +0200
> 
> Helmut Eller <eller.helmut <at> gmail.com> writes:
> 
> >> I don't think there's a convention to add leading underscores to Emacs
> >> symbols.
> >
> > Is there a convention to avoid leading underscores?
> 
> The C and C++ standards reserve some forms.
> 
> AFAIR, C++ reserves all names with a leading underscore at file scope.
> Don't remember what C does. I've always avoided leading underscore, so I
> don't have to remember :-).

Yes, leading underscores are reserved, both in C and C++, and should
not be used by application code.

Message #26 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: gerd.moellmann <at> gmail.com, 79667 <at> debbugs.gnu.org, pipcet <at> protonmail.com,
 zcheeze0 <at> gmail.com
Subject: Re: bug#79667: 31.0.50;
 igc Xawd3d EMACS crashes when the buffer scrollbar is used: Emacs
 fatal error: assertion failed: WINDOWP (a)
Date: Fri, 24 Oct 2025 13:53:23 +0300

> Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
>  79667 <at> debbugs.gnu.org, "Josh P." <zcheeze0 <at> gmail.com>
> From: Helmut Eller <eller.helmut <at> gmail.com>
> Date: Fri, 24 Oct 2025 09:40:41 +0200
> 
> >> +
> >> +#define INITIAL_TABLE_SIZE 0
> >> +
> >> +void
> >> +syms_of_gc_handles (void)
> >> +{
> >> +  _global_handle_table.objects = Qnil;
> >
> > Why not just initialize to the empty vector?
> 
> Because then an empty vector would end up in the dump.

We have infrastructure to initialize stuff after a dump, so maybe you
could use that.  See pdumper_do_now_and_after_load.

Message #29 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 79667 <at> debbugs.gnu.org, "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50;
 igc Xawd3d EMACS crashes when the buffer scrollbar is used: Emacs
 fatal error: assertion failed: WINDOWP (a)
Date: Fri, 24 Oct 2025 16:26:55 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> On Thu, Oct 23 2025, Pip Cet wrote:
>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>>> On Tue, Oct 21 2025, Josh P. wrote:
>>>
>>>> Hello, there is a fatal error in the igc branch that, when scrolling
>>>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>>>
>>>> emacs -Q
>>>>
>>>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>>>> buffer)
>>>>
>>>> scroll "violently" downwards and upwards with the scrollwheel, then do
>>>> it "violently" with with the pointer being used for the scrollbar.
>>>>
>>>> SIGABRT.
>>>>
>>>> The symptoms are, that when the buffer is long, and you attempt to
>>>> scroll it, at times, will crash. It is the latest checkout of
>>>> origin/feature/igc.
>>>>
>>>> Hello, there is a fatal error in the igc branch that, when scrolling
>>>> under the Xawd3d toolkit, emacs crashes, here is how to reproduce it:
>>>>
>>>> emacs -Q
>>>>
>>>> C-x i igc.c RET (it happens when there is quite a bit to scroll in a
>>>> buffer)
>>>>
>>>> scroll "violently" downwards and upwards with the scrollwheel, then do
>>>> it "violently" with with the pointer being used for the scrollbar.
>>>>
>>>> SIGABRT.
>>>>
>>>> The symptoms are, that when the buffer is long, and you attempt to
>>>> scroll it, at times, will crash. It is the latest checkout of
>>>> origin/feature/igc.
>>>
>>> Pip and Gerd, what would you think of introducing a GC-handle type
>>
>> I think we're definitely going to have to introduce some sort of
>> combined handle mechanism for turning a void * into a "constant"
>> (traced) Lisp_Object.
>>
>> Which way they are implemented should be, ideally, configurable until we
>> figure out what's best:
>>
>> 1. Pointer to a Lisp_Object
>> 1a. ... which is ambiguously rooted (glib_user_data)
>> 1b. ... which we ensure is fixed in some other way
>> 2. Index into a table, cast to void *
>> 2a. ... the table is a vector
>> 2b. ... the table is an ambiguous root
>> 2c. ... the table is fixed in some other way
>> 3. Index into a hash table, cast to void *
>> 3a. ... the hash table is a hash table
>> 3b. ... the hash table is some custom structure
>> 4. Pointer to an auxiliary structure containing one of the above
>>
>> All of these can be combined with reference counting in various ways.
>>
>> You've gone for 4. I chose 1a (for glib_user_data), and put the refcount
>> in the igc_root instead of the auxiliary structure (on my branch).
>>
>> Another option would be to use Lisp_Objects directly, using the unused
>> tag to mark the GC handle type. Just like symbols-with-positions get
>> resolved to bare symbols, GC handles would get resolved to other
>> objects. That would allow us to special-case lispsym entries and
>> fixnums, which are common values that don't need extra handling.
>>
>>> as in the attached patch?
>>
>> Some comments below.
>>
>>> I imagine GC handles to be used mostly for GUI toolkit callbacks, like
>>> the one in this bug.  Perhaps GC handles could also be used in some
>>
>> That's why glib_user_data is defined in gtkutil.h, yes :-)
>>
>>> way for module_global_references.
>>
>> Maybe, but the complexity requirements are different for those.
>>
>>> The patch implements GC handles that support reference counting; it
>>> was easy to do but was not needed for the bug.
>>
>> The reference counting is not needed, right?
>
> Yes.

What for? It doesn't appear to be used anywhere.

>>> diff --git a/src/gc-handles.c b/src/gc-handles.c
>>> new file mode 100644
>>> index 00000000000..79a011e7324
>>> --- /dev/null
>>> +++ b/src/gc-handles.c
>>> @@ -0,0 +1,165 @@
>>> +/* GC-handles can be used to retain Lisp_Objects. A GC-handle keeps
>>> +   its Lisp_Object alive until the GC-handle is freed.  */
>>> +
>>> +/* Deficiencies of the current implementation:
>>> +   - no thread safety
>>> +   - handles are not dumpable
>>> +*/
>>> +
>>> +#include "gc-handles.h"
>>> +
>>> +struct gc_handle
>>> +{
>>> +  size_t index;
>>> +  size_t refcount;
>>> +};
>>> +
>>> +struct handle_table
>>> +{
>>> +  struct gc_handle **handles;
>>> +  Lisp_Object objects;
>>> +  size_t length;
>>> +};
>>> +
>>> +static struct handle_table _global_handle_table;
>>
>> I don't think there's a convention to add leading underscores to Emacs
>> symbols.
>
> Is there a convention to avoid leading underscores?

My understanding is that leading underscores are reserved for certain
purposes by the C standard, and I wouldn't be surprised if the GNU
coding standards had something to say on the matter. So, yes, I think
there is a strong convention to avoid them.

>>> +
>>> +#define INITIAL_TABLE_SIZE 0
>>> +
>>> +void
>>> +syms_of_gc_handles (void)
>>> +{
>>> +  _global_handle_table.objects = Qnil;
>>
>> Why not just initialize to the empty vector?
>
> Because then an empty vector would end up in the dump.

zero_vector is always in the dump in pdumped Emacs, and it's the only
empty vector.

>>> +  staticpro (&_global_handle_table.objects);
>>> +}
>>> +
>>> +static size_t
>>> +handle_table_size (struct handle_table *tab)
>>> +{
>>> +  return ASIZE (tab->objects);
>>> +}
>>> +
>>> +static void
>>> +init_handle_table (struct handle_table *tab)
>>> +{
>>> +  size_t size = INITIAL_TABLE_SIZE;
>>> +  tab->handles = xzalloc (size * sizeof *tab->handles);
>>
>> No need to allocate a zero-entry vector, xfree (NULL) works fine.
>
> INITIAL_TABLE_SIZE is not necessarily 0.

That complication also seems unnecessary.

>>> +  tab->objects = make_vector (size, Qnil);
>>> +  tab->length = 0;
>>
>> I'd prefer "count", if this is meant to be the number of entries that
>> are in use.
>
> It's the length of sub-vectors that contain valid entries.

I think that's confusing.

>>> +}
>>> +
>>> +static struct handle_table *
>>> +global_handle_table (void)
>>> +{
>>> +  struct handle_table *tab = &_global_handle_table;
>>> +  if (tab->handles == 0)
>>> +    init_handle_table (tab);
>>> +  return tab;
>>> +}
>>
>> No need for this function if you just initialize to empty_zero_vector,
>> AFAICT.
>>> +
>>> +static void
>>> +grow_handle_table (struct handle_table *tab)
>>> +{
>>> +  size_t old_size = handle_table_size (tab);
>>> +  size_t new_size = old_size * 133 / 100 + 1;
>>
>> That will grow very slowly at small sizes. It might be a good idea to
>> start with a reasonably large number.
>>
>>> +  Lisp_Object v = make_vector (new_size, Qnil);
>>> +  struct gc_handle **old = tab->handles;
>>> +  struct gc_handle **new = xzalloc (new_size * sizeof *new);
>>> +  for (size_t i = 0; i < old_size; i++)
>>> +    {
>>> +      ASET (v, i, AREF (tab->objects, i));
>>> +      new[i] = old[i];
>>> +    }
>>> +  tab->objects = v;
>>> +  tab->handles = new;
>>> +  xfree (old);
>>> +  eassert (handle_table_size (tab) == new_size);
>>> +}
>>> +
>>> +static size_t
>>> +alloc_index (struct handle_table *tab)
>>> +{
>>> +  size_t idx = tab->length;
>>> +  if (idx == handle_table_size (tab))
>>> +    grow_handle_table (tab);
>>> +  tab->length++;
>>> +  return idx;
>>> +}
>>> +
>>> +struct gc_handle *
>>> +alloc_gc_handle (Lisp_Object obj)
>>> +{
>>> +  if (NILP (obj))
>>> +    return NULL;
>>
>> Why?
>
> A minor optimization for a possibly common value.

Better to fix refcounting for that case.

>> I think it's better to return non-NULL values on success (module
>> functions need a special failure value IIUC).
>
> It seems that alloc_gc_handle only fails if make_vector or xzalloc
> signal an error.

OOM, in other words.

>>> +  struct handle_table *tab = global_handle_table ();
>>> +  size_t idx = alloc_index (tab);
>>> +  ASET (tab->objects, idx, obj);
>>> +  struct gc_handle *h = xmalloc (sizeof *h);
>>> +  h->index = idx;
>>> +  h->refcount = 1;
>>> +  tab->handles[idx] = h;
>>> +  return h;
>>> +}
>>> +
>>> +struct gc_handle *
>>> +clone_gc_handle (struct gc_handle *h)
>>> +{
>>> +  if (h == NULL)
>>> +    return NULL;
>>> +  h->refcount++;
>>> +  return h;
>>> +}
>>> +
>>> +static void
>>> +maybe_shrink_handle_table (struct handle_table *tab)
>>> +{
>>> +  /* Not yet implemented */
>>> +}
>>> +
>>> +static void
>>> +swap_remove (struct handle_table *tab, size_t idx)
>>> +{
>>> +  eassert (idx < tab->length);
>>> +  size_t last = tab->length - 1;
>>> +  if (idx != last)
>>> +    {
>>> +      ASET (tab->objects, idx, AREF (tab->objects, last));
>>> +      struct gc_handle *h = tab->handles[last];
>>> +      tab->handles[idx] = h;
>>> +      h->index = idx;
>>> +    }
>>> +  ASET (tab->objects, last, Qnil);
>>> +  tab->handles[last] = NULL;
>>> +  tab->length--;
>>> +  maybe_shrink_handle_table (tab);
>>> +}
>>
>> This seems complicated. If you must reuse index values, why not just use
>> a free list?
>
> A freelist would be more complicated.

Or we use the hash table code which already has a free list?

>>> diff --git a/src/xterm.c b/src/xterm.c
>>> index 3798439d03f..684943e7afe 100644
>>> --- a/src/xterm.c
>>> +++ b/src/xterm.c
>>
>> I think it'd be better to use glib_user_data here (not because it's
>> better, but because it already exists and we shouldn't add what is now
>> the third solution to this problem).
>
> glib_user_data creates an ambiguous root.  Seems undesirable to me and
> should be avoided.

Indeed. We should convert glib_user_data to whatever solution we
ultimately decide on, but having some of the GUI code use gc_handles and
some glib_user_data seems unnecessary.

gc_handle is a much better name; I'd drop the "struct" part, since as
far as most of the code is concerned it's a pointer to an opaque object
(and that was historically a union rather than a struct, and we might
have to change back).

Anyway, here's my proposal. Shrinks (rarely), refcounting, reuses hash
table code, "struct" dropped from opaque type, less confusing API (glib
uses _ref to increase the refcount on a handle, so it's best not to use
that for dereferencing a handle, I think).

(I set the From: line so it's easier to identify in the archives; just
let me know what you prefer there).

From d24596012fc079bf5405d45fb46340266e7aace8 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Date: Fri, 24 Oct 2025 16:12:31 +0000
Subject: [PATCH] Introduce a GC handle type

Co-authored-by: Helmut Eller <eller.helmut <at> gmail.com>
---
 src/Makefile.in  |  2 +-
 src/emacs.c      |  2 ++
 src/gc-handles.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/gc-handles.h | 17 ++++++++++
 src/xterm.c      | 26 ++++++++++++---
 5 files changed, 124 insertions(+), 5 deletions(-)
 create mode 100644 src/gc-handles.c
 create mode 100644 src/gc-handles.h

diff --git a/src/Makefile.in b/src/Makefile.in
index 9b1aee0aa40..8101d4406da 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -467,7 +467,7 @@ base_obj =
 	$(XWIDGETS_OBJ) 						       \
 	profiler.o decompress.o $(IGCOBJ) 				       \
 	thread.o systhread.o sqlite.o  treesit.o			       \
-	itree.o json.o 							       \
+	itree.o json.o gc-handles.o					       \
 	$(MSDOS_OBJ) $(MSDOS_X_OBJ) $(NS_OBJ) $(CYGWIN_OBJ) $(FONT_OBJ)        \
 	$(W32_OBJ) $(WINDOW_SYSTEM_OBJ) $(XGSELOBJ)			       \
 	$(HAIKU_OBJ) $(PGTK_OBJ) $(ANDROID_OBJ)
diff --git a/src/emacs.c b/src/emacs.c
index effd60561e2..5951c6c8c82 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -117,6 +117,7 @@ #define MAIN_PROGRAM
 
 #include "getpagesize.h"
 #include "gnutls.h"
+#include "gc-handles.h"
 
 #ifdef HAVE_HAIKU
 #include <kernel/OS.h>
@@ -2498,6 +2499,7 @@ android_emacs_init (int argc, char **argv, char *dump_file)
       syms_of_profiler ();
       syms_of_pdumper ();
       syms_of_json ();
+      syms_of_gc_handles ();
 
       keys_of_keyboard ();
 
diff --git a/src/gc-handles.c b/src/gc-handles.c
new file mode 100644
index 00000000000..bc3198ca9ce
--- /dev/null
+++ b/src/gc-handles.c
@@ -0,0 +1,82 @@
+/* GC-handles can be used to retain Lisp_Objects. A GC-handle keeps
+   its Lisp_Object alive until the GC-handle is freed.  */
+
+#include "gc-handles.h"
+
+struct gc_handle_struct
+{
+  size_t index;
+  size_t refcount;
+};
+
+static Lisp_Object global_handle_table;
+
+void
+syms_of_gc_handles (void)
+{
+  global_handle_table = CALLN (Fmake_hash_table, QCtest, Qeq);
+  staticpro (&global_handle_table);
+}
+
+static void
+shrink_handle_table (void)
+{
+  Lisp_Object new_table = CALLN (Fmake_hash_table, QCtest, Qeq);
+  DOHASH (XHASH_TABLE (global_handle_table), k, v)
+    {
+      gc_handle gch = xmint_pointer (v);
+      Fputhash (k, v, new_table);
+      gch->index = hash_find (XHASH_TABLE (new_table), k);
+    }
+  global_handle_table = new_table;
+}
+
+gc_handle
+gc_handle_for (Lisp_Object obj)
+{
+  Lisp_Object handle;
+  handle = Fgethash (obj, global_handle_table, Qnil);
+  if (NILP (handle))
+    {
+      gc_handle gch = xmalloc (sizeof (*gch));
+      gch->refcount = 1;
+      handle = make_mint_ptr (gch);
+      Fputhash (obj, handle, global_handle_table);
+      gch->index = hash_find (XHASH_TABLE (global_handle_table), obj);
+      return gch;
+    }
+  else
+    {
+      gc_handle gch = xmint_pointer (handle);
+      gch->refcount++;
+      return gch;
+    }
+}
+
+Lisp_Object
+gc_handle_value (gc_handle gch)
+{
+  return HASH_KEY (XHASH_TABLE (global_handle_table), gch->index);
+}
+
+void
+free_gc_handle (gc_handle h)
+{
+  h->refcount--;
+  if (h->refcount == 0)
+    {
+      Fremhash (gc_handle_value (h), global_handle_table);
+      xfree (h);
+      /* FIXME: better (but still safe) heuristic needed.  */
+      if (XFIXNUM (Fhash_table_count (global_handle_table)) <
+	  XFIXNUM (Fhash_table_size (global_handle_table)) / 4 - 128)
+	shrink_handle_table ();
+    }
+}
+
+gc_handle
+gc_handle_for_pvec (struct vectorlike_header *h)
+{
+  Lisp_Object obj = make_lisp_ptr (h, Lisp_Vectorlike);
+  return gc_handle_for (obj);
+}
diff --git a/src/gc-handles.h b/src/gc-handles.h
new file mode 100644
index 00000000000..0b658021ac9
--- /dev/null
+++ b/src/gc-handles.h
@@ -0,0 +1,17 @@
+#ifndef EMACS_GC_HANDLES_H
+#define EMACS_GC_HANDLES_H
+
+#include "config.h"
+#include "lisp.h"
+
+struct gc_handle_struct;
+typedef struct gc_handle_struct *gc_handle;
+
+gc_handle gc_handle_for (Lisp_Object);
+gc_handle gc_handle_for_pvec (struct vectorlike_header *);
+void free_gc_handle (gc_handle);
+Lisp_Object gc_handle_value (gc_handle);
+
+void syms_of_gc_handles (void);
+
+#endif
diff --git a/src/xterm.c b/src/xterm.c
index 3798439d03f..34653fa6593 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -719,6 +719,7 @@ Copyright (C) 1989, 1993-2025 Free Software Foundation, Inc.
 #include "sysselect.h"
 #include "menu.h"
 #include "pdumper.h"
+#include "gc-handles.h"
 
 #ifdef USE_X_TOOLKIT
 #include <X11/Shell.h>
@@ -15928,7 +15929,9 @@ xg_end_scroll_callback (GtkWidget *widget,
 static void
 xaw_jump_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 {
-  struct scroll_bar *bar = client_data;
+  gc_handle bar_handle = client_data;
+  Lisp_Object bar_obj = gc_handle_value (bar_handle);
+  struct scroll_bar *bar = XSCROLL_BAR (bar_obj);
   float *top_addr = call_data;
   float top = *top_addr;
   float shown;
@@ -15995,7 +15998,9 @@ xaw_jump_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 static void
 xaw_scroll_callback (Widget widget, XtPointer client_data, XtPointer call_data)
 {
-  struct scroll_bar *bar = client_data;
+  gc_handle bar_handle = client_data;
+  Lisp_Object bar_obj = gc_handle_value (bar_handle);
+  struct scroll_bar *bar = XSCROLL_BAR (bar_obj);
   /* The position really is stored cast to a pointer.  */
   int position = (intptr_t) call_data;
   Dimension height, width;
@@ -16050,6 +16055,14 @@ xaw_scroll_callback (Widget widget, XtPointer client_data, XtPointer call_data)
     }
 }
 
+static void
+xaw_destroy_scrollbar_callback (Widget widget, XtPointer client_data,
+				XtPointer call_data)
+{
+  gc_handle handle = client_data;
+  free_gc_handle (handle);
+}
+
 #endif /* not USE_GTK and not USE_MOTIF */
 
 #define SCROLL_BAR_NAME "verticalScrollBar"
@@ -16268,10 +16281,15 @@ x_create_toolkit_scroll_bar (struct frame *f, struct scroll_bar *bar)
       }
   }
 
+  gc_handle bar_handle = gc_handle_for_pvec (&bar->header);
   /* Define callbacks.  */
-  XtAddCallback (widget, XtNjumpProc, xaw_jump_callback, (XtPointer) bar);
+  XtAddCallback (widget, XtNjumpProc, xaw_jump_callback,
+		 (XtPointer) bar_handle);
   XtAddCallback (widget, XtNscrollProc, xaw_scroll_callback,
-		 (XtPointer) bar);
+		 (XtPointer) bar_handle);
+  XtAddCallback (widget, XtNdestroyCallback,
+		 xaw_destroy_scrollbar_callback,
+		 (XtPointer) bar_handle);
 
   /* Realize the widget.  Only after that is the X window created.  */
   XtRealizeWidget (widget);
-- 
2.51.1

Message #32 received at 79667 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 79667 <at> debbugs.gnu.org, "Josh P." <zcheeze0 <at> gmail.com>
Subject: Re: bug#79667: 31.0.50; igc Xawd3d EMACS crashes when the buffer
 scrollbar is used: Emacs fatal error: assertion failed: WINDOWP (a)
Date: Sat, 25 Oct 2025 17:45:24 +0200

On Fri, Oct 24 2025, Pip Cet wrote:

[...]
>>>> The patch implements GC handles that support reference counting; it
>>>> was easy to do but was not needed for the bug.
>>>
>>> The reference counting is not needed, right?
>>
>> Yes.
>
> What for? It doesn't appear to be used anywhere.

I meant: yes, refcounting is not needed.

[...]
>> glib_user_data creates an ambiguous root.  Seems undesirable to me and
>> should be avoided.
>
> Indeed. We should convert glib_user_data to whatever solution we
> ultimately decide on,

Great.  Finally some progress.

> but having some of the GUI code use gc_handles and
> some glib_user_data seems unnecessary.

Of course.

> gc_handle is a much better name; I'd drop the "struct" part, since as
> far as most of the code is concerned it's a pointer to an opaque object
> (and that was historically a union rather than a struct, and we might
> have to change back).
>
> Anyway, here's my proposal. Shrinks (rarely), refcounting, reuses hash
> table code, "struct" dropped from opaque type, less confusing API (glib
> uses _ref to increase the refcount on a handle, so it's best not to use
> that for dereferencing a handle, I think).

I suppose it is acceptable to exploit the peculiarities of the hashtable
implementation.  So let's go with this implementation.

> (I set the From: line so it's easier to identify in the archives; just
> let me know what you prefer there).

Fine with me.  Committed.

Helmut

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.