bug#24368: 25.1; Assertion failure in attach_marker

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

To reproduce, put the following code into *scratch* of emacs -Q:


(defvar old-state nil)
(defvar new-state nil)

(progn
   (setq display-buffer-alist
	'(("\\*\\(?:help\\)\\*" display-buffer-in-side-window
	   (side . bottom) (slot . -1))))

   (temp-buffer-resize-mode 1)
   (add-to-list 'window-persistent-parameters '(window-preserved-size . t))
   (add-to-list 'window-persistent-parameters '(window-side . t))
   (add-to-list 'window-persistent-parameters '(window-slot . t))
   (describe-function 'describe-function)
   (other-window 1))

(defun old ()
   (interactive)
   (setq old-state (window-state-get (frame-root-window)))
   (delete-window))

(defun new ()
   (interactive)
   (setq new-state (window-state-get (frame-root-window)))
   (window-state-put old-state (frame-root-window) t)
   (window-state-put new-state (window--major-non-side-window)))

;; (eval-buffer)


Evaluate *scratch* - this should get you two windows, one showing
*scratch* and one showing *Help*.  Now do M-x old which should get you
one window showing *scratch* followed by M-x new.  The latter gets me an
assertion failure with the following backtrace:

(gdb) bt
#0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at emacs.c:354
#1  0x011f1adf in die (msg=0x164b8c2 "charpos == bytepos", file=0x164b760 "marker.c", line=434) at alloc.c:7223
#2  0x011a21de in attach_marker (m=0x186b888, b=0x170a978, charpos=271, bytepos=275) at marker.c:434
#3  0x011a2765 in set_marker_internal (marker=..., position=..., buffer=..., restricted=true) at marker.c:517
#4  0x011a2835 in set_marker_restricted (marker=..., position=..., buffer=...) at marker.c:538
#5  0x010b898d in Fset_window_start (window=..., pos=..., noforce=...) at window.c:1679
#6  0x0122094a in Ffuncall (nargs=3, args=0x82cbf8) at eval.c:2700
#7  0x0127d425 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=0, args=0x0) at bytecode.c:880
#8  0x01221d16 in funcall_lambda (fun=..., nargs=2, arg_vector=0x82d1d0) at eval.c:2921
#9  0x01220cc2 in Ffuncall (nargs=3, args=0x82d1c8) at eval.c:2742
#10 0x0127d425 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=0, args=0x0) at bytecode.c:880
#11 0x01221d16 in funcall_lambda (fun=..., nargs=3, arg_vector=0x82d660) at eval.c:2921
#12 0x012211ae in apply_lambda (fun=..., args=..., count=15) at eval.c:2794
#13 0x0121ec64 in eval_sub (form=...) at eval.c:2211
#14 0x0121742d in Fprogn (body=...) at eval.c:426
#15 0x01221c18 in funcall_lambda (fun=..., nargs=0, arg_vector=0x82de40) at eval.c:2914
#16 0x01220e4e in Ffuncall (nargs=1, args=0x82de38) at eval.c:2754
#17 0x01212ce6 in Ffuncall_interactively (nargs=1, args=0x82de38) at callint.c:252
#18 0x01220763 in Ffuncall (nargs=2, args=0x82de30) at eval.c:2673
#19 0x0121f088 in Fapply (nargs=3, args=0x82de30) at eval.c:2274
#20 0x012134c2 in Fcall_interactively (function=..., record_flag=..., keys=...) at callint.c:389
#21 0x0122094a in Ffuncall (nargs=4, args=0x82e018) at eval.c:2700
#22 0x0127d425 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=2, args=0x82e608) at bytecode.c:880
#23 0x012216b4 in funcall_lambda (fun=..., nargs=2, arg_vector=0x82e5f8) at eval.c:2855
#24 0x01220cc2 in Ffuncall (nargs=3, args=0x82e5f0) at eval.c:2742
#25 0x0127d425 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=3, args=0x82ecf8) at bytecode.c:880
#26 0x012216b4 in funcall_lambda (fun=..., nargs=3, arg_vector=0x82ece0) at eval.c:2855
#27 0x01220cc2 in Ffuncall (nargs=4, args=0x82ecd8) at eval.c:2742
#28 0x01212ce6 in Ffuncall_interactively (nargs=4, args=0x82ecd8) at callint.c:252
#29 0x01220763 in Ffuncall (nargs=5, args=0x82ecd0) at eval.c:2673
#30 0x0121f749 in Fapply (nargs=3, args=0x82efe0) at eval.c:2321
#31 0x012134c2 in Fcall_interactively (function=..., record_flag=..., keys=...) at callint.c:389
#32 0x0122094a in Ffuncall (nargs=4, args=0x82f1c8) at eval.c:2700
#33 0x0127d425 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=1, args=0x82f780) at bytecode.c:880
#34 0x012216b4 in funcall_lambda (fun=..., nargs=1, arg_vector=0x82f778) at eval.c:2855
#35 0x01220cc2 in Ffuncall (nargs=2, args=0x82f770) at eval.c:2742
#36 0x01220013 in call1 (fn=..., arg1=...) at eval.c:2552
#37 0x011515c3 in command_loop_1 () at keyboard.c:1479
#38 0x0121b586 in internal_condition_case (bfun=0x1150af7 <command_loop_1>, handlers=..., hfun=0x114fd8f <cmd_error>) at eval.c:1309
#39 0x01150591 in command_loop_2 (ignore=...) at keyboard.c:1107
#40 0x0121a72a in internal_catch (tag=..., func=0x1150555 <command_loop_2>, arg=...) at eval.c:1074
#41 0x01150517 in command_loop () at keyboard.c:1086
#42 0x0114f7a7 in recursive_edit_1 () at keyboard.c:692
#43 0x0114fa68 in Frecursive_edit () at keyboard.c:763
#44 0x0114d17e in main (argc=2, argv=0xa32658) at emacs.c:1626

Lisp Backtrace:
"set-window-start" (0x82cc00)
"window--state-put-2" (0x82d1d0)
"window-state-put" (0x82d660)
"new" (0x82de40)
"funcall-interactively" (0x82de38)
"call-interactively" (0x82e020)
"command-execute" (0x82e5f8)
"execute-extended-command" (0x82ece0)
"funcall-interactively" (0x82ecd8)
"call-interactively" (0x82f1d0)
"command-execute" (0x82f778)
(gdb)


In GNU Emacs 25.1.1 (i686-pc-mingw32)
  of 2016-08-22 built on MACHNO
Repository revision: 927afa1ef6514e62162068d7dbdc9d8381a71598
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
Configured using:
  'configure --prefix=/c/emacs-git/next --with-wide-int
  --enable-checking=yes --enable-check-lisp-object-type=yes 'CFLAGS=-O0
  -g3''

martin

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

The reason behind this bug is the following construct in
‘window--state-get-1’:

             ,@(when buffer
                 ;; All buffer related things go in here.
		(let ((point (window-point window))
		      (start (window-start window)))
		  `((buffer
		     ,(buffer-name buffer)
		     (selected . ,selected)
		     (hscroll . ,(window-hscroll window))
		     (fringes . ,(window-fringes window))
		     (margins . ,(window-margins window))
		     (scroll-bars . ,(window-scroll-bars window))
		     (vscroll . ,(window-vscroll window))
		     (dedicated . ,(window-dedicated-p window))
		     (point . ,(if writable point
                                  (copy-marker point
                                               (buffer-local-value
                                                'window-point-insertion-type
                                                buffer))))
		     (start . ,(if writable start (copy-marker start)))))))))

This saves `point' and `start' as markers in the wrong buffer - in my
example *help* and not *scratch*.  A silly bug but I don't even know how
to put in a ‘with-current-buffer’ with all those backslashes :-(

The ensuing assertion failure should not happen though - somehow we
don't recognize that we set a marker in the wrong buffer.

martin

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

> to put in a ‘with-current-buffer’ with all those backslashes :-(
                                                    backquotes, obviously

martin

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

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

> A silly bug but I don't even know how
> to put in a ‘with-current-buffer’ with all those backslashes :-(

OK.  Trivial patch attached.

martin



[-- Attachment #2: window-state-put.diff --]
[-- Type: text/plain, Size: 1252 bytes --]

--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5289,12 +5289,17 @@ window--state-get-1
 		     (scroll-bars . ,(window-scroll-bars window))
 		     (vscroll . ,(window-vscroll window))
 		     (dedicated . ,(window-dedicated-p window))
-		     (point . ,(if writable point
-                                 (copy-marker point
-                                              (buffer-local-value
-                                               'window-point-insertion-type
-                                               buffer))))
-		     (start . ,(if writable start (copy-marker start)))))))))
+		     (point . ,(if writable
+                                   point
+                                 (with-current-buffer buffer
+                                   (copy-marker point
+                                                (buffer-local-value
+                                                 'window-point-insertion-type
+                                                 buffer)))))
+		     (start . ,(if writable
+                                   start
+                                 (with-current-buffer buffer
+                                   (copy-marker start))))))))))
 	 (tail
 	  (when (memq type '(vc hc))
 	    (let (list)

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > The ensuing assertion failure should not happen though - somehow we
 > don't recognize that we set a marker in the wrong buffer.

FWIW, in attach_marker if m->buffer != b the check

   if (BUF_Z (b) == BUF_Z_BYTE (b))
     eassert (charpos == bytepos);
   else
     eassert (charpos <= bytepos);

will fail when b has the same char and byte sizes but charpos and
bytepos differ in m->buffer (like *Help* with its various quotes).  A
thinko.

martin

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Mon, 05 Sep 2016 17:04:08 +0200
> From: martin rudalics <rudalics@gmx.at>
> 
> FWIW, in attach_marker if m->buffer != b the check
> 
>    if (BUF_Z (b) == BUF_Z_BYTE (b))
>      eassert (charpos == bytepos);
>    else
>      eassert (charpos <= bytepos);
> 
> will fail when b has the same char and byte sizes but charpos and
> bytepos differ in m->buffer (like *Help* with its various quotes).  A
> thinko.

Obviously, the BUF_z vs BUF_Z_BYTE test should be done for the same
buffer whose character and byte positions we compare in the
assertions.

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Mon, 05 Sep 2016 16:45:07 +0200
> From: martin rudalics <rudalics@gmx.at>
> 
> > A silly bug but I don't even know how
> > to put in a ‘with-current-buffer’ with all those backslashes :-(
> 
> OK.  Trivial patch attached.

Thanks.  If no comments are published in a few days, please push.

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > Obviously, the BUF_z vs BUF_Z_BYTE test should be done for the same
 > buffer whose character and byte positions we compare in the
 > assertions.

How?  In attach_frame, the buffer where the positions came from is not
known.  Anyway, I passed an extra boolean to attach_marker to fix that
and now I get a similar assertion violation in temp_set_point_both.
This is a can of worms :-(

(gdb) bt
#0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at emacs.c:354
#1  0x011f1b5b in die (msg=0x1661424 "BUF_ZV (buffer) != BUF_ZV_BYTE (buffer) || charpos == bytepos", file=0x1660f60 "intervals.c", line=1793) at alloc.c:7223
#2  0x012a29b2 in temp_set_point_both (buffer=0x170a978, charpos=271, bytepos=275) at intervals.c:1793
#3  0x010ba6f8 in unshow_buffer (w=0x1b60258) at window.c:2083
#4  0x010bf82e in Fset_window_buffer (window=..., buffer_or_name=..., keep_margins=...) at window.c:3373
#5  0x012209c6 in Ffuncall (nargs=3, args=0x82d1d8) at eval.c:2700
#6  0x0127d4a1 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=0, args=0x0) at bytecode.c:880
#7  0x01221d92 in funcall_lambda (fun=..., nargs=2, arg_vector=0x82d670) at eval.c:2921
#8  0x0122122a in apply_lambda (fun=..., args=..., count=15) at eval.c:2794
#9  0x0121ece0 in eval_sub (form=...) at eval.c:2211
#10 0x012174a9 in Fprogn (body=...) at eval.c:426
#11 0x01221c94 in funcall_lambda (fun=..., nargs=0, arg_vector=0x82de40) at eval.c:2914
#12 0x01220eca in Ffuncall (nargs=1, args=0x82de38) at eval.c:2754
#13 0x01212d62 in Ffuncall_interactively (nargs=1, args=0x82de38) at callint.c:252
#14 0x012207df in Ffuncall (nargs=2, args=0x82de30) at eval.c:2673
#15 0x0121f104 in Fapply (nargs=3, args=0x82de30) at eval.c:2274
#16 0x0121353e in Fcall_interactively (function=..., record_flag=..., keys=...) at callint.c:389
#17 0x012209c6 in Ffuncall (nargs=4, args=0x82e018) at eval.c:2700
#18 0x0127d4a1 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=2, args=0x82e608) at bytecode.c:880
#19 0x01221730 in funcall_lambda (fun=..., nargs=2, arg_vector=0x82e5f8) at eval.c:2855
#20 0x01220d3e in Ffuncall (nargs=3, args=0x82e5f0) at eval.c:2742
#21 0x0127d4a1 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=3, args=0x82ecf8) at bytecode.c:880
#22 0x01221730 in funcall_lambda (fun=..., nargs=3, arg_vector=0x82ece0) at eval.c:2855
#23 0x01220d3e in Ffuncall (nargs=4, args=0x82ecd8) at eval.c:2742
#24 0x01212d62 in Ffuncall_interactively (nargs=4, args=0x82ecd8) at callint.c:252
#25 0x012207df in Ffuncall (nargs=5, args=0x82ecd0) at eval.c:2673
#26 0x0121f7c5 in Fapply (nargs=3, args=0x82efe0) at eval.c:2321
#27 0x0121353e in Fcall_interactively (function=..., record_flag=..., keys=...) at callint.c:389
#28 0x012209c6 in Ffuncall (nargs=4, args=0x82f1c8) at eval.c:2700
#29 0x0127d4a1 in exec_byte_code (bytestr=..., vector=..., maxdepth=..., args_template=..., nargs=1, args=0x82f780) at bytecode.c:880
#30 0x01221730 in funcall_lambda (fun=..., nargs=1, arg_vector=0x82f778) at eval.c:2855
#31 0x01220d3e in Ffuncall (nargs=2, args=0x82f770) at eval.c:2742
#32 0x0122008f in call1 (fn=..., arg1=...) at eval.c:2552
#33 0x011515c3 in command_loop_1 () at keyboard.c:1479
#34 0x0121b602 in internal_condition_case (bfun=0x1150af7 <command_loop_1>, handlers=..., hfun=0x114fd8f <cmd_error>) at eval.c:1309
#35 0x01150591 in command_loop_2 (ignore=...) at keyboard.c:1107
#36 0x0121a7a6 in internal_catch (tag=..., func=0x1150555 <command_loop_2>, arg=...) at eval.c:1074
#37 0x01150517 in command_loop () at keyboard.c:1086
#38 0x0114f7a7 in recursive_edit_1 () at keyboard.c:692
#39 0x0114fa68 in Frecursive_edit () at keyboard.c:763
#40 0x0114d17e in main (argc=2, argv=0xa32658) at emacs.c:1626

Lisp Backtrace:
"set-window-buffer" (0x82d1e0)
"window-state-put" (0x82d670)
"new" (0x82de40)
"funcall-interactively" (0x82de38)
"call-interactively" (0x82e020)
"command-execute" (0x82e5f8)
"execute-extended-command" (0x82ece0)
"funcall-interactively" (0x82ecd8)
"call-interactively" (0x82f1d0)
"command-execute" (0x82f778)
(gdb)

martin

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Mon, 05 Sep 2016 19:34:13 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
>  > Obviously, the BUF_z vs BUF_Z_BYTE test should be done for the same
>  > buffer whose character and byte positions we compare in the
>  > assertions.
> 
> How?  In attach_frame, the buffer where the positions came from is not
> known.

It looks like the callers already take care of that?  Or did you find
a case where this is not happening?

> Anyway, I passed an extra boolean to attach_marker to fix that
> and now I get a similar assertion violation in temp_set_point_both.
> This is a can of worms :-(

Sorry, I guess you lost me: what did you try to fix and how?

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

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

 >> How?  In attach_frame, the buffer where the positions came from is not
 >> known.
 >
 > It looks like the callers already take care of that?  Or did you find
 > a case where this is not happening?

How would you then explain the assertion failure I reported initially?

 >> Anyway, I passed an extra boolean to attach_marker to fix that
 >> and now I get a similar assertion violation in temp_set_point_both.
 >> This is a can of worms :-(
 >
 > Sorry, I guess you lost me: what did you try to fix and how?

I applied the attached patch.

martin

[-- Attachment #2: attach_marker.diff --]
[-- Type: text/plain, Size: 1726 bytes --]

diff --git a/src/marker.c b/src/marker.c
index febdb17..92ba7dd 100644
--- a/src/marker.c
+++ b/src/marker.c
@@ -426,11 +426,12 @@ DEFUN ("marker-position", Fmarker_position, Smarker_position, 1, 1, 0,

 static void
 attach_marker (struct Lisp_Marker *m, struct buffer *b,
-	       ptrdiff_t charpos, ptrdiff_t bytepos)
+	       ptrdiff_t charpos, ptrdiff_t bytepos,
+	       bool from_other_buffer)
 {
   /* In a single-byte buffer, two positions must be equal.
      Otherwise, every character is at least one byte.  */
-  if (BUF_Z (b) == BUF_Z_BYTE (b))
+  if ((!from_other_buffer) && (BUF_Z (b) == BUF_Z_BYTE (b)))
     eassert (charpos == bytepos);
   else
     eassert (charpos <= bytepos);
@@ -514,7 +515,10 @@ set_marker_internal (Lisp_Object marker, Lisp_Object position,
 	  (restricted ? BUF_BEGV_BYTE (b) : BUF_BEG_BYTE (b),
 	   bytepos, restricted ? BUF_ZV_BYTE (b) : BUF_Z_BYTE (b));

-      attach_marker (m, b, charpos, bytepos);
+      attach_marker (m, b, charpos, bytepos,
+		     MARKERP (position)
+		     ? b != XMARKER (position)->buffer
+		     : false);
     }
   return marker;
 }
@@ -552,7 +556,7 @@ set_marker_both (Lisp_Object marker, Lisp_Object buffer,
   m = XMARKER (marker);

   if (b)
-    attach_marker (m, b, charpos, bytepos);
+    attach_marker (m, b, charpos, bytepos, false);
   else
     unchain_marker (m);
   return marker;
@@ -575,7 +579,8 @@ set_marker_restricted_both (Lisp_Object marker, Lisp_Object buffer,
       attach_marker
 	(m, b,
 	 clip_to_bounds (BUF_BEGV (b), charpos, BUF_ZV (b)),
-	 clip_to_bounds (BUF_BEGV_BYTE (b), bytepos, BUF_ZV_BYTE (b)));
+	 clip_to_bounds (BUF_BEGV_BYTE (b), bytepos, BUF_ZV_BYTE (b)),
+	 false);
     }
   else
     unchain_marker (m);

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Mon, 05 Sep 2016 19:58:19 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
>  >> How?  In attach_frame, the buffer where the positions came from is not
>  >> known.
>  >
>  > It looks like the callers already take care of that?  Or did you find
>  > a case where this is not happening?
> 
> How would you then explain the assertion failure I reported initially?

I thought you've explained it yourself?  It was a bug, whereby the
marker was set up incorrectly.  Why would we want the assertions to be
protected from bugs?

Alternatively, if we do want to avoid assertions in such cases, let's
simply signal an error when the conditions don't hold.

>  >> Anyway, I passed an extra boolean to attach_marker to fix that
>  >> and now I get a similar assertion violation in temp_set_point_both.
>  >> This is a can of worms :-(
>  >
>  > Sorry, I guess you lost me: what did you try to fix and how?
> 
> I applied the attached patch.

And which test case triggers the assertion in temp_set_point_both,
after applying that patch?

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > I thought you've explained it yourself?  It was a bug, whereby the
 > marker was set up incorrectly.  Why would we want the assertions to be
 > protected from bugs?

The bug in ‘window-state-get’ is a separate bug.  Its consequences
should be silly window start and point positions.  Emacs should continue
to be operative.

The assertion failure is triggered whenever we copy a marker whose
character position differs from its byte position into a buffer whose
character size equals its byte size.  Such behavior is more than silly.
It may lead to the loss of a user's work.  Agreed?

 >> I applied the attached patch.
 >
 > And which test case triggers the assertion in temp_set_point_both,
 > after applying that patch?

The same as in my original report.  You just have to apply the patch for
marker.c first but _not_ the patch for window.el.

martin

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

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

I now wrote a better patch for the attach_marker issue (attached).  For
the remainder, the problem is as follows (with emacs -Q): Evaluating

(progn
   (describe-function 'describe-function)
   (let* ((help (get-buffer "*Help*"))
	 (marker (with-current-buffer help
		   (copy-marker (point-max)))))
     (goto-char marker)))

fails with (error "Marker points into wrong buffer" #<marker at 271 in *Help*>)
while evaluating

(progn
   (describe-function 'describe-function)
   (let* ((help (get-buffer "*Help*"))
	 (marker (with-current-buffer help
		   (copy-marker (point-max)))))
     (set-window-start (selected-window) marker)))

crashes in init_iterator and

(progn
   (describe-function 'describe-function)
   (let* ((help (get-buffer "*Help*"))
	 (marker (with-current-buffer help
		   (copy-marker (point-max)))))
     (with-selected-window (get-buffer-window help)
       (set-window-point (get-buffer-window "*scratch*") marker))))

crashes in set_point_both.

We can either make these fail as in the ‘goto-char’ case or handle them
as with attach_marker.

The former is simpler but would have to be motivated somehow.  IMHO
‘goto-char’ should not signal an error in that case - it's perfectly
legitimate to use the position from another buffer's marker as value.

The latter is tedious to implement but inherently more correct.

Your choice ;-)

martin

[-- Attachment #2: attach_marker.diff --]
[-- Type: text/plain, Size: 1665 bytes --]

diff --git a/src/marker.c b/src/marker.c
index febdb17..f2c4d24 100644
--- a/src/marker.c
+++ b/src/marker.c
@@ -426,11 +426,12 @@ DEFUN ("marker-position", Fmarker_position, Smarker_position, 1, 1, 0,

 static void
 attach_marker (struct Lisp_Marker *m, struct buffer *b,
-	       ptrdiff_t charpos, ptrdiff_t bytepos)
+	       ptrdiff_t charpos, ptrdiff_t bytepos,
+	       struct buffer *sb)
 {
   /* In a single-byte buffer, two positions must be equal.
      Otherwise, every character is at least one byte.  */
-  if (BUF_Z (b) == BUF_Z_BYTE (b))
+  if ((BUF_Z (sb) == BUF_Z_BYTE (sb)))
     eassert (charpos == bytepos);
   else
     eassert (charpos <= bytepos);
@@ -514,7 +515,8 @@ set_marker_internal (Lisp_Object marker, Lisp_Object position,
 	  (restricted ? BUF_BEGV_BYTE (b) : BUF_BEG_BYTE (b),
 	   bytepos, restricted ? BUF_ZV_BYTE (b) : BUF_Z_BYTE (b));

-      attach_marker (m, b, charpos, bytepos);
+      attach_marker (m, b, charpos, bytepos,
+		     MARKERP (position) ? XMARKER (position)->buffer : b);
     }
   return marker;
 }
@@ -552,7 +554,7 @@ set_marker_both (Lisp_Object marker, Lisp_Object buffer,
   m = XMARKER (marker);

   if (b)
-    attach_marker (m, b, charpos, bytepos);
+    attach_marker (m, b, charpos, bytepos, b);
   else
     unchain_marker (m);
   return marker;
@@ -575,7 +577,8 @@ set_marker_restricted_both (Lisp_Object marker, Lisp_Object buffer,
       attach_marker
 	(m, b,
 	 clip_to_bounds (BUF_BEGV (b), charpos, BUF_ZV (b)),
-	 clip_to_bounds (BUF_BEGV_BYTE (b), bytepos, BUF_ZV_BYTE (b)));
+	 clip_to_bounds (BUF_BEGV_BYTE (b), bytepos, BUF_ZV_BYTE (b)),
+	 b);
     }
   else
     unchain_marker (m);

bug#24368: 25.1; Assertion failure in attach_marker

[Noam Postavsky]

On Tue, Sep 6, 2016 at 2:44 AM, martin rudalics <rudalics@gmx.at> wrote:
> We can either make these fail as in the ‘goto-char’ case or handle them
> as with attach_marker.
>
> The former is simpler but would have to be motivated somehow.  IMHO
> ‘goto-char’ should not signal an error in that case - it's perfectly
> legitimate to use the position from another buffer's marker as value.

If you want to ignore the marker's buffer you can do (goto-char
(marker-position marker)); if you pass the whole marker object, it
makes sense that the buffer is checked.

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Tue, 06 Sep 2016 08:44:52 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
> I now wrote a better patch for the attach_marker issue (attached).

On second thought, I don't think attach_marker should be touched at
all.  It is too low-level, and the assertions compile to nothing in a
build without --enable-checking, so we will be left without any
protection in the production version.

Instead, I think the callers of attach_marker should make sure charpos
and bytepos come from the same buffer, and are therefore in sync.  The
patch below implements that; WDYT?

> For the remainder, the problem is as follows (with emacs -Q):
> Evaluating

> (progn
>   (describe-function 'describe-function)
>   (let* ((help (get-buffer "*Help*"))
>          (marker (with-current-buffer help
>                    (copy-marker (point-max)))))
>     (goto-char marker)))
> 
> fails with (error "Marker points into wrong buffer" #<marker at 271 in *Help*>)
> while evaluating
> 
> (progn
>   (describe-function 'describe-function)
>   (let* ((help (get-buffer "*Help*"))
>          (marker (with-current-buffer help
>                    (copy-marker (point-max)))))
>     (set-window-start (selected-window) marker)))
> 
> crashes in init_iterator and
> 
> (progn
>   (describe-function 'describe-function)
>   (let* ((help (get-buffer "*Help*"))
>          (marker (with-current-buffer help
>                    (copy-marker (point-max)))))
>     (with-selected-window (get-buffer-window help)
>       (set-window-point (get-buffer-window "*scratch*") marker))))
> 
> crashes in set_point_both.
> 
> We can either make these fail as in the ‘goto-char’ case or handle them
> as with attach_marker.
> 
> The former is simpler but would have to be motivated somehow.  IMHO
> ‘goto-char’ should not signal an error in that case - it's perfectly
> legitimate to use the position from another buffer's marker as value.

I agree that erroring out of goto-char in this case is sub-optimal at
best.

> The latter is tedious to implement but inherently more correct.
> 
> Your choice ;-)

I chose neither ;-)  The patch below seems to fix all of your test
cases, including the original problem with window-state-get, even
before it was fixed.  It also lifts the restriction on goto-char.

Comments?

P.S. After fixing the crashes and correcting window--state-get-1,
running your original test case causes "C-x 1" stop working: it has no
effect.  Looks like this is because Emacs thinks the window is the
major non-side window, although actually I have *scratch* and *Help*,
so the command ought to work.  Is this related or unrelated to the
issue at hand?

--- src/marker.c~0	2016-05-08 07:05:54.000000000 +0300
+++ src/marker.c	2016-09-06 12:25:46.480269500 +0300
@@ -507,7 +507,11 @@ set_marker_internal (Lisp_Object marker,
       charpos = clip_to_bounds
 	(restricted ? BUF_BEGV (b) : BUF_BEG (b), charpos,
 	 restricted ? BUF_ZV (b) : BUF_Z (b));
-      if (bytepos == -1)
+      /* Don't believe BYTEPOS if it comes from a different buffer,
+	 since that buffer might have a very different correspondence
+	 between character and byte positions.  */
+      if (bytepos == -1
+	  || !(MARKERP (position) && XMARKER (position)->buffer == b))
 	bytepos = buf_charpos_to_bytepos (b, charpos);
       else
 	bytepos = clip_to_bounds
--- src/intervals.c~0	2016-03-20 14:34:33.000000000 +0200
+++ src/intervals.c	2016-09-06 12:09:54.663524800 +0300
@@ -1821,11 +1821,16 @@ set_point (ptrdiff_t charpos)
 void
 set_point_from_marker (Lisp_Object marker)
 {
+  ptrdiff_t charpos = clip_to_bounds (BEGV, marker_position (marker), ZV);
+  ptrdiff_t bytepos = marker_byte_position (marker);
+
+  /* Don't trust the byte position if the marker belongs to a
+     different buffer.  */
   if (XMARKER (marker)->buffer != current_buffer)
-    signal_error ("Marker points into wrong buffer", marker);
-  set_point_both
-    (clip_to_bounds (BEGV, marker_position (marker), ZV),
-     clip_to_bounds (BEGV_BYTE, marker_byte_position (marker), ZV_BYTE));
+    bytepos = buf_charpos_to_bytepos (current_buffer, charpos);
+  else
+    bytepos = clip_to_bounds (BEGV_BYTE, bytepos, ZV_BYTE);
+  set_point_both (charpos, bytepos);
 }
 
 /* If there's an invisible character at position POS + TEST_OFFS in the

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > If you want to ignore the marker's buffer you can do (goto-char
 > (marker-position marker)); if you pass the whole marker object, it
 > makes sense that the buffer is checked.

It's pure cosmetics.  With emacs -Q evaluate:

(let ((marker-1 (make-marker))
       (marker-2 (make-marker)))
   (describe-function 'describe-function)
   (with-current-buffer "*Help*"
     (set-marker marker-1 (point-max)))
   (set-marker marker-2 marker-1)
   (goto-char marker-2))

Do you see what I mean?

martin

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Tue, 6 Sep 2016 10:50:49 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, 24368@debbugs.gnu.org
> 
> > IMHO
> > ‘goto-char’ should not signal an error in that case - it's perfectly
> > legitimate to use the position from another buffer's marker as value.
> 
> If you want to ignore the marker's buffer you can do (goto-char
> (marker-position marker)); if you pass the whole marker object, it
> makes sense that the buffer is checked.

The error doesn't come from got-char, it comes from a utility function
it calls.  So this isn't an application-level issue, and cannot be
solved by a specially-formatted call to got-char.

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > On second thought, I don't think attach_marker should be touched at
 > all.  It is too low-level, and the assertions compile to nothing in a
 > build without --enable-checking, so we will be left without any
 > protection in the production version.

Right.  The init_iterator problem cannot be solved this way.

 > Instead, I think the callers of attach_marker should make sure charpos
 > and bytepos come from the same buffer, and are therefore in sync.  The
 > patch below implements that; WDYT?

It fixes all issues I've encountered so far ;-)

 > I agree that erroring out of goto-char in this case is sub-optimal at
 > best.

Could we drop that restriction with your changes?  I'd rather have this
handled safely at one place only.

 > I chose neither ;-)  The patch below seems to fix all of your test
 > cases, including the original problem with window-state-get, even
 > before it was fixed.  It also lifts the restriction on goto-char.
 >
 > Comments?

Install!

 > P.S. After fixing the crashes and correcting window--state-get-1,
 > running your original test case causes "C-x 1" stop working: it has no
 > effect.  Looks like this is because Emacs thinks the window is the
 > major non-side window, although actually I have *scratch* and *Help*,
 > so the command ought to work.  Is this related or unrelated to the
 > issue at hand?

I'm not sure what you mean with "stop working".  The idea is that C-x 1
when invoked in *scratch* should leave the *Help* window alone.  If you
split *scratch* via C-x 2 and after that you do C-x 1, only the other
*scratch* window should get deleted.  C-x 1 invoked in *Help* should
cause an error.

I'm currently trying to fix errors and write documentation on side
windows - that's how I stumbled into this marker issue in the first
place.

martin

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Tue, 06 Sep 2016 18:09:58 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
>  > I agree that erroring out of goto-char in this case is sub-optimal at
>  > best.
> 
> Could we drop that restriction with your changes?  I'd rather have this
> handled safely at one place only.

The patch fixes that as well, didn't you notice?  Or am I missing
something?

>  > I chose neither ;-)  The patch below seems to fix all of your test
>  > cases, including the original problem with window-state-get, even
>  > before it was fixed.  It also lifts the restriction on goto-char.
>  >
>  > Comments?
> 
> Install!

Will do.

>  > P.S. After fixing the crashes and correcting window--state-get-1,
>  > running your original test case causes "C-x 1" stop working: it has no
>  > effect.  Looks like this is because Emacs thinks the window is the
>  > major non-side window, although actually I have *scratch* and *Help*,
>  > so the command ought to work.  Is this related or unrelated to the
>  > issue at hand?
> 
> I'm not sure what you mean with "stop working".  The idea is that C-x 1
> when invoked in *scratch* should leave the *Help* window alone.

Really?  Isn't that confusing to do that silently?  It confused me,
for one.

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Tue, 06 Sep 2016 19:39:45 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 24368@debbugs.gnu.org
> 
> >  > I chose neither ;-)  The patch below seems to fix all of your test
> >  > cases, including the original problem with window-state-get, even
> >  > before it was fixed.  It also lifts the restriction on goto-char.
> >  >
> >  > Comments?
> > 
> > Install!
> 
> Will do.

Done.  Please close the bug, if there are no issues left.

Btw, it would be good to have all your test cases in the test suite.

Thanks.

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 >>   > I agree that erroring out of goto-char in this case is sub-optimal at
 >>   > best.
 >>
 >> Could we drop that restriction with your changes?  I'd rather have this
 >> handled safely at one place only.
 >
 > The patch fixes that as well, didn't you notice?  Or am I missing
 > something?

It's perfect.

 >>   > P.S. After fixing the crashes and correcting window--state-get-1,
 >>   > running your original test case causes "C-x 1" stop working: it has no
 >>   > effect.  Looks like this is because Emacs thinks the window is the
 >>   > major non-side window, although actually I have *scratch* and *Help*,
 >>   > so the command ought to work.  Is this related or unrelated to the
 >>   > issue at hand?
 >>
 >> I'm not sure what you mean with "stop working".  The idea is that C-x 1
 >> when invoked in *scratch* should leave the *Help* window alone.
 >
 > Really?  Isn't that confusing to do that silently?  It confused me,
 > for one.

Actually, it's the only standard CEDET/ECB specific feature I
implemented by default.  With ECB, windows outside the editing area are
persistent, you can't even delete them via ‘delete-window’ IIRC.  This,
or having ‘other-window’ skip side windows, must be specified explicitly
with the side windows approach.

I'll add an option to make this easily customizable.

martin

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

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

 > Done.  Please close the bug, if there are no issues left.

I still intend to apply the change to ‘window-state-get-1’ (it simply
looks bad when looking at the produced state to see the wrong buffers).
After that I will close the bug.

 > Btw, it would be good to have all your test cases in the test suite.

I've never written anything for the test suite.  Would the attached file
fit?

Thanks, martin

[-- Attachment #2: marker-tests.el --]
[-- Type: application/emacs-lisp, Size: 2240 bytes --]

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Wed, 07 Sep 2016 16:52:05 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
>  > Btw, it would be good to have all your test cases in the test suite.
> 
> I've never written anything for the test suite.  Would the attached file
> fit?

Yes, LGTM.  Thanks.

One comment: to make the tests future-proof, bind test-quoting-style
to a specific value that will cause the *Help* buffer have the
non-ASCII quotes.

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 > One comment: to make the tests future-proof, bind test-quoting-style
 > to a specific value that will cause the *Help* buffer have the
 > non-ASCII quotes.

Like

   (let ((text-quoting-style 'curve))
     (describe-function 'describe-function))

?

martin

bug#24368: 25.1; Assertion failure in attach_marker

[Eli Zaretskii]

> Date: Wed, 07 Sep 2016 17:19:02 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 24368@debbugs.gnu.org
> 
>  > One comment: to make the tests future-proof, bind test-quoting-style
>  > to a specific value that will cause the *Help* buffer have the
>  > non-ASCII quotes.
> 
> Like
> 
>    (let ((text-quoting-style 'curve))
>      (describe-function 'describe-function))
> 
> ?

Yes, exactly.

bug#24368: 25.1; Assertion failure in attach_marker

[martin rudalics]

 >  > Done.  Please close the bug, if there are no issues left.
 >
 > I still intend to apply the change to ‘window-state-get-1’ (it simply
 > looks bad when looking at the produced state to see the wrong buffers).
 > After that I will close the bug.

No issues left from my part, bug closed.

martin