Re: [PATCH 00/11] More testsuite fixes
On Jul 18 14:37, Jon Turney wrote: > 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. Great, looks good! Thanks, Corinna
Re: [PATCH 00/11] More testsuite fixes
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.
From 18ddda696137106eaa397a01bc06fc97c59df02d Mon Sep 17 00:00:00 2001
From: Jon Turney
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
---
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
Re: [PATCH 00/11] More testsuite fixes
Hi Jon, On Jul 17 12:58, Jon Turney wrote: > On 13/07/2023 12:38, Jon Turney wrote: > > > > cancel11: some funkiness I can't work out, causing the save/restoring > > signal handlers around system() to not > > work correctly > > So, the test here: is the SIGINT handle restored correctly if the thread > executing system() is cancelled. This test fails, because it's not. > > It seems like that scenario was explicitly considered when this test was > added in https://cygwin.com/pipermail/cygwin-patches/2003q1/003378.html > > I think maybe this is a regression introduced in > https://cygwin.com/cgit/newlib-cygwin/commit/?id=3cb9da14617c58c2821c80d48f0bd80a2deb5fdf > > child_info_spawn::worker calls waitpid() which ultimately calls cygwait() > which notices the thread's cancel event is signalled and acts as a > cancellation point. > > Attached is a patch which adds back the restoration of signal handlers on > thread cancellation. > > I can't find any hints in the mailing lists around 2013-04 about what > problem that change is fixing, but given the commentary, this might be > reintroducing another problem, though. Maybe, but the patch is slim enough so we might get away with it. > From a798750d271e20402a0a5efc4ac073f5948ad5b7 Mon Sep 17 00:00:00 2001 > From: Jon Turney > Date: Sun, 16 Jul 2023 14:46:00 +0100 > Subject: [PATCH] Cygwin: Restore pthread cleanup of signal handlers during > system() > > Removed in 3cb9da14 which describes it as 'ill-advised' (additional > context doesn't appear to be available). Actually, "ill-conceived". Beats my why, though. > 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... Fixes and Signed-off-by tags? Thanks, Corinna
Re: [PATCH 00/11] More testsuite fixes
On 13/07/2023 12:38, Jon Turney wrote:
cancel11: some funkiness I can't work out, causing the save/restoring signal
handlers around system() to not
work correctly
So, the test here: is the SIGINT handle restored correctly if the thread
executing system() is cancelled. This test fails, because it's not.
It seems like that scenario was explicitly considered when this test was
added in https://cygwin.com/pipermail/cygwin-patches/2003q1/003378.html
I think maybe this is a regression introduced in
https://cygwin.com/cgit/newlib-cygwin/commit/?id=3cb9da14617c58c2821c80d48f0bd80a2deb5fdf
child_info_spawn::worker calls waitpid() which ultimately calls
cygwait() which notices the thread's cancel event is signalled and acts
as a cancellation point.
Attached is a patch which adds back the restoration of signal handlers
on thread cancellation.
I can't find any hints in the mailing lists around 2013-04 about what
problem that change is fixing, but given the commentary, this might be
reintroducing another problem, though.From a798750d271e20402a0a5efc4ac073f5948ad5b7 Mon Sep 17 00:00:00 2001
From: Jon Turney
Date: Sun, 16 Jul 2023 14:46:00 +0100
Subject: [PATCH] Cygwin: Restore pthread cleanup of signal handlers during
system()
Removed in 3cb9da14 which describes it as 'ill-advised' (additional
context doesn't appear to be available).
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.
---
winsup/cygwin/spawn.cc | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 84dd74e28..3696ac9b5 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -257,11 +257,15 @@ struct system_call_handle
}
~system_call_handle ()
{
-if (is_system_call ())
+ }
+ 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
@@ -912,8 +916,10 @@ child_info_spawn::worker (const char *prog_arg, const char
*const *argv,
case _P_WAIT:
case _P_SYSTEM:
system_call.arm ();
+ pthread_cleanup_push (system_call_handle::cleanup, &system_call);
if (waitpid (cygpid, &res, 0) != cygpid)
res = -1;
+ pthread_cleanup_pop (1);
term_spawn_worker.cleanup ();
break;
case _P_DETACH:
--
2.39.0
Re: [PATCH 00/11] More testsuite fixes
On Jul 13 12:38, Jon Turney wrote: > This gets us from : > > FAIL: cygload > FAIL: devdsp.c > FAIL: ltp/access05.c > FAIL: ltp/fcntl07.c > FAIL: ltp/symlink01.c > FAIL: ltp/symlink03.c > FAIL: ltp/umask03.c > FAIL: pthread/cancel11.c > FAIL: pthread/cancel3.c > FAIL: pthread/cancel5.c > FAIL: pthread/inherit1.c > FAIL: pthread/priority1.c > FAIL: pthread/priority2.c > FAIL: systemcall.c > > to: > > FAIL: cygload > FAIL: devdsp.c > FAIL: ltp/umask03.c > FAIL: pthread/cancel11.c > FAIL: pthread/priority1.c > > Notes on the remaining failures: Please add Signed-off-by tags to your patches. Thanks, Corinna
