[Emacs-diffs] master 9800df6: Let debugger handle process spawn errors on w32 (Bug#33016)

[Noam Postavsky]

branch: master
commit 9800df69cb7003bda1f2b98d6f11e89ba95afb9b
Author: Noam Postavsky <address@hidden>
Commit: Noam Postavsky <address@hidden>

    Let debugger handle process spawn errors on w32 (Bug#33016)
    
    Since child_setup() is called between block_input()...unblock_input(),
    when an error is signaled the Lisp debugger is prevented from
    starting.  Therefore, let the callers signal the error instead (which
    they already do for non-w32 platforms, just the error message needs an
    update).
    * src/callproc.c (child_setup) [WINDOWSNT]: Don't call
    report_file_error here.
    (call_process) [WINDOWNT]:
    * src/process.c (create_process) [WINDOWSNT]: Call report_file_errno
    here instead, after the unblock_input() call, same as for !WINDOWSNT.
    * src/lisp.h (CHILD_SETUP_ERROR_DESC): New preprocessor define.  Flip
    the containing ifndef DOS_NT branches so that it's ifdef DOS_NT.
    * src/eval.c (when_entered_debugger): Remove.
    (syms_of_eval) <internal-when-entered-debugger>: Define it as a Lisp
    integer variable instead.
    (maybe_call_debugger): Update comment.
    * test/src/process-tests.el (make-process-w32-debug-spawn-error):
    * test/src/callproc-tests.el (call-process-w32-debug-spawn-error): New
    tests.
---
 src/callproc.c             |  7 ++-----
 src/eval.c                 | 24 ++++++++++++++----------
 src/lisp.h                 |  9 ++++++---
 src/process.c              |  2 +-
 test/src/callproc-tests.el | 22 ++++++++++++++++++++++
 test/src/process-tests.el  | 20 ++++++++++++++++++++
 6 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index a3d0960..2cdf84d 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -681,7 +681,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int 
filefd,
   unblock_input ();
 
   if (pid < 0)
-    report_file_errno ("Doing vfork", Qnil, child_errno);
+    report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, child_errno);
 
   /* Close our file descriptors, except for callproc_fd[CALLPROC_PIPEREAD]
      since we will use that to read input from.  */
@@ -1174,7 +1174,7 @@ exec_failed (char const *name, int err)
    executable directory by the parent.
 
    On GNUish hosts, either exec or return an error number.
-   On MS-Windows, either return a pid or signal an error.
+   On MS-Windows, either return a pid or return -1 and set errno.
    On MS-DOS, either return an exit status or signal an error.  */
 
 CHILD_SETUP_TYPE
@@ -1319,9 +1319,6 @@ child_setup (int in, int out, int err, char **new_argv, 
bool set_pgrp,
   /* Spawn the child.  (See w32proc.c:sys_spawnve).  */
   cpid = spawnve (_P_NOWAIT, new_argv[0], new_argv, env);
   reset_standard_handles (in, out, err, handles);
-  if (cpid == -1)
-    /* An error occurred while trying to spawn the process.  */
-    report_file_error ("Spawning child process", Qnil);
   return cpid;
 
 #else  /* not WINDOWSNT */
diff --git a/src/eval.c b/src/eval.c
index e9f118c..fa7b2d0 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -52,15 +52,6 @@ Lisp_Object Vautoload_queue;
    is shutting down.  */
 Lisp_Object Vrun_hooks;
 
-/* The value of num_nonmacro_input_events as of the last time we
-   started to enter the debugger.  If we decide to enter the debugger
-   again when this is still equal to num_nonmacro_input_events, then we
-   know that the debugger itself has an error, and we should just
-   signal the error instead of entering an infinite loop of debugger
-   invocations.  */
-
-static intmax_t when_entered_debugger;
-
 /* The function from which the last `signal' was called.  Set in
    Fsignal.  */
 /* FIXME: We should probably get rid of this!  */
@@ -1835,7 +1826,8 @@ maybe_call_debugger (Lisp_Object conditions, Lisp_Object 
sig, Lisp_Object data)
          ? debug_on_quit
          : wants_debugger (Vdebug_on_error, conditions))
       && ! skip_debugger (conditions, combined_data)
-      /* RMS: What's this for?  */
+      /* See commentary on definition of
+         `internal-when-entered-debugger'.  */
       && when_entered_debugger < num_nonmacro_input_events)
     {
       call_debugger (list2 (Qerror, combined_data));
@@ -4170,6 +4162,18 @@ Note that `debug-on-error', `debug-on-quit' and friends
 still determine whether to handle the particular condition.  */);
   Vdebug_on_signal = Qnil;
 
+  /* The value of num_nonmacro_input_events as of the last time we
+   started to enter the debugger.  If we decide to enter the debugger
+   again when this is still equal to num_nonmacro_input_events, then we
+   know that the debugger itself has an error, and we should just
+   signal the error instead of entering an infinite loop of debugger
+   invocations.  */
+  DEFSYM (Qinternal_when_entered_debugger, "internal-when-entered-debugger");
+  DEFVAR_INT ("internal-when-entered-debugger", when_entered_debugger,
+              doc: /* The number of keyboard events as of last time `debugger' 
was called.
+Used to avoid infinite loops if the debugger itself has an error.
+Don't set this unless you're sure that can't happen.  */);
+
   /* When lexical binding is being used,
    Vinternal_interpreter_environment is non-nil, and contains an alist
    of lexically-bound variable, or (t), indicating an empty
diff --git a/src/lisp.h b/src/lisp.h
index 681efc3..2915944 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4480,11 +4480,14 @@ extern void syms_of_process (void);
 extern void setup_process_coding_systems (Lisp_Object);
 
 /* Defined in callproc.c.  */
-#ifndef DOS_NT
-# define CHILD_SETUP_TYPE _Noreturn void
-#else
+#ifdef DOS_NT
 # define CHILD_SETUP_TYPE int
+# define CHILD_SETUP_ERROR_DESC "Spawning child process"
+#else
+# define CHILD_SETUP_TYPE _Noreturn void
+# define CHILD_SETUP_ERROR_DESC "Doing vfork"
 #endif
+
 extern CHILD_SETUP_TYPE child_setup (int, int, int, char **, bool, 
Lisp_Object);
 extern void init_callproc_1 (void);
 extern void init_callproc (void);
diff --git a/src/process.c b/src/process.c
index 6770a5e..0c44037 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2233,7 +2233,7 @@ create_process (Lisp_Object process, char **new_argv, 
Lisp_Object current_dir)
   unblock_input ();
 
   if (pid < 0)
-    report_file_errno ("Doing vfork", Qnil, vfork_errno);
+    report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, vfork_errno);
   else
     {
       /* vfork succeeded.  */
diff --git a/test/src/callproc-tests.el b/test/src/callproc-tests.el
index 7b30a25..f351b6e 100644
--- a/test/src/callproc-tests.el
+++ b/test/src/callproc-tests.el
@@ -37,3 +37,25 @@
         (split-string-and-unquote (buffer-string)))
     (should (equal initial-shell "nil"))
     (should-not (equal initial-shell shell))))
+
+(ert-deftest call-process-w32-debug-spawn-error ()
+  "Check that debugger runs on `call-process' failure (Bug#33016)."
+  (skip-unless (eq system-type 'windows-nt))
+  (let* ((debug-on-error t)
+         (have-called-debugger nil)
+         (debugger (lambda (&rest _)
+                     (setq have-called-debugger t)
+                     ;; Allow entering the debugger later in the same
+                     ;; test run, before going back to the command
+                     ;; loop.
+                     (setq internal-when-entered-debugger -1))))
+    (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+                (condition-case-unless-debug ()
+                    ;; On Windows, "nul.FOO" act like an always-empty
+                    ;; file for any FOO, in any directory.  So this
+                    ;; passes Emacs' test for the file's existence,
+                    ;; and ensures we hit an error in the w32 process
+                    ;; spawn code.
+                    (call-process "c:/nul.exe")
+                  (error :got-error))))
+    (should have-called-debugger)))
diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 5dbf441..0bb7ebe 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -215,6 +215,26 @@
                                       (string-to-list "stdout\n")
                                       (string-to-list "stderr\n"))))))
 
+(ert-deftest make-process-w32-debug-spawn-error ()
+  "Check that debugger runs on `make-process' failure (Bug#33016)."
+  (skip-unless (eq system-type 'windows-nt))
+  (let* ((debug-on-error t)
+         (have-called-debugger nil)
+         (debugger (lambda (&rest _)
+                     (setq have-called-debugger t)
+                     ;; Allow entering the debugger later in the same
+                     ;; test run, before going back to the command
+                     ;; loop.
+                     (setq internal-when-entered-debugger -1))))
+    (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+                (condition-case-unless-debug ()
+                    ;; Emacs doesn't search for absolute filenames, so
+                    ;; the error will be hit in the w32 process spawn
+                    ;; code.
+                    (make-process :name "test" :command 
'("c:/No-Such-Command"))
+                  (error :got-error))))
+    (should have-called-debugger)))
+
 (ert-deftest make-process/file-handler/found ()
   "Check that the ‘:file-handler’ argument of ‘make-process’
 works as expected if a file name handler is found."