bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument

[Ioannis Kappas]

[-- Attachment #1.1: Type: text/plain, Size: 2849 bytes --]

There appears to be a bug in emacs-27 on windows-nt when a call is
made to a process' sentinel in response to a kill- or
interrupt-process call.

The second argument to the sentinel (the "string describing the
change") is set to a "unknown signal" rather than to "interrupt\n" as
it was the case in the latest emacs-26 or in emacs-28 branches:

| system-version | second argument passed to the sentinel |
|----------------+----------------------------------------|
|        26.3.50 | interrupted\n                          |
|        27.1.91 | unknown signal\n                       |
|        28.0.50 | interrupted\n                          |

The expectation is that, under windows-nt, the process sentinel should
pass in an "interrupted\n" value to the change description
argument when interrupt-process has successfully interrupted a process.

An ert test is included to showcase the issue that only fails in
emacs-27 on windows-nt. The test starts an idle emacs process,
sets-process-sentinel,
calls interrupt-process on it, and then compares the sentinel's second
argument whether it is the de facto expected "interrupted\n" text or
otherwise. The test fails on the latest emasc-27 since the argument that
emacs is passing in is "unknown signal".

To execute, save the test file and execute the following command:

emacs.exe -batch -l ert -l set-process-sentinel-test.el -f
ert-run-tests-batch-and-exit

The test will fail on emacs-27 but passes as expected in emacs-26 or the
master branch.

Initial analysis indicated this could be an emacs-27 regression caused by
the move to pdumper,
affecting any functionality that converts signal numbers to text
description. Analysis to follow.

---
set-process-sentinel is a built-in function in ‘src/process.c’.

(set-process-sentinel PROCESS SENTINEL)

Give PROCESS the sentinel SENTINEL; nil for default.
The sentinel is called as a function when the process changes state.
It gets two arguments: the process, and a string describing the change.


In GNU Emacs 27.1 (build 1, x86_64-w64-mingw32)
 of 2020-11-19 built on fv-az68-340
Repository revision: ec297125a76481c55390d0b329e541907879d6f3
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0

Configured using:
 'configure --prefix=/mingw64 --build=x86_64-w64-mingw32 --with-modules
 --without-dbus --without-compress-install 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe' CPPFLAGS=-D__USE_MINGW_ANSI_STDIO=1
 'LDFLAGS=-pipe
 -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--default-image-base-high''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2
HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER GMP

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

[-- Attachment #1.2: Type: text/html, Size: 5059 bytes --]

[-- Attachment #2: set-process-sentinel-test.el --]
[-- Type: application/octet-stream, Size: 1383 bytes --]

;;; -*- lexical-binding: t; -*-
(require 'ert)

(ert-deftest process-sentinel-interrupt-event ()
  "Test that interrupting a process under windows sends the
  \"interrupt\" event to the process sentinel. "
  :expected-result (if (eq 'windows-nt system-type)
		       :passed
		     :failed)

  (message ":system %s :version %s" system-configuration emacs-version)

  (with-temp-buffer 
    (let* ((proc-buf (current-buffer))

	   ;; start a new emacs process to wait idlesly to be
	   ;; interrupted
	   (cmd "emacs -Q --batch --eval=\"(sit-for 50000)\"")
	   (proc (start-file-process-shell-command
                  "test/process-sentinel-signal-event" proc-buf cmd))

	   (events '()))
      
      ;; capture any incoming events
      (set-process-sentinel proc (lambda (proc event)
				   (push event events)
				   ))
      
      ;; wait for the process to start
      (sleep-for 2)
      (should (equal 'run (process-status proc)))

      ;; interrupt process and wait to die
      (interrupt-process proc)
      (sleep-for 2)

      ;; should have received SIGINT
      (should (equal 'signal (process-status proc)))
      (should (equal 2 (process-exit-status proc)))      

      ;; and the change description should be "interrupt"
      (should (equal '("interrupt\n") events))

      (message "events %s" events)
      )))

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument Previous Next

[Ioannis Kappas]

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

Analysis of the issue

It appears as if the underlying signal number to description conversion
method is broken in emacs-27 on windows-nt causing the regression.

Looking at the code, it looks like the issue is around the area which
attempts to convert a signal number to a string description. On emacs
version prior and including emacs-27, the conversion is done by
directly accessing the _sys_siglist[] table provided in the unix
systems, mapping signal numbers to signal descriptions, e.g. SIGINT ->
"Interrupted". On native windows though, this table does not exist, and
emacs simulates it in src/sysdep.c:init_signals() so as to conform with
the rest of the code which expects this table to be there.

init_signals() only populates the descriptions in the C table when emacs is
not
!initialized, i.e. during the dumping phase. When emacs is thus ran
normally, it expects this table to have been loaded from the dump into
memory.

This table though does not appear to make it through to the dump in
emacs-27.  Having a look at the new pdumper, it looks like that it
performs differently than its predecessor. It seems as if it
only cover lisp constructs, while Unexec was also dumping the data
section of the process from memory? If true then, it implies that this
table is not eligible for dumping any more (since it is a C array), but
should always be
initialised when emacs is invoked by the user.  This has a very
simple solution, taking out the if (!initialized) line in
src/sysdep.c:init_signals():

diff --git a/src/sysdep.c b/src/sysdep.c
index f94ce4d492..5b4ec68e6b 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1980,7 +1980,6 @@ init_signals (void)
 #endif

 #if !HAVE_DECL_SYS_SIGLIST && !defined _sys_siglist
-  if (! initialized)
     {
       sys_siglist[SIGABRT] = "Aborted";
 # ifdef SIGAIO

The ert test attached to this bug report then passes.

The above is not an issue in emacs-28 because the conversion from
signal number to description is delegated to Gnulib with Paul Eggert's
df589d36817a8804d67f133890b2f453aefdf3c1 "Simplify by using Gnulib
sigdescr_np module" commit.

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

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument Previous Next

[Eli Zaretskii]

> From: Ioannis Kappas <ioannis.kappas@gmail.com>
> Date: Thu, 4 Feb 2021 07:36:23 +0000
> 
> It appears as if the underlying signal number to description conversion
> method is broken in emacs-27 on windows-nt causing the regression.
> 
> Looking at the code, it looks like the issue is around the area which
> attempts to convert a signal number to a string description. On emacs
> version prior and including emacs-27, the conversion is done by
> directly accessing the _sys_siglist[] table provided in the unix
> systems, mapping signal numbers to signal descriptions, e.g. SIGINT ->
> "Interrupted". On native windows though, this table does not exist, and
> emacs simulates it in src/sysdep.c:init_signals() so as to conform with
> the rest of the code which expects this table to be there.
> 
> init_signals() only populates the descriptions in the C table when emacs is not
> !initialized, i.e. during the dumping phase. When emacs is thus ran
> normally, it expects this table to have been loaded from the dump into
> memory.
> 
> This table though does not appear to make it through to the dump in
> emacs-27.  Having a look at the new pdumper, it looks like that it
> performs differently than its predecessor. It seems as if it
> only cover lisp constructs, while Unexec was also dumping the data
> section of the process from memory? If true then, it implies that this
> table is not eligible for dumping any more (since it is a C array), but should always be
> initialised when emacs is invoked by the user.  This has a very
> simple solution, taking out the if (!initialized) line in
> src/sysdep.c:init_signals():

Thanks for the analysis and the patch.  I'd prefer to fix this in a
slightly different way, which keeps the support for building Emacs
with unexec.  Does the following patch work for you?

diff --git a/src/sysdep.c b/src/sysdep.c
index f94ce4d..d100a5c 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1980,7 +1980,8 @@ init_signals (void)
 #endif
 
 #if !HAVE_DECL_SYS_SIGLIST && !defined _sys_siglist
-  if (! initialized)
+  if (! initialized
+      || dumped_with_pdumper_p ())
     {
       sys_siglist[SIGABRT] = "Aborted";
 # ifdef SIGAIO

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument Previous Next

[Ioannis Kappas]

Looks good, passes on both the original  ert test case I’ve attached to the bug report and the particular case I’ve encountered the issue on (bringing down a REPL with cider).


> On 4 Feb 2021, at 16:18, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Ioannis Kappas <ioannis.kappas@gmail.com>
>> Date: Thu, 4 Feb 2021 07:36:23 +0000
>> 
>> It appears as if the underlying signal number to description conversion
>> method is broken in emacs-27 on windows-nt causing the regression.
>> 
>> Looking at the code, it looks like the issue is around the area which
>> attempts to convert a signal number to a string description. On emacs
>> version prior and including emacs-27, the conversion is done by
>> directly accessing the _sys_siglist[] table provided in the unix
>> systems, mapping signal numbers to signal descriptions, e.g. SIGINT ->
>> "Interrupted". On native windows though, this table does not exist, and
>> emacs simulates it in src/sysdep.c:init_signals() so as to conform with
>> the rest of the code which expects this table to be there.
>> 
>> init_signals() only populates the descriptions in the C table when emacs is not
>> !initialized, i.e. during the dumping phase. When emacs is thus ran
>> normally, it expects this table to have been loaded from the dump into
>> memory.
>> 
>> This table though does not appear to make it through to the dump in
>> emacs-27.  Having a look at the new pdumper, it looks like that it
>> performs differently than its predecessor. It seems as if it
>> only cover lisp constructs, while Unexec was also dumping the data
>> section of the process from memory? If true then, it implies that this
>> table is not eligible for dumping any more (since it is a C array), but should always be
>> initialised when emacs is invoked by the user.  This has a very
>> simple solution, taking out the if (!initialized) line in
>> src/sysdep.c:init_signals():
> 
> Thanks for the analysis and the patch.  I'd prefer to fix this in a
> slightly different way, which keeps the support for building Emacs
> with unexec.  Does the following patch work for you?
> 
> diff --git a/src/sysdep.c b/src/sysdep.c
> index f94ce4d..d100a5c 100644
> --- a/src/sysdep.c
> +++ b/src/sysdep.c
> @@ -1980,7 +1980,8 @@ init_signals (void)
> #endif
> 
> #if !HAVE_DECL_SYS_SIGLIST && !defined _sys_siglist
> -  if (! initialized)
> +  if (! initialized
> +      || dumped_with_pdumper_p ())
>     {
>       sys_siglist[SIGABRT] = "Aborted";
> # ifdef SIGAIO

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument Previous Next

[Eli Zaretskii]

> From: Ioannis Kappas <ioannis.kappas@gmail.com>
> Date: Thu, 4 Feb 2021 18:12:06 +0000
> Cc: 46284@debbugs.gnu.org
> 
> Looks good, passes on both the original  ert test case I’ve attached to the bug report and the particular case I’ve encountered the issue on (bringing down a REPL with cider).

Thanks, I installed this on the emacs-27 branch.

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ioannis Kappas <ioannis.kappas@gmail.com>
>> Date: Thu, 4 Feb 2021 18:12:06 +0000
>> Cc: 46284@debbugs.gnu.org
>>
>> Looks good, passes on both the original  ert test case I’ve attached to the bug report and the particular case I’ve encountered the issue on (bringing down a REPL with cider).
>
> Thanks, I installed this on the emacs-27 branch.

It seems like this bug was fixed.  Should this be closed or is there
more to do here?

bug#46284: 27.1; emacs-27: windows-nt regression with process sentinel's change description argument

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 20 Apr 2021 22:04:14 -0500
> Cc: Ioannis Kappas <ioannis.kappas@gmail.com>, 46284@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Ioannis Kappas <ioannis.kappas@gmail.com>
> >> Date: Thu, 4 Feb 2021 18:12:06 +0000
> >> Cc: 46284@debbugs.gnu.org
> >>
> >> Looks good, passes on both the original  ert test case I’ve attached to the bug report and the particular case I’ve encountered the issue on (bringing down a REPL with cider).
> >
> > Thanks, I installed this on the emacs-27 branch.
> 
> It seems like this bug was fixed.  Should this be closed or is there
> more to do here?

Nothing to do, thanks.  Closing.