Re: Not protecting Lisp objects from GC

[Pip Cet]

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

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

It reverts a hastily-conceived behavior change in font.c which may have
reduced some crashes and triggered some others.

As I said:

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

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

> separate issue: if font_style_to_value extends the style table, the
  ^^^^^^^^

If it's a separate issue, we should fix it separately.

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

> I don't think this is right,

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

> 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.
Removing DEFVAR_LISP_NOPRO means no "fix" is needed here.

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

Considering the "if it ever happens" part, let's find a solution for
feature/igc first.  Any merge of these changes will require manual
intervention, given the complex commit history so far, in order to
ensure no problematic comments or behavior changes remain on the merged
master branch.

> 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.  */);

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, please provide a corrected patch, and
indicate what the base version is.  Thanks!

The alloc.c part was also dropped.

Sorry this was long and technical.

My impression is Stefan Kangas made a compromise proposal, to fix this
on master; you disagreed.

I suggested thinking about a second compromise proposal (if I implied
this would be sufficient, I misspoke), fixing this on feature/igc and
leaving the obsolete code on master until feature/igc is merged, if that
ever happens; you disagreed.

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.

Pip