bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Dani Moncayo]

Recipe from emacs -Q:  a <SPC> b M-a M-% a <SPC> b <RET> c <RET> <SPC>

After doing that, my Emacs session suddenly ends.


In GNU Emacs 24.2.50.1 (i386-mingw-nt6.1.7601)
 of 2012-09-12 on DANI-PC
Bzr revision: 109993 rudalics@gmx.at-20120912154917-kih4q2dfeeyuyf5k
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -I../../libs/libxpm-3.5.8/include -I../../libs/libxpm-3.5.8/src
 -I../../libs/libpng-1.4.10 -I../../libs/zlib-1.2.6
 -I../../libs/giflib-4.1.4-1/include -I../../libs/jpeg-6b-4/include
 -I../../libs/tiff-3.8.2-1/include
 -I../../libs/libxml2-2.7.8-w32-bin/include/libxml2
 -I../../libs/gnutls-3.0.16/include
 -I../../libs/libiconv-1.14-2-mingw32-dev/include'

Important settings:
  value of $LANG: ESN
  locale-coding-system: cp1252
  default enable-multibyte-characters: t


-- 
Dani Moncayo

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Eli Zaretskii]

> Date: Wed, 12 Sep 2012 20:06:03 +0200
> From: Dani Moncayo <dmoncayo@gmail.com>
> 
> Recipe from emacs -Q:  a <SPC> b M-a M-% a <SPC> b <RET> c <RET> <SPC>
> 
> After doing that, my Emacs session suddenly ends.

It exits due to an assertion violation.  Looks like the recent changes
that introduced fatal_error_backtrace are a misfeature on Windows.

I'm looking at the reason for the assertion violation; the
fatal_error_backtrace thingy is next.

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Eli Zaretskii]

> Date: Wed, 12 Sep 2012 21:39:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 12426@debbugs.gnu.org
> 
> > Date: Wed, 12 Sep 2012 20:06:03 +0200
> > From: Dani Moncayo <dmoncayo@gmail.com>
> > 
> > Recipe from emacs -Q:  a <SPC> b M-a M-% a <SPC> b <RET> c <RET> <SPC>
> > 
> > After doing that, my Emacs session suddenly ends.
> 
> It exits due to an assertion violation.  Looks like the recent changes
> that introduced fatal_error_backtrace are a misfeature on Windows.
> 
> I'm looking at the reason for the assertion violation; the
> fatal_error_backtrace thingy is next.

The reason for the assertion violation is the assertion in
marker_position:

  eassert (BUF_BEG (buf) <= m->charpos && m->charpos <= BUF_Z (buf));

This was introduced recently by Dmitry, in revision 108906.  A similar
assertion was added to marker_byte_position.

Dmitry, why did you add them?  Who said that a marker cannot
legitimately have a position outside of its buffer's range of
character positions?

For the record, here's the backtrace that leads to the assertion:

  (gdb) bt
  #0  0x01332b80 in exit ()
  #1  0x01001447 in fatal_error_backtrace (sig=22, backtrace_limit=2147483647)
      at emacs.c:335
  #2  0x01043379 in die (
      msg=0x156236c "assertion failed: BUF_BEG (buf) <= m->charpos && m->charpos <= BUF_Z (buf)", file=0x1561e66 "marker.c", line=637) at alloc.c:6749
  #3  0x0110ebd6 in marker_position (marker=55669099) at marker.c:637
  #4  0x010ce3a9 in recenter_overlay_lists (buf=0x3447e00, pos=192)
      at buffer.c:3432
  #5  0x010cebd3 in adjust_overlays_for_delete (pos=192, length=3)
      at buffer.c:3555
  #6  0x011128ad in replace_range (from=192, to=195, new=56457521,
      prepare=true, inherit=false, markers=true) at insdel.c:1399
  #7  0x01104833 in Freplace_match (newtext=56457521, fixedcase=54794266,
      literal=54794290, string=54794266, subexp=54794266) at search.c:2622
  #8  0x0103652c in Ffuncall (nargs=4, args=0x82f0f4) at eval.c:2826
  #9  0x0112664f in exec_byte_code (bytestr=21187937, vector=21188029,
      maxdepth=32, args_template=54794266, nargs=0, args=0x0) at bytecode.c:898
  #10 0x01037261 in funcall_lambda (fun=21187877, nargs=5, arg_vector=0x82f368)
      at eval.c:3042
  #11 0x0103674b in Ffuncall (nargs=6, args=0x82f364) at eval.c:2859
  #12 0x0112664f in exec_byte_code (bytestr=21188625, vector=21189325,
      maxdepth=44, args_template=54794266, nargs=0, args=0x0) at bytecode.c:898
  #13 0x01037261 in funcall_lambda (fun=21188533, nargs=9, arg_vector=0x82f5d8)
      at eval.c:3042
  #14 0x0103674b in Ffuncall (nargs=10, args=0x82f5d4) at eval.c:2859
  #15 0x0112664f in exec_byte_code (bytestr=21172369, vector=21172429,
      maxdepth=40, args_template=54794266, nargs=0, args=0x0) at bytecode.c:898
  #16 0x01037261 in funcall_lambda (fun=21172309, nargs=5, arg_vector=0x82f844)
      at eval.c:3042
  #17 0x0103674b in Ffuncall (nargs=6, args=0x82f840) at eval.c:2859
  #18 0x01035550 in Fapply (nargs=2, args=0x82f8f0) at eval.c:2340
  #19 0x01035ad4 in apply1 (fn=57177114, arg=58780478) at eval.c:2578
  #20 0x011238ae in Fcall_interactively (function=57177114,
      record_flag=54794266, keys=54815661) at callint.c:378
  #21 0x01036424 in Ffuncall (nargs=4, args=0x82fb30) at eval.c:2817
  #22 0x01035ba9 in call3 (fn=54911794, arg1=57177114, arg2=54794266,
      arg3=54794266) at eval.c:2635
  #23 0x010200e2 in Fcommand_execute (cmd=57177114, record_flag=54794266,
      keys=54794266, special=54794266) at keyboard.c:10378
  #24 0x010065e5 in command_loop_1 () at keyboard.c:1626
  #25 0x01032221 in internal_condition_case (bfun=0x100588b <command_loop_1>,
      handlers=54844922, hfun=0x100506e <cmd_error>) at eval.c:1319
  #26 0x010054c8 in command_loop_2 (ignore=54794266) at keyboard.c:1193
  #27 0x01031be3 in internal_catch (tag=54834754,
      func=0x10054a5 <command_loop_2>, arg=54794266) at eval.c:1076
  #28 0x01005480 in command_loop () at keyboard.c:1172
  #29 0x01004a2c in recursive_edit_1 () at keyboard.c:793
  #30 0x01004d4e in Frecursive_edit () at keyboard.c:857
  #31 0x010029d5 in main (argc=2, argv=0xa42808) at emacs.c:1660

  Lisp Backtrace:
  "replace-match" (0x82f0f8)
  "replace-match-maybe-edit" (0x82f368)
  "perform-replace" (0x82f5d8)
  "query-replace" (0x82f844)
  "call-interactively" (0x82fb34)

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On 09/12/2012 11:02 PM, Eli Zaretskii wrote:

> The reason for the assertion violation is the assertion in
> marker_position:
>
>    eassert (BUF_BEG (buf) <= m->charpos && m->charpos <= BUF_Z (buf));
>
> This was introduced recently by Dmitry, in revision 108906.  A similar
> assertion was added to marker_byte_position.
>
> Dmitry, why did you add them?  Who said that a marker cannot
> legitimately have a position outside of its buffer's range of
> character positions?

IIUC marker can have out-of-buffer-range position, but only somewhere in the
middle of an insertion/deletion/replace operation; out-of-range position should
be detected and immediately fixed by adjust_markers_for_{delete,insert,replace}.

BTW, look at this code from replace_range:

  /* Adjust the overlay center as needed.  This must be done after
      adjusting the markers that bound the overlays.  */
   adjust_overlays_for_delete (from, nchars_del);
   adjust_overlays_for_insert (from, inschars);

   /* Adjust markers for the deletion and the insertion.  */
   if (markers)
     adjust_markers_for_replace (from, from_byte, nchars_del, nbytes_del,
				inschars, outgoing_insbytes);

The comment explicitly says that overlays should be adjusted _after_ markers,
but the code adjusts overlays and then markers :-(. Since an overlays are
bounded by markers, the comment looks correct but the code isn't. I suppose
that the code snippets above should be swapped.

Dmitry


[-- Attachment #2: adjust.patch --]
[-- Type: text/plain, Size: 955 bytes --]

=== modified file 'src/insdel.c'
--- src/insdel.c	2012-09-11 04:22:03 +0000
+++ src/insdel.c	2012-09-13 02:47:36 +0000
@@ -1394,16 +1394,16 @@
 
   eassert (GPT <= GPT_BYTE);
 
-  /* Adjust the overlay center as needed.  This must be done after
-     adjusting the markers that bound the overlays.  */
-  adjust_overlays_for_delete (from, nchars_del);
-  adjust_overlays_for_insert (from, inschars);
-
   /* Adjust markers for the deletion and the insertion.  */
   if (markers)
     adjust_markers_for_replace (from, from_byte, nchars_del, nbytes_del,
 				inschars, outgoing_insbytes);
 
+  /* Adjust the overlay center as needed.  This must be done after
+     adjusting the markers that bound the overlays.  */
+  adjust_overlays_for_delete (from, nchars_del);
+  adjust_overlays_for_insert (from, inschars);
+
   offset_intervals (current_buffer, from, inschars - nchars_del);
 
   /* Get the intervals for the part of the string we are inserting--

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Eli Zaretskii]

> Date: Thu, 13 Sep 2012 07:01:48 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: dmoncayo@gmail.com, 12426@debbugs.gnu.org
> 
> IIUC marker can have out-of-buffer-range position, but only somewhere in the
> middle of an insertion/deletion/replace operation; out-of-range position should
> be detected and immediately fixed by adjust_markers_for_{delete,insert,replace}.

But marker_position and marker_byte_position are simple getters of
these two attributes of a marker.  If these attributes can be out of
range for some window of time, then the getters shouldn't enforce this
limitation.  Otherwise, they are getters that cannot be used in some
situations, which is IMO bad SE.  At the very least that should be
documented.

adjust_markers_for_* functions access the marker positions directly,
bypassing these two getters.  If we want to enforce the assertions you
added, we should change OVERLAY_POSITION not to use the getter, but
access the struct directly, too.

> BTW, look at this code from replace_range:
> 
>   /* Adjust the overlay center as needed.  This must be done after
>       adjusting the markers that bound the overlays.  */
>    adjust_overlays_for_delete (from, nchars_del);
>    adjust_overlays_for_insert (from, inschars);
> 
>    /* Adjust markers for the deletion and the insertion.  */
>    if (markers)
>      adjust_markers_for_replace (from, from_byte, nchars_del, nbytes_del,
> 				inschars, outgoing_insbytes);
> 
> The comment explicitly says that overlays should be adjusted _after_ markers,
> but the code adjusts overlays and then markers :-(. Since an overlays are
> bounded by markers, the comment looks correct but the code isn't. I suppose
> that the code snippets above should be swapped.

This was introduced in revision 40908 by Richard, and looks quite
deliberate.  So switching the order might not be TRT, especially since
adjust_overlays_for_insert recenters the overlays around the buffer
position in question, which makes access to overlays faster.

I tried to look in the various related mailing lists for the reason of
the change in revision 40908, but couldn't find anything appropriate.
Richard, do you happen to remember the reason?  The change and its log
entry are below; it looks like this was accompanied by some changes in
Lisp as well.

2001-11-11  Richard M. Stallman  <rms@gnu.org>

	* insdel.c (replace_range): Use adjust_markers_for_replace
	instead of adjust_markers_for_delete and adjust_markers_for_insert.

	* intervals.h: Declare set_text_properties and set_text_properties_1.

	* textprop.c (set_text_properties_1): New subroutine
	broken out of set_text_properties.
	(set_text_properties): Use set_text_properties_1.

	* intervals.c (graft_intervals_into_buffer):
	Use set_text_properties_1 to clear out properties.

	* search.c (Freplace_match): Use replace_range to insert
	and delete.  Don't request property inheritance from
	surrounding text.

=== modified file 'src/insdel.c'
--- src/insdel.c	2001-10-26 12:02:21 +0000
+++ src/insdel.c	2001-11-11 20:04:45 +0000
@@ -1423,13 +1423,6 @@ replace_range (from, to, new, prepare, i
   if (! EQ (current_buffer->undo_list, Qt))
     deletion = make_buffer_string_both (from, from_byte, to, to_byte, 1);
 
-  if (markers)
-    /* Relocate all markers pointing into the new, larger gap
-       to point at the end of the text before the gap.
-       Do this before recording the deletion,
-       so that undo handles this after reinserting the text.  */
-    adjust_markers_for_delete (from, from_byte, to, to_byte);
-
   GAP_SIZE += nbytes_del;
   ZV -= nchars_del;
   Z -= nchars_del;
@@ -1489,10 +1482,11 @@ replace_range (from, to, new, prepare, i
      adjusting the markers that bound the overlays.  */
   adjust_overlays_for_delete (from, nchars_del);
   adjust_overlays_for_insert (from, inschars);
+
+  /* Adjust markers for the deletion and the insertion.  */
   if (markers)
-    adjust_markers_for_insert (from, from_byte,
-			       from + inschars, from_byte + outgoing_insbytes,
-			       0);
+    adjust_markers_for_replace (from, from_byte, nchars_del, nbytes_del,
+				inschars, outgoing_insbytes);
 
   offset_intervals (current_buffer, from, inschars - nchars_del);
 

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Dmitry Antipov]

On 09/13/2012 08:47 PM, Eli Zaretskii wrote:

> But marker_position and marker_byte_position are simple getters of
> these two attributes of a marker.  If these attributes can be out of
> range for some window of time, then the getters shouldn't enforce this
> limitation.  Otherwise, they are getters that cannot be used in some
> situations, which is IMO bad SE.  At the very least that should be
> documented.

IIUC no, since this window of time is very short and it's entirely
within adjust_markers_for_* functions.

Also note that adjust_after_replace and del_range_2 adjusts markers
first and overlays next, but replace_range and replace_range_2 adjusts
overlays first; moreover, both replace_range and replace_range_2
has comments which says that "markers first, overlays next" is the
correct order. This is pretty strange.

Dmitry

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Eli Zaretskii]

> Date: Fri, 14 Sep 2012 16:35:08 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Richard Stallman <rms@gnu.org>, dmoncayo@gmail.com, 
>  12426@debbugs.gnu.org
> 
> On 09/13/2012 08:47 PM, Eli Zaretskii wrote:
> 
> > But marker_position and marker_byte_position are simple getters of
> > these two attributes of a marker.  If these attributes can be out of
> > range for some window of time, then the getters shouldn't enforce this
> > limitation.  Otherwise, they are getters that cannot be used in some
> > situations, which is IMO bad SE.  At the very least that should be
> > documented.
> 
> IIUC no, since this window of time is very short and it's entirely
> within adjust_markers_for_* functions.

The length of the window has no importance whatsoever.  It just takes
a couple of machine instructions to trigger the assertion violation.

This isn't a stock exchange or a poker game, where we could build on
chances.  This is software that is supposed to be stable, and you are
talking about its most inner and intimate internals.

If you can figure out how to avoid the crash, please do.  Otherwise,
please remove the eassert calls, because they make Emacs completely
unusable.

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Dani Moncayo]

I've just built the current trunk (revno 110075) and the problem
doesn't appear with the original recipe.

I think Dmitry has fixed it in revno 110028.

If so, this bug could be closed.

-- 
Dani Moncayo

bug#12426: 24.2.50; Emacs is closed unexpectedly after query-replace

[Eli Zaretskii]

> Date: Wed, 12 Sep 2012 21:39:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 12426@debbugs.gnu.org
> 
> > Date: Wed, 12 Sep 2012 20:06:03 +0200
> > From: Dani Moncayo <dmoncayo@gmail.com>
> > 
> > Recipe from emacs -Q:  a <SPC> b M-a M-% a <SPC> b <RET> c <RET> <SPC>
> > 
> > After doing that, my Emacs session suddenly ends.
> 
> It exits due to an assertion violation.  Looks like the recent changes
> that introduced fatal_error_backtrace are a misfeature on Windows.
> 
> I'm looking at the reason for the assertion violation; the
> fatal_error_backtrace thingy is next.

The second part is fixed in trunk revision 109997, Emacs no longer
silently exits with the recipe in this bug report.