[PATCH 1/3] Rename struct ucontext to struct __mcontext

[PATCH 1/3] Rename struct ucontext to struct __mcontext

[Jon TURNEY]

	* include/cygwin/signal.h : Rename struct ucontext to struct
	__mcontext.  Fix layout differences from the Win32 API CONTEXT
	type.  Remove unused member _internal.  Rename member which
	corresponds to ContextFlags.  Add cr2 member.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog               |  7 +++++++
 winsup/cygwin/include/cygwin/signal.h | 28 +++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 0d07bb1..be612d5 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,10 @@
+2015-04-01  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
+	* include/cygwin/signal.h : Rename struct ucontext to struct
+	__mcontext.  Fix layout differences from the Win32 API CONTEXT
+	type.  Remove unused member _internal.  Rename member which
+	corresponds to ContextFlags.  Add cr2 member.
+
 2015-03-31  Renato Silva  <br.renatosilva@gmail.com>
 
 	* net.cc (cygwin_gethostname): Fix buffer size error handling.
diff --git a/winsup/cygwin/include/cygwin/signal.h b/winsup/cygwin/include/cygwin/signal.h
index 58bbff0..04e65aa 100644
--- a/winsup/cygwin/include/cygwin/signal.h
+++ b/winsup/cygwin/include/cygwin/signal.h
@@ -18,6 +18,10 @@
 extern "C" {
 #endif
 
+/*
+  Define a struct __mcontext, which should be identical in layout to the Win32
+  API type CONTEXT with the addition of oldmask and cr2 fields at the end.
+*/
 #ifdef __x86_64__
 
 struct _uc_fpxreg {
@@ -45,7 +49,7 @@ struct _fpstate
   __uint32_t padding[24];
 };
 
-struct ucontext
+struct __mcontext
 {
   __uint64_t p1home;
   __uint64_t p2home;
@@ -53,7 +57,7 @@ struct ucontext
   __uint64_t p4home;
   __uint64_t p5home;
   __uint64_t p6home;
-  __uint32_t cr2;
+  __uint32_t ctxflags;
   __uint32_t mxcsr;
   __uint16_t cs;
   __uint16_t ds;
@@ -86,14 +90,15 @@ struct ucontext
   __uint64_t r15;
   __uint64_t rip;
   struct _fpstate fpregs;
+  __uint64_t vregs[52];
   __uint64_t vcx;
   __uint64_t dbc;
   __uint64_t btr;
   __uint64_t bfr;
   __uint64_t etr;
   __uint64_t efr;
-  __uint8_t _internal;
   __uint64_t oldmask;
+  __uint64_t cr2;
 };
 
 #else /* !x86_64 */
@@ -117,9 +122,9 @@ struct _fpstate
   __uint32_t nxst;
 };
 
-struct ucontext
+struct __mcontext
 {
-  __uint32_t cr2;
+  __uint32_t ctxflags;
   __uint32_t dr0;
   __uint32_t dr1;
   __uint32_t dr2;
@@ -143,15 +148,20 @@ struct ucontext
   __uint32_t eflags;
   __uint32_t esp;
   __uint32_t ss;
-  __uint8_t _internal;
+  __uint32_t reserved[128];
   __uint32_t oldmask;
+  __uint32_t cr2;
 };
 
 #endif /* !x86_64 */
 
-/* Needed for GDB.   It only compiles in the context copy code if this
-   macro s defined. */
-#define __COPY_CONTEXT_SIZE ((size_t) (uintptr_t) &((struct ucontext *) 0)->_internal)
+/* Needed for GDB.  It only compiles in the context copy code if this macro is
+   defined.  This is not sizeof(CONTEXT) due to historical accidents. */
+#ifdef __x86_64__
+#define __COPY_CONTEXT_SIZE 816
+#else
+#define __COPY_CONTEXT_SIZE 204
+#endif
 
 typedef union sigval
 {
-- 
2.1.4

[PATCH 0/3] Make detailed exception information available to signal handlers (v4)

[Jon TURNEY]

Thanks for your comments so far.

Changes since v3:

Rename cr2 member of struct __ucontext to ctxflags
Add cr2 member after oldmask to struct __ucontext
Keep __COPY_CONSTRUCT_SIZE as it's historical value

Add uc_flags member to ucontext_t in case of future need
Set uc_sigmask
Use RtlCopyContext() if we don't have a context
Add a FIXME for uc_stack mentioning sigaltstack
Simplify additions to call_signal_handler() since a ucontext_t is now always passed
Just include signal.h in sys/ucontext.h

Change CW_EXCEPTION_RECORD_FROM_SIGINFO_T to return a copy of the EXCEPTION_RECORD
#define CW_EXCEPTION_RECORD_FROM_SIGINFO_T 

Jon TURNEY (3):
  Rename struct ucontext to struct __mcontext
  Provide ucontext to signal handlers
  Add cygwin_internal() operation to retrieve the EXCEPTION_RECORD from
    a siginfo_t *

 winsup/cygwin/ChangeLog               | 21 +++++++++++++++++++++
 winsup/cygwin/exception.h             |  1 +
 winsup/cygwin/exceptions.cc           | 22 ++++++++++++++++++++--
 winsup/cygwin/external.cc             | 14 ++++++++++++++
 winsup/cygwin/include/cygwin/signal.h | 28 +++++++++++++++++++---------
 winsup/cygwin/include/sys/cygwin.h    |  5 ++++-
 winsup/cygwin/include/sys/ucontext.h  | 26 ++++++++++++++++++++++++++
 winsup/cygwin/include/ucontext.h      | 16 ++++++++++++++++
 8 files changed, 121 insertions(+), 12 deletions(-)
 create mode 100644 winsup/cygwin/include/sys/ucontext.h
 create mode 100644 winsup/cygwin/include/ucontext.h

-- 
2.1.4

[PATCH 3/3] Add cygwin_internal() operation to retrieve the EXCEPTION_RECORD from a siginfo_t *

[Jon TURNEY]

	* external.cc (cygwin_internal): Add operation to retrieve a copy
	of the EXCEPTION_RECORD from a siginfo_t *.
	* include/sys/cygwin.h (cygwin_getinfo_types): Ditto.
	* exception.h (cygwin_exception): Add exception_record accessor.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog            |  7 +++++++
 winsup/cygwin/exception.h          |  1 +
 winsup/cygwin/external.cc          | 14 ++++++++++++++
 winsup/cygwin/include/sys/cygwin.h |  5 ++++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index cff94f7..50f58e5 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,12 @@
 2015-04-01  Jon TURNEY  <jon.turney@dronecode.org.uk>
 
+	* external.cc (cygwin_internal): Add operation to retrieve a copy
+	of the EXCEPTION_RECORD from a siginfo_t *.
+	* include/sys/cygwin.h (cygwin_getinfo_types): Ditto.
+	* exception.h (cygwin_exception): Add exception_record accessor.
+
+2015-04-01  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
 	* include/sys/ucontext.h : New header.
 	* include/ucontext.h : Ditto.
 	* exceptions.cc (call_signal_handler): Provide ucontext_t
diff --git a/winsup/cygwin/exception.h b/winsup/cygwin/exception.h
index 3686bb0..0478daf 100644
--- a/winsup/cygwin/exception.h
+++ b/winsup/cygwin/exception.h
@@ -175,4 +175,5 @@ public:
     framep (in_framep), ctx (in_ctx), e (in_e), h (NULL) {}
   void dumpstack ();
   PCONTEXT context () const {return ctx;}
+  EXCEPTION_RECORD *exception_record () const {return e;}
 };
diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
index 5fac4bb..e379df1 100644
--- a/winsup/cygwin/external.cc
+++ b/winsup/cygwin/external.cc
@@ -27,6 +27,7 @@ details. */
 #include "environ.h"
 #include "cygserver_setpwd.h"
 #include "pwdgrp.h"
+#include "exception.h"
 #include <unistd.h>
 #include <stdlib.h>
 #include <wchar.h>
@@ -688,6 +689,19 @@ cygwin_internal (cygwin_getinfo_types t, ...)
 	res = 0;
 	break;
 
+      case CW_EXCEPTION_RECORD_FROM_SIGINFO_T:
+	{
+	  siginfo_t *si = va_arg(arg, siginfo_t *);
+	  EXCEPTION_RECORD *er = va_arg(arg, EXCEPTION_RECORD *);
+	  if (si && si->si_cyg && er)
+	    {
+	      memcpy(er, ((cygwin_exception *)si->si_cyg)->exception_record(),
+		     sizeof(EXCEPTION_RECORD));
+	      res = 0;
+	    }
+	}
+	break;
+
       default:
 	set_errno (ENOSYS);
     }
diff --git a/winsup/cygwin/include/sys/cygwin.h b/winsup/cygwin/include/sys/cygwin.h
index edfcc56..2ec6086 100644
--- a/winsup/cygwin/include/sys/cygwin.h
+++ b/winsup/cygwin/include/sys/cygwin.h
@@ -1,3 +1,4 @@
+
 /* sys/cygwin.h
 
    Copyright 1997, 1998, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008,
@@ -153,7 +154,8 @@ typedef enum
     CW_CYGNAME_FROM_WINNAME,
     CW_FIXED_ATEXIT,
     CW_GETNSS_PWD_SRC,
-    CW_GETNSS_GRP_SRC
+    CW_GETNSS_GRP_SRC,
+    CW_EXCEPTION_RECORD_FROM_SIGINFO_T,
   } cygwin_getinfo_types;
 
 #define CW_LOCK_PINFO CW_LOCK_PINFO
@@ -214,6 +216,7 @@ typedef enum
 #define CW_FIXED_ATEXIT CW_FIXED_ATEXIT
 #define CW_GETNSS_PWD_SRC CW_GETNSS_PWD_SRC
 #define CW_GETNSS_GRP_SRC CW_GETNSS_GRP_SRC
+#define CW_EXCEPTION_RECORD_FROM_SIGINFO_T CW_EXCEPTION_RECORD_FROM_SIGINFO_T
 
 /* Token type for CW_SET_EXTERNAL_TOKEN */
 enum
-- 
2.1.4

[PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

Add ucontext.h header, defining ucontext_t and mcontext_t types.

Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
context information.

	* include/sys/ucontext.h : New header.
	* include/ucontext.h : Ditto.
	* exceptions.cc (call_signal_handler): Provide ucontext_t
	parameter to signal handler function.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog              |  7 +++++++
 winsup/cygwin/exceptions.cc          | 22 ++++++++++++++++++++--
 winsup/cygwin/include/sys/ucontext.h | 26 ++++++++++++++++++++++++++
 winsup/cygwin/include/ucontext.h     | 16 ++++++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 winsup/cygwin/include/sys/ucontext.h
 create mode 100644 winsup/cygwin/include/ucontext.h

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index be612d5..cff94f7 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,12 @@
 2015-04-01  Jon TURNEY  <jon.turney@dronecode.org.uk>
 
+	* include/sys/ucontext.h : New header.
+	* include/ucontext.h : Ditto.
+	* exceptions.cc (call_signal_handler): Provide ucontext_t
+	parameter to signal handler function.
+
+2015-04-01  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
 	* include/cygwin/signal.h : Rename struct ucontext to struct
 	__mcontext.  Fix layout differences from the Win32 API CONTEXT
 	type.  Remove unused member _internal.  Rename member which
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index af53457..be6f26c 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -16,6 +16,7 @@ details. */
 #include <stdlib.h>
 #include <syslog.h>
 #include <wchar.h>
+#include <ucontext.h>
 
 #include "cygtls.h"
 #include "pinfo.h"
@@ -1489,15 +1490,32 @@ _cygtls::call_signal_handler ()
       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
+        RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
+
+      /* 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 */
 
-      /* no ucontext_t information provided yet, so third arg is NULL */
-      thisfunc (thissig, &thissi, NULL);
+      thisfunc (thissig, &thissi, &thiscontext);
       incyg = true;
 
       set_signal_mask (_my_tls.sigmask, this_oldmask);
diff --git a/winsup/cygwin/include/sys/ucontext.h b/winsup/cygwin/include/sys/ucontext.h
new file mode 100644
index 0000000..9362d90
--- /dev/null
+++ b/winsup/cygwin/include/sys/ucontext.h
@@ -0,0 +1,26 @@
+/* ucontext.h
+
+   Copyright 2015 Red Hat, Inc.
+
+This file is part of Cygwin.
+
+This software is a copyrighted work licensed under the terms of the
+Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
+details. */
+
+#ifndef _SYS_UCONTEXT_H_
+#define _SYS_UCONTEXT_H_
+
+#include <signal.h>
+
+typedef struct __mcontext mcontext_t;
+
+typedef struct __ucontext {
+	struct __ucontext *uc_link;
+	sigset_t	uc_sigmask;
+	stack_t	uc_stack;
+	mcontext_t	uc_mcontext;
+	unsigned long int	uc_flags;
+} ucontext_t;
+
+#endif /* !_SYS_UCONTEXT_H_ */
diff --git a/winsup/cygwin/include/ucontext.h b/winsup/cygwin/include/ucontext.h
new file mode 100644
index 0000000..4240597
--- /dev/null
+++ b/winsup/cygwin/include/ucontext.h
@@ -0,0 +1,16 @@
+/* ucontext.h
+
+   Copyright 2015 Red Hat, Inc.
+
+This file is part of Cygwin.
+
+This software is a copyrighted work licensed under the terms of the
+Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
+details. */
+
+#ifndef _UCONTEXT_H
+#define _UCONTEXT_H
+
+#include <sys/ucontext.h>
+
+#endif /* _UCONTEXT_H */
-- 
2.1.4

Re: [PATCH 1/3] Rename struct ucontext to struct __mcontext

[Corinna Vinschen]

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

On Apr  1 14:19, Jon TURNEY wrote:
> 	* include/cygwin/signal.h : Rename struct ucontext to struct
> 	__mcontext.  Fix layout differences from the Win32 API CONTEXT
> 	type.  Remove unused member _internal.  Rename member which
> 	corresponds to ContextFlags.  Add cr2 member.

GTG.


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 --]

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Corinna Vinschen]

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

On Apr  1 14:19, Jon TURNEY wrote:
> Add ucontext.h header, defining ucontext_t and mcontext_t types.
> 
> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
> context information.
> 
> 	* include/sys/ucontext.h : New header.
> 	* include/ucontext.h : Ditto.
> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
> 	parameter to signal handler function.

Patch is ok with a single change:  Please add a "FIXME?" comment to:

  else
    RtlCaptureContext();

On second thought, calling RtlCaptureContext here is probably wrong.

What we really need is the context of the thread when calling
call_signal_handler I think.  It would be better to call RtlCaptureContext
before calling call_signal_handler.  But this requires a change in how
call_signal_handler is called.

We should discuss this at one point, I think.


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 --]

Re: [PATCH 3/3] Add cygwin_internal() operation to retrieve the EXCEPTION_RECORD from a siginfo_t *

[Corinna Vinschen]

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

On Apr  1 14:19, Jon TURNEY wrote:
> 	* external.cc (cygwin_internal): Add operation to retrieve a copy
> 	of the EXCEPTION_RECORD from a siginfo_t *.
> 	* include/sys/cygwin.h (cygwin_getinfo_types): Ditto.
> 	* exception.h (cygwin_exception): Add exception_record accessor.

Looks good, please apply.


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 --]

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

On 01/04/2015 15:22, Corinna Vinschen wrote:
> On Apr  1 14:19, Jon TURNEY wrote:
>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>
>> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
>> context information.
>>
>> 	* include/sys/ucontext.h : New header.
>> 	* include/ucontext.h : Ditto.
>> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
>> 	parameter to signal handler function.
>
> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>
>    else
>      RtlCaptureContext();
>
> On second thought, calling RtlCaptureContext here is probably wrong.
>
> What we really need is the context of the thread when calling
> call_signal_handler I think.

I had the same thought, but this is going to be quite tricky to achieve.

This patch depends on the change to newlib to add stack_t, so I will 
wait for that to land before committing this patch.

> It would be better to call RtlCaptureContext
> before calling call_signal_handler.  But this requires a change in how
> call_signal_handler is called.
>
> We should discuss this at one point, I think.

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Corinna Vinschen]

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

On Apr  1 18:36, Jon TURNEY wrote:
> On 01/04/2015 15:22, Corinna Vinschen wrote:
> >On Apr  1 14:19, Jon TURNEY wrote:
> >>Add ucontext.h header, defining ucontext_t and mcontext_t types.
> >>
> >>Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
> >>context information.
> >>
> >>	* include/sys/ucontext.h : New header.
> >>	* include/ucontext.h : Ditto.
> >>	* exceptions.cc (call_signal_handler): Provide ucontext_t
> >>	parameter to signal handler function.
> >
> >Patch is ok with a single change:  Please add a "FIXME?" comment to:
> >
> >   else
> >     RtlCaptureContext();
> >
> >On second thought, calling RtlCaptureContext here is probably wrong.
> >
> >What we really need is the context of the thread when calling
> >call_signal_handler I think.
> 
> I had the same thought, but this is going to be quite tricky to achieve.
> 
> This patch depends on the change to newlib to add stack_t, so I will wait
> for that to land before committing this patch.

I ACKed the newlib patch already.  You can simply omit the RTEMS and
Cygwin checks.  Sebastian is right, there's no good reason to guard the
definition like this.


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 --]

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

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

On 01/04/2015 15:22, Corinna Vinschen wrote:
> On Apr  1 14:19, Jon TURNEY wrote:
>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>
>> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
>> context information.
>>
>> 	* include/sys/ucontext.h : New header.
>> 	* include/ucontext.h : Ditto.
>> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
>> 	parameter to signal handler function.
>
> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>
>    else
>      RtlCaptureContext();
>
> On second thought, calling RtlCaptureContext here is probably wrong.

Wrong and also dangerous.

This causes random crashes on x86.

It seems that RtlCaptureContext requires the framepointer of the calling 
function in ebp, which it uses to report the rip and rsp of it's caller.

It also seems that gcc can decide to optimize the setting of the 
framepointer away, irrespective of the fact that -fomit-frame-pointer is 
not used when building exceptions.cc

If _cygtls::call_signal_handler() happens to get called with ebp 
pointing to an invalid memory address, as seems to happen occasionally, 
we will fault in RtlCaptureContext.  (in all cases, the eip and ebp in 
the returned context are incorrect)

I wrote the attached patch, which fakes a callframe for 
RtlCaptureContext to avoid these possible crashes, but this needs more 
work to correctly report eip and ebp

However, I'm not sure that is worthwhile effort as it's heading in the 
wrong direction, because ....

> What we really need is the context of the thread when calling
> call_signal_handler I think.  It would be better to call RtlCaptureContext
> before calling call_signal_handler.  But this requires a change in how
> call_signal_handler is called.


[-- Attachment #2: 0001-Avoid-random-crashes-in-RtlCaptureContext-on-x86.patch --]
[-- Type: text/plain, Size: 2776 bytes --]

From 646ec4c2c1bc89c4243b69643f172eb20d5876c1 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Fri, 3 Apr 2015 22:26:21 +0100
Subject: [PATCH] Avoid random crashes in RtlCaptureContext on x86

RtlCaptureContext requires the framepointer of the calling function in ebp,
which it uses to report the rip and rsp of it's caller. But it seems that gcc
may decide to optimize the setting of the framepointer away, irrespective of
-fomit-frame-pointer.

If _cygtls::call_signal_handler() is called with ebp pointing to an invalid
memory address, as seems to happen occasionally, we will fault in
RtlCaptureContext.

This patch manually creates a stcall callframe around RtlCaptureContext to be
safe, but I'm not sure that's the best approach.

RtlCaptureContext does so little, it's probably just as effective to write our
own version.

This is heading further in the wrong direction, as the context that is wanted
isn't the current context, but the context at the time of the signal.

There are a couple of other uses of RtlCaptureContext(), are they ok?
---
 winsup/cygwin/exceptions.cc | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 0d1f36d..0ca6dd8 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1496,8 +1496,24 @@ _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? */
+#ifdef __x86_64__
+          RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
+#else
+          memset(&thiscontext.uc_mcontext, 0, sizeof(struct __mcontext));
+          thiscontext.uc_mcontext.ctxflags = CONTEXT_FULL;
+          /* RtlCaptureContext requires the framepointer of the calling function
+             in ebp, which it uses to report the rip and rsp of it's caller.
+             But it seems that gcc may decide to optimize the setting of the
+             framepointer away, irrespective of -fomit-frame-pointer.  Manually
+             create a stcall callframe to be safe. */
+          __asm__ ( "mov %%esp,%%ebp\n"
+                    "push %0\n"
+                    "call _RtlCaptureContext@4\n"
+                    : : "r" (&thiscontext.uc_mcontext) : "%ebp", "%eax", "%ecx", "%edx" );
+#endif
+        }
 
       /* FIXME: If/when sigaltstack is implemented, this will need to do
          something more complicated */
-- 
2.1.4

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Corinna Vinschen]

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

On Apr  3 23:09, Jon TURNEY wrote:
> On 01/04/2015 15:22, Corinna Vinschen wrote:
> >On Apr  1 14:19, Jon TURNEY wrote:
> >>Add ucontext.h header, defining ucontext_t and mcontext_t types.
> >>
> >>Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
> >>context information.
> >>
> >>	* include/sys/ucontext.h : New header.
> >>	* include/ucontext.h : Ditto.
> >>	* exceptions.cc (call_signal_handler): Provide ucontext_t
> >>	parameter to signal handler function.
> >
> >Patch is ok with a single change:  Please add a "FIXME?" comment to:
> >
> >   else
> >     RtlCaptureContext();
> >
> >On second thought, calling RtlCaptureContext here is probably wrong.
> 
> Wrong and also dangerous.
> 
> This causes random crashes on x86.
> 
> It seems that RtlCaptureContext requires the framepointer of the calling
> function in ebp, which it uses to report the rip and rsp of it's caller.
> 
> It also seems that gcc can decide to optimize the setting of the
> framepointer away, irrespective of the fact that -fomit-frame-pointer is not
> used when building exceptions.cc
> 
> If _cygtls::call_signal_handler() happens to get called with ebp pointing to
> an invalid memory address, as seems to happen occasionally, we will fault in
> RtlCaptureContext.  (in all cases, the eip and ebp in the returned context
> are incorrect)
> 
> I wrote the attached patch, which fakes a callframe for RtlCaptureContext to
> avoid these possible crashes, but this needs more work to correctly report
> eip and ebp

Maybe it's simpler than that?  Looking into the GCC info pages, I found
this:

     Starting with GCC version 4.6, the default setting (when not
     optimizing for size) for 32-bit GNU/Linux x86 and 32-bit Darwin x86
     targets has been changed to '-fomit-frame-pointer'.  The default
     can be reverted to '-fno-omit-frame-pointer' by configuring GCC
     with the '--enable-frame-pointer' configure option.

     Enabled at levels '-O', '-O2', '-O3', '-Os'.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So it seems adding -fomit-frame-pointer file by file in Makefile.in
(when building with -O2) is moot and only has an effect when building
unoptimized, otherwise all files are built with -fomit-frame-pointer
anyway.

So, what if we drop all the -fomit-frame-pointer from Makefile.in and
add an

  exceptions_CFLAGS:=-fno-omit-frame-pointer

Does that help?


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 --]

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

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

On 04/04/2015 09:40, Corinna Vinschen wrote:
> On Apr  3 23:09, Jon TURNEY wrote:
>> On 01/04/2015 15:22, Corinna Vinschen wrote:
>>> On Apr  1 14:19, Jon TURNEY wrote:
>>>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>>>
>>>> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
>>>> context information.
>>>>
>>>> 	* include/sys/ucontext.h : New header.
>>>> 	* include/ucontext.h : Ditto.
>>>> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
>>>> 	parameter to signal handler function.
>>>
>>> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>>>
>>>    else
>>>      RtlCaptureContext();
>>>
>>> On second thought, calling RtlCaptureContext here is probably wrong.
>>
>> Wrong and also dangerous.
>>
>> This causes random crashes on x86.
>>
>> It seems that RtlCaptureContext requires the framepointer of the calling
>> function in ebp, which it uses to report the rip and rsp of it's caller.
>>
>> It also seems that gcc can decide to optimize the setting of the
>> framepointer away, irrespective of the fact that -fomit-frame-pointer is not
>> used when building exceptions.cc
>>
>> If _cygtls::call_signal_handler() happens to get called with ebp pointing to
>> an invalid memory address, as seems to happen occasionally, we will fault in
>> RtlCaptureContext.  (in all cases, the eip and ebp in the returned context
>> are incorrect)
>>
>> I wrote the attached patch, which fakes a callframe for RtlCaptureContext to
>> avoid these possible crashes, but this needs more work to correctly report
>> eip and ebp
>
> Maybe it's simpler than that?  Looking into the GCC info pages, I found
> this:
>
>       Starting with GCC version 4.6, the default setting (when not
>       optimizing for size) for 32-bit GNU/Linux x86 and 32-bit Darwin x86
>       targets has been changed to '-fomit-frame-pointer'.  The default
>       can be reverted to '-fno-omit-frame-pointer' by configuring GCC
>       with the '--enable-frame-pointer' configure option.
>
>       Enabled at levels '-O', '-O2', '-O3', '-Os'.
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Very good.  I hadn't noticed that sentence and was just looking at the 
"-O also turns on -fomit-frame-pointer on machines where doing so does 
not interfere with debugging. "

> So it seems adding -fomit-frame-pointer file by file in Makefile.in
> (when building with -O2) is moot and only has an effect when building
> unoptimized, otherwise all files are built with -fomit-frame-pointer
> anyway.
>
> So, what if we drop all the -fomit-frame-pointer from Makefile.in and
> add an
>
>    exceptions_CFLAGS:=-fno-omit-frame-pointer
>
> Does that help?

Yes, that seems to do the trick.  Patch attached.


[-- Attachment #2: 0001-Compile-exceptions.cc-with-fno-omit-frame-pointer-on.patch --]
[-- Type: text/plain, Size: 4250 bytes --]

From cd8532832c92d10c1a3c1fb9ec705df0ea003e28 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Sat, 4 Apr 2015 16:18:56 +0100
Subject: [PATCH] Compile exceptions.cc with -fno-omit-frame-pointer on x86

Selectively using -fomit-frame-pointer when -O is used doesn't make sense
anymore, apparently since gcc 4.6, -O implies -fomit-frame-pointer.

exceptions.cc must be compiled with -fno-omit-frame-pointer on x86, as it uses
RtlCaptureContext, which requires a frame pointer.

	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
	various files, it is already the default.  Set
	-fno-omit-frame-pointer for exceptions.cc on x86.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog   |  6 +++++
 winsup/cygwin/Makefile.in | 57 ++++-------------------------------------------
 2 files changed, 10 insertions(+), 53 deletions(-)

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 5ada35d..a8f4e00 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,9 @@
+2015-04-04  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
+	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
+	various files, it is already the default.  Set
+	-fno-omit-frame-pointer for exceptions.cc on x86.
+
 2015-04-03  Takashi Yano  <takashi.yano@nifty.ne.jp>
 
 	* fhandler_tty.cc (fhandler_pty_slave::read): Change calculation of
diff --git a/winsup/cygwin/Makefile.in b/winsup/cygwin/Makefile.in
index 9b82f0a..af9faf6 100644
--- a/winsup/cygwin/Makefile.in
+++ b/winsup/cygwin/Makefile.in
@@ -449,59 +449,10 @@ INSTOBJS:=automode.o binmode.o textmode.o textreadmode.o
 TARGET_LIBS:=$(LIB_NAME) $(CYGWIN_START) $(GMON_START) $(LIBGMON_A) $(SUBLIBS) $(INSTOBJS) $(EXTRALIBS)
 
 ifneq "${filter -O%,$(CFLAGS)}" ""
-cygheap_CFLAGS:=-fomit-frame-pointer
-cygthread_CFLAGS:=-fomit-frame-pointer
-cygtls_CFLAGS:=-fomit-frame-pointer
-cygwait_CFLAGS=-fomit-frame-pointer
-delqueue_CFLAGS:=-fomit-frame-pointer
-devices_CFLAGS:=-fomit-frame-pointer
-dir_CFLAGS:=-fomit-frame-pointer
-dlfcn_CFLAGS:=-fomit-frame-pointer
-dll_init_CFLAGS:=-fomit-frame-pointer
-dtable_CFLAGS:=-fomit-frame-pointer -fcheck-new
-fcntl_CFLAGS:=-fomit-frame-pointer
-fenv_CFLAGS:=-fomit-frame-pointer
-fhandler_CFLAGS:=-fomit-frame-pointer
-fhandler_clipboard_CFLAGS:=-fomit-frame-pointer
-fhandler_console_CFLAGS:=-fomit-frame-pointer
-fhandler_disk_file_CFLAGS:=-fomit-frame-pointer
-fhandler_dsp_CFLAGS:=-fomit-frame-pointer
-fhandler_floppy_CFLAGS:=-fomit-frame-pointer
-fhandler_netdrive_CFLAGS:=-fomit-frame-pointer
-fhandler_proc_CFLAGS:=-fomit-frame-pointer
-fhandler_process_CFLAGS:=-fomit-frame-pointer
-fhandler_random_CFLAGS:=-fomit-frame-pointer
-fhandler_raw_CFLAGS:=-fomit-frame-pointer
-fhandler_registry_CFLAGS:=-fomit-frame-pointer
-fhandler_serial_CFLAGS:=-fomit-frame-pointer
-fhandler_socket_CFLAGS:=-fomit-frame-pointer
-fhandler_syslog_CFLAGS:=-fomit-frame-pointer
-fhandler_tape_CFLAGS:=-fomit-frame-pointer
-fhandler_termios_CFLAGS:=-fomit-frame-pointer
-fhandler_tty_CFLAGS:=-fomit-frame-pointer
-fhandler_virtual_CFLAGS:=-fomit-frame-pointer
-fhandler_windows_CFLAGS:=-fomit-frame-pointer
-fhandler_zero_CFLAGS:=-fomit-frame-pointer
-flock_CFLAGS:=-fomit-frame-pointer
-grp_CFLAGS:=-fomit-frame-pointer
-libstdcxx_wrapper_CFLAGS:=-fomit-frame-pointer
-localtime_CFLAGS:=-fwrapv
-malloc_CFLAGS:=-fomit-frame-pointer -O3
-malloc_wrapper_CFLAGS:=-fomit-frame-pointer
-miscfuncs_CFLAGS:=-fomit-frame-pointer
-net_CFLAGS:=-fomit-frame-pointer
-passwd_CFLAGS:=-fomit-frame-pointer
-path_CFLAGS=-fomit-frame-pointer
-regcomp_CFLAGS=-fomit-frame-pointer
-regerror_CFLAGS=-fomit-frame-pointer
-regexec_CFLAGS=-fomit-frame-pointer
-regfree_CFLAGS=-fomit-frame-pointer
-shared_CFLAGS:=-fomit-frame-pointer
-sync_CFLAGS:=-fomit-frame-pointer -O3
-smallprint_CFLAGS:=-fomit-frame-pointer
-syscalls_CFLAGS:=-fomit-frame-pointer
-sysconf_CFLAGS:=-fomit-frame-pointer
-uinfo_CFLAGS:=-fomit-frame-pointer
+ifeq ($(target_cpu),i686)
+# on x86, exceptions.cc must be compiled with a frame-pointer as it uses RtlCaptureContext()
+exceptions_CFLAGS:=-fno-omit-frame-pointer
+endif
 endif
 
 fhandler_proc_CFLAGS+=-DUSERNAME="\"$(USER)\"" -DHOSTNAME="\"$(HOSTNAME)\""
-- 
2.1.4

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

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

On 04/04/2015 17:06, Jon TURNEY wrote:
> On 04/04/2015 09:40, Corinna Vinschen wrote:
>> So, what if we drop all the -fomit-frame-pointer from Makefile.in and
>> add an
>>
>>    exceptions_CFLAGS:=-fno-omit-frame-pointer
>>
>> Does that help?
>
> Yes, that seems to do the trick.  Patch attached.

Aargh.  Some of these things are not like the others.  Let's try that 
again...


[-- Attachment #2: 0001-Compile-exceptions.cc-with-fno-omit-frame-pointer-on.patch --]
[-- Type: text/plain, Size: 4316 bytes --]

From 2bd7105c244e48c76746665985a7314a3a2aff83 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Sat, 4 Apr 2015 23:31:03 +0100
Subject: [PATCH] Compile exceptions.cc with -fno-omit-frame-pointer on x86

Selectively using -fomit-frame-pointer when -O is used doesn't make sense
anymore, apparently since gcc 4.6, -O implies -fomit-frame-pointer.

exceptions.cc must be compiled with -fno-omit-frame-pointer on x86, as it uses
RtlCaptureContext, which requires a frame pointer.

	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
	various files, it is already the default.  Set
	-fno-omit-frame-pointer for exceptions.cc on x86.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/ChangeLog   |  6 +++++
 winsup/cygwin/Makefile.in | 59 ++++++-----------------------------------------
 2 files changed, 13 insertions(+), 52 deletions(-)

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 5ada35d..a8f4e00 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,9 @@
+2015-04-04  Jon TURNEY  <jon.turney@dronecode.org.uk>
+
+	* Makefile.in : Remove setting -fomit-frame-pointer for compiling
+	various files, it is already the default.  Set
+	-fno-omit-frame-pointer for exceptions.cc on x86.
+
 2015-04-03  Takashi Yano  <takashi.yano@nifty.ne.jp>
 
 	* fhandler_tty.cc (fhandler_pty_slave::read): Change calculation of
diff --git a/winsup/cygwin/Makefile.in b/winsup/cygwin/Makefile.in
index 9b82f0a..d827754 100644
--- a/winsup/cygwin/Makefile.in
+++ b/winsup/cygwin/Makefile.in
@@ -449,59 +449,14 @@ INSTOBJS:=automode.o binmode.o textmode.o textreadmode.o
 TARGET_LIBS:=$(LIB_NAME) $(CYGWIN_START) $(GMON_START) $(LIBGMON_A) $(SUBLIBS) $(INSTOBJS) $(EXTRALIBS)
 
 ifneq "${filter -O%,$(CFLAGS)}" ""
-cygheap_CFLAGS:=-fomit-frame-pointer
-cygthread_CFLAGS:=-fomit-frame-pointer
-cygtls_CFLAGS:=-fomit-frame-pointer
-cygwait_CFLAGS=-fomit-frame-pointer
-delqueue_CFLAGS:=-fomit-frame-pointer
-devices_CFLAGS:=-fomit-frame-pointer
-dir_CFLAGS:=-fomit-frame-pointer
-dlfcn_CFLAGS:=-fomit-frame-pointer
-dll_init_CFLAGS:=-fomit-frame-pointer
-dtable_CFLAGS:=-fomit-frame-pointer -fcheck-new
-fcntl_CFLAGS:=-fomit-frame-pointer
-fenv_CFLAGS:=-fomit-frame-pointer
-fhandler_CFLAGS:=-fomit-frame-pointer
-fhandler_clipboard_CFLAGS:=-fomit-frame-pointer
-fhandler_console_CFLAGS:=-fomit-frame-pointer
-fhandler_disk_file_CFLAGS:=-fomit-frame-pointer
-fhandler_dsp_CFLAGS:=-fomit-frame-pointer
-fhandler_floppy_CFLAGS:=-fomit-frame-pointer
-fhandler_netdrive_CFLAGS:=-fomit-frame-pointer
-fhandler_proc_CFLAGS:=-fomit-frame-pointer
-fhandler_process_CFLAGS:=-fomit-frame-pointer
-fhandler_random_CFLAGS:=-fomit-frame-pointer
-fhandler_raw_CFLAGS:=-fomit-frame-pointer
-fhandler_registry_CFLAGS:=-fomit-frame-pointer
-fhandler_serial_CFLAGS:=-fomit-frame-pointer
-fhandler_socket_CFLAGS:=-fomit-frame-pointer
-fhandler_syslog_CFLAGS:=-fomit-frame-pointer
-fhandler_tape_CFLAGS:=-fomit-frame-pointer
-fhandler_termios_CFLAGS:=-fomit-frame-pointer
-fhandler_tty_CFLAGS:=-fomit-frame-pointer
-fhandler_virtual_CFLAGS:=-fomit-frame-pointer
-fhandler_windows_CFLAGS:=-fomit-frame-pointer
-fhandler_zero_CFLAGS:=-fomit-frame-pointer
-flock_CFLAGS:=-fomit-frame-pointer
-grp_CFLAGS:=-fomit-frame-pointer
-libstdcxx_wrapper_CFLAGS:=-fomit-frame-pointer
+dtable_CFLAGS:=-fcheck-new
 localtime_CFLAGS:=-fwrapv
-malloc_CFLAGS:=-fomit-frame-pointer -O3
-malloc_wrapper_CFLAGS:=-fomit-frame-pointer
-miscfuncs_CFLAGS:=-fomit-frame-pointer
-net_CFLAGS:=-fomit-frame-pointer
-passwd_CFLAGS:=-fomit-frame-pointer
-path_CFLAGS=-fomit-frame-pointer
-regcomp_CFLAGS=-fomit-frame-pointer
-regerror_CFLAGS=-fomit-frame-pointer
-regexec_CFLAGS=-fomit-frame-pointer
-regfree_CFLAGS=-fomit-frame-pointer
-shared_CFLAGS:=-fomit-frame-pointer
-sync_CFLAGS:=-fomit-frame-pointer -O3
-smallprint_CFLAGS:=-fomit-frame-pointer
-syscalls_CFLAGS:=-fomit-frame-pointer
-sysconf_CFLAGS:=-fomit-frame-pointer
-uinfo_CFLAGS:=-fomit-frame-pointer
+malloc_CFLAGS:=-O3
+sync_CFLAGS:=-O3
+ifeq ($(target_cpu),i686)
+# on x86, exceptions.cc must be compiled with a frame-pointer as it uses RtlCaptureContext()
+exceptions_CFLAGS:=-fno-omit-frame-pointer
+endif
 endif
 
 fhandler_proc_CFLAGS+=-DUSERNAME="\"$(USER)\"" -DHOSTNAME="\"$(HOSTNAME)\""
-- 
2.1.4

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Corinna Vinschen]

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

On Apr  5 17:58, Jon TURNEY wrote:
> On 04/04/2015 17:06, Jon TURNEY wrote:
> >On 04/04/2015 09:40, Corinna Vinschen wrote:
> >>So, what if we drop all the -fomit-frame-pointer from Makefile.in and
> >>add an
> >>
> >>   exceptions_CFLAGS:=-fno-omit-frame-pointer
> >>
> >>Does that help?
> >
> >Yes, that seems to do the trick.  Patch attached.
> 
> Aargh.  Some of these things are not like the others.  Let's try that
> again...

Thanks, applied.


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 --]

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

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

On 01/04/2015 18:36, Jon TURNEY wrote:
> On 01/04/2015 15:22, Corinna Vinschen wrote:
>> On Apr  1 14:19, Jon TURNEY wrote:
>>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>>
>>> Provide sigaction sighandlers with a ucontext_t parameter, containing
>>> stack and
>>> context information.
>>>
>>>     * include/sys/ucontext.h : New header.
>>>     * include/ucontext.h : Ditto.
>>>     * exceptions.cc (call_signal_handler): Provide ucontext_t
>>>     parameter to signal handler function.
>>
>> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>>
>>    else
>>      RtlCaptureContext();
>>
>> On second thought, calling RtlCaptureContext here is probably wrong.
>>
>> What we really need is the context of the thread when calling
>> call_signal_handler I think.
>
> I had the same thought, but this is going to be quite tricky to achieve.

>> It would be better to call RtlCaptureContext
>> before calling call_signal_handler.  But this requires a change in how
>> call_signal_handler is called.
>>
>> We should discuss this at one point, I think.

I noticed that we already prepare a context for continuing after the 
signal for the debugger, so perhaps this is not quite as complex as I 
thought and something like the attached is needed.

It's very hard to reason about if this is doing the right thing when the 
signal is delivered across threads, though.


[-- Attachment #2: 0001-Use-the-same-continuation-context-in-a-signal-as-we-.patch --]
[-- Type: text/plain, Size: 1694 bytes --]

From d6385aa66e26e86832e1e95222e8026146bb63df Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Thu, 23 Apr 2015 14:45:05 +0100
Subject: [PATCH] Use the same continuation context in a signal as we would
 send to the debugger

---
 winsup/cygwin/exceptions.cc | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 4a6c21e..c97cc19 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1503,12 +1503,7 @@ _cygtls::call_signal_handler ()
 	  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);
-	    }
+	    memcpy (&context.uc_mcontext, &thread_context, sizeof(CONTEXT));
 
 	  /* FIXME: If/when sigaltstack is implemented, this will need to do
 	     something more complicated */
@@ -1549,9 +1544,10 @@ void
 _cygtls::signal_debugger (siginfo_t& si)
 {
   HANDLE th;
-  /* If si.si_cyg is set then the signal was already sent to the debugger. */
+  /* If si.si_cyg is set then the signal was caused by an exception which has
+     already been sent to the debugger. */
   if (isinitialized () && !si.si_cyg && (th = (HANDLE) *this)
-      && being_debugged () && SuspendThread (th) >= 0)
+      && SuspendThread (th) >= 0)
     {
       CONTEXT c;
       c.ContextFlags = CONTEXT_FULL;
-- 
2.1.4

Re: [PATCH 2/3] Provide ucontext to signal handlers

[Corinna Vinschen]

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

Hi Jon,

On Apr 23 14:53, Jon TURNEY wrote:
> On 01/04/2015 18:36, Jon TURNEY wrote:
> >On 01/04/2015 15:22, Corinna Vinschen wrote:
> >>It would be better to call RtlCaptureContext
> >>before calling call_signal_handler.  But this requires a change in how
> >>call_signal_handler is called.
> >>
> >>We should discuss this at one point, I think.
> 
> I noticed that we already prepare a context for continuing after the signal
> for the debugger, so perhaps this is not quite as complex as I thought and
> something like the attached is needed.

signal_debugger() is (basically) called for all signals, but in case
there's no GDB attached, only signals for which a signal handler
function is called need the context.  Isn't it a bit heavyweight to
suspend and capture the context for all signals then, perhaps?

> It's very hard to reason about if this is doing the right thing when the
> signal is delivered across threads, though.

Indeed.


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 --]