[PATCH 0/3] signal handler ucontext improvements

[PATCH 0/3] signal handler ucontext improvements

[Jon TURNEY]

A few further patches improving the ucontext created for a signal handler

Jon TURNEY (3):
  Initialize context before RtlContextCapture
  Only construct ucontext for SA_SIGINFO signal handlers
  Set mcontext.cr2 to the faulting address

 winsup/cygwin/ChangeLog     | 16 +++++++++++++
 winsup/cygwin/exceptions.cc | 58 +++++++++++++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.1.4

[PATCH 1/3] Initialize context before RtlContextCapture

[Jon TURNEY]

	* exceptions.cc (call_signal_handler): Zero initialize context and set
	context flags, as RlCaptureContext doesn't.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog     | 5 +++++
 winsup/cygwin/exceptions.cc | 8 ++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 0d1f36d..9d50c1e 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1496,8 +1496,12 @@ _cygtls::call_signal_handler ()
       if (thissi.si_cyg)
         memcpy (&thiscontext.uc_mcontext, ((cygwin_exception *)thissi.si_cyg)->context(), sizeof(CONTEXT));
       else
-        RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
-        /* FIXME: Really this should be the context which the signal interrupted? */
+        {
+          /* FIXME: Really this should be the context which the signal interrupted? */
+          memset(&thiscontext.uc_mcontext, 0, sizeof(struct __mcontext));
+          thiscontext.uc_mcontext.ctxflags = CONTEXT_FULL;
+          RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
+        }
 
       /* FIXME: If/when sigaltstack is implemented, this will need to do
          something more complicated */
-- 
2.1.4

[PATCH 2/3] Only construct ucontext for SA_SIGINFO signal handlers

[Jon TURNEY]

	* exceptions.cc (call_signal_handler): Only bother to construct
	the ucontext for signal handlers with SA_SIGINFO set.  Set
	mcontext.oldmask.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog     |  6 +++++
 winsup/cygwin/exceptions.cc | 54 +++++++++++++++++++++++++++------------------
 2 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 9d50c1e..0223052 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1485,42 +1485,52 @@ _cygtls::call_signal_handler ()
       debug_only_printf ("dealing with signal %d", sig);
       this_sa_flags = sa_flags;
 
+      sigset_t this_oldmask = set_process_mask_delta ();
+
       /* Save information locally on stack to pass to handler. */
       int thissig = sig;
       siginfo_t thissi = infodata;
       void (*thisfunc) (int, siginfo_t *, void *) = func;
 
-      ucontext_t thiscontext;
-      thiscontext.uc_link = 0;
-      thiscontext.uc_flags = 0;
-      if (thissi.si_cyg)
-        memcpy (&thiscontext.uc_mcontext, ((cygwin_exception *)thissi.si_cyg)->context(), sizeof(CONTEXT));
-      else
+      ucontext_t context;
+      ucontext_t *thiscontext = NULL;
+
+      /* Only make a context for SA_SIGINFO handlers */
+      if (this_sa_flags & SA_SIGINFO)
         {
-          /* FIXME: Really this should be the context which the signal interrupted? */
-          memset(&thiscontext.uc_mcontext, 0, sizeof(struct __mcontext));
-          thiscontext.uc_mcontext.ctxflags = CONTEXT_FULL;
-          RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
+          context.uc_link = 0;
+          context.uc_flags = 0;
+          if (thissi.si_cyg)
+            memcpy (&context.uc_mcontext, ((cygwin_exception *)thissi.si_cyg)->context(), sizeof(CONTEXT));
+          else
+            {
+              /* FIXME: Really this should be the context which the signal interrupted? */
+              memset(&context.uc_mcontext, 0, sizeof(struct __mcontext));
+              context.uc_mcontext.ctxflags = CONTEXT_FULL;
+              RtlCaptureContext ((CONTEXT *)&context.uc_mcontext);
+            }
+
+          /* FIXME: If/when sigaltstack is implemented, this will need to do
+             something more complicated */
+          context.uc_stack.ss_sp = NtCurrentTeb ()->Tib.StackBase;
+          context.uc_stack.ss_flags = 0;
+          if (!NtCurrentTeb ()->DeallocationStack)
+            context.uc_stack.ss_size = (uintptr_t)NtCurrentTeb ()->Tib.StackLimit - (uintptr_t)NtCurrentTeb ()->Tib.StackBase;
+          else
+            context.uc_stack.ss_size = (uintptr_t)NtCurrentTeb ()->DeallocationStack - (uintptr_t)NtCurrentTeb ()->Tib.StackBase;
+
+          context.uc_sigmask = context.uc_mcontext.oldmask = this_oldmask;
+
+          thiscontext = &context;
         }
 
-      /* FIXME: If/when sigaltstack is implemented, this will need to do
-         something more complicated */
-      thiscontext.uc_stack.ss_sp = NtCurrentTeb ()->Tib.StackBase;
-      thiscontext.uc_stack.ss_flags = 0;
-      if (!NtCurrentTeb ()->DeallocationStack)
-        thiscontext.uc_stack.ss_size = (uintptr_t)NtCurrentTeb ()->Tib.StackLimit - (uintptr_t)NtCurrentTeb ()->Tib.StackBase;
-      else
-        thiscontext.uc_stack.ss_size = (uintptr_t)NtCurrentTeb ()->DeallocationStack - (uintptr_t)NtCurrentTeb ()->Tib.StackBase;
-
-      sigset_t this_oldmask = set_process_mask_delta ();
-      thiscontext.uc_sigmask = this_oldmask;
       int this_errno = saved_errno;
       reset_signal_arrived ();
       incyg = false;
       sig = 0;		/* Flag that we can accept another signal */
       unlock ();	/* unlock signal stack */
 
-      thisfunc (thissig, &thissi, &thiscontext);
+      thisfunc (thissig, &thissi, thiscontext);
       incyg = true;
 
       set_signal_mask (_my_tls.sigmask, this_oldmask);
-- 
2.1.4

[PATCH 3/3] Set mcontext.cr2 to the faulting address

[Jon TURNEY]

	* exceptions.cc (call_signal_handler): Set mcontext.cr2 to the
	faulting address.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog     | 5 +++++
 winsup/cygwin/exceptions.cc | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 0223052..919122e 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1521,6 +1521,10 @@ _cygtls::call_signal_handler ()
 
           context.uc_sigmask = context.uc_mcontext.oldmask = this_oldmask;
 
+          context.uc_mcontext.cr2 = (thissi.si_signo == SIGSEGV
+                                     || thissi.si_signo == SIGBUS)
+            ? (uintptr_t) thissi.si_addr : 0;
+
           thiscontext = &context;
         }
 
-- 
2.1.4

Re: [PATCH 3/3] Set mcontext.cr2 to the faulting address

[Corinna Vinschen]

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

Hi Jon,


Just a formatting nit:

On Apr  9 14:13, Jon TURNEY wrote:
> 	* exceptions.cc (call_signal_handler): Set mcontext.cr2 to the
> 	faulting address.
> 
> Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
> ---
>  winsup/cygwin/ChangeLog     | 5 +++++
>  winsup/cygwin/exceptions.cc | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index 0223052..919122e 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -1521,6 +1521,10 @@ _cygtls::call_signal_handler ()
>  
>            context.uc_sigmask = context.uc_mcontext.oldmask = this_oldmask;
>  
> +          context.uc_mcontext.cr2 = (thissi.si_signo == SIGSEGV
> +                                     || thissi.si_signo == SIGBUS)
> +            ? (uintptr_t) thissi.si_addr : 0;

Please use leading TABs with ts=8 throughout, and I'd prefer if the ?
in the above expression is right under the paren two lines above, like
this:

	  context.uc_mcontext.cr2 = (thissi.si_signo == SIGSEGV
				     || thissi.si_signo == SIGBUS)
				    ? (uintptr_t) thissi.si_addr : 0;

With these changes, all patches are ok to push.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]