unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#7587: 23.2; `format-mode-line' makes Emacs crash
@ 2010-12-07 21:01 Michael Heerdegen
  2010-12-10 16:02 ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2010-12-07 21:01 UTC (permalink / raw)
  To: 7587

Hello,

if I evaluate the following sexp in a fresh emacs -Q (e.g. with
`eval-expression'):

(progn
  (face-remap-add-relative 'mode-line :height 1.0)
  (format-mode-line mode-line-format t))

Emacs instantly crashes.



In GNU Emacs 23.2.1 (i486-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2010-08-14 on raven, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.10707000
configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=lucid' '--with-toolkit-scroll-bars' '--without-gconf' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t






^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-07 21:01 bug#7587: 23.2; `format-mode-line' makes Emacs crash Michael Heerdegen
@ 2010-12-10 16:02 ` Eli Zaretskii
  2010-12-10 16:39   ` Eli Zaretskii
  2010-12-10 20:50   ` Stefan Monnier
  0 siblings, 2 replies; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-10 16:02 UTC (permalink / raw)
  To: michael_heerdegen; +Cc: 7587

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Tue, 07 Dec 2010 22:01:25 +0100
> Cc: 
> 
> if I evaluate the following sexp in a fresh emacs -Q (e.g. with
> `eval-expression'):
> 
> (progn
>   (face-remap-add-relative 'mode-line :height 1.0)
>   (format-mode-line mode-line-format t))
> 
> Emacs instantly crashes.

This happens because, for some reason I don't understand, when
face-remapping-alist is non-nil, lookup_basic_face insists on getting
only one of the basic faces, and otherwise aborts:

  if (NILP (Vface_remapping_alist))
    return face_id;		/* Nothing to do.  */

  switch (face_id)
    {
    case DEFAULT_FACE_ID:		name = Qdefault;		break;
    case MODE_LINE_FACE_ID:		name = Qmode_line;		break;
    case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
    case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
    case FRINGE_FACE_ID:		name = Qfringe;			break;
    case SCROLL_BAR_FACE_ID:		name = Qscroll_bar;		break;
    case BORDER_FACE_ID:		name = Qborder;			break;
    case CURSOR_FACE_ID:		name = Qcursor;			break;
    case MOUSE_FACE_ID:			name = Qmouse;			break;
    case MENU_FACE_ID:			name = Qmenu;			break;

    default:
      abort ();	    /* the caller is supposed to pass us a basic face id */
    }

This is inconsistent, because if face-remapping-alist _is_ nil, the
function returns face_id unaltered, no matter if it is or isn't a
basic face.

Any objections to the following patch?  On the release branch as well?

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2010-08-06 10:12:41 +0000
+++ src/xfaces.c	2010-12-10 15:58:25 +0000
@@ -4673,9 +4673,6 @@ lookup_basic_face (struct frame *f, int 
     case CURSOR_FACE_ID:		name = Qcursor;			break;
     case MOUSE_FACE_ID:			name = Qmouse;			break;
     case MENU_FACE_ID:			name = Qmenu;			break;
-
-    default:
-      abort ();	    /* the caller is supposed to pass us a basic face id */
     }
 
   /* Do a quick scan through Vface_remapping_alist, and return immediately






^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-10 16:02 ` Eli Zaretskii
@ 2010-12-10 16:39   ` Eli Zaretskii
  2010-12-10 20:50   ` Stefan Monnier
  1 sibling, 0 replies; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-10 16:39 UTC (permalink / raw)
  To: michael_heerdegen, 7587

> Date: Fri, 10 Dec 2010 18:02:21 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 7587@debbugs.gnu.org
> 
> === modified file 'src/xfaces.c'
> --- src/xfaces.c	2010-08-06 10:12:41 +0000
> +++ src/xfaces.c	2010-12-10 15:58:25 +0000
> @@ -4673,9 +4673,6 @@ lookup_basic_face (struct frame *f, int 
>      case CURSOR_FACE_ID:		name = Qcursor;			break;
>      case MOUSE_FACE_ID:			name = Qmouse;			break;
>      case MENU_FACE_ID:			name = Qmenu;			break;
> -
> -    default:
> -      abort ();	    /* the caller is supposed to pass us a basic face id */
>      }
>  
>    /* Do a quick scan through Vface_remapping_alist, and return immediately

Sorry, wrong patch.  I meant this one:

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2010-08-06 10:12:41 +0000
+++ src/xfaces.c	2010-12-10 16:36:37 +0000
@@ -4675,7 +4675,7 @@ lookup_basic_face (struct frame *f, int 
     case MENU_FACE_ID:			name = Qmenu;			break;
 
     default:
-      abort ();	    /* the caller is supposed to pass us a basic face id */
+      return face_id;		/* Nothing to do.  */
     }
 
   /* Do a quick scan through Vface_remapping_alist, and return immediately






^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-10 16:02 ` Eli Zaretskii
  2010-12-10 16:39   ` Eli Zaretskii
@ 2010-12-10 20:50   ` Stefan Monnier
  2010-12-10 22:40     ` Eli Zaretskii
  1 sibling, 1 reply; 13+ messages in thread
From: Stefan Monnier @ 2010-12-10 20:50 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: michael_heerdegen, 7587

> This happens because, for some reason I don't understand, when
> face-remapping-alist is non-nil, lookup_basic_face insists on getting
> only one of the basic faces, and otherwise aborts:

I don't know anything about this code, but the name `lookup_basic_face'
suggests it only works for basic faces, so maybe the problem is that the
function gets incorrectly called for a non-basic face.


        Stefan





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-10 20:50   ` Stefan Monnier
@ 2010-12-10 22:40     ` Eli Zaretskii
  2010-12-16  2:17       ` Chong Yidong
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-10 22:40 UTC (permalink / raw)
  To: Stefan Monnier, Miles Bader; +Cc: michael_heerdegen, 7587

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  7587@debbugs.gnu.org
> Date: Fri, 10 Dec 2010 15:50:31 -0500
> 
> > This happens because, for some reason I don't understand, when
> > face-remapping-alist is non-nil, lookup_basic_face insists on getting
> > only one of the basic faces, and otherwise aborts:
> 
> I don't know anything about this code

Who does?  Miles, do you?

> but the name `lookup_basic_face' suggests it only works for basic
> faces

Actually, it works for non-basic faces as well--but only if
face-remapping-alist is nil.  See this part of it, which I cited:

  if (NILP (Vface_remapping_alist))
    return face_id;		/* Nothing to do.  */

The patch I suggest simply make lookup_basic_face behave consistently
regardless of the value of face-remapping-alist.

> so maybe the problem is that the function gets incorrectly called
> for a non-basic face.

It _does_ get called with a basic face (`mode-line'), but that face
has been remapped.  Remapping actually creates a new face ID in the
face cache, but lookup_basic_face gets an ID (an integer), not a
symbol Qmode_line etc.  So if the face ID that lookup_basic_face gets
is not one of the IDs reserved for the 12 basic faces, _and_
face-remapping-alist is non-nil, lookup_basic_face always pukes.

Here's more context about the chain of calls that leads to the crash:

The call to lookup_basic_face comes from init_iterator:

  /* Perhaps remap BASE_FACE_ID to a user-specified alternative.  */
  if (! NILP (Vface_remapping_alist))
    remapped_base_face_id = lookup_basic_face (XFRAME (w->frame), base_face_id);

init_iterator does this because it expects a truly basic face in
base_face_id, and it wants to use its face-remapped variety, as the
user would expect.  But in this case, we pass to it an already
remapped basic face, see this part of format-mode-line:

  if (!NILP (face))
    {
      if (EQ (face, Qt))
	face = (EQ (window, selected_window) ? Qmode_line : Qmode_line_inactive);
      face_id = lookup_named_face (XFRAME (WINDOW_FRAME (w)), face, 0);
    }
  ...
  init_iterator (&it, w, -1, -1, NULL, face_id);

lookup_named_face returns the face ID of the remapped mode-line face.
So in this case, face_id is already an ID of the remapped mode-line
face, and returning it unaltered from lookup_basic_face is TRT.

As an alternative, we could refrain from calling lookup_basic_face in
init_iterator if the argument base_face_id is not one of the 12 basic
face IDs.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-10 22:40     ` Eli Zaretskii
@ 2010-12-16  2:17       ` Chong Yidong
  2010-12-16  4:39         ` Drew Adams
  2010-12-16  5:35         ` Eli Zaretskii
  0 siblings, 2 replies; 13+ messages in thread
From: Chong Yidong @ 2010-12-16  2:17 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: michael_heerdegen, 7587, Miles Bader

Eli Zaretskii <eliz@gnu.org> writes:

> The call to lookup_basic_face comes from init_iterator:
> init_iterator does this because it expects a truly basic face in
> base_face_id, and it wants to use its face-remapped variety, as the
> user would expect.  But in this case, we pass to it an already
> remapped basic face, see this part of format-mode-line:

> lookup_named_face returns the face ID of the remapped mode-line face.
> So in this case, face_id is already an ID of the remapped mode-line
> face, and returning it unaltered from lookup_basic_face is TRT.

format-mode-line should not pass non-basic faces to init_iterator.  I'm
not even sure what the FACE argument to format-mode-line is useful for;
it doesn't seem to be used anywhere in the sources.

I've changed format-mode-line so that if you pass a face argument to
FACES, it should be one of `default', `mode-line', etc.  Any other face
gets treated as `default'.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-16  2:17       ` Chong Yidong
@ 2010-12-16  4:39         ` Drew Adams
  2010-12-16  5:37           ` Eli Zaretskii
  2010-12-16  5:35         ` Eli Zaretskii
  1 sibling, 1 reply; 13+ messages in thread
From: Drew Adams @ 2010-12-16  4:39 UTC (permalink / raw)
  To: 'Chong Yidong', 'Eli Zaretskii'
  Cc: michael_heerdegen, 7587, 'Miles Bader'

> I've changed format-mode-line so that if you pass a face argument to
> FACES, it should be one of `default', `mode-line', etc.  Any 
> other face gets treated as `default'.

I guess that restriction will be documented in the doc string, then? Thx.






^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-16  2:17       ` Chong Yidong
  2010-12-16  4:39         ` Drew Adams
@ 2010-12-16  5:35         ` Eli Zaretskii
  2010-12-18  8:56           ` Eli Zaretskii
  1 sibling, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-16  5:35 UTC (permalink / raw)
  To: Chong Yidong; +Cc: michael_heerdegen, 7587

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Miles Bader <miles@gnu.org>,
>         michael_heerdegen@web.de, 7587@debbugs.gnu.org
> Date: Thu, 16 Dec 2010 10:17:51 +0800
> 
> format-mode-line should not pass non-basic faces to init_iterator.

Where do you see this condition enforced or assumed in the code?  I
see only the commentary before init_iterator, but I'm not sure which
code (except lookup_basic_face) would really puke or DTWT with a
non-basic face.  And support for face remapping already means that you
can sneak a non-basic face in, anyway.

> I'm not even sure what the FACE argument to format-mode-line is
> useful for

The doc string and the ELisp manual clearly answer that question:
format-mode-line can be used to compute the formatted string without
displaying it.  When you use this API that way, you can specify a face
for the parts of mode line that don't have their faces specified in
the mode-line format.  IOW, it sounds like a deliberate feature, given
the description in the ELisp manual.

> it doesn't seem to be used anywhere in the sources.

?? init_iterator uses it (well, its index in the face cache) to
initialize the appropriate member of the iterator object.

> I've changed format-mode-line so that if you pass a face argument to
> FACES, it should be one of `default', `mode-line', etc.  Any other face
> gets treated as `default'.

Given some history of discussions here, and the fact that no one seems
to be sure how this code should really work and why, I wonder why you
went ahead with the change without discussing it first.  I'm afraid
that this change breaks backward compatibility for no good reason.

Anyway, this being an incompatible change, it should be in NEWS.  In
addition, the ELisp manual should be modified accordingly.

Also, there seems to be a bug in the change itself, in this line:

  +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-16  4:39         ` Drew Adams
@ 2010-12-16  5:37           ` Eli Zaretskii
  0 siblings, 0 replies; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-16  5:37 UTC (permalink / raw)
  To: Drew Adams; +Cc: michael_heerdegen, 7587, cyd, miles

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <michael_heerdegen@web.de>, <7587@debbugs.gnu.org>,
>         "'Miles Bader'" <miles@gnu.org>
> Date: Wed, 15 Dec 2010 20:39:01 -0800
> 
> > I've changed format-mode-line so that if you pass a face argument to
> > FACES, it should be one of `default', `mode-line', etc.  Any 
> > other face gets treated as `default'.
> 
> I guess that restriction will be documented in the doc string, then? Thx.

That's part of the change committed by Chong, yes.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-16  5:35         ` Eli Zaretskii
@ 2010-12-18  8:56           ` Eli Zaretskii
  2010-12-20 15:29             ` Chong Yidong
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-18  8:56 UTC (permalink / raw)
  To: cyd, michael_heerdegen, 7587

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 16 Dec 2010 00:35:51 -0500
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> 
> Anyway, this being an incompatible change, it should be in NEWS.  In
> addition, the ELisp manual should be modified accordingly.
> 
> Also, there seems to be a bug in the change itself, in this line:
> 
>   +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID

I fixed these now.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-18  8:56           ` Eli Zaretskii
@ 2010-12-20 15:29             ` Chong Yidong
  2010-12-20 18:33               ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Chong Yidong @ 2010-12-20 15:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: michael_heerdegen, 7587

Eli Zaretskii <eliz@gnu.org> writes:

>>   +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID
>
> I fixed these now.

Thanks.

> > I'm not even sure what the FACE argument to format-mode-line is
> > useful for
>
> The doc string and the ELisp manual clearly answer that question:
> format-mode-line can be used to compute the formatted string without
> displaying it.  When you use this API that way, you can specify a face
> for the parts of mode line that don't have their faces specified in
> the mode-line format.  IOW, it sounds like a deliberate feature, given
> the description in the ELisp manual.
>
> > it doesn't seem to be used anywhere in the sources.
>
> ?? init_iterator uses it (well, its index in the face cache) to
> initialize the appropriate member of the iterator object.

I mean, format-mode-line with a non-nil FACE argument doesn't seem to be
used anywhere.

> Given some history of discussions here, and the fact that no one seems
> to be sure how this code should really work and why, I wonder why you
> went ahead with the change without discussing it first.  I'm afraid
> that this change breaks backward compatibility for no good reason.

Given that the backward compatible behavior is to segfault, this
breakage might be acceptable.

Less flippantly, the FACE argument, if non-nil, is still applied as a
text property to the returned string.  So I don't think there's going to
be any noticeable difference (again, we don't have any concrete use
cases, so it's difficult to say).  The docstring change might not even
be necessary.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-20 15:29             ` Chong Yidong
@ 2010-12-20 18:33               ` Eli Zaretskii
  2010-12-25 10:18                 ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-20 18:33 UTC (permalink / raw)
  To: Chong Yidong; +Cc: michael_heerdegen, 7587

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> Date: Mon, 20 Dec 2010 23:29:48 +0800
> 
> I mean, format-mode-line with a non-nil FACE argument doesn't seem to be
> used anywhere.

Not in Emacs sources, no.  But it's a feature that some application
could plausibly want.

> > Given some history of discussions here, and the fact that no one seems
> > to be sure how this code should really work and why, I wonder why you
> > went ahead with the change without discussing it first.  I'm afraid
> > that this change breaks backward compatibility for no good reason.
> 
> Given that the backward compatible behavior is to segfault, this
> breakage might be acceptable.

It only crashed if some faces were remapped, otherwise it would "just
work".  (And it isn't a segfault, it's a deliberate call to `abort'.)

> Less flippantly, the FACE argument, if non-nil, is still applied as a
> text property to the returned string.

Which makes me wonder why we need to pass to init_iterator anything
but DEFAULT_FACE_ID...

> So I don't think there's going to be any noticeable difference
> (again, we don't have any concrete use cases, so it's difficult to
> say).  The docstring change might not even be necessary.

If FACE is still applied, then the doc string change (and part of what
I wrote in NEWS and the ELisp manual) should indeed be reverted.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#7587: 23.2; `format-mode-line' makes Emacs crash
  2010-12-20 18:33               ` Eli Zaretskii
@ 2010-12-25 10:18                 ` Eli Zaretskii
  0 siblings, 0 replies; 13+ messages in thread
From: Eli Zaretskii @ 2010-12-25 10:18 UTC (permalink / raw)
  To: cyd; +Cc: michael_heerdegen, 7587

> Date: Mon, 20 Dec 2010 20:33:22 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> 
> > Less flippantly, the FACE argument, if non-nil, is still applied as a
> > text property to the returned string.
> 
> Which makes me wonder why we need to pass to init_iterator anything
> but DEFAULT_FACE_ID...
> 
> > So I don't think there's going to be any noticeable difference
> > (again, we don't have any concrete use cases, so it's difficult to
> > say).  The docstring change might not even be necessary.
> 
> If FACE is still applied, then the doc string change (and part of what
> I wrote in NEWS and the ELisp manual) should indeed be reverted.

Done.





^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2010-12-25 10:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-07 21:01 bug#7587: 23.2; `format-mode-line' makes Emacs crash Michael Heerdegen
2010-12-10 16:02 ` Eli Zaretskii
2010-12-10 16:39   ` Eli Zaretskii
2010-12-10 20:50   ` Stefan Monnier
2010-12-10 22:40     ` Eli Zaretskii
2010-12-16  2:17       ` Chong Yidong
2010-12-16  4:39         ` Drew Adams
2010-12-16  5:37           ` Eli Zaretskii
2010-12-16  5:35         ` Eli Zaretskii
2010-12-18  8:56           ` Eli Zaretskii
2010-12-20 15:29             ` Chong Yidong
2010-12-20 18:33               ` Eli Zaretskii
2010-12-25 10:18                 ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).