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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 46284 in the body.
You can then email your comments to 46284 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.1; emacs-27: windows-nt regression with process sentinel's change
 description argument
Date: Wed, 3 Feb 2021 22:50:54 +0000

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

[set-process-sentinel-test.el (application/octet-stream, attachment)]

Message #8 received at 46284 <at> debbugs.gnu.org (full text, mbox):

From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
To: 46284 <at> debbugs.gnu.org
Subject: 27.1; emacs-27: windows-nt regression with process sentinel's change
 description argument Previous Next
Date: Thu, 4 Feb 2021 07:36:23 +0000

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/html, inline)]

Message #11 received at 46284 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ioannis Kappas <ioannis.kappas <at> gmail.com>
Cc: 46284 <at> debbugs.gnu.org
Subject: Re: bug#46284: 27.1;
 emacs-27: windows-nt regression with process sentinel's change
 description argument Previous Next
Date: Thu, 04 Feb 2021 18:18:47 +0200

> From: Ioannis Kappas <ioannis.kappas <at> 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

Message #14 received at 46284 <at> debbugs.gnu.org (full text, mbox):

From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 46284 <at> debbugs.gnu.org
Subject: Re: bug#46284: 27.1; emacs-27: windows-nt regression with process
 sentinel's change description argument Previous Next
Date: Thu, 4 Feb 2021 18:12:06 +0000

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 <at> gnu.org> wrote:
> 
>> From: Ioannis Kappas <ioannis.kappas <at> 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

Message #17 received at 46284 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ioannis Kappas <ioannis.kappas <at> gmail.com>
Cc: 46284 <at> debbugs.gnu.org
Subject: Re: bug#46284: 27.1; emacs-27: windows-nt regression with process
 sentinel's change description argument Previous Next
Date: Thu, 04 Feb 2021 20:22:48 +0200

> From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
> Date: Thu, 4 Feb 2021 18:12:06 +0000
> Cc: 46284 <at> 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.

Message #20 received at 46284 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Ioannis Kappas <ioannis.kappas <at> gmail.com>, 46284 <at> debbugs.gnu.org
Subject: Re: bug#46284: 27.1; emacs-27: windows-nt regression with process
 sentinel's change description argument
Date: Tue, 20 Apr 2021 22:04:14 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
>> Date: Thu, 4 Feb 2021 18:12:06 +0000
>> Cc: 46284 <at> 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?

Message #25 received at 46284-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 46284-done <at> debbugs.gnu.org, ioannis.kappas <at> gmail.com
Subject: Re: bug#46284: 27.1; emacs-27: windows-nt regression with process
 sentinel's change description argument
Date: Wed, 21 Apr 2021 11:52:26 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 20 Apr 2021 22:04:14 -0500
> Cc: Ioannis Kappas <ioannis.kappas <at> gmail.com>, 46284 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Ioannis Kappas <ioannis.kappas <at> gmail.com>
> >> Date: Thu, 4 Feb 2021 18:12:06 +0000
> >> Cc: 46284 <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.