Re: Not protecting Lisp objects from GC

[Eli Zaretskii]

> Date: Wed, 29 Jan 2025 09:46:24 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gerd.moellmann@gmail.com, 
> emacs-devel@gnu.org
> 
> "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.

The patch you proposed, like that of Stefan, reverts the fix for a
separate issue: if font_style_to_value extends the style table, the
Vfont_* variables keep their old values instead of reflecting the
changes to the style table.  I don't think this is right, so I'd
prefer to keep that fix, as it doesn't affect, and isn't affected by,
removal of DEFVAR_LISP_NOPRO and defvar_lisp_nopro.  Am I missing
something?

> 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.

I meant to make the change on feature/igc, indeed, and then have it
merged when the branch lands on master.

To avoid asking you and Stefan do something that I can easily do
myself, I made the change I had in mind, see the patch below.  It is a
proper subset of the patch you proposed, AFAICT.  I didn't yet install
it, because I have a feeling I'm missing something here, since both
you and Stefan wanted to remove the changes in commit 8d471adecef, and
I don't understand why.

If there are no objections to the patch below, I will install it on
feature/igc.  If you do have objections, please describe them.

Thanks.

diff --git a/src/font.c b/src/font.c
index dfe479f..5ee3615 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,14 +5979,14 @@ 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.  */);
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);
 }