Re: Not protecting Lisp objects from GC

[Pip Cet]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>>>> Date: Mon, 27 Jan 2025 22:18:22 +0000
>>>> From: Pip Cet <pipcet@protonmail.com>
>>>> Cc: Eli Zaretskii <eliz@gnu.org>, Gerd Möllmann 
>>>> <gerd.moellmann@gmail.com>, emacs-devel@gnu.org
>>>>
>>>> I proposed these two patches as a minimal fix:
>>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75521#19
>>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75521#88
>>>
>>> Yes, I think we should use those two, thanks.
>>
>> Great, hoping it'll happen soon then.  I hope Stefan Kangas and Gerd
>> agree.  (But see below about the "which branch" question).
> ...
>> I'm not sure which patch you're planning to install on feature/igc;
>> maybe you should just push what you think would be the best equivalent
>> of Stefan's suggestion here.
>
> Pip, could you please install the above two mentioned patches on
> feature/igc?

Unfortunately, there are merge conflicts.  I resolved those, and here's
the patch I was going to apply until I reread

https://lists.gnu.org/archive/html/emacs-devel/2025-01/msg01065.html

More below.

>From f177decb503698304bd2f1a31c33118cd4121f21 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Subject: [PATCH] Remove DEFVAR_LISP_NOPRO (bug#75521)

* src/alloc.c (process_mark_stack): Adjust comment.
* src/font.c (syms_of_font): Use 'DEFVAR_LISP_NOPRO', not
'DEFVAR_LISP'.  Adjust comment.
(font_style_to_value): Restore previous behavior of letting
user-visible and internal font style tables diverge.
* src/lisp.h (DEFVAR_LISP_NOPRO): Macro removed.
* src/lread.c (defvar_lisp_nopro): Function removed.
(defvar_lisp): Inline defvar_lisp_nopro here.
---
 src/alloc.c |  4 +---
 src/font.c  | 29 +++++------------------------
 src/lisp.h  | 21 +--------------------
 src/lread.c | 14 ++------------
 4 files changed, 9 insertions(+), 59 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 9437bedf650..2f962d4d861 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7632,9 +7632,7 @@ #define CHECK_ALLOCATED_AND_LIVE_SYMBOL()         ((void) 
0)
                /* If the value is forwarded to a buffer or keyboard field,
                   these are marked when we see the corresponding object.
                   And if it's forwarded to a C variable, either it's not
-                  a Lisp_Object var, or it's staticpro'd already, or it's
-                  reachable from font_style_table which is also
-                  staticpro'd.  */
+                  a Lisp_Object var or it's staticpro'd already.  */
                break;
              default: emacs_abort ();
              }
diff --git a/src/font.c b/src/font.c
index dfe479f9355..e6c41e41258 100644
--- a/src/font.c
+++ b/src/font.c
@@ -418,24 +418,8 @@ font_style_to_value (enum font_property_index prop, 
Lisp_Object val,
       eassert (len < 255);
       elt = make_vector (2, make_fixnum (100));
       ASET (elt, 1, val);
-      Lisp_Object new_table = CALLN (Fvconcat, table, make_vector (1, elt));
-      /* Update the corresponding variable with the new value of the
-         modified slot of font_style_table.  */
-      switch (prop)
-       {
-       case FONT_WEIGHT_INDEX:
-         Vfont_weight_table = new_table;
-         break;
-       case FONT_SLANT_INDEX:
-         Vfont_slant_table = new_table;
-         break;
-       case FONT_WIDTH_INDEX:
-         Vfont_width_table = new_table;
-         break;
-       default:
-         break;
-       }
-      ASET (font_style_table, prop - FONT_WEIGHT_INDEX, new_table);
+      ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
+           CALLN (Fvconcat, table, make_vector (1, elt)));
       return (100 << 8) | (i << 4);
     }
   else
@@ -5970,7 +5954,7 @@ syms_of_font (void)
      table used by the font display code.  So we make them read-only,
      to avoid this confusing situation.  */
 
-  DEFVAR_LISP_NOPRO ("font-weight-table", Vfont_weight_table,
+  DEFVAR_LISP ("font-weight-table", Vfont_weight_table,
               doc: /*  Vector of valid font weight values.
 Each element has the form:
     [NUMERIC-VALUE SYMBOLIC-NAME ALIAS-NAME ...]
@@ -5979,23 +5963,20 @@ syms_of_font (void)
   Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
   make_symbol_constant (intern_c_string ("font-weight-table"));
 
-  DEFVAR_LISP_NOPRO ("font-slant-table", Vfont_slant_table,
+  DEFVAR_LISP ("font-slant-table", Vfont_slant_table,
               doc: /*  Vector of font slant symbols vs the corresponding 
numeric values.
 See `font-weight-table' for the format of the vector.
 This variable cannot be set; trying to do so will signal an error.  */);
   Vfont_slant_table = BUILD_STYLE_TABLE (slant_table);
   make_symbol_constant (intern_c_string ("font-slant-table"));
 
-  DEFVAR_LISP_NOPRO ("font-width-table", Vfont_width_table,
+  DEFVAR_LISP ("font-width-table", Vfont_width_table,
               doc: /*  Alist of font width symbols vs the corresponding 
numeric values.
 See `font-weight-table' for the format of the vector.
 This variable cannot be set; trying to do so will signal an error.  */);
   Vfont_width_table = BUILD_STYLE_TABLE (width_table);
   make_symbol_constant (intern_c_string ("font-width-table"));
 
-  /* Because the above 3 variables are slots in the vector we create
-     below, and because that vector is staticpro'd, we don't explicitly
-     staticpro the variables, to avoid wasting slots in staticvec[].  */
   staticpro (&font_style_table);
   font_style_table = CALLN (Fvector, Vfont_weight_table, Vfont_slant_table,
                            Vfont_width_table);
diff --git a/src/lisp.h b/src/lisp.h
index 98c56e8ea1e..71b07f8dbc8 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3780,7 +3780,6 @@ call0 (Lisp_Object fn)
 }
 
 extern void defvar_lisp (struct Lisp_Fwd const *, char const *);
-extern void defvar_lisp_nopro (struct Lisp_Fwd const *, char const *);
 extern void defvar_bool (struct Lisp_Fwd const *, char const *);
 extern void defvar_int (struct Lisp_Fwd const *, char const *);
 extern void defvar_kboard (struct Lisp_Fwd const *, char const *);
@@ -3800,31 +3799,13 @@ call0 (Lisp_Object fn)
       #define cons_cells_consed globals.f_cons_cells_consed
 
    All C code uses the `cons_cells_consed' name.  This is all done
-   this way to support indirection for multi-threaded Emacs.
-
-   DEFVAR_LISP staticpro's the variable; DEFVAR_LISP_NOPRO does not, and
-   is used for variables that are protected in other ways (e.g., because
-   they can be accessed from another variable, which is itself
-   protected, see font_style_table on font.c as an example).  This is
-   not used in the HAVE_MPS build, where DEFVAR_LISP_NOPRO is equivalent
-   to DEFVAR_LISP.  */
-
+   this way to support indirection for multi-threaded Emacs.  */
 #define DEFVAR_LISP(lname, vname, doc)                 \
   do {                                                 \
     static struct Lisp_Fwd const o_fwd                 \
       = {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname};        \
     defvar_lisp (&o_fwd, lname);                       \
   } while (false)
-#ifdef HAVE_MPS
-#define DEFVAR_LISP_NOPRO DEFVAR_LISP
-#else
-#define DEFVAR_LISP_NOPRO(lname, vname, doc)           \
-  do {                                                 \
-    static struct Lisp_Fwd const o_fwd                 \
-      = {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname};        \
-    defvar_lisp_nopro (&o_fwd, lname);                 \
-  } while (false)
-#endif
 #define DEFVAR_BOOL(lname, vname, doc)                         \
   do {                                                         \
     static struct Lisp_Fwd const b_fwd                         \
diff --git a/src/lread.c b/src/lread.c
index 95ef9fbb628..e42e3f51b23 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -5492,25 +5492,15 @@ defvar_bool (struct Lisp_Fwd const *b_fwd, char const 
*namestring)
 }
 
 /* Similar but define a variable whose value is the Lisp Object stored
-   at address.  Two versions: with and without gc-marking of the C
-   variable.  The nopro version is used when that variable will be
-   gc-marked for some other reason, since marking the same slot twice
-   can cause trouble with strings.  */
+   at address.  */
 void
-defvar_lisp_nopro (struct Lisp_Fwd const *o_fwd, char const *namestring)
+defvar_lisp (struct Lisp_Fwd const *o_fwd, char const *namestring)
 {
   eassert (o_fwd->type == Lisp_Fwd_Obj);
   Lisp_Object sym = intern_c_string (namestring);
   XBARE_SYMBOL (sym)->u.s.declared_special = true;
   XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
   SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
-}
-
-void
-defvar_lisp (struct Lisp_Fwd const *o_fwd, char const *namestring)
-{
-  eassert (o_fwd->type == Lisp_Fwd_Obj);
-  defvar_lisp_nopro (o_fwd, namestring);
   staticpro (o_fwd->u.objvar);
 }
 
-- 
2.47.1

Rereading

https://lists.gnu.org/archive/html/emacs-devel/2025-01/msg01065.html

leaves me with a distinct urge to respond to it in detail, so I decided
not to, for now, since those urges don't usually go anywhere.

So my reaction is simply to be left wondering what, if anything, Eli
wants to agree to.

Eli, is it okay to apply this patch?  I'm ignoring the master branch for
now, this is only about feature/igc.  A normal merge process for
feature/igc, if it ever happens, would then make
8d471adecef540d49807dad114f7a11fb3fe2a95 go away on the master branch.

Pip