Subject: Re: bug#75521: scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX

From: Pip Cet <[email-masked]>
Date: Mon, 13 Jan 2025 14:16:04 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Mon, 13 Jan 2025 12:57:03 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > According to this comment:
>> >
>> >   /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
>> >   static Lisp_Object font_style_table;
>> >
>> > Vfont_weight_table is actually an element of font_style_table, and
>> > that one is protected:
>>
>> But then font_style_table is sometimes modified so it points to
>> (i.e. contains as its element) a new vector, and I don't see how
>> Vfont_weight_table is protected then.
>
> Modified where?

font_style_to_value:

      if (! noerror)
	return -1;
      eassert (len < 255);
      elt = make_vector (2, make_fixnum (100));
      ASET (elt, 1, val);
      ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
	    CALLN (Fvconcat, table, make_vector (1, elt)));
      return (100 << 8) | (i << 4);

It's not clear to me when this code is reached (usually, noerror is
false and we don't modify font_style_table), but if it ever is reached,
we lose (at least when --enable-checking or in temacs).

The crash is easy to fix, but it's not at all clear to me that modifying
font_style_table shouldn't also modify Vfont_weight_table etc.

However, it is clear that the _NOPRO isn't required or helpful, and
since that's the last usage, let's just remove this remnant of GCPRO
times.

If, in addition, we want to keep Vfont_weight_table synchronized with
the table used by the font code to resolve weight symbols, we can do
this (this would change behavior in those cases that previously resulted
in a crash):

diff --git a/src/font.c b/src/font.c
index e6c41e41258..9b3d294640e 100644
--- a/src/font.c
+++ b/src/font.c
@@ -47,7 +47,22 @@ Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011
 #define DEFAULT_ENCODING Qiso8859_1
 
 /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
-static Lisp_Object font_style_table;
+static Lisp_Object *font_style_table (int index)
+{
+  Lisp_Object *ret;
+  if (index == FONT_WEIGHT_INDEX)
+    ret = &Vfont_weight_table;
+  else if (index == FONT_SLANT_INDEX)
+    ret = &Vfont_slant_table;
+  else if (index == FONT_WIDTH_INDEX)
+    ret = &Vfont_width_table;
+  else
+    emacs_abort ();
+
+  CHECK_VECTOR (*ret);
+
+  return ret;
+}
 
 /* Structure used for tables mapping weight, slant, and width numeric
    values and their names.  */
@@ -376,10 +391,9 @@ font_pixel_size (struct frame *f, Lisp_Object spec)
 font_style_to_value (enum font_property_index prop, Lisp_Object val,
                      bool noerror)
 {
-  Lisp_Object table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
+  Lisp_Object table = *font_style_table (prop);
   int len;
 
-  CHECK_VECTOR (table);
   len = ASIZE (table);
 
   if (SYMBOLP (val))
@@ -418,8 +432,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);
-      ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
-	    CALLN (Fvconcat, table, make_vector (1, elt)));
+      *font_style_table(prop) =
+	CALLN (Fvconcat, table, make_vector (1, elt));
       return (100 << 8) | (i << 4);
     }
   else
@@ -461,8 +475,7 @@ font_style_symbolic (Lisp_Object font, enum font_property_index prop,
 
   if (NILP (val))
     return Qnil;
-  table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
-  CHECK_VECTOR (table);
+  table = *font_style_table (prop);
   i = XFIXNUM (val) & 0xFF;
   eassert (((i >> 4) & 0xF) < ASIZE (table));
   elt = AREF (table, ((i >> 4) & 0xF));
@@ -588,13 +601,12 @@ font_prop_validate_style (Lisp_Object style, Lisp_Object val)
   if (FIXNUMP (val))
     {
       EMACS_INT n = XFIXNUM (val);
-      CHECK_VECTOR (AREF (font_style_table, prop - FONT_WEIGHT_INDEX));
       if (((n >> 4) & 0xF)
-	  >= ASIZE (AREF (font_style_table, prop - FONT_WEIGHT_INDEX)))
+	  >= ASIZE (*font_style_table (prop)))
 	val = Qerror;
       else
 	{
-	  Lisp_Object elt = AREF (AREF (font_style_table, prop - FONT_WEIGHT_INDEX), (n >> 4) & 0xF);
+	  Lisp_Object elt = AREF (*font_style_table (prop), (n >> 4) & 0xF);
 
 	  CHECK_VECTOR (elt);
 	  if ((n & 0xF) + 1 >= ASIZE (elt))
@@ -5949,11 +5961,6 @@ syms_of_font (void)
 gets the repertory information by an opened font and ENCODING.  */);
   Vfont_encoding_alist = Qnil;
 
-  /* FIXME: These 3 vars are not quite what they appear: setq on them
-     won't have any effect other than disconnect them from the style
-     table used by the font display code.  So we make them read-only,
-     to avoid this confusing situation.  */
-
   DEFVAR_LISP ("font-weight-table", Vfont_weight_table,
 	       doc: /*  Vector of valid font weight values.
 Each element has the form:
@@ -5961,25 +5968,18 @@ syms_of_font (void)
 NUMERIC-VALUE is an integer, and SYMBOLIC-NAME and ALIAS-NAME are symbols.
 This variable cannot be set; trying to do so will signal an error.  */);
   Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
-  make_symbol_constant (intern_c_string ("font-weight-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 ("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"));
-
-  staticpro (&font_style_table);
-  font_style_table = CALLN (Fvector, Vfont_weight_table, Vfont_slant_table,
-			    Vfont_width_table);
 
   DEFVAR_LISP ("font-log", Vfont_log, doc: /*
 A list that logs font-related actions and results, for debugging.