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

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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 23:09:27 +0200

> Date: Tue, 14 Jan 2025 17:02:41 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
> 
> "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).

I was asking about the problem where that code makes the values of
Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
That's the only problem I tried to solve with that patch.  That
Vfont_weight_table etc. are also updated to reflect the changes in
font_style_table is a nice benefit.

If you agree that this problem will be solved by the patch, then I'd
like to install it, because it's very simple and localized, so the
risk to break something is IMO very low.

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

I think this wasn't reported because that code is seldom if ever
executed.  I couldn't find a way to enter it.  But I see no reason to
remove it at this point.

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

That can be alleviated a bit by adding commentary which explains this
subtlety.

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

Vfont_weight_table etc. are supposed not to be modifiable (it should
cause Emacs to signal an error).  If you can show a recipe for causing
a crash by modifying them, I will certainly see why we don't behave as
the doc string says.

In any case, this is a separate issue.

> I think we should fix this code not to crash with or without
> --enable-checking.

Crashes should be avoided, I agree.

> It may be best to simply remove the Lisp variables entirely.

I don't want to remove them because they are used, albeit in a small
number of places.

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

The code as implemented cannot allow the value to exceed 255, because
each of the 3 style properties is restricted to be a vector of at most
256 elements.  See the comment about that in font.h.

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

The original code (from Emacs 23) called emacs_abort here, so it
aborted in both development and production builds.  We could restore
that.  I don't think it matters much, since I cannot imagine a
real-life situation where we get to that limit.  And it's a separate
issue as well.

> font.c needs careful analysis

That is certainly true (and the same goes for fontset.c, btw).

> because there are too many apparent bugs
> in it right now (it also assumes that Vfont_encoding_alist isn't
> circular, for example).

I don't think it's quite as buggy as you think.  It does have quite a
few places which are hard to explain, because the original experts who
developed that part of Emacs are no longer active in Emacs
development.

> "Minimal" changes are not the right thing to do here.

Let's disagree about that.  Emacs is a large, old, and very stable
program, so "minimal changes" are IMO exactly the right thing, as long
as we don't change things radically.  E.g., if someone wanted to
completely redesign and reimplement how Emacs selects fonts, and could
show convincingly that the new design will give us clear advantages in
speed and/or correctness, then of course "minimal changes" won't
apply.  But otherwise making potentially disruptive changes in code we
don't understand well which works quite well on several very different
platforms is not TRT in my book.

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

Emacs originally didn't distinguish between "normal" and "medium".

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

Probably one more sign that this code is not used in practice.

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

Please elaborate: when and how this could not be true.

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

We could do that, but the original code didn't, either.  IOW, this is
a separate issue.

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

I was asking about the fix to the single issue I intended to fix.

Previous Next

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