bug#10164: 24.0.91; Instant crash enabling linum-mode

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Tim Crews]

[-- Attachment #1: Type: text/html, Size: 4447 bytes --]

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> Date: Tue, 29 Nov 2011 19:05:20 -0700
> From: Tim Crews <tim.crews@code-affinity.com>
> 
>     Start Emacs with runemacs -Q --no-init-file
>     C-x C-f foo.txt
>     M-x linum-mode
>     (Emacs doesn't crash yet)
>     Type anything.  Emacs instantly crashes.

Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
crashes here:

	      xassert (!row->enabled_p
		       || row->mode_line_p
		       || verify_row_hash (row));

Evidently, it calls verify_row_hash before it tests row->mode_line_p.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 29 Nov 2011 19:05:20 -0700
>> From: Tim Crews <tim.crews@code-affinity.com>
>> 
>>     Start Emacs with runemacs -Q --no-init-file
>>     C-x C-f foo.txt
>>     M-x linum-mode
>>     (Emacs doesn't crash yet)
>>     Type anything.  Emacs instantly crashes.
>
> Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
> crashes here:
>
> 	      xassert (!row->enabled_p
> 		       || row->mode_line_p
> 		       || verify_row_hash (row));
>
> Evidently, it calls verify_row_hash before it tests row->mode_line_p.

Are you sure?  Without interprocedural analysis the compiler cannot know
that `verify_row_hash' does not alter row->enabled_p, so it cannot
change the evaluation order.
BTW, the argument for row_hash should be const.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: Tim Crews <tim.crews@code-affinity.com>,  10164@debbugs.gnu.org
> Date: Wed, 30 Nov 2011 07:52:40 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Tue, 29 Nov 2011 19:05:20 -0700
> >> From: Tim Crews <tim.crews@code-affinity.com>
> >> 
> >>     Start Emacs with runemacs -Q --no-init-file
> >>     C-x C-f foo.txt
> >>     M-x linum-mode
> >>     (Emacs doesn't crash yet)
> >>     Type anything.  Emacs instantly crashes.
> >
> > Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
> > crashes here:
> >
> > 	      xassert (!row->enabled_p
> > 		       || row->mode_line_p
> > 		       || verify_row_hash (row));
> >
> > Evidently, it calls verify_row_hash before it tests row->mode_line_p.
> 
> Are you sure?

How else can I explain this display from GDB:

  (gdb) prow
  y=0 x=0 pwid=673 a+d=12+4=16 phys=12+4=16 vis=16
  used=(LMargin=0,Text=84,RMargin=0) Hash=263825844
  start=0 end=0 ENA MODEL

?  MODEL says that this row has its mode_line_p flag set.

Just to be sure, I ran the recipe again under GDB, setting a
breakpoint inside verify_row_hash thusly:

  (gdb) break verify_row_hash if row->mode_line_p != 0

and sure enough, it breaks as soon as I turn on linum-mode, with ROW
that shows the glyphs in the mode line.

> Without interprocedural analysis the compiler cannot know
> that `verify_row_hash' does not alter row->enabled_p, so it cannot
> change the evaluation order.

I don't know.  Maybe GCC does perform such an analysis.  Or maybe it
decides that the result of this comparison in verify_row_hash:

  row->hash == row_hash (row)

will not change even if row_hash does modify its argument ROW.  Or
maybe it's a bug in GCC.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: Tim Crews <tim.crews@code-affinity.com>,  10164@debbugs.gnu.org
>> Date: Wed, 30 Nov 2011 07:52:40 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> Date: Tue, 29 Nov 2011 19:05:20 -0700
>> >> From: Tim Crews <tim.crews@code-affinity.com>
>> >> 
>> >>     Start Emacs with runemacs -Q --no-init-file
>> >>     C-x C-f foo.txt
>> >>     M-x linum-mode
>> >>     (Emacs doesn't crash yet)
>> >>     Type anything.  Emacs instantly crashes.
>> >
>> > Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
>> > crashes here:
>> >
>> > 	      xassert (!row->enabled_p
>> > 		       || row->mode_line_p
>> > 		       || verify_row_hash (row));
>> >
>> > Evidently, it calls verify_row_hash before it tests row->mode_line_p.
>> 
>> Are you sure?
>
> How else can I explain this display from GDB:

Compiler bug?


>   (gdb) prow
>   y=0 x=0 pwid=673 a+d=12+4=16 phys=12+4=16 vis=16
>   used=(LMargin=0,Text=84,RMargin=0) Hash=263825844
>   start=0 end=0 ENA MODEL
>
> ?  MODEL says that this row has its mode_line_p flag set.
>
> Just to be sure, I ran the recipe again under GDB, setting a
> breakpoint inside verify_row_hash thusly:
>
>   (gdb) break verify_row_hash if row->mode_line_p != 0
>
> and sure enough, it breaks as soon as I turn on linum-mode, with ROW
> that shows the glyphs in the mode line.
>
>> Without interprocedural analysis the compiler cannot know
>> that `verify_row_hash' does not alter row->enabled_p, so it cannot
>> change the evaluation order.
>
> I don't know.  Maybe GCC does perform such an analysis.  Or maybe it

It can't by default, `row_hash' is in a different file, so it's not
available when compiling dispnew.c.

> decides that the result of this comparison in verify_row_hash:
>
>   row->hash == row_hash (row)
>
> will not change even if row_hash does modify its argument ROW.  Or
> maybe it's a bug in GCC.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Andy Moreton]

On Wed 30 Nov 2011, Dan Nicolaescu wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Date: Tue, 29 Nov 2011 19:05:20 -0700
>>> From: Tim Crews <tim.crews@code-affinity.com>
>>> 
>>>     Start Emacs with runemacs -Q --no-init-file
>>>     C-x C-f foo.txt
>>>     M-x linum-mode
>>>     (Emacs doesn't crash yet)
>>>     Type anything.  Emacs instantly crashes.
>>
>> Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
>> crashes here:
>>
>> 	      xassert (!row->enabled_p
>> 		       || row->mode_line_p
>> 		       || verify_row_hash (row));
>>
>> Evidently, it calls verify_row_hash before it tests row->mode_line_p.
>
> Are you sure?  Without interprocedural analysis the compiler cannot know
> that `verify_row_hash' does not alter row->enabled_p, so it cannot
> change the evaluation order.
> BTW, the argument for row_hash should be const.

The emacs build in this bug report is configured with
system-configuration-options is a variable defined in `C source code'.
Its value is "--with-gcc (4.6) --no-opt --cflags
-ID:/devel/emacs/libs/libXpm-3.5.8/include
-ID:/devel/emacs/libs/libXpm-3.5.8/src
-ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
-ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
-ID:/devel/emacs/libs/giflib-4.1.4-1/include
-ID:/devel/emacs/libs/jpeg-6b-4/include
-ID:/devel/emacs/libs/tiff-3.8.2-1/include
-ID:/devel/emacs/libs/gnutls-2.10.1/include --ldflags
-LD:/devel/emacs/libs/gnutls-2.10.1/lib"

This does not include "-fno-omit-frame-pointer", which is known to
important for MinGW GCC 4.6. Perhaps that is the real issue here ?

    AndyM

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Juanma Barranquero]

On Wed, Nov 30, 2011 at 16:01, Andy Moreton <andrewjmoreton@gmail.com> wrote:

> The emacs build in this bug report is configured with
[...]
> This does not include "-fno-omit-frame-pointer", which is known to
> important for MinGW GCC 4.6. Perhaps that is the real issue here ?

I see it with optimized and non-optimized builds (and the optimized
build has -fno-omit-frame-pointer).

    Juanma

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> Arrgh!  This is GCC 4.6.x "as-is" code reordering in action.  Emacs
> crashes here:
> 
> 	      xassert (!row->enabled_p
>     	               || row->mode_line_p
> 	     	       || verify_row_hash (row));
> 
> Evidently, it calls verify_row_hash before it tests row->mode_line_p

Fixed in revision 106555 on the trunk.

Chong, I hope this is in time for the next pretest.  It would be a
pity to release a pretest with such a glaring problem.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Juanma Barranquero]

On Wed, Nov 30, 2011 at 13:29, Eli Zaretskii <eliz@gnu.org> wrote:

> Chong, I hope this is in time for the next pretest.  It would be a
> pity to release a pretest with such a glaring problem.

But verify_row_hash does nothing except checking and returning a flag,
so the new code will never cause an assertion failure...

    Juanma


=== modified file 'src/dispnew.c'
--- src/dispnew.c       2011-11-28 01:07:01 +0000
+++ src/dispnew.c       2011-11-30 12:25:09 +0000
@@ -608,9 +608,10 @@
                  row->glyphs[LAST_AREA]
                    = row->glyphs[LEFT_MARGIN_AREA] + dim.width;
                }
-             xassert (!row->enabled_p
-                      || row->mode_line_p
-                      || verify_row_hash (row));
+#if XASSERTS
+             if (row->enabled_p && !row->mode_line_p)
+               verify_row_hash (row));
+#endif
              ++row;

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 30 Nov 2011 13:38:17 +0100
> Cc: Tim Crews <tim.crews@code-affinity.com>, Chong Yidong <cyd@gnu.org>, 10164@debbugs.gnu.org
> 
> But verify_row_hash does nothing except checking and returning a flag,
> so the new code will never cause an assertion failure...

*Blush*

Thanks for fixing, I somehow managed to delete the xassert itself..

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 30 Nov 2011 15:32:25 +0100
> 
> On Wed, Nov 30, 2011 at 14:57, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > You mean, you still see the crash after the change??
> 
> Yes.

I see it myself.

Chong, please hold off the pretest while I debug this.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> On Wed, Nov 30, 2011 at 14:57, Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> > You mean, you still see the crash after the change??
>>
>> Yes.
>
> I see it myself.
>
> Chong, please hold off the pretest while I debug this.

I bumped the version number in the trunk before seeing this latest
message.  But, I haven't made the pretest tag yet---will wait.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Chong Yidong]

Chong Yidong <cyd@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> On Wed, Nov 30, 2011 at 14:57, Eli Zaretskii <eliz@gnu.org> wrote:
>>>
>>> > You mean, you still see the crash after the change??
>>>
>>> Yes.
>>
>> I see it myself.
>>
>> Chong, please hold off the pretest while I debug this.
>
> I bumped the version number in the trunk before seeing this latest
> message.  But, I haven't made the pretest tag yet---will wait.

And FWIW, this bug seems to be Windows-only, since I don't see any crash
on GNU/Linux.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Juanma Barranquero]

> And FWIW, this bug seems to be Windows-only, since I don't see any crash
> on GNU/Linux.

How do you test it? I get the crash with

  emacs -Q
  M-x linum-mode <RET>

but not with

  emacs -Q -f linum-mode

    Juanma

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

>> And FWIW, this bug seems to be Windows-only, since I don't see any crash
>> on GNU/Linux.
>
> How do you test it? I get the crash with
>
>   emacs -Q
>   M-x linum-mode <RET>
>
> but not with
>
>   emacs -Q -f linum-mode

Disregard that---I had accidentally recompiled without asserts enabled.
I see the abort now.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: Juanma Barranquero <lekktu@gmail.com>,  10164@debbugs.gnu.org,  tim.crews@code-affinity.com
> Date: Thu, 01 Dec 2011 00:29:37 +0800
> 
> > Chong, please hold off the pretest while I debug this.
> 
> I bumped the version number in the trunk before seeing this latest
> message.  But, I haven't made the pretest tag yet---will wait.

Thanks.  The bug is fixed in revision 106561.

It was simply wrong to try to verify rows' hash values at that spot,
because the code immediately before that reallocates the glyph arrays,
so the contents of a glyph row could be complete garbage.  Therefore,
I removed the assertion.

I'm amazed that this assertion didn't trigger until now.  I guess too
few people compile with asserts.  Or maybe most calls to
adjust_glyph_matrix don't really reallocate, since display margins are
a relatively rarely used feature.

Sorry about this, it was a shameful thinko on my part to introduce
that assertion in the first place.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  The bug is fixed in revision 106561.
>
> It was simply wrong to try to verify rows' hash values at that spot,
> because the code immediately before that reallocates the glyph arrays,
> so the contents of a glyph row could be complete garbage.  Therefore,
> I removed the assertion.
>
> I'm amazed that this assertion didn't trigger until now.  I guess too
> few people compile with asserts.  Or maybe most calls to
> adjust_glyph_matrix don't really reallocate, since display margins are
> a relatively rarely used feature.
>
> Sorry about this, it was a shameful thinko on my part to introduce
> that assertion in the first place.

No worries, and thanks for looking into the problem on such short
notice.  I'll make the pretest shortly.

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm amazed that this assertion didn't trigger until now.  I guess too
> few people compile with asserts.

You might want to add another option:
configure --enable-checking=xasserts 

to enable asserts, it's easier to enable checks when one does not have
to edit files. 

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: eliz@gnu.org
> Date: Wed, 30 Nov 2011 13:10:47 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm amazed that this assertion didn't trigger until now.  I guess too
> > few people compile with asserts.
> 
> You might want to add another option:
> configure --enable-checking=xasserts 
> 
> to enable asserts, it's easier to enable checks when one does not have
> to edit files. 

I thought that's what "configure --enable-asserts" did.  Doesn't it?
Or is that option inconvenient in some way?

bug#10164: 24.0.91; Instant crash enabling linum-mode

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: eliz@gnu.org
>> Date: Wed, 30 Nov 2011 13:10:47 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'm amazed that this assertion didn't trigger until now.  I guess too
>> > few people compile with asserts.
>> 
>> You might want to add another option:
>> configure --enable-checking=xasserts 
>> 
>> to enable asserts, it's easier to enable checks when one does not have
>> to edit files. 
>
> I thought that's what "configure --enable-asserts" did.  Doesn't it?
> Or is that option inconvenient in some way?

I just didn't know about it...

bug#10164: closed (Instant crash enabling linum-mode)

[Tim Crews]

[-- Attachment #1: Type: text/html, Size: 794 bytes --]

bug#10164: closed (Instant crash enabling linum-mode)

[Eli Zaretskii]

> Date: Wed, 30 Nov 2011 10:29:39 -0700
> From: Tim Crews <tim.crews@code-affinity.com>
> 
> > I'm amazed that this assertion didn't trigger until now.  I guess too
> > few people compile with asserts.  Or maybe most calls to
> > adjust_glyph_matrix don't really reallocate, since display margins are
> > a relatively rarely used feature.
> 
> I'm confused about that part.  I discovered the bug using the binary
> build that came from  http://alpha.gnu.org/gnu/emacs/windows/.
> Isn't this where most people would get their copy of Emacs 24?

I don't know.  Quite a few people who track Emacs development build
Emacs themselves.  I do, for example.

> Wasn't it compiled with asserts?

Yes, it was.  So is my Emacs.  I just don't use linum-mode.  The
moment I tried it, it crashed for me as well.