RE: [RFC] win32-nat.c: Handle EXCEPTION_INVALID_HANDLE as SIGSYS

["Pierre Muller" <muller at ics dot u-strasbg dot fr>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Christopher Faylor
> Sent: Tuesday, October 23, 2007 11:48 PM
> To: gdb-patches@sourceware.org
> Subject: Re: [RFC] win32-nat.c: Handle EXCEPTION_INVALID_HANDLE as
> SIGSYS
> 
> On Mon, Oct 22, 2007 at 03:44:01PM +0200, Pierre Muller wrote:
> >  This patch does several things,
> >that should probably be separated into several smaller
> >patches, but I send it here together as a RFC.
> >
> >  The patch mainly fixes the problems described in the thread starting
> with:
> >http://sourceware.org/ml/gdb/2007-10/msg00131.html
> >
> >  The patch first implements recognition of
> >the exception EXCEPTION_INVALID_HANDLE generated
> >by a call to CloseHandle with an invalid parameter.
> >
> >  The new variable stop_on_invalid_handle,
> >which can be set via "set stoponinvalidhandle",
> >allows to either ignore that exception completely
> >(with just a comment if verbose is set, default behavior),
> >or to map it to SIGSYS, which can then be handled as any
> >target signal via "handle SIGSYS ..."
> >
> >  Combined to this I integrated the new variable
> >old_behavior, which allowed me to disable
> >all CloseHandle calls present in the win32-nat.c source
> >that resulted in later INVALID_HANDLE exception.
> >  These exceptions come from the fact that the
> >win32  API specifications say that the thread and process
> >handles are closed by the system after the
> >corresponding debug events, and thus they should never be closed by
> >the debugger itself.
> 
> So if I got this wrong and closed some handles that shouldn't have been
> closed why doesn't this patch just get rid of those cases?

  According to the win32 API specifications, it is indeed
wrong to close the thread and process handles,
as these are closed by the system when you call ContinueDebugEvent
after EXIT_THREAD_DEBUG_EVENT and EXIT_PROCESS_DEBUG_EVENT.

> This patch seems amazingly complicated to me for something which seems
> to just be trapping a behavior that is apparently inappropriate for gdb
> to be doing in the first place.  However, if there still is a need for
> these CloseHandles then it seems like there should be just one macro or
> function which does the equivalent of the many cases of
> 
> if (old_behavior)
>   CloseHandle(...)
> 
> that are sprinkled throughout this patch.

  The reason for the introduction of this old_behavior variable,
was to be able to easily retest the old behavior
(by using "set old_behavior =1" while debugging gdb itself)
in case some troubles appear  with this CloseHandle calls removed.

  If you use
(top-gdb) set stoponinvalidhandle
(top-gdb) start
(top-gdb) set old_behavior = 1

you will be able to see that indeed (at least under WinXP)
you get a EXCEPTION_INVALID_HANDLE.

  
> And, amusingly enough, this patch illustrates, within days of making
> mingw available, exactly the kind of situation I didn't want to get
> into
> by allowing gdb to build under MinGW.  It forces me to evaluate a
> multipage patch which isn't needed for Cygwin.

  If I cut my patch into three parts:
1) Handle EXCEPTION_INVALID_HANDLE exception as a SIGSYS target signal
2) Add the stoponinvalidhandle
3) Remove wrong CloseHandle calls

then, after the 1) is applied,
you will get several SIGSYS when debugging gdb with itself.
  I suppose that this would introduce lots of
new failures to the testsuite, especially in the gdb.gdb subdirectory.

  It is to avoid passing by this annoying stage
that submitted all together.

  I can submit points 1 and 2 without point 3,
as the default of point 2 is to ignore EXCEPTION_INVALID_HANDLE.

  I would really prefer to keep trace of
the old CloseHandles, in case we discover that under special
circumstances, or for some versions of the system,
these calls are needed.

  I could of course group all those under a
new function
  MaybeCloseHandle
that would use a variable named
  DoCloseThreadAndProcessHandles
that could be set to a non zero value with a new set/show command.

Any better suggestion most welcome,

Pierre Muller