bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Klaus-Dieter Bauer]

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

The error-handling of `make-process' for a non-existents
executable is inconsistent between invocation as a command
(on PATH) and invocation as a full path.

To reproduce, start `emacs -Q'.

Entering

    M-x eval-expression RET
      (make-process :name "test" :command '("No Such Command"))

will bring up the debugger with

    (file-missing "Searching for program" "No such file or directory"
"nosuchcommand")

However, entering

    M-x eval-expression RET
      (make-process :name "test" :command '("c:/No Such Command"))

will merely display in the echo-area message:

    eval: Spawning child process: Invalid argument

I stumbled upon this when debugging a quick-and-dirty
script, that called a program by absolute path. When a new
version of the program changed the name of the executable
(tex2lyx2.3 -> tex2lyx), this issue occurred, and hindered
debugging the problem.

The wording of the message might indicate a
Windows-specific issue.

regards, Klaus



In GNU Emacs 26.1 (build 1, x86_64-w64-mingw32)
 of 2018-05-30 built on CIRROCUMULUS
Repository revision: 07f8f9bc5a51f5aa94eb099f3e15fbe0c20ea1ea
Windowing system distributor 'Microsoft Corp.', version 10.0.17134
Recent messages:
(#<process test<1>> #<process test>)
Quit
Entering debugger...
Back to top level
eval: Spawning child process: Invalid argument
Quit
Type C-x 1 to delete the help window, C-M-v to scroll help.
Quit
Entering debugger...
Back to top level
read--expression: Trailing garbage following expression
Configured using:
 'configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS THREADS LCMS2

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1252

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq dired
dired-loaddefs format-spec rfc822 mml mml-sec password-cache epa derived
epg epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils cl-extra
help-fns radix-tree help-mode easymenu cl-print byte-opt gv bytecomp
byte-compile cl-loaddefs cl-lib cconv debug elec-pair time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win
w32-vars term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote w32notify w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 100958 5519)
 (symbols 56 20557 1)
 (miscs 48 45 202)
 (strings 32 31151 1771)
 (string-bytes 1 808081)
 (vectors 16 14466)
 (vector-slots 8 493429 6988)
 (floats 8 53 305)
 (intervals 56 327 7)
 (buffers 992 14))

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

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
> Date: Thu, 11 Oct 2018 14:55:27 +0200
> 
> Entering
> 
>     M-x eval-expression RET
>       (make-process :name "test" :command '("No Such Command"))
> 
> will bring up the debugger with 
> 
>     (file-missing "Searching for program" "No such file or directory" "nosuchcommand")
> 
> However, entering
> 
>     M-x eval-expression RET 
>       (make-process :name "test" :command '("c:/No Such Command"))
> 
> will merely display in the echo-area message:
> 
>     eval: Spawning child process: Invalid argument
> 
> I stumbled upon this when debugging a quick-and-dirty
> script, that called a program by absolute path. When a new
> version of the program changed the name of the executable
> (tex2lyx2.3 -> tex2lyx), this issue occurred, and hindered
> debugging the problem.
> 
> The wording of the message might indicate a 
> Windows-specific issue.

The error in the second case is Windows specific, but the
inconsistency isn't: on Unix the second case "succeeds", in that it
returns a process object without any error messages.

The error message you see in the first case is because Emacs searches
for the program along exec-path (because it is not an absolute file
name).  In the second case this search is not done, because the file
name is already absolute.

So I don't think this is a bug.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Klaus-Dieter Bauer]

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

On Thu, Oct 11, 2018 at 4:22 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
> > Date: Thu, 11 Oct 2018 14:55:27 +0200
> >
> > Entering
> >
> >     M-x eval-expression RET
> >       (make-process :name "test" :command '("No Such Command"))
> >
> > will bring up the debugger with
> >
> >     (file-missing "Searching for program" "No such file or directory"
> "nosuchcommand")
> >
> > However, entering
> >
> >     M-x eval-expression RET
> >       (make-process :name "test" :command '("c:/No Such Command"))
> >
> > will merely display in the echo-area message:
> >
> >     eval: Spawning child process: Invalid argument
> >
> > I stumbled upon this when debugging a quick-and-dirty
> > script, that called a program by absolute path. When a new
> > version of the program changed the name of the executable
> > (tex2lyx2.3 -> tex2lyx), this issue occurred, and hindered
> > debugging the problem.
> >
> > The wording of the message might indicate a
> > Windows-specific issue.
>
> The error in the second case is Windows specific, but the
> inconsistency isn't: on Unix the second case "succeeds", in that it
> returns a process object without any error messages.
>
> The error message you see in the first case is because Emacs searches
> for the program along exec-path (because it is not an absolute file
> name).  In the second case this search is not done, because the file
> name is already absolute.
>
> So I don't think this is a bug.
>

Now I understand the intent of the implementation better. However, the
Unix/Windows difference still seems like a bug to me.

On Unix, the elisp will succeed, but the output and exit-status of the
process clarify the issue.
On Windows, a non-local exit occurs due to the resulting exception.
As example:

    (let (p)
      (setq p
        (make-process :name "test" :command '("/tmp/nosuchcommand") :buffer
(current-buffer)))
      ;; -- Subsequent code never reached on Windows
      (while (process-live-p p)
        (sleep-for 0.01))
      (message "(Process exit status %d)"
        (process-exit-status p)))


So on Windows two issues occur:
  - The exception doesn't indicate what went wrong.
  - The control-flow of the Elisp program is different from Unix.

This different seems, like it may give rise to Windows-specific bugs, that
would be unnecessarily hard to debug.

Then again, calling programs by full path is probably rare, so it's
probably a pretty low-priority issue.

- Klaus

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

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
> Date: Fri, 19 Oct 2018 10:03:00 +0200
> Cc: 33016@debbugs.gnu.org
> 
> On Unix, the elisp will succeed, but the output and exit-status of the process clarify the issue.
> On Windows, a non-local exit occurs due to the resulting exception.
> As example:
> 
>      (let (p)
>        (setq p
>          (make-process :name "test" :command '("/tmp/nosuchcommand") :buffer (current-buffer)))
>        ;; -- Subsequent code never reached on Windows
>        (while (process-live-p p)
>          (sleep-for 0.01))
>        (message "(Process exit status %d)"
>          (process-exit-status p)))
> 
> So on Windows two issues occur:
>   - The exception doesn't indicate what went wrong.
>   - The control-flow of the Elisp program is different from Unix.
> 
> This different seems, like it may give rise to Windows-specific bugs, that would be unnecessarily hard to
> debug.

That's true, but the way Emacs invokes async subprocesses on Windows
cannot be similar to Unix, because Windows lacks the 'fork' system
call.  Therefore, on Windows, the Emacs process itself invokes the
program, whereas on Unix this is done by a separate "forked" process,
which means Emacs on Unix simply doesn't know whether running the
program failed, until much later.

What this means is if some Lisp program wants to produce a consistent
behavior in these situations, it should have slightly different
application-level code for Posix and non-Posix hosts.

> Then again, calling programs by full path is probably rare, so it's probably a pretty low-priority issue.

Right.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

On Thu, 11 Oct 2018 at 08:57, Klaus-Dieter Bauer
<bauer.klaus.dieter@gmail.com> wrote:

>     M-x eval-expression RET
>       (make-process :name "test" :command '("c:/No Such Command"))
>
> will merely display in the echo-area message:
>
>     eval: Spawning child process: Invalid argument

The confusing thing here is that the error is signaled between
block_input()...unblock_input(), which prevents the debugger from
triggering. E.g., the "-unless-debug" part in the expression below
appears not to work, even though the error flows normally in other
respects:

(condition-case-unless-debug err
    (make-process :name "test" :command '("c:/No Such Command"))
  (error (list :error err)))
;=> (:error (file-error "Spawning child process" "Invalid argument"))

The attached patch fixes this by moving the signal to after the unblock_input().

[-- Attachment #2: v1-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/octet-stream, Size: 2934 bytes --]

From d8716c1406301512fbbfce7f1eeb45fc8d531fd1 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 13:16:38 -0400
Subject: [PATCH v1] Let debugger handle process spawn errors on w32
 (Bug#33016)

When an error is signaled between block_input()...unblock_input(), the
Lisp debugger is prevented from starting.
* src/callproc.c (child_setup): Move the report_file_error call from here...
* src/process.c (create_process) [WINDOWSNT]: ... to here, after the
unblock_input() call.
---
 src/callproc.c | 7 ++-----
 src/lisp.h     | 6 ++++++
 src/process.c  | 2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index fa12d02..a011250 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -695,7 +695,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.  */
@@ -1189,7 +1189,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
@@ -1334,9 +1334,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/lisp.h b/src/lisp.h
index 08c6dbd..c7eb471 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4233,6 +4233,12 @@ extern void setup_process_coding_systems (Lisp_Object);
 #else
 # define CHILD_SETUP_TYPE int
 #endif
+#ifdef WINDOWSNT
+# define CHILD_SETUP_ERROR_DESC "Spawning child process"
+#else
+# 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 2df51cf..b8b7a3e 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2204,7 +2204,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.  */
-- 
2.6.2.windows.1

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Mon, 8 Apr 2019 14:34:41 -0400
> Cc: 33016@debbugs.gnu.org
> 
> >     M-x eval-expression RET
> >       (make-process :name "test" :command '("c:/No Such Command"))
> >
> > will merely display in the echo-area message:
> >
> >     eval: Spawning child process: Invalid argument
> 
> The confusing thing here is that the error is signaled between
> block_input()...unblock_input(), which prevents the debugger from
> triggering. E.g., the "-unless-debug" part in the expression below
> appears not to work, even though the error flows normally in other
> respects:
> 
> (condition-case-unless-debug err
>     (make-process :name "test" :command '("c:/No Such Command"))
>   (error (list :error err)))
> ;=> (:error (file-error "Spawning child process" "Invalid argument"))
> 
> The attached patch fixes this by moving the signal to after the unblock_input().

Thanks, but could we have a test for this, please?

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

On Mon, 8 Apr 2019 at 14:59, Eli Zaretskii <eliz@gnu.org> wrote:

> > The confusing thing here is that the error is signaled between
> > block_input()...unblock_input(), which prevents the debugger from
> > triggering.

> > The attached patch fixes this by moving the signal to after the unblock_input().
>
> Thanks, but could we have a test for this, please?

Yes (I had initially thought it wouldn't work because of the way ert
uses the debugger internally, but it actually turns out fine).

By the way, I modified the error message in call_process in addition
to create_process for completeness, but I can't see a way to trigger
this for call_process: it searches for PROGRAM and signals an error
early, regardless of whether the filename is absolute or not.

[-- Attachment #2: v2-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/octet-stream, Size: 4513 bytes --]

From d7762f2599cbd2fed1b661b60dacf6fd444a7f39 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 17:57:22 -0400
Subject: [PATCH v2] 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.
* test/src/process-tests.el (make-process-w32-spawn-nonexistent): New
test.
---
 src/callproc.c            |  7 ++-----
 src/lisp.h                |  6 ++++++
 src/process.c             |  2 +-
 test/src/process-tests.el | 12 ++++++++++++
 4 files changed, 21 insertions(+), 6 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/lisp.h b/src/lisp.h
index a0a7cbd..6f39265 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4477,6 +4477,12 @@ extern void setup_process_coding_systems (Lisp_Object);
 #else
 # define CHILD_SETUP_TYPE int
 #endif
+#ifdef WINDOWSNT
+# define CHILD_SETUP_ERROR_DESC "Spawning child process"
+#else
+# 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 802ac02..ff08657 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2232,7 +2232,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/process-tests.el b/test/src/process-tests.el
index 5dbf441..779665ad 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -215,6 +215,18 @@ process-tests--mixable
                                       (string-to-list "stdout\n")
                                       (string-to-list "stderr\n"))))))
 
+(ert-deftest make-process-w32-spawn-nonexistent ()
+  "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))))
+    (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+                (condition-case-unless-debug ()
+                    (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."
-- 
2.6.2.windows.1

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Tue, 9 Apr 2019 10:13:58 -0400
> Cc: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>, 33016@debbugs.gnu.org
> 
> By the way, I modified the error message in call_process in addition
> to create_process for completeness, but I can't see a way to trigger
> this for call_process: it searches for PROGRAM and signals an error
> early, regardless of whether the filename is absolute or not.

One way is to delete the program between the time Emacs searches for
it and the time it actually invokes it.  Another way is to make the
program be a file whose name includes non-ASCII characters outside of
the current system codepage (I'm assuming the search for the program
uses file-oriented primitives which support any Unicode characters).

Having said that, this isn't worth too much of your time, if those
ideas cannot be easily implemented, or turn out wrong, and no others
present themselves.

Thanks.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

On Tue, 9 Apr 2019 at 10:33, Eli Zaretskii <eliz@gnu.org> wrote:

> > but I can't see a way to trigger
> > this for call_process: it searches for PROGRAM and signals an error
> > early, regardless of whether the filename is absolute or not.
>
> One way is to delete the program between the time Emacs searches for
> it and the time it actually invokes it.  Another way is to make the
> program be a file whose name includes non-ASCII characters outside of
> the current system codepage (I'm assuming the search for the program
> uses file-oriented primitives which support any Unicode characters).
>
> Having said that, this isn't worth too much of your time, if those
> ideas cannot be easily implemented, or turn out wrong, and no others
> present themselves.

I was inspired by your suggestions to think of a simpler idea: use "C:/nul.exe".

There is unfortunately one additional wrinkle: each of the test passes
on its own, but when running both together the second one fails due to
this check in maybe_call_debugger:

      /* RMS: What's this for?  */
      && when_entered_debugger < num_nonmacro_input_events)

RMS' question is (now) answered in the commentary for when_entered_debugger:

/* 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;

So I guess we'd need some way of resetting it from Lisp? (which does
open the tiny danger of a debugger inf-looping by resetting
when_entered_debugger and then hitting an error).
As far as I can tell, the normal debugger resets it by calling
recursive-edit, but there's no way to return from that without human
intervention (I think?).

[-- Attachment #2: v3-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/x-patch, Size: 6209 bytes --]

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 10 Apr 2019 17:58:43 -0400
> Cc: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>, 33016@debbugs.gnu.org
> 
> I was inspired by your suggestions to think of a simpler idea: use "C:/nul.exe".
> 
> There is unfortunately one additional wrinkle: each of the test passes
> on its own, but when running both together the second one fails due to
> this check in maybe_call_debugger:
> 
>       /* RMS: What's this for?  */
>       && when_entered_debugger < num_nonmacro_input_events)
> 
> RMS' question is (now) answered in the commentary for when_entered_debugger:
> 
> /* 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;
> 
> So I guess we'd need some way of resetting it from Lisp?

Doesn't it work to simply set its value before the second test?

> As far as I can tell, the normal debugger resets it by calling
> recursive-edit, but there's no way to return from that without human
> intervention (I think?).

Doesn't abort-recursive-edit work noninteractively?

> +                    ;; On Windows, "nul.FOO" is the empty file for any
> +                    ;; FOO, in any directory.  So this passes Emacs'

Instead of "is the empty file", I'd say something like "resolves to
the null device, reading from which sets the EOF condition".

Thanks.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

On Thu, 11 Apr 2019 at 10:05, Eli Zaretskii <eliz@gnu.org> wrote:

> > static intmax_t when_entered_debugger;
> >
> > So I guess we'd need some way of resetting it from Lisp?
>
> Doesn't it work to simply set its value before the second test?

Yes, or in each test, since the tests don't necessarily have knowledge
of what order they're called in (I think it's currently alphabetical
order of test name). See attached diff (against the state of my v3
patch), but it seems a bit silly to make the variable Lisp accessible
just for this obscure test case. I don't see any other way though.

> > As far as I can tell, the normal debugger resets it by calling
> > recursive-edit, but there's no way to return from that without human
> > intervention (I think?).
>
> Doesn't abort-recursive-edit work noninteractively?

Yes, but how can I arrange for it to be called without stopping to
read commands from the user first? E.g., in the following
abort-recursive-edit is too late to do any good:

(progn
  (recursive-edit)
  (abort-recursive-edit))

Using pre-command-hook is also too late, the user has to type
something to trigger the beginning of a certain command first.

(let ((pre-command-hook #'abort-recursive-edit))
  (recursive-edit))

> > +                    ;; On Windows, "nul.FOO" is the empty file for any
> > +                    ;; FOO, in any directory.  So this passes Emacs'
>
> Instead of "is the empty file", I'd say something like "resolves to
> the null device, reading from which sets the EOF condition".

Hmm, while technically more accurate, it seems like a little too much
detail to be useful; I think saying "acts like an always-empty file"
should be enough.

[-- Attachment #2: reset-when-entered.debugger.diff --]
[-- Type: application/octet-stream, Size: 3234 bytes --]

diff --git i/src/.gdbinit w/src/.gdbinit
index b8b3031..32b2680 100644
--- i/src/.gdbinit
+++ w/src/.gdbinit
@@ -1227,6 +1227,7 @@ if defined_HAVE_X_WINDOWS
   break x_error_quitter
 end
 
+break Fredraw_display
 
 # Put the Python code at the end of .gdbinit so that if GDB does not
 # support Python, GDB will do all the above initializations before
diff --git i/src/eval.c w/src/eval.c
index e9f118c..6c1a2bb 100644
--- i/src/eval.c
+++ w/src/eval.c
@@ -59,7 +59,7 @@ Lisp_Object Vrun_hooks;
    signal the error instead of entering an infinite loop of debugger
    invocations.  */
 
-static intmax_t when_entered_debugger;
+//static intmax_t when_entered_debugger;
 
 /* The function from which the last `signal' was called.  Set in
    Fsignal.  */
@@ -4170,6 +4170,12 @@ Note that `debug-on-error', `debug-on-quit' and friends
 still determine whether to handle the particular condition.  */);
   Vdebug_on_signal = Qnil;
 
+  DEFSYM (Qwhen_entered_debugger, "when-entered-debugger");
+  DEFVAR_INT ("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 i/test/src/callproc-tests.el w/test/src/callproc-tests.el
index a2728a3..28f7975 100644
--- i/test/src/callproc-tests.el
+++ w/test/src/callproc-tests.el
@@ -43,7 +43,12 @@
   (skip-unless (eq system-type 'windows-nt))
   (let* ((debug-on-error t)
          (have-called-debugger nil)
-         (debugger (lambda (&rest _) (setq have-called-debugger t))))
+         (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 when-entered-debugger -1))))
     (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
                 (condition-case-unless-debug ()
                     ;; On Windows, "nul.FOO" is the empty file for any
diff --git i/test/src/process-tests.el w/test/src/process-tests.el
index 25a52b7..640f292 100644
--- i/test/src/process-tests.el
+++ w/test/src/process-tests.el
@@ -220,7 +220,12 @@ process-tests--mixable
   (skip-unless (eq system-type 'windows-nt))
   (let* ((debug-on-error t)
          (have-called-debugger nil)
-         (debugger (lambda (&rest _) (setq have-called-debugger t))))
+         (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 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

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 11 Apr 2019 13:34:25 -0400
> Cc: Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>, 33016@debbugs.gnu.org
> 
> > Doesn't it work to simply set its value before the second test?
> 
> Yes, or in each test, since the tests don't necessarily have knowledge
> of what order they're called in (I think it's currently alphabetical
> order of test name). See attached diff (against the state of my v3
> patch), but it seems a bit silly to make the variable Lisp accessible
> just for this obscure test case. I don't see any other way though.

OK.  That diff includes some unrelated stuff, though -- you didn't
mean to install it as is, right?

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

> OK.  That diff includes some unrelated stuff, though -- you didn't
> mean to install it as is, right?

Ah, I left in the .gdbinit change completely by accident.  The other
stuff is related, but wasn't cleaned up properly yet.  Here's a proper patch:


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 9098 bytes --]

From a57d40ff5cab1bbd8f1a71bb9f062164a4f357d9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 17:57:22 -0400
Subject: [PATCH v4] 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.
* test/src/process-tests.el (make-process-w32-debug-spawn-error):
* test/src/callproc-tests.el (call-process-w32-debug-spawn-error): New
tests.

[2. text/x-diff; reset-when-entered.debugger.diff]...
---
 src/callproc.c             |  7 ++-----
 src/eval.c                 | 21 ++++++++++++---------
 src/lisp.h                 |  9 ++++++---
 src/process.c              |  2 +-
 test/src/callproc-tests.el | 21 +++++++++++++++++++++
 test/src/process-tests.el  | 20 ++++++++++++++++++++
 6 files changed, 62 insertions(+), 18 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index a3d09609d7..2cdf84d9a8 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 e9f118c5cb..adc94724f3 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!  */
@@ -4170,6 +4161,18 @@ Note that `debug-on-error', `debug-on-quit' and friends
 still determine whether to handle the particular condition.  */);
   Vdebug_on_signal = Qnil;
 
+  DEFSYM (Qinternal_when_entered_debugger, "internal-when-entered-debugger");
+  /* 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.  */
+  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 681efc3b52..2915944ffe 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 6770a5ed88..0c44037162 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 7b30a251cc..28f7975fcc 100644
--- a/test/src/callproc-tests.el
+++ b/test/src/callproc-tests.el
@@ -37,3 +37,24 @@ (ert-deftest initial-environment-preserved ()
         (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 when-entered-debugger -1))))
+    (should (eq :got-error ;; NOTE: `should-error' would inhibit debugger.
+                (condition-case-unless-debug ()
+                    ;; On Windows, "nul.FOO" is the 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 5dbf441e8c..640f292df5 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -215,6 +215,26 @@ (ert-deftest make-process/mix-stderr ()
                                       (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 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."
-- 
2.11.0

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 33016@debbugs.gnu.org,  bauer.klaus.dieter@gmail.com
> Date: Thu, 11 Apr 2019 20:44:15 -0400
> 
> > OK.  That diff includes some unrelated stuff, though -- you didn't
> > mean to install it as is, right?
> 
> Ah, I left in the .gdbinit change completely by accident.  The other
> stuff is related, but wasn't cleaned up properly yet.  Here's a proper patch:

Thanks.

> +                    ;; On Windows, "nul.FOO" is the 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")

What happened to mentioning the null device in this comment?

> +                     (setq when-entered-debugger -1))))

This should be internal-when-entered-debugger, right?  And the same in
the other test.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

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

On Fri, 12 Apr 2019 at 04:45, Eli Zaretskii <eliz@gnu.org> wrote:

> > +                    ;; On Windows, "nul.FOO" is the 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")
>
> What happened to mentioning the null device in this comment?

Yeah, I forgot to change this comment (though I don't think
specifically naming the null device is needed).

> > +                     (setq when-entered-debugger -1))))
>
> This should be internal-when-entered-debugger, right?  And the same in
> the other test.

Oops, right, that's what I get for rushing things. I think got
everything this time (include replacing RMS' question in eval.c).

[-- Attachment #2: v5-0001-Let-debugger-handle-process-spawn-errors-on-w32-B.patch --]
[-- Type: application/octet-stream, Size: 9322 bytes --]

From b85d57e7d5e0201502d9fa4f933334da75ef2a61 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 8 Apr 2019 17:57:22 -0400
Subject: [PATCH v5] 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 @@ process-tests--mixable
                                       (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."
-- 
2.6.2.windows.1

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 12 Apr 2019 14:20:46 -0400
> Cc: 33016@debbugs.gnu.org, Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
> 
> Oops, right, that's what I get for rushing things. I think got
> everything this time (include replacing RMS' question in eval.c).

OK, thanks.

bug#33016: 26.1; (make-process ...) doesn't signal an error, when executable given as absolute Windows path does not exist

[Noam Postavsky]

tags 33016 fixed
close 33016 27.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Fri, 12 Apr 2019 14:20:46 -0400
>> Cc: 33016@debbugs.gnu.org, Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>
>> 
>> Oops, right, that's what I get for rushing things. I think got
>> everything this time (include replacing RMS' question in eval.c).
>
> OK, thanks.

Pushed to master.

9800df69cb 2019-04-14T22:43:38-04:00 "Let debugger handle process spawn errors on w32 (Bug#33016)"