GNU bug report logs - #75521
scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: bug#75521: scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX
Date: Tue, 14 Jan 2025 17:02:41 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> Date: Mon, 13 Jan 2025 22:07:21 +0200
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>>
>> Anyway, please don't install anything yet.  I want to take a closer
>> look at this code and its usage, and I've ran out of time for today.
>
> WDYT about the patch below?  It's supposed to keep the 3 variables in
> sync with the corresponding slots in font_style_table, if the slots
> are ever modified.

If you meant to ask whether the patch will momentarily render this
specific bug harmless, I believe it will (but expose other bugs which
will still cause crashes).

However, I don't like it one bit, and reviewing it finds many other
things that seem like bugs here.

I've already answered the question of what I think about this approach:
"It's a very fragile approach, as evidenced by the fact that it was
broken for so long, and there's absolutely no benefit to it.  The next
person to attempt to change this code won't understand why we use a
non-staticpro'd variable which is kept in sync with a staticpro'd one
(because there is no reason to do so), and probably break things again."

Note that it's not just in theory that it's fragile: I have experimented
with changes to the nativecomp code which would have broken it, because
make_symbol_constant shouldn't just trap writes, it should also allow
the nativecomp code to generate better code.

It also changes behavior, of course, and the new behavior doesn't make
much sense: with or without --enable-checking, it's trivial to cause
further crashes by modifying the Vfont_weight_table variable's value,
for example.

I think we should fix this code not to crash with or without
--enable-checking.  It may be best to simply remove the Lisp variables
entirely.

> diff --git a/src/font.c b/src/font.c
> index 8638226..77fc74b 100644
> --- a/src/font.c
> +++ b/src/font.c
> @@ -418,8 +418,23 @@ font_style_to_value (enum font_property_index prop, Lisp_Object val,
>        eassert (len < 255);

I don't see why this eassert makes sense.  If we ever reach this code,
why do we assume we can't reach it 256 times?  If we reach it 256 times
(or thereabouts), why crash Emacs?  If we want to crash Emacs in this
case, why only crash it if --enable-checking has been specified?

font.c needs careful analysis, because there are too many apparent bugs
in it right now (it also assumes that Vfont_encoding_alist isn't
circular, for example).  "Minimal" changes are not the right thing to do
here.

>        elt = make_vector (2, make_fixnum (100));

This is also strange: 100 is the "medium" weight; we should be using 80,
which is the "normal" one.  100 is the correct default slant and width,
though.

>        ASET (elt, 1, val);
> -      ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> -	    CALLN (Fvconcat, table, make_vector (1, elt)));
> +      Lisp_Object new_table = CALLN (Fvconcat, table, make_vector (1, elt));

This is also strange: the tables are documented to be sorted
numerically, but I don't see any code that relies on that.  vconcat
certainly doesn't leave the tables sorted.

I do see code, e.g. font_style_symbolic, which assumes that (i>>4) is
the index corresponding to value i; this is not currently true, and
font_style_symbolic will either eassert or fail to find the right entry.
In the absence of checking, it will most likely cause crashes...

> +      /* Update the corresponding variable with the new value.  */
> +      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;

"eassume (0);" is the right thing to do here, IMHO.  If the default
label isn't optimized out, it's redundant code.  If we ever reach the
default label, something else is severely wrong, so the
eassume-as-eassert thing would save us.

> +	}
> +      ASET (font_style_table, prop - FONT_WEIGHT_INDEX, new_table);
>        return (100 << 8) | (i << 4);

I need to check what this return value is used for: if 0x64f0 is the
right return value for i == 0xf, why is it 0x6500 for i == 0x10?

So, sorry, further fixes required, a minimal fix won't work.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.