GNU bug report logs - #72692
Emacs 31.05 (40eecd594ac) get SIGSEGV on Linux (Linux 6.6.45 Kde Wayland)

Previous Next

Full log

Message #127 received at 72692 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: execvy <at> gmail.com, 72692 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#72692: Emacs 31.05 (40eecd594ac) get SIGSEGV on Linux (Linux
 6.6.45 Kde Wayland)
Date: Mon, 26 Aug 2024 15:39:18 +0300

> Date: Mon, 26 Aug 2024 05:52:11 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Juri Linkov <juri <at> linkov.net>, execvy <at> gmail.com, 72692 <at> debbugs.gnu.org
> 
> >> -      f->face_change = true;
> >
> > Sorry about that.  I've now reverted that change; we will have to find
> > a cheaper solution for the original problem.
> 
> This patch, moving down the setting of f->face_change to when we
> definitely know it's required, seems to avoid the
> always-in-the-foreground problem, but I'm not sure it fixes Juri's
> performance issue:

We should find the correct patch first, and think about performance
only after that, of course.  And in this case, I'm not sure that the
fontset is the only problem, nor that when we free it, we must in all
the cases re-realize all the faces.

If all we want is to set the face_change flag without causing
performance issues, then the patch below does it, and AFAIU should
prevent the segfaults in this bug (please check, as I'm unable to
trigger the segfaults):

diff --git a/src/xfaces.c b/src/xfaces.c
index 684b6cc..1baa364 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -733,9 +733,13 @@ recompute_basic_faces (struct frame *f)
 {
   if (FRAME_FACE_CACHE (f))
     {
+      bool non_basic_faces_cached =
+	FRAME_FACE_CACHE (f)->used >= BASIC_FACE_ID_SENTINEL;
       clear_face_cache (false);
       if (!realize_basic_faces (f))
 	emacs_abort ();
+      if (non_basic_faces_cached)
+	f->face_change = true;
     }
 }
 
The idea here is that if all we have in the frame's face cache are the
basic faces, then they are all ASCII faces, and therefore the issue
with non-ASCII faces still holding the freed fontset ID should not
arise.  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, 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.

Once again, I'm not yet sure if this is TRT.

Btw, thinking again about the original problem, I don't think I
understand why free_face_fontset could cause a segfault.  All that
function does is remove the fontset from the cache and put nil into
its slot.  Thereafter trying to access the fontset by its ID will
yield nil.  So how can this cause trouble?  The two ways I can think
of are:

  . Some code that calls FONTSET_FROM_ID and does not check that the
    result is nil.  We indeed have a couple of places like that, but
    it should be easy to fix them.  However, can this cause a
    segfault?
  . While some face holds to a (stale) fontset ID, a new fontset is
    created and stored in the cache at the slot corresponding to that
    stale ID.  So accessing this ID will yield a wrong, but still
    valid fontset.  Can this cause a segfault?

Are there any other scenarios?  (And in your reproducer, the second
scenario above was AFAIU disabled by never decreasing next_fontset_id,
so only the first one could cause some trouble?)

> However, I would still prefer a solution which doesn't, even for a short
> period of time, leave the non-ASCII faces' ->ascii_face pointing to a
> face which has been freed, and their ->fontset identifying a fontset
> which might be nil.

This goes against the design of the faces and their interaction with
the display code, I think.

> Is it true that all non-ASCII faces share their ASCII face's hash and
> thus live in the same collision chain?  We could walk that and unrealize
> them all when the ASCII face is unrealized, then.
> 
> Something like this:

If all we want is to free all the faces that belong to a certain frame
and share the same ascii_face, why not simply walk the frame's face
cache looking for such faces, and free each one of them?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.