posix_spawn-internal: improvements on native Windows

[Bruno Haible]

Here are some improvements for the posix_spawn* implementation on
native Windows.

The first patch is a small fix (only visible if, among the file
action, there is an _addopen of a file in O_APPEND mode, followed by
a _adddup2 of that file descriptor).

The next 4 patches optimize away most DuplicateHandle calls. In
particular, the last among these patches applies some optimization
rules:

 (1)dup2 (oldfd, newfd) ... close (newfd)
 -> nop ... nop

 (2)dup2 (oldfd, newfd) ... open (newfd, ...)
 -> nop ... open (newfd, ...)

 (3)dup2 (oldfd, newfd) ... dup2 (otherfd, newfd)
 -> nop ... dup2 (otherfd, newfd)

 (4)dup2 (oldfd, newfd)   ... close (oldfd)
 -> dup2_close (oldfd, newfd) ... nop

 (5)dup2 (oldfd, newfd)   ... open (oldfd, ...)
 -> dup2_close (oldfd, newfd) ... open (oldfd, ...)

 (6)dup2 (oldfd, newfd)   ... dup2 (otherfd, oldfd)
 -> dup2_close (oldfd, newfd) ... dup2 (otherfd, oldfd)

where dup2_close shuffles around file descriptors without doing any
Windows API call.

The rules (1), (2), (3), (5), (6) are rarely relevant in practice;
however, rule (4) is widely applicable.


2022-09-09  Bruno Haible  

posix_spawn-internal: Optimize DuplicateHandle calls on native Windows.
* lib/windows-spawn.h (DELAYED_DUP2_OLDFD, DELAYED_DUP2_NEWFD): New
macros.
(struct IHANDLE): Add a linked_fd field.
* lib/spawni.c (SPAWN_INTERNAL_OPTIMIZE_DUPLICATEHANDLE): New macro.
(do_delayed_dup2, do_remaining_delayed_dup2): New functions.
(close_inheritable_handles): Don't close handles in DELAYED_DUP2_NEWFD
entries.
(do_close): Add a third parameter. Optimize delayed dup2 calls.
(do_open): Use do_close.
(do_dup2): Likewise. Prepare for optimizing the DuplicateHandle call.
(__spawni): Do the remaining delayed dup2 invocations after the loop
over the actions.

posix_spawn-internal: Refactor.
* lib/windows-spawn.h (struct IHANDLE): New type.
(struct inheritable_handles): Combine handles and flags into a single
array.
* lib/windows-spawn.c (init_inheritable_handles, compose_handles_block,
spawnpvech): Update.
* lib/spawni.c (grow_inheritable_handles, shrink_inheritable_handles,
do_open, do_dup2, do_close): Update.

posix_spawn-internal: Optimize DuplicateHandle calls on native Windows.
* lib/spawni.c (open_handle): Return an inheritable HANDLE.
(do_open): Don't call DuplicateHandle. Remove curr_process parameter.
(__spawni): Update.

posix_spawn-internal: Optimize DuplicateHandle calls on native Windows.
* lib/windows-spawn.h (KEEP_OPEN_IN_PARENT): New macro.
* lib/windows-spawn.c (init_inheritable_handles): When a handle is
already inheritable, don't bother duplicating it; instead, just mark it
as KEEP_OPEN_IN_PARENT.
* lib/spawni.c (shrink_inheritable_handles, close_inheritable_handles,
do_open, do_dup2, do_close): Don't close handles that are marked as
KEEP_OPEN_IN_PARENT.

2022-09-09  Bruno Haible  

posix_spawn-internal: Don't lose flags while duplicating an fd.
* lib/spawni.c (do_dup2): Fix the flags of the new fd.

>From 31eae47a8c3b645d8e096541ad0e8906b9490e16 Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sat, 10 Sep 2022 02:26:18 +0200
Subject: [PATCH 1/5] posix_spawn-internal: Don't lose flags while duplicating
 an fd.

* lib/spawni.c (do_dup2): Fix the flags of the new fd.
---
 ChangeLog| 5 +
 lib/spawni.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 44d486f212..3b4db6dfff 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2022-09-09  Bruno Haible  
+
+	posix_spawn-internal: Don't lose flags while duplicating an fd.
+	* lib/spawni.c (do_dup2): Fix the flags of the new fd.
+
 2022-09-09  Bruno Haible  
 
 	spawn-pipe: Fix pipe-filter-* test hangs (regression 2020-12-24).
diff --git a/lib/spawni.c b/lib/spawni.c
index 8125ce19ee..b9b0589460 100644
--- a/lib/spawni.c
+++ b/lib/spawni.c
@@ -471,7 +471,8 @@ do_dup2 (struct inheritable_handles *inh_handles, int oldfd, int newfd,
   errno = EBADF; /* arbitrary */
   return -1;
 }
-  inh_handles->flags[newfd] = KEEP_OPEN_IN_CHILD;
+  inh_handles->flags[newfd] =
+(unsigned char) inh_handles->flags[oldfd] | KEEP_OPEN_IN_CHILD;
 }
   return 0;
 }
-- 
2.34.1

>From 3b68fa987fef59df34752a117d6df872f1d97b6c Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sat, 10 Sep 2022 02:27:05 +0200
Subject: [PATCH 2/5] posix_spawn-internal: Optimize DuplicateHandle calls on
 native Windows.

* lib/windows-spawn.h (KEEP_OPEN_IN_PARENT): New macro.
* lib/windows-spawn.c (init_inheritable_handles): 

spawn-pipe: Fix pipe-filter-* test hangs (regression 2020-12-24)

[Bruno Haible]

Since 2020-12-25, the 4 pipe-filter-* tests were hanging on native Windows.
This patch, together with the previous one, fixes the issue.

The change that triggered the malfunction was switching the spawn-pipe
implementation from a code specific to native Windows to a generic code
that uses posix_spawn, in nearly exactly the same way as we use on Unix
platforms.

The root cause was that the old, specific-to-Windows code followed the
algorithm indicated in
:
   "To use the _pipe function to communicate between a parent
process and a child process, each process must have only one
descriptor open on the pipe. The descriptors must be opposites:
if the parent has a read descriptor open, then the child must have
a write descriptor open. The easiest way to do this is to bitwise
"or" (|) the _O_NOINHERIT flag with textmode. Then, use _dup or
_dup2 to create an inheritable copy of the pipe descriptor that
you want to pass to the child. Close the original descriptor, and
then spawn the child process. On returning from the spawn call,
close the duplicate descriptor in the parent process."
whereas the new, generic code did it differently: It called _pipe
without the _O_NOINHERIT flag, thus producing file descriptors
which Windows knew were inheritable. Half of these file descriptors
were then closed. But the remaining two file descriptors were inheritable,
and apparently this is sufficient for breaking the "end of input" /
"end of output" recognition. In other words, Windows behaves as if
some other child processes _exist_ which use these HANDLEs, even
though no such child processes actually exist at the given moment.

The previous patch fixed this; but now the HANDLEs are no longer
forwarded to the child process. The fix is to proceed in three
different steps, as described in
:
  1. The set of open file descriptors for the child process shall initially
 be the same set as is open for the calling process.
  3. The file actions specified by the spawn file actions object shall be
 performed in the order in which they were added.
  4. Any file descriptor that has its FD_CLOEXEC flag set (see fcntl) shall
 be closed.
I had coded things in such a way that steps 1 and 4 were done together. But
this does not work, because as part of step 3 we have
dup2 (oldfd, newfd)
instructions, where oldfd has the FD_CLOEXEC flag set and newfd (after the dup2)
call has the FD_CLOEXEC flag cleared, per
.

Fixing this, and everything works.

There is another difference between the old, specific-to-Windows code and the
generic code: The former creates a process with process_creation_flags = 0,
whereas the generic code creates a process with process_creation_flags =
DETACHED_PROCESS. But so far this has not caused problems. If someone has a
problem with it, they can use -DSPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN=1.


2022-09-09  Bruno Haible  

spawn-pipe: Fix pipe-filter-* test hangs (regression 2020-12-24).
* lib/windows-spawn.h (struct inheritable_handles): Widen the per-fd
flags from 8 bits to 16 bits.
(KEEP_OPEN_IN_CHILD): New macro.
(init_inheritable_handles): Change description of what it does when
duplicate == true.
* lib/windows-spawn.c (init_inheritable_handles): If duplicate == true,
add all fds to the array, regardless whether they are scheduled to be
preserved in the child process.
(compose_handles_block): Update.
(spawnpvech): Update.
* lib/spawni.c (grow_inheritable_handles): Update.
(shrink_inheritable_handles): Also close the handles not marked with
KEEP_OPEN_IN_CHILD.
(do_open, do_dup2): Mark the new fd with KEEP_OPEN_IN_CHILD.

diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h
index 0be407bf93..4414e1b3c7 100644
--- a/lib/windows-spawn.h
+++ b/lib/windows-spawn.h
@@ -85,10 +85,12 @@ extern char * compose_envblock (const char * const *envp)
   _GL_ATTRIBUTE_MALLOC _GL_ATTRIBUTE_DEALLOC_FREE;
 
 
-/* This struct keeps track of which handles to pass to a subprocess, and with
-   which flags.  All of the handles here are inheritable.
+/* This struct keeps track of which handles to potentially pass to a 
subprocess,
+   and with which flags.  All of the handles here are inheritable.
Regarding handle inheritance, see
-    
 */
+   .
+   Whether a handle is actually scheduled for being preserved in the child
+   process is determined by the KEEP_OPEN_IN_CHILD bit in the flags.  */
 struct inheritable_handles
 {
   /* The number of occupied entries in the two arrays below.
@@ -103,13 +105,19 @@ 

spawn-pipe: Fix possible hangs in programs that spawn several children

[Bruno Haible]

This patch fixes a possible hang in programs that use the 'spawn-pipe' module 
and
also spawn other child processes. It is also needed for fixing the hanging
pipe-filter-* tests on native Windows.


2022-09-09  Bruno Haible  

spawn-pipe: Fix possible hangs in programs that spawn several children.
* lib/spawn-pipe.c (create_pipe) [Unix]: Create the ifd[] and ofd[] file
descriptors with the close-on-exec flag set.

diff --git a/lib/spawn-pipe.c b/lib/spawn-pipe.c
index f132624e98..ac803f48c4 100644
--- a/lib/spawn-pipe.c
+++ b/lib/spawn-pipe.c
@@ -193,25 +193,24 @@ create_pipe (const char *progname,
 }
 }
 
-#if ((defined _WIN32 && !defined __CYGWIN__) && 
SPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN) || defined __KLIBC__
-
-  /* Native Windows API.
- This uses _pipe(), dup2(), and _spawnv().  It could also be implemented
- using the low-level functions CreatePipe(), DuplicateHandle(),
- CreateProcess() and _open_osfhandle(); see the GNU make and GNU clisp
- and cvs source code.  */
-  char *argv_mem_to_free;
   int ifd[2];
   int ofd[2];
-  int child;
-  int nulloutfd;
-  int stdinfd;
-  int stdoutfd;
-
-  const char **argv = prepare_spawn (prog_argv, _mem_to_free);
-  if (argv == NULL)
-xalloc_die ();
 
+  /* It is important to create the file descriptors with the close-on-exec bit
+ set.
+ * In the child process that we are about to create here, the file
+   descriptors ofd[0] -> STDIN_FILENO and ifd[1] -> STDOUT_FILENO will be
+   preserved across exec, because each dup2 call scheduled by
+   posix_spawn_file_actions_adddup2 creates a file descriptor with the
+   close-on-exec bit clear.  Similarly on native Windows, where we use
+   explicit DuplicateHandle calls, and on kLIBC, where we use explicit dup2
+   calls.
+ * In the parent process, we close ofd[0] and ifd[1]; so, ofd[1] and ofd[0]
+   are still open. But if the parent process spawns another child process
+   later, if ofd[1] and ofd[0] were inherited by that child process, the
+   "end of input" / "end of output" detection would not work any more.  The
+   parent or the child process would block, as long as that other child
+   process is running.  */
   if (pipe_stdout)
 if (pipe2_safer (ifd, O_BINARY | O_CLOEXEC) < 0)
   error (EXIT_FAILURE, errno, _("cannot create pipe"));
@@ -227,6 +226,23 @@ create_pipe (const char *progname,
  *
  */
 
+#if ((defined _WIN32 && !defined __CYGWIN__) && 
SPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN) || defined __KLIBC__
+
+  /* Native Windows API.
+ This uses _pipe(), dup2(), and _spawnv().  It could also be implemented
+ using the low-level functions CreatePipe(), DuplicateHandle(),
+ CreateProcess() and _open_osfhandle(); see the GNU make and GNU clisp
+ and cvs source code.  */
+  char *argv_mem_to_free;
+  int child;
+  int nulloutfd;
+  int stdinfd;
+  int stdoutfd;
+
+  const char **argv = prepare_spawn (prog_argv, _mem_to_free);
+  if (argv == NULL)
+xalloc_die ();
+
   child = -1;
 
 # if (defined _WIN32 && !defined __CYGWIN__) && 
SPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN
@@ -444,8 +460,6 @@ create_pipe (const char *progname,
 #else
 
   /* Unix API.  */
-  int ifd[2];
-  int ofd[2];
   sigset_t blocked_signals;
   posix_spawn_file_actions_t actions;
   bool actions_allocated;
@@ -454,21 +468,6 @@ create_pipe (const char *progname,
   int err;
   pid_t child;
 
-  if (pipe_stdout)
-if (pipe_safer (ifd) < 0)
-  error (EXIT_FAILURE, errno, _("cannot create pipe"));
-  if (pipe_stdin)
-if (pipe_safer (ofd) < 0)
-  error (EXIT_FAILURE, errno, _("cannot create pipe"));
-/* Data flow diagram:
- *
- *   writesystem read
- *parent  ->   ofd[1]   ->   ofd[0]   ->   child   if pipe_stdin
- *parent  <-   ifd[0]   <-   ifd[1]   <-   child   if pipe_stdout
- *   read system write
- *
- */
-
   if (slave_process)
 {
   sigprocmask (SIG_SETMASK, NULL, _signals);

pipe-filter-gi tests: Fix long-standing failure on native Windows

[Bruno Haible]

In a testdir with SPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN set to 1 instead of 0,
there is a test failure:
  FAIL: test-pipe-filter-gi2.sh

This patch fixes it. Similar to the corresponding patch
"pipe-filter-ii tests: Fix long-standing failure on native Windows" from
2021-06-15.


2022-09-09  Bruno Haible  

pipe-filter-gi tests: Fix long-standing failure on native Windows.
* tests/test-pipe-filter-gi2-main.c: Include binary-io.h.
(main): Avoid NL to CRLF conversion on standard output.
* tests/test-pipe-filter-gi2-child.c: Include , binary-io.h.
(main): Avoid NL to CRLF conversion on standard output.

diff --git a/tests/test-pipe-filter-gi2-child.c 
b/tests/test-pipe-filter-gi2-child.c
index f9f7884deb..684d907de5 100644
--- a/tests/test-pipe-filter-gi2-child.c
+++ b/tests/test-pipe-filter-gi2-child.c
@@ -20,10 +20,15 @@
 
 #include 
 #include 
+#include 
+
+#include "binary-io.h"
 
 int
 main ()
 {
+  set_binary_mode (STDOUT_FILENO, O_BINARY);
+
   /* Repeatedly: Read two integers i and j, then output all integers in the
  range i..j, one per line.  */
   for (;;)
diff --git a/tests/test-pipe-filter-gi2-main.c 
b/tests/test-pipe-filter-gi2-main.c
index 09d0f55412..e0f97e4c0c 100644
--- a/tests/test-pipe-filter-gi2-main.c
+++ b/tests/test-pipe-filter-gi2-main.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 
+#include "binary-io.h"
 #include "full-write.h"
 #include "macros.h"
 
@@ -74,6 +75,8 @@ main (int argc, char **argv)
 
   ASSERT (argc == 2);
 
+  set_binary_mode (STDOUT_FILENO, O_BINARY);
+
   /* Test writing to a nonexistent program traps sooner or later.  */
   {
 int rc;

Re: Fwd: gnulib posix_spawn_file_actions_addclose

[Bruno Haible]

I committed:
> 2022-09-07  Bruno Haible  
> 
>   posix_spawn_file_actions_addclose tests: Avoid test failure on musl.
>   Reported by Valery Ushakov  in
>   .
>   * modules/posix_spawn_file_actions_addclose-tests (configure.ac): Invoke
>   gl_MUSL_LIBC.
>   * tests/test-posix_spawn_file_actions_addclose.c (main): Skip one of the
>   tests on musl libc.

This patch was incomplete. Here's the needed addendum.


2022-09-09  Bruno Haible  

posix_spawn_file_actions_addclose tests: Fix mistake from 2022-09-07.
* modules/posix_spawn_file_actions_addclose-tests (Files): Add
m4/musl.m4.

diff --git a/modules/posix_spawn_file_actions_addclose-tests 
b/modules/posix_spawn_file_actions_addclose-tests
index 701e04dc17..4cc8623c16 100644
--- a/modules/posix_spawn_file_actions_addclose-tests
+++ b/modules/posix_spawn_file_actions_addclose-tests
@@ -2,6 +2,7 @@ Files:
 tests/test-posix_spawn_file_actions_addclose.c
 tests/signature.h
 tests/macros.h
+m4/musl.m4
 
 Depends-on:
 getdtablesize

Re: [PATCH] Never call gl_FUNC_GETCWD_PATH_MAX on Darwin

[Kirill A. Korinsky]

Thanks for the feedback.


I guess that the right approach to fix it is described here: 
https://lists.gnu.org/archive/html/bug-gnulib/2022-01/msg00085.html

--
wbr, Kirill



signature.asc
Description: Message signed with OpenPGP

Re: bool and C23

[Bruno Haible]

Paul Eggert wrote:
> To help move ahead on this, I propose the attached patches to Gnulib. 
> They use a simpler version of the Autoconf macro, named gl_C_BOOL to 
> avoid collision with any future improvements to Autoconf. My hope is 
> that gl_C_BOOL can be renamed to AC_C_BOOL, or something like that. The 
> idea is that AC_C_BOOL assumes C99 or later; if you want to port to 
> pre-C99 (which pretty much nobody does these days), you can use the 
> Gnulib stdbool module.
> 
> The first patch adds a Gnulib module c-bool, which acts like the 
> existing Gnulib module stdbool except it supports C23-style bool (no 
> include file) instead of C99-style bool.

I have two objections against this patch:

* A technical one: In
  
  Zack and I agreed that the AH_VERBATIM code should be of the form
 #ifndef __cplusplus
 ...
 #endif

  Also, in 
  I suggested to ensure that the '#include ' goes to the end of
  config.h. IIUC, a way to do this is to replace
AH_VERBATIM([bool], [...code...])
  with
AH_VERBATIM([zzbool], [...code...])

* A major design objection: While from the Autoconf perspective, it is natural
  to have two macros AC_HEADER_STDBOOL and AC_C_BOOL, from the Gnulib
  perspective it is not good to have two modules 'stdbool' and 'c-bool'.

  The problems that I'm seeing are:

- How do we explain it to the Gnulib users?
  You have written this in the doc:
"If your code needs to be portable to pre-C99 platforms,
 it needs to also use the @code{stdbool} module."
  So the user needs to think about whether to use 'stdbool',
  or 'c-bool', or both (!).

- So far, we have two different modules when there are two
  conflicting standards. For example, getopt-posix vs. getopt-gnu.
  But it is easy to explain: "If you want getopt to behave like
  specified in POSIX, use getopt-posix. If you want it to behave
  like glibc, use getopt-gnu." But different modules for different
  versions of the same standard? We have avoided this so far.

- Different modules for different ISO C versions also means that
  every package would need to migrate at some point. Namely, swap
  'stdbool' against 'c-bool' in the autogen.sh script / bootstrap.conf /
  local gnulib module dependencies. It would be a disgrace to force
  this on our users.

- We would need to make sure that these two modules don't interfere.
  For example, if there are two gnulib-tool invocations, like in
  
https://www.gnu.org/software/gnulib/manual/html_node/Multiple-instances.html ,
  e.g. 'c-bool' used by a library and 'stdbool' by the program,
  we would need to ensure that things go well.

  I would suggest to keep *one* module, and keep it named 'stdbool'.
  Its meaning will be "provide bool, true, false according to the standards".
  It can invoke AC_HEADER_STDBOOL and AC_C_BOOL under the hood.
  The important point is that the migration from older to newer ISO standard
  versions is transparent (not troublesome) for the Gnulib user.

> The second patch is mostly mechanical: it changes the other Gnulib 
> modules to prefer c-bool to stdbool.

Once this patch is reduced to modifying only lib/ and tests/ — no changes to
modules/ —, I volunteer to test it
  a) on various existing platforms,
  b) on clang 15, which has "false and true first-class language features"
 (see https://releases.llvm.org/15.0.0/tools/clang/docs/ReleaseNotes.html ).

Bruno