Subject: bug#76538: 31.0.50; 31.0.50; 31.0.50; feature/igc: using magit-section-cycle-global (S-TAB) and magit-section-toggle (TAB) in some random ways blocks GNU Emacs.

From: Pip Cet <[email-masked]>
Date: Mon, 03 Mar 2025 10:42:25 +0000

Gerd Möllmann <[email-masked]> writes:

> Helmut Eller <[email-masked]> writes:
>
>> On Mon, Mar 03 2025, Gerd Möllmann wrote:
>>
>>> That's not what I meant, sorry, let me try to ask differently. Without
>>> modifying the code, which situation would you call risky? Is it a signal
>>> that interrupts the mutator, at the place above for instance? And then
>>> lets MPS do what? Or is it a concern for the case that MPS becomes
>>> concurrent?
>>
>> Yes, concurrency.  A concurrent mutator thread could trigger GC at any
>> moment
>>
>> Helmut
>
> Thanks, and I agree of course. Something for future generations to
> solve, from my POV at least. Should it ever happen :-).

Here's my current patch series for finalizing markers.  Still needs
testing to see whether it actually avoids the problem (or will once we
stop the world for finalizers, as we should), but it doesn't crash
immediately.

BTW, I'm currently running with a background thread eagerly triggering
GC, which allowed me to find the missing bt.function trace, but hasn't
uncovered other bugs so far.  So that is concurrent GC, even if it's
unlikely to be a performance win right now :-)

I have no idea why I hit the assert which required the second patch. It
seems to me like a bug in the xdisp.c code, but it's xdisp.c, so who
knows?

Even though we now emulate the alloc.c code quite precisely, I think
feature/igc will accumulate many more markers than alloc.c, since weak
objects are collected quite rarely by MPS, IME.  Maybe we need to mark
"automatic" markers which are never exposed to Lisp and splat them in
DO_MARKERS if there are too many of them?

A more convoluted approach would be to alternate between considering
markers and calculating the charpos for the "best" known marker: do one
marker, then one character, repeat.  That sort of thing is good for
theoretical complexity but rarely useful in practice...

>From 31f3a7f25efe3840d42535f65147c40a7298357b Mon Sep 17 00:00:00 2001
From: Pip Cet <[email-masked]>
Subject: [PATCH 1/2] [MPS] finalize markers rather than unchaining them in the
 collector

* src/buffer.h (marker_it_next): Skip splatted markers.
* src/igc.c (fix_marker_vector): Don't call 'unchain'.
(finalize_marker): New function, calling 'unchain'.
(finalize_vector): Finalize markers.
(maybe_finalize): Adjust
---
 src/buffer.h |  8 ++++++--
 src/igc.c    | 25 +++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/buffer.h b/src/buffer.h
index ffe6770ba7b..5a460b9dcee 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -775,8 +775,12 @@ marker_it_valid (struct marker_it *it)
 INLINE void
 marker_it_next (struct marker_it *it)
 {
-  it->slot = XFIXNUM (IGC_MA_NEXT (it->v, it->slot));
-  it->obj = it->slot < 0 ? Qnil : IGC_MA_MARKER (it->v, it->slot);
+  do
+    {
+      it->slot = XFIXNUM (IGC_MA_NEXT (it->v, it->slot));
+      it->obj = it->slot < 0 ? Qnil : IGC_MA_MARKER (it->v, it->slot);
+    }
+  while (!MARKERP (it->obj) && it->slot > 0);
 }
 
 INLINE struct Lisp_Marker *
diff --git a/src/igc.c b/src/igc.c
index ec714cad435..54c56309502 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -2165,10 +2165,6 @@ fix_marker_vector (mps_ss_t ss, struct Lisp_Vector *v)
 
 	Lisp_Object old = IGC_MA_MARKER (v, slot);
 	IGC_FIX12_OBJ (ss, &IGC_MA_MARKER (v, slot));
-
-	/* FIXME/igc: this is right for marker vectors only.  */
-	if (NILP (IGC_MA_MARKER (v, slot)) && !NILP (old))
-	  unchain (v, slot);
       }
   }
   MPS_SCAN_END (ss);
@@ -3567,6 +3563,20 @@ finalize_finalizer (struct Lisp_Finalizer *f)
     }
 }
 
+static void
+finalize_marker (struct Lisp_Marker *m)
+{
+  if (m->buffer && BUF_MARKERS (m->buffer))
+    {
+      struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (m->buffer));
+      if (!BASE_EQ (IGC_MA_MARKER (v, m->slot), make_lisp_ptr (m, Lisp_Vectorlike)) &&
+	  !NILP (IGC_MA_MARKER (v, m->slot)))
+	emacs_abort ();
+      unchain (v, m->slot);
+      fprintf (stderr, "unchained %zd\n", (size_t) m->slot);
+    }
+}
+
 /* Turn an existing pseudovector into a PVEC_FREE, keeping its size.  */
 
 static void
@@ -3641,6 +3651,10 @@ finalize_vector (mps_addr_t v)
       finalize_finalizer (v);
       break;
 
+    case PVEC_MARKER:
+      finalize_marker (v);
+      break;
+
 #ifndef IN_MY_FORK
     case PVEC_OBARRAY:
 #endif
@@ -3669,7 +3683,6 @@ finalize_vector (mps_addr_t v)
     case PVEC_XWIDGET:
     case PVEC_XWIDGET_VIEW:
     case PVEC_TERMINAL:
-    case PVEC_MARKER:
     case PVEC_MODULE_GLOBAL_REFERENCE:
       igc_assert (!"finalization not implemented");
       break;
@@ -3745,6 +3758,7 @@ maybe_finalize (mps_addr_t client, enum pvec_type tag)
     case PVEC_NATIVE_COMP_UNIT:
     case PVEC_SUBR:
     case PVEC_FINALIZER:
+    case PVEC_MARKER:
       {
 	mps_res_t res = mps_finalize (global_igc->arena, &ref);
 	IGC_CHECK_RES (res);
@@ -3759,7 +3773,6 @@ maybe_finalize (mps_addr_t client, enum pvec_type tag)
     case PVEC_WEAK_HASH_TABLE:
     case PVEC_NORMAL_VECTOR:
     case PVEC_FREE:
-    case PVEC_MARKER:
     case PVEC_OVERLAY:
     case PVEC_SYMBOL_WITH_POS:
     case PVEC_MISC_PTR:
-- 
2.48.1

>From 9148e4c387294b06a471c25d07279d207b1d45c9 Mon Sep 17 00:00:00 2001
From: Pip Cet <[email-masked]>
Subject: [PATCH 2/2] XXX why is this change necessary?

---
 src/xdisp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 577d5b1b401..2c349bb9adf 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -6714,9 +6714,15 @@ handle_composition_prop (struct it *it)
       string = it->string;
       s = SDATA (string) + pos_byte;
       if (STRING_MULTIBYTE (string))
-	it->c = STRING_CHAR (s);
+	{
+	  it->c = STRING_CHAR (s);
+	  it->len = CHAR_BYTES (it->c);
+	}
       else
-	it->c = *s;
+	{
+	  it->c = *s;
+	  it->len = 1;
+	}
     }
   else
     {
@@ -6724,6 +6730,7 @@ handle_composition_prop (struct it *it)
       pos_byte = IT_BYTEPOS (*it);
       string = Qnil;
       it->c = FETCH_CHAR (pos_byte);
+      it->len = it->multibyte_p ? CHAR_BYTES (it->c) : 1;
     }
 
   /* If there's a valid composition and point is not inside of the
-- 
2.48.1