Backtrace printing in batch mode ignores all customizations

Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

Apparently it is because of this commit:

7228488effa78dcb75284cb6d247b24804e0e7f5
Author:     Stefan Monnier <monnier@iro.umontreal.ca>
AuthorDate: 2018-04-02 00:23:20 -0400
[...]

* lisp/emacs-lisp/debug.el (debug): Don't hang upon error in initial-frame.

[...]
+   ((and (eq t (framep (selected-frame)))
+         (equal "initial_terminal" (terminal-name)))
+    ;; We're in the initial-frame (where `message' just outputs to stdout) so
+    ;; there's no tty or GUI frame to display the backtrace and interact with
+    ;; it: just dump a backtrace to stdout.
+    ;; This happens for example while handling an error in code from
+    ;; early-init.el with --debug-init.
+    (message "Error: %S" args)
[...]

The condition seems to always be true when `noninteractive' is t. Is
this intentional? It appears to mean that all debug/backtrace
customizations in batch mode are meaningless, since debugger just goes
the "failsafe" route and prints backtrace with simple `message' calls
now.

Please CC me, I'm not subscribed.

Paul

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Mon, 16 Dec 2019 23:34:42 +0100
> 
> +   ((and (eq t (framep (selected-frame)))
> +         (equal "initial_terminal" (terminal-name)))
> +    ;; We're in the initial-frame (where `message' just outputs to stdout) so
> +    ;; there's no tty or GUI frame to display the backtrace and interact with
> +    ;; it: just dump a backtrace to stdout.
> +    ;; This happens for example while handling an error in code from
> +    ;; early-init.el with --debug-init.
> +    (message "Error: %S" args)
> [...]
> 
> The condition seems to always be true when `noninteractive' is t. Is
> this intentional? It appears to mean that all debug/backtrace
> customizations in batch mode are meaningless, since debugger just goes
> the "failsafe" route and prints backtrace with simple `message' calls
> now.

Can you please elaborate about the debug/backtrace customizations you
did, which are bypassed due to this code?  What are those
customizations supposed to do?

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

It's not really about _my_ customizations. Basically, this "failsafe"
mode in `debug' bypasses even e.g. built-in Emacs
`debugger-print-function'.

Here is an example (it has nothing to do with `registry', I just
needed some EIEIO class to illustrate):

$ emacs -Q --batch --eval "(let ((debug-on-error t)) (require
'registry) (defun foo (x) bar) (foo (registry-db)))"
Error: (error (void-variable bar))
 (foo #s(registry-db unbound nil 2305843009213693951 0.1 nil nil
#s(hash-table size 100 test eql rehash-size 2.0 rehash-threshold
0.8125 data ( ...)) #s(hash-table size 10000 test equal rehash-size
2.0 rehash-threshold 0.8125 data ( ...))))
 (let ((debug-on-error t)) (require 'registry) (defun foo (x) bar)
(foo (registry-db)))
 (eval (let ((debug-on-error t)) (require 'registry) (defun foo (x)
bar) (foo (registry-db))) t)
 (command-line-1 ("--eval" "(let ((debug-on-error t)) (require
'registry) (defun foo (x) bar) (foo (registry-db)))"))
 (command-line)
 (normal-top-level)

And now with a workaround that disables this "failsafe" mode:

$ emacs -Q --batch --eval "(advice-add 'terminal-name :override
(lambda () \"workaround\"))" --eval "(let ((debug-on-error t))
(require 'registry) (defun foo (x) bar) (foo (registry-db)))"
Debugger entered--Lisp error: (void-variable bar)
 foo(#<registry-db registry-db-1575c8f95a00>)
 (let ((debug-on-error t)) (require 'registry) (defun foo (x) bar)
(foo (registry-db)))
 eval((let ((debug-on-error t)) (require 'registry) (defun foo (x)
bar) (foo (registry-db))) t)
 command-line-1(("--eval" "(advice-add 'terminal-name :override
(lambda () \"w..." "--eval" "(let ((debug-on-error t)) (require
'registry) (def..."))
 command-line()
 normal-top-level()

Without the workaround, `debugger-print-function' (i.e. `cl-prin1') is
never called and in the second line of output you can see
"#s(registry-db ..."). With the workaround `cl-prin1' is called where
it should be, and you can see "#<registry-db ..." instead.

I'm not sure if the "failsafe" mode is meant to operate in batch mode
or not: I traced it down to the commit where it was added, but the
commit doesn't state the reason for the change. If "failsafe" is only
meant for when exceptions happen in normal mode before Emacs is
properly initialized, then adding "(not noninteractive)" to its
condition should be enough. If it is actually meant (also) for batch
mode, it should work more like the "normal" debugging backtrace
generating code, so that exceptions you see inside normal Emacs and
when printed from batch mode look the same.

Paul


On Sat, 21 Dec 2019 at 10:22, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Mon, 16 Dec 2019 23:34:42 +0100
> >
> > +   ((and (eq t (framep (selected-frame)))
> > +         (equal "initial_terminal" (terminal-name)))
> > +    ;; We're in the initial-frame (where `message' just outputs to stdout) so
> > +    ;; there's no tty or GUI frame to display the backtrace and interact with
> > +    ;; it: just dump a backtrace to stdout.
> > +    ;; This happens for example while handling an error in code from
> > +    ;; early-init.el with --debug-init.
> > +    (message "Error: %S" args)
> > [...]
> >
> > The condition seems to always be true when `noninteractive' is t. Is
> > this intentional? It appears to mean that all debug/backtrace
> > customizations in batch mode are meaningless, since debugger just goes
> > the "failsafe" route and prints backtrace with simple `message' calls
> > now.
>
> Can you please elaborate about the debug/backtrace customizations you
> did, which are bypassed due to this code?  What are those
> customizations supposed to do?

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> +    ;; We're in the initial-frame (where `message' just outputs to stdout) so
> +    ;; there's no tty or GUI frame to display the backtrace and interact with
> +    ;; it: just dump a backtrace to stdout.
> +    ;; This happens for example while handling an error in code from
> +    ;; early-init.el with --debug-init.
> +    (message "Error: %S" args)
> [...]
>
> The condition seems to always be true when `noninteractive' is t.

The `noninteractive` case is indeed not the one for which this code
was added.  I guess we can conditionalize it on `noninteractive`, but it
would be worthwhile looking at the code which causes the
`noninteractive` case (without the above "failsafe") to dump
a stacktrace on stderr, to see if the two could be unified?


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

I submitted a proposed patch as bug #38927
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38927) a few hours ago
since I thought this thread got ignored. It wasn't enough to just
decouple non-interactive from the "failsafe": it got rusty in the
meanwhile and no longer worked after just adding "(not
noninteractive)" condition. In the patch I didn't try to unify your
"failsafe" mode with non-interactive use, since I don't really know
what you wrote the commit for.

But yes, it does look logical to unify the two cases. I would try
removing the failsafe mode altogether, add sth. like

    (let ((backtrace-to-stderr (or noninteractive
[current-failsafe-condition]))) ...

and replace the two uses of `noninteractive' later with this new
variable. But I don't really know how to test this.

Paul

On Sat, 4 Jan 2020 at 23:44, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > +    ;; We're in the initial-frame (where `message' just outputs to stdout) so
> > +    ;; there's no tty or GUI frame to display the backtrace and interact with
> > +    ;; it: just dump a backtrace to stdout.
> > +    ;; This happens for example while handling an error in code from
> > +    ;; early-init.el with --debug-init.
> > +    (message "Error: %S" args)
> > [...]
> >
> > The condition seems to always be true when `noninteractive' is t.
>
> The `noninteractive` case is indeed not the one for which this code
> was added.  I guess we can conditionalize it on `noninteractive`, but it
> would be worthwhile looking at the code which causes the
> `noninteractive` case (without the above "failsafe") to dump
> a stacktrace on stderr, to see if the two could be unified?
>
>
>         Stefan
>

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> noninteractive)" condition. In the patch I didn't try to unify your
> "failsafe" mode with non-interactive use, since I don't really know
> what you wrote the commit for.

The commit was specifically prompted by the case mentioned in the comment:

      ;; This happens for example while handling an error in code from
      ;; early-init.el with --debug-init.

I can't remember the exact previous behavior, but it was not right (you
got neither a good *backtrace* because the GUI wasn't up, nor a clean
stacktrace on stdout/stderr).


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

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

Here is a proposed patch that unifies the two print-backtrace-and-quit
cases (batch mode and the failsafe). The patch is large mostly because
of required reindenting, in fact only 15-20 lines are changed. I
tested that it works as expected in batch mode, but don't know how to
test the failsafe, I never did this. Please test yourself. I also
verified that it doesn't change the normal (interactive) backtrace.

How I tested in batch mode:

$ emacs --batch --eval "(let ((debug-on-error t)) (require 'registry)
(defun bar () (bar)) (defun foo (x) (bar)) (foo (registry-db)))"

- must use `cl-prin1' for the dummy registry object;
- must properly cut out the middle from the intentionally very long backtrace.

I don't have access to Emacs repository, so I cannot apply it myself.
I can only write emails and hope that someone applies the patches or
otherwise does something about the problem.

Paul

On Sun, 5 Jan 2020 at 00:31, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > noninteractive)" condition. In the patch I didn't try to unify your
> > "failsafe" mode with non-interactive use, since I don't really know
> > what you wrote the commit for.
>
> The commit was specifically prompted by the case mentioned in the comment:
>
>       ;; This happens for example while handling an error in code from
>       ;; early-init.el with --debug-init.
>
> I can't remember the exact previous behavior, but it was not right (you
> got neither a good *backtrace* because the GUI wasn't up, nor a clean
> stacktrace on stdout/stderr).
>
>
>         Stefan
>

[-- Attachment #2: 0001-Reuse-backtrace-formatting-improvements-in-batch-mod.patch --]
[-- Type: text/x-patch, Size: 13838 bytes --]

From b845aa80815b3b413822a16e688343ef9236a4df Mon Sep 17 00:00:00 2001
From: Paul Pogonyshev <pogonyshev@gmail.com>
Date: Sun, 12 Jan 2020 16:37:15 +0100
Subject: [PATCH] Reuse backtrace formatting improvements in batch mode
 (bug#38927)

---
 lisp/emacs-lisp/debug.el | 291 +++++++++++++++++++--------------------
 1 file changed, 141 insertions(+), 150 deletions(-)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index f67aa89728..2e7f7d7e2b 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -168,158 +168,149 @@ debug
 If `inhibit-redisplay' is non-nil when this function is called,
 the debugger will not be entered."
   (interactive)
-  (cond
-   (inhibit-redisplay
-    ;; Don't really try to enter debugger within an eval from redisplay.
-    debugger-value)
-   ((and (eq t (framep (selected-frame)))
-         (equal "initial_terminal" (terminal-name)))
-    ;; We're in the initial-frame (where `message' just outputs to stdout) so
-    ;; there's no tty or GUI frame to display the backtrace and interact with
-    ;; it: just dump a backtrace to stdout.
-    ;; This happens for example while handling an error in code from
-    ;; early-init.el with --debug-init.
-    (message "Error: %S" args)
-    (let ((print-escape-newlines t)
-          (print-escape-control-characters t)
-          (print-level 8)
-          (print-length 50)
-          (skip t))             ;Skip the first frame (i.e. the `debug' frame)!
-      (mapbacktrace (lambda (_evald func args _flags)
-                      (if skip
-                          (setq skip nil)
-                        (message "  %S" (cons func args))))
-                    'debug)))
-   (t
-    (unless noninteractive
-      (message "Entering debugger..."))
-    (let (debugger-value
-	  (debugger-previous-state
-           (if (get-buffer "*Backtrace*")
-               (with-current-buffer (get-buffer "*Backtrace*")
-                 (debugger--save-buffer-state))))
-          (debugger-args args)
-	  (debugger-buffer (get-buffer-create "*Backtrace*"))
-	  (debugger-old-buffer (current-buffer))
-	  (debugger-window nil)
-	  (debugger-step-after-exit nil)
-          (debugger-will-be-back nil)
-	  ;; Don't keep reading from an executing kbd macro!
-	  (executing-kbd-macro nil)
-	  ;; Save the outer values of these vars for the `e' command
-	  ;; before we replace the values.
-	  (debugger-outer-match-data (match-data))
-	  (debugger-with-timeout-suspend (with-timeout-suspend)))
-      ;; Set this instead of binding it, so that `q'
-      ;; will not restore it.
-      (setq overriding-terminal-local-map nil)
-      ;; Don't let these magic variables affect the debugger itself.
-      (let ((last-command nil) this-command track-mouse
-	    (inhibit-trace t)
-	    unread-command-events
-	    unread-post-input-method-events
-	    last-input-event last-command-event last-nonmenu-event
-	    last-event-frame
-	    overriding-local-map
-	    load-read-function
-	    ;; If we are inside a minibuffer, allow nesting
-	    ;; so that we don't get an error from the `e' command.
-	    (enable-recursive-minibuffers
-	     (or enable-recursive-minibuffers (> (minibuffer-depth) 0)))
-	    (standard-input t) (standard-output t)
-	    inhibit-redisplay
-	    (cursor-in-echo-area nil)
-	    (window-configuration (current-window-configuration)))
-	(unwind-protect
-	    (save-excursion
-	      (when (eq (car debugger-args) 'debug)
-		;; Skip the frames for backtrace-debug, byte-code,
-		;; debug--implement-debug-on-entry and the advice's `apply'.
-		(backtrace-debug 4 t)
-		;; Place an extra debug-on-exit for macro's.
-		(when (eq 'lambda (car-safe (cadr (backtrace-frame 4))))
-		  (backtrace-debug 5 t)))
-              (with-current-buffer debugger-buffer
-                (unless (derived-mode-p 'debugger-mode)
-	          (debugger-mode))
-	        (debugger-setup-buffer debugger-args)
-	        (when noninteractive
-		  ;; If the backtrace is long, save the beginning
-		  ;; and the end, but discard the middle.
-		  (when (> (count-lines (point-min) (point-max))
-			   debugger-batch-max-lines)
-		    (goto-char (point-min))
-		    (forward-line (/ 2 debugger-batch-max-lines))
-		    (let ((middlestart (point)))
-		      (goto-char (point-max))
-		      (forward-line (- (/ 2 debugger-batch-max-lines)
-				       debugger-batch-max-lines))
-		      (delete-region middlestart (point)))
-		    (insert "...\n"))
-		  (goto-char (point-min))
-		  (message "%s" (buffer-string))
-		  (kill-emacs -1)))
-	      (pop-to-buffer
-	       debugger-buffer
-	       `((display-buffer-reuse-window
-		  display-buffer-in-previous-window
-		  display-buffer-below-selected)
-		 . ((window-min-height . 10)
-                    (window-height . fit-window-to-buffer)
-		    ,@(when (and (window-live-p debugger-previous-window)
-				 (frame-visible-p
-				  (window-frame debugger-previous-window)))
-		        `((previous-window . ,debugger-previous-window))))))
-	      (setq debugger-window (selected-window))
-	      (if (eq debugger-previous-window debugger-window)
-		  (when debugger-jumping-flag
-		    ;; Try to restore previous height of debugger
-		    ;; window.
-		    (condition-case nil
-			(window-resize
-			 debugger-window
-			 (- debugger-previous-window-height
-			    (window-total-height debugger-window)))
-		      (error nil)))
-		(setq debugger-previous-window debugger-window))
-	      (message "")
-	      (let ((standard-output nil)
-		    (buffer-read-only t))
-		(message "")
-		;; Make sure we unbind buffer-read-only in the right buffer.
-		(save-excursion
-		  (recursive-edit))))
-	  (when (and (window-live-p debugger-window)
-		     (eq (window-buffer debugger-window) debugger-buffer))
-	    ;; Record height of debugger window.
-	    (setq debugger-previous-window-height
-		  (window-total-height debugger-window)))
-	  (if debugger-will-be-back
-	      ;; Restore previous window configuration (Bug#12623).
-	      (set-window-configuration window-configuration)
+  (if inhibit-redisplay
+      ;; Don't really try to enter debugger within an eval from redisplay.
+      debugger-value
+    (let ((only-backtrace (or noninteractive
+                              ;; If we're in the initial-frame (where `message' just
+                              ;; outputs to stdout) so there's no tty or GUI frame to
+                              ;; display the backtrace and interact with it: just dump a
+                              ;; backtrace to stdout.  This happens for example while
+                              ;; handling an error in code from early-init.el with
+                              ;; --debug-init.
+                              (and (eq t (framep (selected-frame)))
+                                   (equal "initial_terminal" (terminal-name)))))
+          ;; Don't let `inhibit-message' get in our way (especially important if
+          ;; `only-backtrace' evaluated to a non-nil value.
+          (inhibit-message nil))
+      (unless only-backtrace
+        (message "Entering debugger..."))
+      (let (debugger-value
+	    (debugger-previous-state
+             (if (get-buffer "*Backtrace*")
+                 (with-current-buffer (get-buffer "*Backtrace*")
+                   (debugger--save-buffer-state))))
+            (debugger-args args)
+	    (debugger-buffer (get-buffer-create "*Backtrace*"))
+	    (debugger-old-buffer (current-buffer))
+	    (debugger-window nil)
+	    (debugger-step-after-exit nil)
+            (debugger-will-be-back nil)
+	    ;; Don't keep reading from an executing kbd macro!
+	    (executing-kbd-macro nil)
+	    ;; Save the outer values of these vars for the `e' command
+	    ;; before we replace the values.
+	    (debugger-outer-match-data (match-data))
+	    (debugger-with-timeout-suspend (with-timeout-suspend)))
+        ;; Set this instead of binding it, so that `q'
+        ;; will not restore it.
+        (setq overriding-terminal-local-map nil)
+        ;; Don't let these magic variables affect the debugger itself.
+        (let ((last-command nil) this-command track-mouse
+	      (inhibit-trace t)
+	      unread-command-events
+	      unread-post-input-method-events
+	      last-input-event last-command-event last-nonmenu-event
+	      last-event-frame
+	      overriding-local-map
+	      load-read-function
+	      ;; If we are inside a minibuffer, allow nesting
+	      ;; so that we don't get an error from the `e' command.
+	      (enable-recursive-minibuffers
+	       (or enable-recursive-minibuffers (> (minibuffer-depth) 0)))
+	      (standard-input t) (standard-output t)
+	      inhibit-redisplay
+	      (cursor-in-echo-area nil)
+	      (window-configuration (current-window-configuration)))
+	  (unwind-protect
+	      (save-excursion
+	        (when (eq (car debugger-args) 'debug)
+		  ;; Skip the frames for backtrace-debug, byte-code,
+		  ;; debug--implement-debug-on-entry and the advice's `apply'.
+		  (backtrace-debug 4 t)
+		  ;; Place an extra debug-on-exit for macro's.
+		  (when (eq 'lambda (car-safe (cadr (backtrace-frame 4))))
+		    (backtrace-debug 5 t)))
+                (with-current-buffer debugger-buffer
+                  (unless (derived-mode-p 'debugger-mode)
+	            (debugger-mode))
+	          (debugger-setup-buffer debugger-args)
+	          (when only-backtrace
+		    ;; If the backtrace is long, save the beginning
+		    ;; and the end, but discard the middle.
+                    (let ((inhibit-read-only t))
+		      (when (> (count-lines (point-min) (point-max))
+			       debugger-batch-max-lines)
+		        (goto-char (point-min))
+		        (forward-line (/ debugger-batch-max-lines 2))
+		        (let ((middlestart (point)))
+		          (goto-char (point-max))
+		          (forward-line (- (/ debugger-batch-max-lines 2)))
+		          (delete-region middlestart (point)))
+		        (insert "...\n")))
+		    (message "%s" (buffer-string))
+		    (kill-emacs -1)))
+	        (pop-to-buffer
+	         debugger-buffer
+	         `((display-buffer-reuse-window
+		    display-buffer-in-previous-window
+		    display-buffer-below-selected)
+		   . ((window-min-height . 10)
+                      (window-height . fit-window-to-buffer)
+		      ,@(when (and (window-live-p debugger-previous-window)
+				   (frame-visible-p
+				    (window-frame debugger-previous-window)))
+		          `((previous-window . ,debugger-previous-window))))))
+	        (setq debugger-window (selected-window))
+	        (if (eq debugger-previous-window debugger-window)
+		    (when debugger-jumping-flag
+		      ;; Try to restore previous height of debugger
+		      ;; window.
+		      (condition-case nil
+			  (window-resize
+			   debugger-window
+			   (- debugger-previous-window-height
+			      (window-total-height debugger-window)))
+		        (error nil)))
+		  (setq debugger-previous-window debugger-window))
+	        (message "")
+	        (let ((standard-output nil)
+		      (buffer-read-only t))
+		  (message "")
+		  ;; Make sure we unbind buffer-read-only in the right buffer.
+		  (save-excursion
+		    (recursive-edit))))
 	    (when (and (window-live-p debugger-window)
 		       (eq (window-buffer debugger-window) debugger-buffer))
-	      (progn
-		;; Unshow debugger-buffer.
-		(quit-restore-window debugger-window debugger-bury-or-kill)
-		;; Restore current buffer (Bug#12502).
-		(set-buffer debugger-old-buffer)))
-            ;; Forget debugger window, it won't be back (Bug#17882).
-            (setq debugger-previous-window nil))
-          ;; Restore previous state of debugger-buffer in case we were
-          ;; in a recursive invocation of the debugger, otherwise just
-          ;; erase the buffer.
-	  (when (buffer-live-p debugger-buffer)
-	    (with-current-buffer debugger-buffer
-	      (if debugger-previous-state
-                  (debugger--restore-buffer-state debugger-previous-state)
-                (setq backtrace-insert-header-function nil)
-                (setq backtrace-frames nil)
-                (backtrace-print))))
-	  (with-timeout-unsuspend debugger-with-timeout-suspend)
-	  (set-match-data debugger-outer-match-data)))
-      (setq debug-on-next-call debugger-step-after-exit)
-      debugger-value))))
+	      ;; Record height of debugger window.
+	      (setq debugger-previous-window-height
+		    (window-total-height debugger-window)))
+	    (if debugger-will-be-back
+	        ;; Restore previous window configuration (Bug#12623).
+	        (set-window-configuration window-configuration)
+	      (when (and (window-live-p debugger-window)
+		         (eq (window-buffer debugger-window) debugger-buffer))
+	        (progn
+		  ;; Unshow debugger-buffer.
+		  (quit-restore-window debugger-window debugger-bury-or-kill)
+		  ;; Restore current buffer (Bug#12502).
+		  (set-buffer debugger-old-buffer)))
+              ;; Forget debugger window, it won't be back (Bug#17882).
+              (setq debugger-previous-window nil))
+            ;; Restore previous state of debugger-buffer in case we were
+            ;; in a recursive invocation of the debugger, otherwise just
+            ;; erase the buffer.
+	    (when (buffer-live-p debugger-buffer)
+	      (with-current-buffer debugger-buffer
+	        (if debugger-previous-state
+                    (debugger--restore-buffer-state debugger-previous-state)
+                  (setq backtrace-insert-header-function nil)
+                  (setq backtrace-frames nil)
+                  (backtrace-print))))
+	    (with-timeout-unsuspend debugger-with-timeout-suspend)
+	    (set-match-data debugger-outer-match-data)))
+        (setq debug-on-next-call debugger-step-after-exit)
+        debugger-value))))
 \f
 (defun debugger--print (obj &optional stream)
   (condition-case err
-- 
2.20.1

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

Paul writes:
> Here is a proposed patch that unifies the two print-backtrace-and-quit
> cases (batch mode and the failsafe). The patch is large mostly because
> of required reindenting, in fact only 15-20 lines are changed.

Looks good to me, thank you for doing it.
Eli, do you think this can go into `emacs-27`?

> I tested that it works as expected in batch mode, but don't know how to
> test the failsafe, I never did this.

Create a dummy ~/.emacs.d/early-init.el where you set `debug-on-error`
and then signal an error.

> Please test yourself.

Seems to work fine in my test, thank you.

> I don't have access to Emacs repository, so I cannot apply it myself.

[ Anything we can do to help with that?  ]

> I can only write emails and hope that someone applies the patches or
> otherwise does something about the problem.

I'll be happy to install as soon as Eli tells me where.


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Paul Pogonyshev <pogonyshev@gmail.com>, Emacs developers
>  <emacs-devel@gnu.org>
> Date: Mon, 13 Jan 2020 14:23:09 -0500
> 
> Paul writes:
> > Here is a proposed patch that unifies the two print-backtrace-and-quit
> > cases (batch mode and the failsafe). The patch is large mostly because
> > of required reindenting, in fact only 15-20 lines are changed.
> 
> Looks good to me, thank you for doing it.
> Eli, do you think this can go into `emacs-27`?

Yes, I think so.  But I have a question and a request:

> +    (let ((only-backtrace (or noninteractive
> +                              ;; If we're in the initial-frame (where `message' just
> +                              ;; outputs to stdout) so there's no tty or GUI frame to
> +                              ;; display the backtrace and interact with it: just dump a
> +                              ;; backtrace to stdout.  This happens for example while
> +                              ;; handling an error in code from early-init.el with
> +                              ;; --debug-init.
> +                              (and (eq t (framep (selected-frame)))
> +                                   (equal "initial_terminal" (terminal-name)))))

Paul said earlier that the latter condition, which tests
terminal-name, is always correct in batch mode.  If that is indeed so,
then why do we need to test noninteractive as well? it's redundant,
no?

And the request is to add a comment explaining the semantics of
only-backtrace.  Not how it is set -- this is clear from the code, --
but what does it mean in terms of the code after that which uses the
value.  Because I don't think the name is descriptive enough, and the
value is tested more than once below.

Btw, I'd be much happier if the condition didn't rely on low-level
implementation details such as the actual name of the initial
terminal, nor on when exactly during startup that terminal gets
deleted.  I think it would be much cleaner to set a variable at the
right place in startup.el (AFAIU, after we call frame-initialize), and
test it in debug.el.  I realize that this condition was copied from
the code we already had in debug.el, but maybe on master we should use
a cleaner solution.

Thanks.

P.S. One other situation where this should be tested is in the daemon
mode.

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

> Paul said earlier that the latter condition, which tests
> terminal-name, is always correct in batch mode.  If that is indeed so,
> then why do we need to test noninteractive as well? it's redundant,
> no?

It is true, but it feels sort of a side-effect to me. You even say
yourself later that you don't like the condition as it goes into
low-level internals. It is extremely non-obvious that it is also true
in non-interactive mode. I think it is better to explicitly have `(or
noninteractive' rather than rely on it implicitly be a subcase of the
second `or' operand (and forget it was meant to be when the second
condition is refactored).

> And the request is to add a comment explaining the semantics of
> only-backtrace.  Not how it is set -- this is clear from the code, --
> but what does it mean in terms of the code after that which uses the
> value.

How about this?

    ;; When operating without any user interaction, we are going to
print current
    ;; backtrace and kill Emacs, because there is no way to accept
debugging commands.

Paul

On Tue, 14 Jan 2020 at 16:41, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: Paul Pogonyshev <pogonyshev@gmail.com>, Emacs developers
> >  <emacs-devel@gnu.org>
> > Date: Mon, 13 Jan 2020 14:23:09 -0500
> >
> > Paul writes:
> > > Here is a proposed patch that unifies the two print-backtrace-and-quit
> > > cases (batch mode and the failsafe). The patch is large mostly because
> > > of required reindenting, in fact only 15-20 lines are changed.
> >
> > Looks good to me, thank you for doing it.
> > Eli, do you think this can go into `emacs-27`?
>
> Yes, I think so.  But I have a question and a request:
>
> > +    (let ((only-backtrace (or noninteractive
> > +                              ;; If we're in the initial-frame (where `message' just
> > +                              ;; outputs to stdout) so there's no tty or GUI frame to
> > +                              ;; display the backtrace and interact with it: just dump a
> > +                              ;; backtrace to stdout.  This happens for example while
> > +                              ;; handling an error in code from early-init.el with
> > +                              ;; --debug-init.
> > +                              (and (eq t (framep (selected-frame)))
> > +                                   (equal "initial_terminal" (terminal-name)))))
>
> Paul said earlier that the latter condition, which tests
> terminal-name, is always correct in batch mode.  If that is indeed so,
> then why do we need to test noninteractive as well? it's redundant,
> no?
>
> And the request is to add a comment explaining the semantics of
> only-backtrace.  Not how it is set -- this is clear from the code, --
> but what does it mean in terms of the code after that which uses the
> value.  Because I don't think the name is descriptive enough, and the
> value is tested more than once below.
>
> Btw, I'd be much happier if the condition didn't rely on low-level
> implementation details such as the actual name of the initial
> terminal, nor on when exactly during startup that terminal gets
> deleted.  I think it would be much cleaner to set a variable at the
> right place in startup.el (AFAIU, after we call frame-initialize), and
> test it in debug.el.  I realize that this condition was copied from
> the code we already had in debug.el, but maybe on master we should use
> a cleaner solution.
>
> Thanks.
>
> P.S. One other situation where this should be tested is in the daemon
> mode.

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Tue, 14 Jan 2020 18:51:31 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Emacs developers <emacs-devel@gnu.org>
> 
> > Paul said earlier that the latter condition, which tests
> > terminal-name, is always correct in batch mode.  If that is indeed so,
> > then why do we need to test noninteractive as well? it's redundant,
> > no?
> 
> It is true, but it feels sort of a side-effect to me. You even say
> yourself later that you don't like the condition as it goes into
> low-level internals. It is extremely non-obvious that it is also true
> in non-interactive mode.

It is as (un)obvious as the test against the name of the initial
terminal, because in batch mode we don't delete that terminal.

> I think it is better to explicitly have `(or noninteractive' rather
> than rely on it implicitly be a subcase of the second `or' operand
> (and forget it was meant to be when the second condition is
> refactored).

As I've said, I'd be okay with removing the test of the terminal name,
and using something much less subtle, but I hesitate to ask to do this
as part of the change on the released branch, since this "unclean"
test is already there.

> > And the request is to add a comment explaining the semantics of
> > only-backtrace.  Not how it is set -- this is clear from the code, --
> > but what does it mean in terms of the code after that which uses the
> > value.
> 
> How about this?
> 
>     ;; When operating without any user interaction, we are going to
> print current
>     ;; backtrace and kill Emacs, because there is no way to accept
> debugging commands.

Fine with me, but maybe for even more clarity, mention what we do in
addition in the case where user interaction is possible, so that the
last part of that sentence is more self-explanatory.

Thanks.

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> Paul said earlier that the latter condition, which tests
> terminal-name, is always correct in batch mode.  If that is indeed so,
> then why do we need to test noninteractive as well? it's redundant,
> no?

Indeed, it is.

> And the request is to add a comment explaining the semantics of
> only-backtrace.  Not how it is set -- this is clear from the code, --
> but what does it mean in terms of the code after that which uses the
> value.  Because I don't think the name is descriptive enough, and the
> value is tested more than once below.

Agreed.

> Btw, I'd be much happier if the condition didn't rely on low-level
> implementation details such as the actual name of the initial
> terminal, nor on when exactly during startup that terminal gets
> deleted.  I think it would be much cleaner to set a variable at the
> right place in startup.el (AFAIU, after we call frame-initialize), and
> test it in debug.el.  I realize that this condition was copied from
> the code we already had in debug.el, but maybe on master we should use
> a cleaner solution.

Setting a var wouldn't be right.  The test:

    (and (eq t (framep (selected-frame)))
         (equal "initial_terminal" (terminal-name)))

is fundamentally *right*: we want this special behavior when we're
operating in the special dummy(stdin/out) terminal because that terminal
does not offer the usual interaction expected by the normal backtrace
debugger.  So the `noninteractive` is an approximation (and is redundant
for that same reason) whereas the above test is really the correct one.
I agree that it is ugly because (framep (selected-frame)) returns `t`
both for "normal" tty frames/terminals as well as for the special
dummy terminal, so we need to depend on a special terminal name to
distinguish those two cases.

Maybe we can improve the code by introducing a function

    (defun terminal-dummy-p ()
      "Return non-nil if the current terminal is the special initial terminal.
    The \"initial\" terminal is the dummy-terminal used when we don't have
    a real terminal to use, such as is the case in batch mode, or while
    running early-init.el, or while starting the emacs-server."
      (and (eq t (framep (selected-frame)))
           (equal "initial_terminal" (terminal-name)))

and then use it there (and maybe we could then cleanup that function
so it doesn't need to rely on a special terminal name).


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: pogonyshev@gmail.com,  emacs-devel@gnu.org
> Date: Tue, 14 Jan 2020 15:30:24 -0500
> 
> > Btw, I'd be much happier if the condition didn't rely on low-level
> > implementation details such as the actual name of the initial
> > terminal, nor on when exactly during startup that terminal gets
> > deleted.  I think it would be much cleaner to set a variable at the
> > right place in startup.el (AFAIU, after we call frame-initialize), and
> > test it in debug.el.  I realize that this condition was copied from
> > the code we already had in debug.el, but maybe on master we should use
> > a cleaner solution.
> 
> Setting a var wouldn't be right.

Why not?

> The test:
> 
>     (and (eq t (framep (selected-frame)))
>          (equal "initial_terminal" (terminal-name)))
> 
> is fundamentally *right*

I didn't say it was wrong, I said it wasn't clean enough, IMO.  We
know exactly where in the startup process the interactive frame
becomes available: after the call to frame-initialize.  Why wouldn't
it be right to set a flag there which debug.el could test, instead of
testing the above?

> Maybe we can improve the code by introducing a function
> 
>     (defun terminal-dummy-p ()
>       "Return non-nil if the current terminal is the special initial terminal.
>     The \"initial\" terminal is the dummy-terminal used when we don't have
>     a real terminal to use, such as is the case in batch mode, or while
>     running early-init.el, or while starting the emacs-server."
>       (and (eq t (framep (selected-frame)))
>            (equal "initial_terminal" (terminal-name)))
> 
> and then use it there (and maybe we could then cleanup that function
> so it doesn't need to rely on a special terminal name).

Even with such a function, my question above still stands.

But anyway, there's no need to wait with installing this on emacs-27
until we end discussing how to make the test cleaner.  The cleanup
should go to master in any case.

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> I didn't say it was wrong, I said it wasn't clean enough, IMO.  We
> know exactly where in the startup process the interactive frame
> becomes available: after the call to frame-initialize.  Why wouldn't
> it be right to set a flag there which debug.el could test, instead of
> testing the above?

If for some reason an interactive terminal is created but the
initial_terminal is still used (and is still the currently selected
terminal), we'd then have to tweak debug.el to make sure that it does
use the interactive terminal rather than the dummy one.

Also, what about an emacs-daemon in which an interactive frame has been
created in the past but has been deleted since?

Maybe a cleaner option than the one I suggested would be:

    (defun frame-get-interactive ()
      "Return a frame which supports interaction.
    All tty and GUI frames qualify, but not the frames on the \"initial\" terminal
    which is the dummy-terminal used when we don't have a real terminal to use,
    such as is the case in batch mode, or while running early-init.el, or while
    starting the emacs-server."
      (if (not (and (eq t (framep (selected-frame)))
                    (equal "initial_terminal" (terminal-name))))
          (selected-frame)
        (dolist (frame (frame-list))
          ...)))


-- Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: pogonyshev@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 15 Jan 2020 12:34:44 -0500
> 
> > I didn't say it was wrong, I said it wasn't clean enough, IMO.  We
> > know exactly where in the startup process the interactive frame
> > becomes available: after the call to frame-initialize.  Why wouldn't
> > it be right to set a flag there which debug.el could test, instead of
> > testing the above?
> 
> If for some reason an interactive terminal is created but the
> initial_terminal is still used (and is still the currently selected
> terminal)

How can this happen?  The code which creates the interactive terminal
immediately proceeds to delete the initial one.

> Also, what about an emacs-daemon in which an interactive frame has been
> created in the past but has been deleted since?

What does the code do now in the daemon?  That was one of the
questions I asked before.

Anyway, with the variable I propose, we can reset it again (e.g., in
server.el) once there's no interactive frames.

But I don't want to argue about this any further, it doesn't seem to
be worth our time.

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> Anyway, with the variable I propose, we can reset it again (e.g., in
> server.el) once there's no interactive frames.
>
> But I don't want to argue about this any further, it doesn't seem to
> be worth our time.

I don't see the point of the variable, really.  The question at hand is
"will the *Backtrace* buffer be usable" and this depends on the current
terminal where it will be "displayed", so the best way to determine the
course of action is to look at the terminal, I think, rather than to
duplicate this info in a variable which then needs to be updated
everytime the situation changes.

We should provide a less ad-hoc way to check whether a given terminal is
dummy, but other than that, I think the current solution is better than
going through a variable.


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

> > I don't have access to Emacs repository, so I cannot apply it myself.
>
> [ Anything we can do to help with that?  ]

I wrote that as a reminder, because it sometimes looks like people
give approvement to my patches and then nothing happens, as if they
expect me to somehow apply them, which I have no way of doing.

This time it has been 10 days since the last email in this thread, but
I see no change in the source repository. Maybe the patch got
forgotten? Or if you wait for something additional from me, please say
so explicitly.

Paul

Re: Backtrace printing in batch mode ignores all customizations

[Stefan Monnier]

> Here is a proposed patch that unifies the two print-backtrace-and-quit
> cases (batch mode and the failsafe).

Thanks, installed,


        Stefan

Re: Backtrace printing in batch mode ignores all customizations

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I wrote that as a reminder, because it sometimes looks like people
  > give approvement to my patches and then nothing happens, as if they
  > expect me to somehow apply them, which I have no way of doing.

It's entirely proper to ping us when it looks like something has fallen
off the table.  Each of us is imperfect.  Reminding each other helps us
become reliable.

Happy hacking.


-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

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

> > Here is a proposed patch that unifies the two print-backtrace-and-quit
> > cases (batch mode and the failsafe).
>
> Thanks, installed,

Didn't we agree together with Eli Zaretskii to install this to emacs-27
branch? As far as I can see, it is currently only in master.

Paul


On Mon, 27 Jan 2020 at 04:55, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Here is a proposed patch that unifies the two print-backtrace-and-quit
> > cases (batch mode and the failsafe).
>
> Thanks, installed,
>
>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 1011 bytes --]

Re: Backtrace printing in batch mode ignores all customizations

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Wed, 29 Jan 2020 12:09:53 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > Thanks, installed,
> 
> Didn't we agree together with Eli Zaretskii to install this to emacs-27
> branch? As far as I can see, it is currently only in master.

I've now cherry-picked this to the emacs-27 branch.

Thanks.

Re: Backtrace printing in batch mode ignores all customizations

[Paul Pogonyshev]

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

> I've now cherry-picked this to the emacs-27 branch.

Thank you.

Paul

On Fri, 31 Jan 2020 at 15:14, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Wed, 29 Jan 2020 12:09:53 +0100
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > > Thanks, installed,
> >
> > Didn't we agree together with Eli Zaretskii to install this to emacs-27
> > branch? As far as I can see, it is currently only in master.
>
> I've now cherry-picked this to the emacs-27 branch.
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 1049 bytes --]