Re: Not protecting Lisp objects from GC

[Eli Zaretskii]

> Date: Thu, 30 Jan 2025 11:52:08 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: stefankangas@gmail.com, gerd.moellmann@gmail.com, emacs-devel@gnu.org
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> >> Date: Thu, 30 Jan 2025 10:41:08 +0000
> >> From: Pip Cet <pipcet@protonmail.com>
> >> Cc: stefankangas@gmail.com, gerd.moellmann@gmail.com, emacs-devel@gnu.org
> >>
> >> "Eli Zaretskii" <eliz@gnu.org> writes:
> >>
> >> > The patch you proposed, like that of Stefan, reverts the fix for a
> >>
> >> It reverts a hastily-conceived behavior change in font.c which may have
> >> reduced some crashes and triggered some others.
> >
> > There was no haste.  You pointed out a problem, and I agreed that it
> > was a problem and installed a fix for it.
> 
> The problem I pointed out was that DEFVAR_LISP_NOPRO was used
> incorrectly.  The fix was to use DFEVAR_LISP instead.

Yes, but as part of that discussion the problematic behavior when
font_style_table is modified by the code was raised.  The changes I'm
talking about, which I want to keep, were installed to fix that
problem.

> So, no, changing behavior in the way you did was not "a fix for it", it
> was a separate behavior change which should have been discussed
> independently.

Yes, it was a separate behavior change.

> I'm sorry, but given that the commit includes the comment about not
> staticproing the variables, the context in which it arose was that of
> fixing a GC vulnerability, and there is no bug number for the separate
> problem this "fixes", my recommendation (not a request; do things as you
> please, obviously) would be to revert it for now, open a bug number,
> wait for actual discussion, maybe even take it into account, then push.
> 
> > It wasn't intentional, sorry.  I will add that to the patch.
> 
> No reason to apologize.  If you want to propose a compromise with a
> corrected patch, that would be excellent and may be a way forward here.

The patch I propose is below.  It now removes the two comments I've
missed in the previous proposal.

diff --git a/src/alloc.c b/src/alloc.c
index 9437bed..eb9385a 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 dfe479f..a1843bc 100644
--- a/src/font.c
+++ b/src/font.c
@@ -5970,7 +5970,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 +5979,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 98c56e8..5bb05ed 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 *);
@@ -3802,12 +3801,7 @@ call0 (Lisp_Object fn)
    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.  */
+   DEFVAR_LISP staticpro's the variable.  */
 
 #define DEFVAR_LISP(lname, vname, doc)                 \
   do {                                                 \
@@ -3815,16 +3809,6 @@ #define DEFVAR_LISP(lname, vname, doc)                   
\
       = {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 95ef9fb..e42e3f5 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);
 }