Re: Not protecting Lisp objects from GC

[Pip Cet]

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

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

The problem I pointed out was that DEFVAR_LISP_NOPRO was used
incorrectly.  The fix was to use DFEVAR_LISP instead.

So, no, changing behavior in the way you did was not "a fix for it", it
was a separate behavior change which should have been discussed
independently.

You asked me, IIUC, for a review.  I pointed out things that should have
been changed before pushing.  Instead, you added the problematic comment
and pushed the originally-proposed version.  I'm sorry if I
mischaracterized that as "hasty".

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

You mean commit 8d471adecef540d49807dad114f7a11fb3fe2a95 ?  It includes
a three-line comment specifically about DEFVAR_LISP_NOPRO.

The discussion that led to it was:

Eli > WDYT about the patch below?  It's supposed to keep the 3 variables in
Eli > sync with the corresponding slots in font_style_table, if the slots
Eli > are ever modified.

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

Eli > I was asking about the problem where that code makes the values of
Eli > Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
Eli > That's the only problem I tried to solve with that patch.

I read that commit, and this last paragraph, as "the point of this patch
is to solve only one problem, that of GC vulnerability" As this
vulnerability was caused by incorrect use of DEFVAR_LISP_NOPRO, and we
have found a better way of avoiding the vulnerability (use DEFVAR_LISP),
I fail to see what the remaining problems you're trying to solve with
that patch are.

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

Above, you said it was only the GC vulnerability you 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

Then so is yours.

> changes in an old and stable code, when a simpler, more localized
> change would do.

I can accept that decision. s/_NOPRO//g is that simple, localized change
you're looking for.  8d471adecef540d49807dad114f7a11fb3fe2a95 is not.

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

I'm sorry, but given that the commit includes the comment about not
staticproing the variables, the context in which it arose was that of
fixing a GC vulnerability, and there is no bug number for the separate
problem this "fixes", my recommendation (not a request; do things as you
please, obviously) would be to revert it for now, open a bug number,
wait for actual discussion, maybe even take it into account, then push.

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

No reason to apologize.  If you want to propose a compromise with a
corrected patch, that would be excellent and may be a way forward here.

It is my strong impression that this discussion could benefit from the
input of Stefan Kangas, if he has the time and inclination to spend more
time on the matter.  But I understand his time is limited and he has to
prioritize things.

So my proposal is for Stefan to briefly ack or nak this; if there's an
ack, I'll wait for his input, even if it's just "can't comment after
all", before responding again.

Pip