Re: Edebug corrupting point in buffers; we need buffer-point and set-buffer-point, perhaps.

[Alan Mackenzie <acm@muc.de>]

Hello, Emacs.

On Mon, Oct 31, 2022 at 11:43:15 +0000, Alan Mackenzie wrote:
> A few weeks ago, I was attempting to edebug a program which itself
> scanned through a buffer B.  That buffer was also displayed in a window
> (or possibly two windows).  Each time the program went into edebug, the
> point in B got corrupted.

> Setting edebug-save-displayed-buffer-points didn't help.

> I now understand what was going wrong.  Without
> edebug-save-displayed-buffer-points, B's point got set to a window point
> from a random window displaying B, caused by a set-window-configuration
> call from edebug.

> With edebug-save-displayed-buffer-points set, the function
> edebug-get-displayed-buffer points was recording window-points for each
> displayed window, but after the recursive edit was writing these
> window-points back into the buffer points.  This is surely a bug.  At the
> least, there is a race condition if two windows display the same buffer.

> In neither of the above scenarios does edebug do anything to restore the
> buffer points after the recursive edit.  This seems to be a bad thing.

> I propose that edebug should get an option to preserve buffer points,
> alongside the existing options which preserve window points.

[ .... ]

I now have a patch which aims to fix the above problem.  To use it,
select a window containing the buffer the function under test will be
scanning (or otherwise manipulating).  Type C-x X F to add that buffer's
name to edebug-save-buffer-points.

During the current or future edebug session, this buffer will now be
protected from having its buffer point corrupted by edebug's other
mechanisms which manipulate window-points.

In fact, I'm not sure that edebug-save-displayed-buffer-points isn't
obsolete - it dates from 1994 (or earlier), a time before there were
frames or window configurations.  It seems to me its function is covered
by the later edebug-save-windows.

Anyhow, here's the patch.  What do you think?



diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 67704bdb51..c4099164bc 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -157,6 +157,23 @@ edebug-save-displayed-buffer-points
 need it."
   :type 'boolean)
 
+(defcustom edebug-save-buffer-points nil
+  "If non-nil save and restore the buffer points in some buffers.
+
+Saving and restoring the buffer point in a buffer is needed if you
+are debugging code which sets point in that buffer, particularly if
+there is also a window displaying that buffer.  Otherwise the buffer
+point (being used by the program) will get overwritten by the
+window point.
+
+If the value is a list of buffer names (recommended), only those
+buffers will have their buffer points restored.  Otherwise, t means
+restore all buffers\\=' points, and nil means none.
+
+A buffer\\='s name can be added to or removed from this list
+dynamically by the key binding \\[edebug-toggle-save-buffer-points]."
+  :type '(choice boolean (repeat string)))
+
 (defcustom edebug-initial-mode 'step
   "Initial execution mode for Edebug, if non-nil.
 If this variable is non-nil, it specifies the initial execution mode
@@ -401,6 +420,32 @@ edebug-set-buffer-points
 		(goto-char (cdr buf-point))))
 	    buffer-points)))
 
+(defun edebug-get-all-buffer-points ()
+  ;; Return an alist of (BUFFER . BUFFER-POINT) for all buffers.
+  (let (bp-list)
+    (dolist (buf (buffer-list))
+      (with-current-buffer buf
+        (push (cons buf (point)) bp-list)))
+    bp-list))
+
+(defun edebug-restore-buffer-points (buffer-points)
+  ;; Restore (some) buffer-points from the argument, an alist with
+  ;; elements of the form (BUFFER . BUFFER-POINT).
+  (cond
+   ((eq edebug-save-buffer-points t)
+    (dolist (elt buffer-points)
+      (unless (eq (car elt) (current-buffer))
+        (with-current-buffer (car elt)
+          (goto-char (cdr elt))))))
+   ((listp edebug-save-buffer-points)
+    (dolist (buf edebug-save-buffer-points)
+      (let ((elt (assq buf buffer-points)))
+        (when elt                       ; ?Always non-nil
+          (unless (eq (car elt) (current-buffer))
+            (with-current-buffer (car elt)
+              (goto-char (cdr elt))))))))
+   (t nil)))
+
 (defun edebug-current-windows (which-windows)
   ;; Get either a full window configuration or some window information.
   (if (listp which-windows)
@@ -2600,6 +2647,8 @@ edebug--display-1
         (edebug-outside-mark (mark t))
 	edebug-outside-windows		; Window or screen configuration.
 	edebug-buffer-points
+        edebug-all-buffer-points
+
 
 	edebug-eval-buffer             ; Declared here so we can kill it below.
 	(eval-result-list (and edebug-eval-list
@@ -2627,7 +2678,9 @@ edebug--display-1
 	      (setq edebug-outside-windows
 		    (edebug-current-windows edebug-save-windows)))
 
-	  (if edebug-save-displayed-buffer-points
+          (setq edebug-all-buffer-points (edebug-get-all-buffer-points))
+
+          (if edebug-save-displayed-buffer-points
 	      (setq edebug-buffer-points (edebug-get-displayed-buffer-points)))
 
 	  ;; First move the edebug buffer point to edebug-point
@@ -2759,6 +2814,11 @@ edebug--display-1
                     (if edebug-save-displayed-buffer-points
                         (edebug-set-buffer-points edebug-buffer-points))
 
+                    ;; Restore (non-displayed) buffer points.
+                    (if edebug-save-buffer-points
+                        (edebug-restore-buffer-points
+                         edebug-all-buffer-points))
+
                     ;; Unrestore trace window's window-point.
                     (if edebug-trace-window
                         (set-window-start edebug-trace-window
@@ -3013,6 +3075,26 @@ edebug-toggle-save-windows
       (edebug-toggle-save-selected-window)
     (edebug-toggle-save-all-windows)))
 
+(defun edebug-toggle-save-buffer-points (arg)
+  "Toggle the saving and restoring of a buffer\\='s buffer point.
+That buffer is the one in the selected window.  With a prefix
+argument, switch the setting on or off for all buffers."
+  (interactive "P")
+  (let* ((cur-win-buf (window-buffer (selected-window)))
+         (win-buf-name (buffer-name cur-win-buf)))
+    (if arg
+        (setq edebug-save-buffer-points (not edebug-save-buffer-points))
+      (cond
+       ((eq t edebug-save-buffer-points)
+        ;; Save all buffers except the one in the current window.
+        (setq edebug-save-buffer-points
+              (mapcar #'buffer-name
+                      (delq cur-win-buf (buffer-list)))))
+       ((memq win-buf-name edebug-save-buffer-points)
+        (setq edebug-save-buffer-points
+              (delq (buffer-name cur-win-buf) edebug-save-buffer-points)))
+       (t (push win-buf-name edebug-save-buffer-points))))))
+
 (defun edebug-where ()
   "Show the debug windows and where we stopped in the program."
   (interactive)
@@ -3850,6 +3934,7 @@ edebug-mode-map
   "p"       #'edebug-bounce-point
   "P"       #'edebug-view-outside        ; same as v
   "W"       #'edebug-toggle-save-windows
+  ;; "F"       #'edebug-toggle-save-buffer-points
 
   ;; misc
   "?"       #'edebug-help
@@ -3904,6 +3991,7 @@ edebug-global-map
   ;; views
   "w"   #'edebug-where
   "W"   #'edebug-toggle-save-windows
+  "F"   #'edebug-toggle-save-buffer-points
 
   ;; quitting
   "q"   #'top-level


> -- 
> Alan Mackenzie (Nuremberg, Germany).