Re: [PATCH 00/11] More testsuite fixes

[Jon Turney <jon.turney@dronecode.org.uk>]

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

On 17/07/2023 15:02, Corinna Vinschen wrote:
> 
>> We can't neatly tuck the pthread_cleanup_push/pop inside the object, as
>> they are implemented as macros which must appear in the same lexical
>> scope.
> 
> You could do it if you call the underlying functions instead.
> pthread_cleanup_push is just a convenience macro which initializes a
> local __pthread_cleanup_handler, see include/pthread.h.  If you add a
> __pthread_cleanup_handler to system_call_handle, you could use it the
> same way as the macro and encapsulate the whole thing inside the object.
> If you want to...

Good point.

Yeah, this seems preferable as it doesn't move the point where we 
restore the signal handlers in the normal flow of execution, which might 
be important, still happening when the system_call_handle object falls 
out of scope and is destroyed.

> 
> Fixes and Signed-off-by tags?
> 

Done.  Revised patch attached.


[-- Attachment #2: 0001-Cygwin-Restore-signal-handlers-on-thread-cancellatio.patch --]
[-- Type: text/plain, Size: 2347 bytes --]

From 18ddda696137106eaa397a01bc06fc97c59df02d Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sun, 16 Jul 2023 14:46:00 +0100
Subject: [PATCH] Cygwin: Restore signal handlers on thread cancellation during
 system()

Add back the restoration of signal handlers modified during system() on
thread cancellation.

Removed in 3cb9da14 which describes it as 'ill-conceived' (additional
context doesn't appear to be available).

We use internal implementation helpers for pthread cleanup chain, so we
can neatly tuck it inside the object, and keep the point when we restore
the signal handlers the same. The pthread_cleanup_push/pop() functions
are implemented as macros which must appear in the same lexical scope.)

Fixes: 3cb9da14617c ("Put signals on hold and use system_call_cleanup class to set and restore signals rather than doing it prior to to running the program.  Remove the ill-conceived pthread_cleanup stuff.")
Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/spawn.cc | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 84dd74e28..c16fe269a 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -228,6 +228,8 @@ struct system_call_handle
   _sig_func_ptr oldint;
   _sig_func_ptr oldquit;
   sigset_t oldmask;
+  __pthread_cleanup_handler cleanup_handler;
+
   bool is_system_call ()
   {
     return oldint != ILLEGAL_SIG_FUNC_PTR;
@@ -253,18 +255,27 @@ struct system_call_handle
 	sigaddset (&child_block, SIGCHLD);
 	sigprocmask (SIG_BLOCK, &child_block, &oldmask);
 	sig_send (NULL, __SIGNOHOLD);
+
+	cleanup_handler = { system_call_handle::cleanup, this, NULL };
+	_pthread_cleanup_push (&cleanup_handler);
       }
   }
   ~system_call_handle ()
   {
     if (is_system_call ())
+      _pthread_cleanup_pop (1);
+  }
+  static void cleanup (void *arg)
+  {
+# define this_ ((system_call_handle *) arg)
+    if (this_->is_system_call ())
       {
-	signal (SIGINT, oldint);
-	signal (SIGQUIT, oldquit);
-	sigprocmask (SIG_SETMASK, &oldmask, NULL);
+	signal (SIGINT, this_->oldint);
+	signal (SIGQUIT, this_->oldquit);
+	sigprocmask (SIG_SETMASK, &(this_->oldmask), NULL);
       }
   }
-# undef cleanup
+# undef this_
 };
 
 child_info_spawn NO_COPY ch_spawn;
-- 
2.39.0