Re: Not protecting Lisp objects from GC

[Eli Zaretskii]

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

> As I said:
> 
> "Not really a benefit: updating the variables might expose other
> (crashable) bugs more."

If you can point them out now, please do.  Otherwise, let's wait for
them to be reported, and let's fix them at that time.  This code is
very old and stable, so I have no reasons to consider it having bugs
unless they are reported with all the relevant details.

> That was assuming DEFVAR_LISP_NOPRO would stay.  If we agree that is to
> be removed, the potential problems remain but the benefits disappear.

The benefit is to remove the problematic and confusing behavior
whereby the code intended to change the style tables, but the
variables exposing the styles to Lisp are not updated.  This benefit
remains.  And it has nothing to do with removing DEFVAR_LISP_NOPRO.

> > separate issue: if font_style_to_value extends the style table, the
>   ^^^^^^^^
> 
> If it's a separate issue, we should fix it separately.

I did fix it separately.  The commit which fixed that didn't touch
DEFVAR_LISP_NOPRO or defvar_lisp_nopro.

> > Vfont_* variables keep their old values instead of reflecting the
> > changes to the style table.
> 
> Indeed. That's surprising, but documented and the established
> behavior, and you changed only half of it.

It's neither documented nor established, AFAICT.  What _is_ documented
is that the Vfont_* variables themselves cannot be changed from Lisp,
but that's not what happens in the scenario which I intended to fix.

> If you want to change this behavior, the right way is to look at the
> second patch I provided, which made that change by removing
> Vfont_style_table entirely, fixing both possibilities of disconnecting
> the internal from the Lisp-visible tables.
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75521;msg=37

That change is too radical, IMO.  I saw no need for such radical
changes in an old and stable code, when a simpler, more localized
change would do.

> > 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.
> 
> The fix is a workaround to avoid crashes caused by DEFVAR_LISP_NOPRO.

No, it is a fix for a separate problem that is not affected by
DEFVAR_LISP_NOPRO.

> This part was dropped:
> 
>  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);
> 
> 
> If that wasn't intentional

It wasn't intentional, sorry.  I will add that to the patch.

> The alloc.c part was also dropped.

Likewise.

> Your counterproposal is a patch reduced even further, to the point where
> it leaves in two of the four comments telling people to avoid staticpro,
> which they can no longer do.  Here, I have to disagree.

Those comments were left in the code by my mistake, I will remove
them.