Re: [PATCH] Make EXCEPTION_POINTERS available to signal handlers

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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

Hi Jon,

On Mar 26 15:25, Jon TURNEY wrote:
> Add ucontext.h header, defining ucontext_t and mcontext_t types.
> 
> Provide sigaction sighandlers with a ucontext_t parameter containing a
> mcontext_t with exception context information, if available.
> 
> 	* include/sys/ucontext.h (__ucontext): New header.
> 	* include/ucontext.h (_UCONTEXT_H): Ditto.
> 	* exception.h (cygwin_exception): Add exception_record accessor.
> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
> 	parameter to signal handler function, if available.

Thanks for this patch.  Looks like a good idea.

But there are a few problems we should discuss first.

> +typedef struct __mcontext {
> +	EXCEPTION_POINTERS *ep;
> +} mcontext_t;
> +
> +typedef struct __ucontext {
> +	struct __ucontext *uc_link;
> +	sigset_t	uc_sigmask;
> +	// We don't have a type stack_t, so we don't have a uc_stack member
> +	mcontext_t	uc_mcontext;
> +} ucontext_t;

* Per the Linux man page this structure should be called `struct
  ucontext' without the underscores.  However, we already have
  definitions of `struct ucontext' in include/cygwin/signal.h... which
  have nothing at all to do with the definition of ucontext on Linux.

  This needs fixing.

  Historically, the ucontext definition in cygwin/signal.h is equivalent
  to the Windows thread CONTEXT definition.  Why ever CONTEXT was used to
  define ucontext beats me in retrospect.
  
  It is used only when sending the thread context to a debugger when a
  signal occured.  The definition of struct ucontext from cygwin/signal.h
  is only used as part of struct _cygtls (struct ucontext thread_context)
  and then thread_context is used in _cygtls::signal_debugger, that's it.
  Cygwin doesn't use it anywhere else.
  
  GDB uses only the definition of __COPY_CONTEXT_SIZE from cygwin/signal.h,
  but not the definition of struct ucontext.  I sent a patch upstream to
  get rid of that dependency.

  We should remove or rename struct ucontext in cygwin/signal.h, so we
  can use that name for your above struct __ucontext.  That leads to the
  next point...

* Since struct ucontext from cygwin/signal.h is actually a redefinition
  of CONTEXT + an oldmask value. it's basically the Cygwin/Windows
  representation of gregset_t + fpregset_t + cr2 + oldmask, aka
  mcontext_t.

  As for oldmask, this can be fetched easily from _my_tls, so in theory
  we can use the current definition of ucontext from cygwin/signal.h as
  mcontext_t.

  But this drops the EXCEPTION_RECORD.  I'm not sure it belongs here.
  Keep in mind that a signal handler is not only called in case an
  exception occured.  I think the context is all a signal handler needs.

* As for stack_t, we have that.  It's defined in newlib's sys/signal.h.
  The stack base and stack size can be fetched from the TEB; with a
  test for a user-defined stack, see pthread_wrapper in miscfuncs.cc.

  While we're at it we should contemplate to define SIGSTKSZ and
  MINSIGSTKSZ along the lines of 64K, I guess.


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