master 5ee43ba0df causing display hangs?

master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

I briefly noticed commit 5ee43ba0dfd96e5d39da556260346bd3c8ff99c1
(identified by bysecting) causes some modes to hang forever during
display (mu4e-view-mode and the underlying gnus-article-mode), with C-g
being ignored.

Looking at the commit itself doesn't make me think of anything wrong, so
I wonder if this could be caused by some customization.

This shouldn't cause an irrecoverable hang though.

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 07 Dec 2019 14:08:43 +0100
> 
> I briefly noticed commit 5ee43ba0dfd96e5d39da556260346bd3c8ff99c1
> (identified by bysecting) causes some modes to hang forever during
> display (mu4e-view-mode and the underlying gnus-article-mode), with C-g
> being ignored.

The smallest self-contained recipe for reproducing the problem will be
appreciated.

Thanks.

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> Date: Sat, 07 Dec 2019 19:42:39 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > I briefly noticed commit 5ee43ba0dfd96e5d39da556260346bd3c8ff99c1
> > (identified by bysecting) causes some modes to hang forever during
> > display (mu4e-view-mode and the underlying gnus-article-mode), with C-g
> > being ignored.
> 
> The smallest self-contained recipe for reproducing the problem will be
> appreciated.

A stab in the dark: does the below fix the problem?

diff --git a/src/xfaces.c b/src/xfaces.c
index 6db4dcd..a2fbed8 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2168,7 +2168,7 @@ face_inherited_attr (struct window *w, struct frame *f,
       if (CONSP (parent_face))
 	{
 	  Lisp_Object tail;
-	  for (tail = parent_face; !NILP (tail); tail = XCDR (tail))
+	  for (tail = parent_face; CONSP (tail); tail = XCDR (tail))
 	    {
 	      ok = get_lface_attributes (w, f, XCAR (tail), inherited_attrs,
 					 false, named_merge_points);

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

On Sat, Dec 07 2019, Eli Zaretskii wrote:
> A stab in the dark: does the below fix the problem?
>
> diff --git a/src/xfaces.c b/src/xfaces.c
> index 6db4dcd..a2fbed8 100644
> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -2168,7 +2168,7 @@ face_inherited_attr (struct window *w, struct frame *f,
>        if (CONSP (parent_face))
>       {
>         Lisp_Object tail;
> -	  for (tail = parent_face; !NILP (tail); tail = XCDR (tail))
> +	  for (tail = parent_face; CONSP (tail); tail = XCDR (tail))
>           {
>             ok = get_lface_attributes (w, f, XCAR (tail), inherited_attrs,
>                                        false, named_merge_points);

Doesn't look like.

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Date: Sat, 07 Dec 2019 19:56:10 +0100
> Cc: emacs-devel@gnu.org
> 
> > --- a/src/xfaces.c
> > +++ b/src/xfaces.c
> > @@ -2168,7 +2168,7 @@ face_inherited_attr (struct window *w, struct frame *f,
> >        if (CONSP (parent_face))
> >       {
> >         Lisp_Object tail;
> > -	  for (tail = parent_face; !NILP (tail); tail = XCDR (tail))
> > +	  for (tail = parent_face; CONSP (tail); tail = XCDR (tail))
> >           {
> >             ok = get_lface_attributes (w, f, XCAR (tail), inherited_attrs,
> >                                        false, named_merge_points);
> 
> Doesn't look like.

Thanks.  Then I guess I will need that recipe to see where I goofed.

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

On Sat, Dec 07 2019, Eli Zaretskii wrote:
> Thanks.  Then I guess I will need that recipe to see where I goofed.

Tried a few times to get a small repro, but this might be easier to be
debugged.

> memcpy (inherited_attrs, attrs, LFACE_VECTOR_SIZE * sizeof (attrs[0]));
> while (UNSPECIFIEDP (attr_val)
>        && !NILP (inherited_attrs[LFACE_INHERIT_INDEX])
>        && !UNSPECIFIEDP (inherited_attrs[LFACE_INHERIT_INDEX]))
>   {
>     Lisp_Object parent_face = inherited_attrs[LFACE_INHERIT_INDEX];
>     bool ok;
>     if (CONSP (parent_face))
>       {
>         Lisp_Object tail;
>         for (tail = parent_face; !NILP (tail); tail = XCDR (tail))
>           {
>             ok = get_lface_attributes (w, f, XCAR (tail), inherited_attrs,
>                                        false, named_merge_points);
>             if (!ok)
>               break;

While attaching with gdb I noticed this is where it gets stuck in a loop.
get_lface_attributes returns !ok, break stops only the for loop, but
since nothing else changed for the upper "while" it goes on forever.

Looking at few lines below, it looks like this needs to break out of the
while() as well?

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Cc: emacs-devel@gnu.org
> Date: Sun, 08 Dec 2019 00:03:35 +0100
> 
> > memcpy (inherited_attrs, attrs, LFACE_VECTOR_SIZE * sizeof (attrs[0]));
> > while (UNSPECIFIEDP (attr_val)
> >        && !NILP (inherited_attrs[LFACE_INHERIT_INDEX])
> >        && !UNSPECIFIEDP (inherited_attrs[LFACE_INHERIT_INDEX]))
> >   {
> >     Lisp_Object parent_face = inherited_attrs[LFACE_INHERIT_INDEX];
> >     bool ok;
> >     if (CONSP (parent_face))
> >       {
> >         Lisp_Object tail;
> >         for (tail = parent_face; !NILP (tail); tail = XCDR (tail))
> >           {
> >             ok = get_lface_attributes (w, f, XCAR (tail), inherited_attrs,
> >                                        false, named_merge_points);
> >             if (!ok)
> >               break;
> 
> While attaching with gdb I noticed this is where it gets stuck in a loop.
> get_lface_attributes returns !ok, break stops only the for loop, but
> since nothing else changed for the upper "while" it goes on forever.
> 
> Looking at few lines below, it looks like this needs to break out of the
> while() as well?

Probably.  But I'd like to understand how come we get that 'false'
return value, to make sure there isn't a larger problem here.  Could
you please look around in the debugger and tell me what face is being
processed in that case and what is its Lisp definition?  Also, I'd
like to see a C and Lisp backtraces from the infloop.  Let me know if
you need help with GDB commands to do that.

Thanks!

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

On Sun, Dec 08 2019, Eli Zaretskii wrote:
> Probably.  But I'd like to understand how come we get that 'false'
> return value, to make sure there isn't a larger problem here.  Could
> you please look around in the debugger and tell me what face is being
> processed in that case and what is its Lisp definition?  Also, I'd
> like to see a C and Lisp backtraces from the infloop.  Let me know if
> you need help with GDB commands to do that.

Any hint to dump lisp from gdb would be appreciated (I've quickly
skimmed through .gdbinit but I'm a bit lost).

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Date: Sun, 08 Dec 2019 17:03:49 +0100
> Cc: emacs-devel@gnu.org
> 
> On Sun, Dec 08 2019, Eli Zaretskii wrote:
> > Probably.  But I'd like to understand how come we get that 'false'
> > return value, to make sure there isn't a larger problem here.  Could
> > you please look around in the debugger and tell me what face is being
> > processed in that case and what is its Lisp definition?  Also, I'd
> > like to see a C and Lisp backtraces from the infloop.  Let me know if
> > you need help with GDB commands to do that.
> 
> Any hint to dump lisp from gdb would be appreciated (I've quickly
> skimmed through .gdbinit but I'm a bit lost).

Type "source /path/to/src/.gdbinit", and then use the "pp" command.
Like this:

  (gdb) pp list_var

where list_var is any C variable whose value is a list.

Thanks.

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

So, I stopped while stuck, so here's a sample of the stack:

#0  0x0000555555886011 in assq_no_quit (key=XIL(0xb490), list=XIL(0x5555562b35f3)) at fns.c:1626
#1  0x00005555556e0be8 in lface_from_face_name_no_resolve
    (f=0x555556065240, face_name=XIL(0xb490), signal_p=false) at xfaces.c:1846
#2  0x00005555556e0cbb in get_lface_attributes_no_remap
    (f=0x555556065240, face_name=XIL(0xb490), attrs=0x7fffffff62b0, signal_p=false)
    at xfaces.c:1888
#3  0x00005555556e0e1d in get_lface_attributes
    (w=0x5555576a24a0, f=0x555556065240, face_name=XIL(0xb490), attrs=0x7fffffff62b0, signal_p=false, named_merge_points=0x7fffffff6460) at xfaces.c:1939
#4  0x00005555556e18ac in face_inherited_attr
    (w=0x5555576a24a0, f=0x555556065240, attrs=0x7fffffff63c0, attr_idx=LFACE_EXTEND_INDEX, named_merge_points=0x7fffffff6460) at xfaces.c:2173
#5  0x00005555556e1b2e in merge_named_face
    (w=0x5555576a24a0, f=0x555556065240, face_name=XIL(0xf84c80), to=0x7fffffff67f0, named_merge_points=0x7fffffff6460, attr_filter=LFACE_EXTEND_INDEX) at xfaces.c:2223
#6  0x00005555556e2bf1 in merge_face_ref
    (w=0x5555576a24a0, f=0x555556065240, face_ref=XIL(0xf84c80), to=0x7fffffff67f0, err_msgs named_merge_points=0x0, attr_filter=LFACE_EXTEND_INDEX) at xfaces.c:2696
#7  0x00005555556ec15c in face_at_buffer_position
    (w=0x5555576a24a0, pos=138, endptr=0x7fffffff6968, limit=238, mouse=false, base_face_id=r_filter=LFACE_EXTEND_INDEX) at xfaces.c:6264
#8  0x00005555555d37f1 in face_at_pos (it=0x7fffffff7ff0, attr_filter=LFACE_EXTEND_INDEX)
    at xdisp.c:4173
#9  0x0000555555604e7a in extend_face_to_end_of_line (it=0x7fffffff7ff0) at xdisp.c:21588
#10 0x000055555560b9cf in display_line (it=0x7fffffff7ff0, cursor_vpos=0) at xdisp.c:23478
#11 0x00005555555fd519 in try_window (window=XIL(0x5555576a24a5), pos=..., flags=1)
    at xdisp.c:19005
#12 0x00005555555faa5d in redisplay_window (window=XIL(0x5555576a24a5), just_this_one_p=fals
    at xdisp.c:18426
#13 0x00005555555f37bf in redisplay_window_0 (window=XIL(0x5555576a24a5)) at xdisp.c:16147
#14 0x000055555587254f in internal_condition_case_1
    (bfun=0x5555555f377d <redisplay_window_0>, arg=XIL(0x5555576a24a5), handlers=XIL(0x7ffffb), hfun=0x5555555f3745 <redisplay_window_error>) at eval.c:1379
#15 0x00005555555f371b in redisplay_windows (window=XIL(0x5555576a24a5)) at xdisp.c:16127
#16 0x00005555555f36cd in redisplay_windows (window=XIL(0x5555576a2295)) at xdisp.c:16121
#17 0x00005555555f264a in redisplay_internal () at xdisp.c:15595
#18 0x00005555555f30cd in redisplay_preserve_echo_area (from_where=12) at xdisp.c:15948
#19 0x00005555558f4ecc in wait_reading_process_output
    (time_limit=30, nsecs=0, read_kbd=-1, do_display=true, wait_for_cell=XIL(0), wait_proc=0st_wait_proc=0) at process.c:5823
#20 0x00005555555a6806 in sit_for (timeout=make_fixnum(30), reading=true, display_option=1)
    at dispnew.c:6037
#21 0x000055555575958f in read_char
    (commandflag=1, map=XIL(0x5555579e7d23), prev_event=XIL(0), used_mouse_menu=0x7fffffffd8d_time=0x0) at keyboard.c:2733
#22 0x0000555555767cb8 in read_key_sequence
    (keybuf=0x7fffffffdae0, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9536
#23 0x0000555555755d77 in command_loop_1 () at keyboard.c:1345
#24 0x00005555558724a8 in internal_condition_case
    (bfun=0x55555575594d <command_loop_1>, handlers=XIL(0x90), hfun=0x5555557550e3 <cmd_erro
    at eval.c:1355
#25 0x0000555555755612 in command_loop_2 (ignore=XIL(0)) at keyboard.c:1091
#26 0x0000555555871d60 in internal_catch
    (tag=XIL(0xd530), func=0x5555557555e5 <command_loop_2>, arg=XIL(0)) at eval.c:1116
#27 0x00005555557555b0 in command_loop () at keyboard.c:1070
#28 0x0000555555754cb4 in recursive_edit_1 () at keyboard.c:714
#29 0x0000555555754e37 in Frecursive_edit () at keyboard.c:786
#30 0x000055555574d46c in main (argc=3, argv=0x7fffffffdfb8) at emacs.c:2054

The loop is stuck at #4, in face_inherited_attr.
As described above, attr_val is not changing:

(gdb) p attr_val
$16 = XIL(0xde90)
(gdb) pp attr_val
unspecified
(gdb) p attr_idx
$17 = LFACE_EXTEND_INDEX
(gdb) pp parent_face
'mu4e-header-value-face

Now, going up the stack

#0  get_lface_attributes (w=0x5555576c3fc0, f=0x55555617f4d0, face_name=XIL(0xb490),
    attrs=0x7fffffff62b0, signal_p=false, named_merge_points=0x7fffffff6460) at xfaces.c:1914
(gdb) pp face_name
quote

mmmh?

(face-all-attributes 'mu4e-header-value-face (window-frame))
=> ((:family . unspecified) (:foundry . unspecified) (:width . unspecified) (:height . unspecified) (:weight . unspecified) (:slant . unspecified) (:underline . unspecified) (:overline . unspecified) (:extend . unspecified) (:strike-through . unspecified) (:box . unspecified) (:inverse-video . unspecified) (:foreground . unspecified) (:background . unspecified) (:stipple . unspecified) (:inherit . message-header-other))
(face-all-attributes 'message-header-other (window-frame))
=> ((:family . unspecified) (:foundry . unspecified) (:width . unspecified) (:height . unspecified) (:weight . unspecified) (:slant . unspecified) (:underline . unspecified) (:overline . unspecified) (:extend . unspecified) (:strike-through . unspecified) (:box . unspecified) (:inverse-video . unspecified) (:foreground . unspecified) (:background . unspecified) (:stipple . unspecified) (:inherit . unspecified))

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

On December 10, 2019 12:57:24 PM GMT+02:00, Yuri D'Elia <wavexx@thregr.org> wrote:
> So, I stopped while stuck, so here's a sample of the stack:
> 
> #0  0x0000555555886011 in assq_no_quit (key=XIL(0xb490),
> list=XIL(0x5555562b35f3)) at fns.c:1626
> #1  0x00005555556e0be8 in lface_from_face_name_no_resolve
> (f=0x555556065240, face_name=XIL(0xb490), signal_p=false) at
> xfaces.c:1846
> #2  0x00005555556e0cbb in get_lface_attributes_no_remap
> (f=0x555556065240, face_name=XIL(0xb490), attrs=0x7fffffff62b0,
> signal_p=false)
>     at xfaces.c:1888
> #3  0x00005555556e0e1d in get_lface_attributes
> (w=0x5555576a24a0, f=0x555556065240, face_name=XIL(0xb490),
> attrs=0x7fffffff62b0, signal_p=false,
> named_merge_points=0x7fffffff6460) at xfaces.c:1939
> #4  0x00005555556e18ac in face_inherited_attr
> (w=0x5555576a24a0, f=0x555556065240, attrs=0x7fffffff63c0,
> attr_idx=LFACE_EXTEND_INDEX, named_merge_points=0x7fffffff6460) at
> xfaces.c:2173
> #5  0x00005555556e1b2e in merge_named_face
> (w=0x5555576a24a0, f=0x555556065240, face_name=XIL(0xf84c80),
> to=0x7fffffff67f0, named_merge_points=0x7fffffff6460,
> attr_filter=LFACE_EXTEND_INDEX) at xfaces.c:2223
> #6  0x00005555556e2bf1 in merge_face_ref
> (w=0x5555576a24a0, f=0x555556065240, face_ref=XIL(0xf84c80),
> to=0x7fffffff67f0, err_msgs named_merge_points=0x0,
> attr_filter=LFACE_EXTEND_INDEX) at xfaces.c:2696

> The loop is stuck at #4, in face_inherited_attr.
> As described above, attr_val is not changing:
> 
> (gdb) p attr_val
> $16 = XIL(0xde90)
> (gdb) pp attr_val
> unspecified
> (gdb) p attr_idx
> $17 = LFACE_EXTEND_INDEX
> (gdb) pp parent_face
> 'mu4e-header-value-face
> 
> Now, going up the stack
> 
> #0  get_lface_attributes (w=0x5555576c3fc0, f=0x55555617f4d0,
> face_name=XIL(0xb490),
> attrs=0x7fffffff62b0, signal_p=false,
> named_merge_points=0x7fffffff6460) at xfaces.c:1914
> (gdb) pp face_name
> quote
> 
> mmmh?


Thanks.

Please show the face that is refetenced in frame #5 as face_name, and all its attributes.  AFAIU, this is the original face being merged here.

The 'quote' thing probably means some attribute is redundantly quoted, but maybe I'm missing something.

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

On Tue, Dec 10 2019, Eli Zaretskii wrote:
> Thanks.
>
> Please show the face that is refetenced in frame #5 as face_name, and
> all its attributes. AFAIU, this is the original face being merged
> here.
>
> The 'quote' thing probably means some attribute is redundantly quoted,
> but maybe I'm missing something.

That's indeed the problem.

#1  0x00005555556e1b2e in merge_named_face (w=0x5555577d4990, f=0x55555617e280,
    face_name=XIL(0x1154e20), to=0x7fffffff67f0, named_merge_points=0x7fffffff62d0,
    attr_filter=LFACE_EXTEND_INDEX) at xfaces.c:2223
(gdb) pp face_name
gnus-header-content

gnus-header-content inherits indeed a redundant quote:

((:family . unspecified) (:foundry . unspecified) (:width . unspecified)
 (:height . unspecified) (:weight . unspecified) (:slant . unspecified)
 (:underline . unspecified) (:overline . unspecified) (:extend . unspecified)
 (:strike-through . unspecified) (:box . unspecified) (:inverse-video . unspecified)
 (:foreground . unspecified) (:background . unspecified) (:stipple . unspecified)
 (:inherit quote mu4e-header-value-face))

I've found where I had this incorrectly set. After removing the quote
the loop properly terminates.

So it looks like a good fix would be first to ensure a failure in the
inner for loop in get_lface_attributes properly terminates. And also to
check for an explicit face symbol in inherit?

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Cc: emacs-devel@gnu.org
> Date: Wed, 11 Dec 2019 12:36:52 +0100
> 
> gnus-header-content inherits indeed a redundant quote:
> 
> ((:family . unspecified) (:foundry . unspecified) (:width . unspecified)
>  (:height . unspecified) (:weight . unspecified) (:slant . unspecified)
>  (:underline . unspecified) (:overline . unspecified) (:extend . unspecified)
>  (:strike-through . unspecified) (:box . unspecified) (:inverse-video . unspecified)
>  (:foreground . unspecified) (:background . unspecified) (:stipple . unspecified)
>  (:inherit quote mu4e-header-value-face))
> 
> I've found where I had this incorrectly set. After removing the quote
> the loop properly terminates.
> 
> So it looks like a good fix would be first to ensure a failure in the
> inner for loop in get_lface_attributes properly terminates. And also to
> check for an explicit face symbol in inherit?

OK, so it's indeed a cockpit error.  I installed the change you
proposed to terminate the outer loop in face_inherited_attr in such
cases.

As for the other part, I'm not sure what you are proposing -- what do
you mean by "check for an explicit face symbol in inherit"?

Thanks.

Re: master 5ee43ba0df causing display hangs?

[Yuri D'Elia]

On Wed, Dec 11 2019, Eli Zaretskii wrote:
> As for the other part, I'm not sure what you are proposing -- what do
> you mean by "check for an explicit face symbol in inherit"?

What I meant was: is it worth it to also check explicitly whether the
value of :inherit is a face using LFACEP (hopefully I got the right
macro name) directly in merge_named_face? Or is it simply redundant?

Re: master 5ee43ba0df causing display hangs?

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Cc: emacs-devel@gnu.org
> Date: Wed, 11 Dec 2019 18:29:54 +0100
> 
> What I meant was: is it worth it to also check explicitly whether the
> value of :inherit is a face using LFACEP (hopefully I got the right
> macro name) directly in merge_named_face? Or is it simply redundant?

It's not redundant, but it takes CPU cycles, and this code is called
in the bowels of the display engine, so it needs to be as fast as
possible.

Moreover, the value of :inherit can legitimately be a list of faces,
not just a single face.  So the test is somewhat complicated, and I'd
rather not do it.

In general, we just ignore invalid face specs in all this code, so I
think it's okay to do that here as well.  Once we are protected from
looping on such mistaken specs, we should be OK, I think.

(Btw, the problem in this case couldn't have been detected in
merge_named_face, because the offending symbol, gnus-header-content,
was indeed a face; it's its :inherit attribute that was faulty.  So
we'd need to try detecting that in face_inherited_attr itself, and
then deal with the fact that the value can be a list.)