Subject: Re: bug#72692: Emacs 31.05 (40eecd594ac) get SIGSEGV on Linux (Linux 6.6.45 Kde Wayland)

From: Pip Cet <[email-masked]>
Date: Wed, 28 Aug 2024 12:07:05 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Tue, 27 Aug 2024 19:23:50 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > As explained in my previous message, the test for having non-basic
>> > faces in the cache is because when we have only the basic faces, the
>> > problem with sharing the fontsets, and with having non-ASCII faces
>> > that point to released ASCII faces, cannot happen, and therefore
>> > there's no need to do anything.
>>
>> Indeed, but that adds a perilous dependency on whether we only have the
>> basic faces (no non-ASCII faces in use) or not.
>
> Why is it "perilous"?  It's an optimization, that's all.  (I don't

Sorry, my confusion, cleared up now.

>> > There's also one caller in fontset.c (which calls Fclear_face_cache
>> > before recompute_basic_faces, but Fclear_face_cache might decide not
>> > to clear the cache).
>> >
>> >> but I do see significant performance differences, so this must be
>> >> wrong.
>> >
>> > What performance difference did you see, and what is "this" that you
>> > think must be wrong?
>>
>> "the above is for those".  It's not those callers that cause the
>> performance issue.
>
> OK, so hopefully the problem will be gone now.

I agree.

>> >> we end up looking at XCHAR_TABLE (Qnil)->extras[3], which segfaults
>> >> because nil is not a char table.
>> >>
>> >> The code path without assertions is similar, it just that the
>> >> XCHAR_TABLE happens further down the call stack, in fontset_find_font.
>> >> See the original backtrace.
>> >
>> > OK, so adding protection against fontset being nil, where we currently
>> > lack that, should take care of these cases, right?
>>
>> It'll turn a segfaulting bug into a wrong-behavior bug.
>
> That depends on what the code we install to handle the nil case will
> do, no?  If we do it correctly (assuming there is a correct way of
> handling this), there will be no wrong behavior, right?

Yes, but that would require much more than checking for nil, it would
mean checking that the fontset entry is still valid and refers to the
same fontset it did originally, not a new fontset.

>> > Faces on this level (i.e. those cached in the frame's face cache and
>> > referenced by their face ID) don't matter in Emacs until they are used
>> > to display something.  The design, AFAIU it, is to make sure the
>> > cached faces are in good shape when we need them for display.
>>
>> Indeed, and dangling pointers hardly qualify as good shape.
>
> There's a misunderstanding here, I think.  The point I was trying to
> make is that you will find in our code a lot of situations and paths
> whereby faces are released while their cache indices (the FACE_ID
> thing) are still stored in places.  A case in point is the current
> glyph matrix of every window: it includes arrays of glyphs, where each
> glyph stores its face id.  But the current matrix is accessed and if
> needed updated only during redisplay, whereas the faces whose cache
> indices it stores could be freed due to other reasons between
> redisplay cycles.  The design therefore calls for the face_change to
> be set whenever the face information cannot be trusted, and that flag
> causes the display engine to throw away and recompute all the basic
> faces before it does anything else.  As a side effect of this, all the
> non-ASCII faces will disappear from the cache, and will be re-realized
> and re-cached when they are needed.
> It should be possible to change this basic design, so that whenever
> faces are released, they are immediately regenerated, but (a) this
> will be a very large job, and (b) I expect it to cause performance
> degradation (unless we also radically change how faces are realized),
> because it will most probably cause us to re-realize faces much more
> frequently.  By contrast, the current design strives to do that
> lazily, only when faces must be valid, and thus avoids expensive
> recomputation of the faces as much as possible.

Thanks for that explanation, I think it addresses my concerns but should
still be documented.  I shall attempt to come up with a documentation
patch.

> The case which we are discussing here is simply one more situation
> where we must signal that faces are no longer reliable, but weren't
> doing that until now.  At least this is how I understand it.

I agree now.

>> >> And please, let's document such "design decisions" (actually, bugs) if
>> >> you refuse to let anyone fix them.  Having a pointer in a structure
>> >> which might point to random memory is a bug unless there is a very loud
>> >> comment advising us of this fact.
>> >
>> > If I come to the conclusion that these are indeed bugs, I will try to
>> > fix them, of course.
>>
>> I still think #72802 is clearly a bug and you said you didn't want it
>> fixed, so forgive my confusion on this matter.
>
> IMO, you over-reacted in that case.  I didn't say I didn't want to
> solve that, I said I didn't want to install the solution on master at
> this time.  I suggested instead to install it on a feature branch,
> which will, I hope, be merged to master some time soon.  This is the
> usual policy of not rushing non-trivial changes that could destabilize
> Emacs until it either is urgent or we destabilize it for some other
> good reason anyway.  We should always keep in mind that quite a few
> people use the master branch for production sessions, so keeping it
> relatively stable has a lot of merit in my book.

Thank you, that is very helpful.  If you don't mind, I'll think about it
some more before deciding how to proceed there.  Obviously merging the
change only if/when the no-purespace branch is merged is perfectly
acceptable to me.  There is, indeed, no rush to do anything there.

Pip