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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 27 Aug 2024 19:23:50 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: execvy@gmail.com, 72692@debbugs.gnu.org, juri@linkov.net
> 
> "Eli Zaretskii" <eliz@gnu.org> 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
think there are any situations now where recompute_basic_faces could
be called when only part of the basic faces is realized, but I
couldn't convince myself in that fact, and having code that clearly
follows the idea, instead of simplifying it in a way that depends on
some assumption, sounded like better strategy, let alone more
future-proof.)

> > that testing against BASIC_FACE_ID_SENTINEL is stronger, because it
> > also prevents setting the flag when we have some faces in the cache,
> > but they are all basic.  Setting the face_change flag when we don't
> > need that will cause us to unnecessarily re-realize the basic faces,
> > so refraining from that prevents performance degradation.
> 
> So it's a performance optimization, and one which helps only users who
> don't use non-ASCII during redisplay.

It's a performance optimization, yes.  But the second part
misinterprets the intent, I think.  I would describe this differently:
when recompute_basic_faces is called and there are non-ASCII faces in
the cache, we _must_ set the face_change flag to recompute all the
faces, because otherwise the non-ASCII faces will be in danger of
being incorrect, and so will be their fontsets.  If there are no
non-ASCII faces in the cache, setting the face_change flag is not
needed, and doing so will make us do unnecessary (and potentially
expensive) work at the next call to init_iterator.

> >> > The performance problem was caused by init_iterator calling
> >> > recompute_basic_faces every time and then shooting itself in the foot
> >> > by immediately setting the face_change flag,
> >>
> >> And it's easily fixed by clearing the face_change flag after
> >> recompute_basic_faces runs, rather than before
> >
> > I don't follow: if we clear the flag after recompute_basic_faces
> > exits, the faces will not be re-realized by next redisplay, and we
> > might be back at the problem of having non-ASCII faces whose ASCII
> > parents were removed from the cache and re-realize.  How will this be
> > useful?  Or what am I missing?
> 
> 'recompute_basic_faces' never realizes non-ASCII faces, and the face
> cache was empty before we called it, so it's safe to leave the flag off.
> The performance problem was caused by setting the flag unnecessarily in
> these circumstances.

Well, now we refrain from setting the flag in the first place, so no
need to reset it.

> >> > but this is now avoided
> >> > because init_iterator only calls recompute_basic_faces if the frame's
> >> > face cache is empty.  Most other places that call
> >> > recompute_basic_faces also do that when the cache is empty (in which
> >> > case we don't have to worry about non-ASCII faces), but some call it
> >> > unconditionally, and the above is for those.
> >>
> >> ??? I don't see any such callers in my backtraces of Emacs with or
> >> without the patch
> >
> > There are 2 such callers in frame.c, one of them was in the backtrace
> > you show in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=72692#98,
> > viz.:
> 
> Yes, but that was the reproducer, not code that ran into performance
> issues.  Setting the background alpha is hardly performance critical :-)

The performance problem was caused by the solution which set the
face_change flag too eagerly.  There was no performance problem before
that.  The original motivation for the change which ended up making
the performance worse was not performance, it was to avoid the bug
which was shown by the reproducer, i.e. to handle the case where
recompute_basic_faces was called in a situation where there were
non-ASCII faces in the cache, as your traces about freeing their
fontset show.

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

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

> >> If the design indeed relies on dangling pointers to freed memory
> >> surviving said freeing (it doesn't), that would be a very bad design
> >> indeed, particularly since this fact isn't documented.
> >
> > 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.

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.

> >> The correct thing to do here is to ensure all references to
> >> ascii_face are removed, by freeing the non-ASCII faces, before we
> >> remove the ascii_face itself.
> >
> > Maybe.  I'm not yet sure, primarily because we use the current code
> > for so many decades.
> 
> Okay, I can live with "maybe".

See above a significant addition to "maybe".

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