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

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

[Pierre Muller]

  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.

  I also changed the type of handle_exception
function, to try to better explain the behavior
of that code.
  The resulting behavior is still non trivial,
especially, I found that I still needed to differentiate
between the Cygwin Kernel32!IsBad check that calls
directly win32_continue, and the unknown_exception
handling that uses win32_resume. Trying to call
win32_continue directly in that case did not
work as expected.

  I tried to check that no handle was left
open, but a separate tool would probably 
be needed for that.
  I tested kill and got no invalid handle exceptions
within GDB with the disabled calls introduced
with the old_behavior variable.
  
  Comments welcome,

Pierre Muller


ChangeLog entry:

2007-10-22  Pierre Muller  <muller@ics.u-strasbg.fr>

	* win32-nat.c (stop_on_invalid_handle): New variable.
	(old_behavior): New variable.
	(win32_delete_thread): Do not close thread handle.
	(handle_output_debug_string): Add output if verbose is set.
	(exception_answer): New enum.
	(handle_exception): Change return type to enum exception_answer.
	(win32_continue): Clarify debug event output.
	Move call to SetThreadContext before ResumeThread calls.
	Check ResumeThread return values.
	(get_win32_debug_event): Check return from WaitForDebugEvent.
	Do not close process handle on EXIT_PROCESS_DEBUG_EVENT.
	Adapt code to new return type of handle_exception function.
	(initialize_win32_nat): Add "set/show StopOnInvalidHandle" commands.





Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.138
diff -u -p -r1.138 win32-nat.c
--- win32-nat.c	16 Oct 2007 18:43:24 -0000	1.138
+++ win32-nat.c	22 Oct 2007 13:06:57 -0000
@@ -152,6 +152,8 @@ static int debug_events = 0;		/* show ev
 static int debug_memory = 0;		/* show target memory accesses */
 static int debug_exceptions = 0;	/* show target exceptions */
 static int useshell = 0;		/* use shell for subprocesses */
+static int stop_on_invalid_handle = 0;  /* stop on STATUS_INVALID_HANDLE */
+static int old_behavior = 0;
 
 /* This vector maps GDB's idea of a register's number into an address
    in the win32 exception context vector.
@@ -316,7 +318,8 @@ win32_init_thread_list (void)
     {
       thread_info *here = th->next;
       th->next = here->next;
-      (void) CloseHandle (here->h);
+      if (old_behavior) 
+        (void) CloseHandle (here->h);
       xfree (here);
     }
   thread_head.next = NULL;
@@ -341,7 +344,13 @@ win32_delete_thread (DWORD id)
     {
       thread_info *here = th->next;
       th->next = here->next;
-      CloseHandle (here->h);
+      if (old_behavior) 
+	{
+          if (info_verbose)
+            printf_unfiltered ("Closing thread handle (h=0x%lx)\n", 
+	      (DWORD) here->h);
+          CloseHandle (here->h);
+	}
       xfree (here);
     }
 }
@@ -822,8 +831,13 @@ handle_output_debug_string (struct targe
   if (!target_read_string
     ((CORE_ADDR) current_event.u.DebugString.lpDebugStringData, &s, 1024,
0)
       || !s || !*s)
-    /* nothing to do */;
-  else if (strncmp (s, _CYGWIN_SIGNAL_STRING, sizeof
(_CYGWIN_SIGNAL_STRING) - 1) != 0)
+    /* nothing to do */
+    return retval;
+  if (info_verbose) 
+    {
+      printf_unfiltered ("Debug string: \"%s\"\n", s);
+    }  
+  if (strncmp (s, _CYGWIN_SIGNAL_STRING, sizeof (_CYGWIN_SIGNAL_STRING) -
1) != 0)
     {
 #ifdef __CYGWIN__
       if (strncmp (s, "cYg", 3) != 0)
@@ -989,7 +1003,15 @@ info_w32_command (char *args, int from_t
   printf_unfiltered ("gdb: Target exception %s at 0x%08lx\n", x, \
   (DWORD) current_event.u.Exception.ExceptionRecord.ExceptionAddress)
 
-static int
+enum exception_answer
+{
+  pass_unknown_exception_to_inferior_directly = -1,
+  pass_exception_to_inferior_directly = 0,
+  stop_on_exception_as_signalled = 1,
+  ignore_exception_and_continue = 2
+};
+
+static enum exception_answer
 handle_exception (struct target_waitstatus *ourstatus)
 {
   thread_info *th;
@@ -1018,7 +1040,7 @@ handle_exception (struct target_waitstat
 	if ((!cygwin_exceptions && (addr >= cygwin_load_start && addr <
cygwin_load_end))
 	    || (find_pc_partial_function (addr, &fn, NULL, NULL)
 		&& strncmp (fn, "KERNEL32!IsBad", strlen ("KERNEL32!IsBad"))
== 0))
-	  return 0;
+	  return pass_exception_to_inferior_directly;
       }
 #endif
       break;
@@ -1094,10 +1116,26 @@ handle_exception (struct target_waitstat
       DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_NONCONTINUABLE_EXCEPTION");
       ourstatus->value.sig = TARGET_SIGNAL_ILL;
       break;
-    default:
+    case STATUS_INVALID_HANDLE: 
+      if (stop_on_invalid_handle) {
+          /* Map this to bad SysCall */
+	  DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_INVALID_HANDLE");
+          ourstatus->value.sig = TARGET_SIGNAL_SYS;
+          break;
+        }
+      else
+        {
+          printf_unfiltered ("Ignoring EXCEPTION_INVALID_HANDLE
exception\n");
+          return ignore_exception_and_continue;
+        }
+      break; 
+   default:
       /* Treat unhandled first chance exceptions specially. */
       if (current_event.u.Exception.dwFirstChance)
-	return -1;
+	{
+	  DEBUG_EXCEPTION_SIMPLE ("unknown (first chance)");
+	  return pass_unknown_exception_to_inferior_directly;
+	}
       printf_unfiltered ("gdb: unknown target exception 0x%08lx at
0x%08lx\n",
 		    current_event.u.Exception.ExceptionRecord.ExceptionCode,
 	(DWORD) current_event.u.Exception.ExceptionRecord.ExceptionAddress);
@@ -1106,7 +1144,7 @@ handle_exception (struct target_waitstat
     }
   exception_count++;
   last_sig = ourstatus->value.sig;
-  return 1;
+  return stop_on_exception_as_signalled;
 }
 
 /* Resume all artificially suspended threads if we are continuing
@@ -1117,11 +1155,20 @@ win32_continue (DWORD continue_status, i
   int i;
   thread_info *th;
   BOOL res;
-
+  char *status_name;
+  switch (continue_status) { 
+  case  DBG_CONTINUE:
+    status_name = "DBG_CONTINUE";
+    break;
+  case DBG_EXCEPTION_NOT_HANDLED:
+    status_name = "DBG_EXCEPTION_NOT_HANDLED";
+    break;
+  default:
+    printf_unfiltered("Unknown continue status 0x%lx\n", continue_status);
+  }
   DEBUG_EVENTS (("ContinueDebugEvent (cpid=%ld, ctid=%ld, %s);\n",
 		  current_event.dwProcessId, current_event.dwThreadId,
-		  continue_status == DBG_CONTINUE ?
-		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
+		  status_name));
   res = ContinueDebugEvent (current_event.dwProcessId,
 			    current_event.dwThreadId,
 			    continue_status);
@@ -1129,10 +1176,6 @@ win32_continue (DWORD continue_status, i
     for (th = &thread_head; (th = th->next) != NULL;)
       if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
 	{
-
-	  for (i = 0; i < th->suspend_count; i++)
-	    (void) ResumeThread (th->h);
-	  th->suspend_count = 0;
 	  if (debug_registers_changed)
 	    {
 	      /* Only change the value of the debug registers */
@@ -1147,8 +1190,24 @@ win32_continue (DWORD continue_status, i
 	      CHECK (SetThreadContext (th->h, &th->context));
 	      th->context.ContextFlags = 0;
 	    }
-	}
 
+	  for (i = 0; i < th->suspend_count; i++)
+	    {
+	      res = ResumeThread (th->h);
+	      if (res == -1) 
+		{
+		  printf_unfiltered ("ResumeThread call failed %lu\n",
+		    GetLastError ());
+		}
+	      if (info_verbose && (i != res))
+		{
+		  printf_unfiltered ("Suspend count mismatch in
win32_continue\n");
+		}
+	      if (res == 1)
+		break;
+	    }
+	  th->suspend_count = 0;
+	}
   debug_registers_changed = 0;
   return res;
 }
@@ -1259,13 +1318,21 @@ get_win32_debug_event (int pid, struct t
   thread_info *th;
   static thread_info dummy_thread_info;
   int retval = 0;
+  int ignore_exception = 0;
   ptid_t ptid = {-1};
 
   last_sig = TARGET_SIGNAL_0;
 
-  if (!(debug_event = WaitForDebugEvent (&current_event, 1000)))
-    goto out;
+  debug_event = WaitForDebugEvent (&current_event, 1000);
 
+  if (!debug_event) 
+    {
+      DWORD res = GetLastError ();
+      if (res != ERROR_SEM_TIMEOUT)
+	printf_unfiltered("WaitForDebugEvent failed. GetLastError=%lu\n",
+	  res);
+      return 0;
+    }
   event_count++;
   continue_status = DBG_CONTINUE;
 
@@ -1346,7 +1413,8 @@ get_win32_debug_event (int pid, struct t
 	break;
       ourstatus->kind = TARGET_WAITKIND_EXITED;
       ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
-      CloseHandle (current_process_handle);
+      if (old_behavior)
+	CloseHandle (current_process_handle);
       retval = main_thread_id;
       break;
 
@@ -1386,14 +1454,17 @@ get_win32_debug_event (int pid, struct t
 	break;
       switch (handle_exception (ourstatus))
 	{
-	case 0:
+	case pass_exception_to_inferior_directly:
 	  continue_status = DBG_EXCEPTION_NOT_HANDLED;
 	  break;
-	case 1:
+	case stop_on_exception_as_signalled:
 	  retval = current_event.dwThreadId;
 	  break;
-	case -1:
-	  last_sig = 1;
+	case ignore_exception_and_continue:
+	  ignore_exception = 1;
+	  break;
+	case pass_unknown_exception_to_inferior_directly:
+	  last_sig = TARGET_SIGNAL_UNKNOWN;
 	  continue_status = -1;
 	  break;
 	}
@@ -1422,8 +1493,10 @@ get_win32_debug_event (int pid, struct t
 
   if (!retval || saw_create != 1)
     {
-      if (continue_status == -1)
-	win32_resume (ptid, 0, 1);
+      if (ignore_exception) 
+	win32_resume (ptid, 0, TARGET_SIGNAL_0);
+      else if (continue_status == -1)
+	win32_resume (ptid, 0, TARGET_SIGNAL_UNKNOWN);
       else
 	CHECK (win32_continue (continue_status, -1));
     }
@@ -1949,11 +2022,11 @@ win32_kill_inferior (void)
       if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
 	break;
     }
-
-  CHECK (CloseHandle (current_process_handle));
+  if (old_behavior)
+    CHECK (CloseHandle (current_process_handle));
 
   /* this may fail in an attached process so don't check. */
-  if (current_thread && current_thread->h)
+  if (old_behavior && current_thread && current_thread->h)
     (void) CloseHandle (current_thread->h);
   target_mourn_inferior ();	/* or just win32_mourn_inferior? */
 }
@@ -2173,6 +2246,15 @@ Show whether to display kernel exception
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
 
+  add_setshow_boolean_cmd ("stoponinvalidhandle", class_support,
+			   &stop_on_invalid_handle, _("\
+Set whether to stop on STATUS_INVALID_HANDLE exception."), _("\
+Show whether to stop on STATUS_INVALID_HANDLE exception."), NULL,
+			   NULL,
+			   NULL, /* FIXME: i18n: */
+			   &setlist, &showlist);
+
+
   add_prefix_cmd ("w32", class_info, info_w32_command,
 		  _("Print information specific to Win32 debugging."),
 		  &info_w32_cmdlist, "info w32 ", 0, &infolist);

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

[Eli Zaretskii]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Cc: "'Pedro Alves'" <pedro_alves@portugalmail.pt>, 	"'Daniel Jacobowitz'" <drow@false.org>
> Date: Mon, 22 Oct 2007 15:44:01 +0200
> 
>   The patch first implements recognition of
> the exception EXCEPTION_INVALID_HANDLE generated
> by a call to CloseHandle with an invalid parameter.

Thanks.

>   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 ..." 

I don't know if Chris (and others) will approve this change, but if
they do, please submit a suitable change for the manual describing
this setting, before committing the code changes.  I don't want us to
have commands that are not documented in the manual.

Thanks again for working on this.

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

[Christopher Faylor]

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?

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.

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.

cgf

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

[Daniel Jacobowitz]

On Tue, Oct 23, 2007 at 05:47:30PM -0400, Christopher Faylor wrote:
> 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?

If I've got this right - and I'm not sure I do - then the patch does
two things.  It stops closing some handles we shouldn't close, and it
adds a new feature which is useful for debugging native Windows
programs which close handles they shouldn't close (or do other bad
things).

If that's right, let's break it up into two please.  Nothing sets
old_behavior, so it's not needed - either the closes are necessary
or they aren't and I suspect most or all Windows versions will be
consistent about which it is.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Eli Zaretskii]

> Date: Tue, 23 Oct 2007 17:47:30 -0400
> From: Christopher Faylor <cgf-use-the-mailinglist-please@sourceware.org>
> 
> 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.

Which is exactly the reason I suggested to make MinGW a separate
target with a separate *-nat.c file.

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

[Pierre Muller]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Eli Zaretskii
> Sent: Wednesday, October 24, 2007 6:08 AM
> To: gdb-patches@sourceware.org
> Subject: Re: [RFC] win32-nat.c: Handle EXCEPTION_INVALID_HANDLE as
> SIGSYS
> 
> > Date: Tue, 23 Oct 2007 17:47:30 -0400
> > From: Christopher Faylor <cgf-use-the-mailinglist-
> please@sourceware.org>
> >
> > 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.
> 
> Which is exactly the reason I suggested to make MinGW a separate
> target with a separate *-nat.c file.

Why?

 - All the changes I propose should apply to both cygwin and mingw
ports.
  The only reason why there is a behavior difference between the cygwin
compiled gdb and the mingw compiled one is that
in the cygwin exception handler,
  EXCEPTION_INVALID_HANDLE is ignored, while mingw exception handler
does not handle it, and thus the problem shows up only
when debugging the mingw compiled gdb executable.


  Pierre

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

[Eli Zaretskii]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Wed, 24 Oct 2007 10:13:34 +0200
> 
> > > 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.
> > 
> > Which is exactly the reason I suggested to make MinGW a separate
> > target with a separate *-nat.c file.
> 
> Why?

Because Chris is annoyed, and I wanted to avoid that.

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

[Pedro Alves]

Eli Zaretskii wrote:
> > Date: Tue, 23 Oct 2007 17:47:30 -0400
> > From: Christopher Faylor
> >
> > 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.
>

I'll do my best to help review MinGW patches.  Often I'll need a bit more
time to get to it, and often will only be able to in weekends.  But, I'll get to
them.

> Which is exactly the reason I suggested to make MinGW a separate
> target with a separate *-nat.c file.
>

Not just yet please :)  I'm sure that when the patches will be split, they'll be
rather smallish.  Hey, it was an RFC, not an RFA!

Cheers,
Pedro Alves

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

[Pierre Muller]

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

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

[Pedro Alves]

Pierre Muller wrote:
>
>   If I cut my patch into three parts:
> 3) Remove wrong CloseHandle calls

I'd prefer if this went in first.  I'm convinced this is the correct
thing to do everywhere.

(Hey, I'm happy in that allows me to remove an ugly #ifdef in
gdbserver/win32-low.c.)

( ( ( If it isn't [the correct thing to do everywhere], then we can
follow Cygwin's lead, and
install a top level SEH handler in gdb that handles this exception.
It could have
always have been done in gdb the first place, even for Cygwin.  Remember, the
exception is only thrown iff a debugger is attached, and a debugger
can easilly not
pass it to the inferior.  That would mean 0 changes to win32-nat.c debug API
usage.  Of course, any other inferior that throws that exception will have
the same problem. ) ) )

> 1) Handle EXCEPTION_INVALID_HANDLE exception as a SIGSYS target signal
> 2) Add the stoponinvalidhandle

EXCEPTION_INVALID_HANDLE is not quite a SIGSYS.  Plus, a Cygwin inferior should
never see this, as you shouldn't be using CloseHandle in user apps there anyway.
Maybe we can take the oportunity to implement more generic Windows exceptions
support, not just EXCEPTION_INVALID_HANDLE.  Something similar to the "handle"
command.

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

Only on MinGW gdb, so they wouldn't qualify as "new" failures.  And only on
tests that load gdb in gdb, or that have a `close' with a broken argument.

>   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'd prefer we don't add such ugliness.

Cheers,
Pedro Alves

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

[Eli Zaretskii]

> Date: Wed, 24 Oct 2007 09:39:58 +0100
> From: "Pedro Alves" <pedro_alves@portugalmail.pt>
> Cc: gdb-patches@sourceware.org
> 
> Maybe we can take the oportunity to implement more generic Windows exceptions
> support, not just EXCEPTION_INVALID_HANDLE.

I think it's a good idea.  In my experience, any serious program that
wants to handle signals and exceptions on Windows cannot avoid
supporting a large number of important EXCEPTION_* exceptions, because
unlike on Posix platforms, most of them are not translated into SIG*
style signals, at least in native Windows programs (as opposed to
Cygwin).

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

[Mark Kettenis]

> Date: Wed, 24 Oct 2007 21:15:51 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Wed, 24 Oct 2007 09:39:58 +0100
> > From: "Pedro Alves" <pedro_alves@portugalmail.pt>
> > Cc: gdb-patches@sourceware.org
> > 
> > Maybe we can take the oportunity to implement more generic Windows exceptions
> > support, not just EXCEPTION_INVALID_HANDLE.
> 
> I think it's a good idea.  In my experience, any serious program that
> wants to handle signals and exceptions on Windows cannot avoid
> supporting a large number of important EXCEPTION_* exceptions, because
> unlike on Posix platforms, most of them are not translated into SIG*
> style signals, at least in native Windows programs (as opposed to
> Cygwin).

People might want to look at how we handle Mach exceptions; see
signal/signal.c and gnu-nat.c.

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

[Daniel Jacobowitz]

On Wed, Oct 24, 2007 at 09:15:51PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 24 Oct 2007 09:39:58 +0100
> > From: "Pedro Alves" <pedro_alves@portugalmail.pt>
> > Cc: gdb-patches@sourceware.org
> > 
> > Maybe we can take the oportunity to implement more generic Windows exceptions
> > support, not just EXCEPTION_INVALID_HANDLE.
> 
> I think it's a good idea.  In my experience, any serious program that
> wants to handle signals and exceptions on Windows cannot avoid
> supporting a large number of important EXCEPTION_* exceptions, because
> unlike on Posix platforms, most of them are not translated into SIG*
> style signals, at least in native Windows programs (as opposed to
> Cygwin).

If whoever works on this is feeling ambitious, I would prefer a
solution that is not too tightly linked to Windows.  I've worked with
at least two different embedded developers this past year who were
confused by the need to map platform-specific exceptions onto Unix
signals.  An embedded PowerPC developer is likely to have a much
better idea what's going on if we tell him that his code triggered
a Machine Check Exception than if we report SIGBUS.

Before we can add this to the remote protocol, the rest of GDB needs
to have some notion more general than "it's a signal".

-- 
Daniel Jacobowitz
CodeSourcery

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

[Eli Zaretskii]

> Date: Wed, 24 Oct 2007 15:37:35 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Pedro Alves <pedro_alves@portugalmail.pt>, muller@ics.u-strasbg.fr,
> 	gdb-patches@sourceware.org
> 
> > I think it's a good idea.  In my experience, any serious program that
> > wants to handle signals and exceptions on Windows cannot avoid
> > supporting a large number of important EXCEPTION_* exceptions, because
> > unlike on Posix platforms, most of them are not translated into SIG*
> > style signals, at least in native Windows programs (as opposed to
> > Cygwin).
> 
> If whoever works on this is feeling ambitious, I would prefer a
> solution that is not too tightly linked to Windows.  I've worked with
> at least two different embedded developers this past year who were
> confused by the need to map platform-specific exceptions onto Unix
> signals.  An embedded PowerPC developer is likely to have a much
> better idea what's going on if we tell him that his code triggered
> a Machine Check Exception than if we report SIGBUS.

I agree.

(Actually, a careful reading of what I wrote would show that I never
suggested to do such a mapping, I just said that it isn't done by the
underlying library.)

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

[Christopher Faylor]

On Wed, Oct 24, 2007 at 03:37:35PM -0400, Daniel Jacobowitz wrote:
>On Wed, Oct 24, 2007 at 09:15:51PM +0200, Eli Zaretskii wrote:
>> > Date: Wed, 24 Oct 2007 09:39:58 +0100
>> > From: "Pedro Alves" <pedro_alves@portugalmail.pt>
>> > Cc: gdb-patches@sourceware.org
>> > 
>> > Maybe we can take the oportunity to implement more generic Windows exceptions
>> > support, not just EXCEPTION_INVALID_HANDLE.
>> 
>> I think it's a good idea.  In my experience, any serious program that
>> wants to handle signals and exceptions on Windows cannot avoid
>> supporting a large number of important EXCEPTION_* exceptions, because
>> unlike on Posix platforms, most of them are not translated into SIG*
>> style signals, at least in native Windows programs (as opposed to
>> Cygwin).
>
>If whoever works on this is feeling ambitious, I would prefer a
>solution that is not too tightly linked to Windows.  I've worked with
>at least two different embedded developers this past year who were
>confused by the need to map platform-specific exceptions onto Unix
>signals.  An embedded PowerPC developer is likely to have a much
>better idea what's going on if we tell him that his code triggered
>a Machine Check Exception than if we report SIGBUS.
>
>Before we can add this to the remote protocol, the rest of GDB needs
>to have some notion more general than "it's a signal".

That makes sense to me too.  I suspect that even Cygwin could use this
if it was general enough.

cgf

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

[Christopher Faylor]

On Wed, Oct 24, 2007 at 09:39:58AM +0100, Pedro Alves wrote:
>Pierre Muller wrote:
>>
>>   If I cut my patch into three parts:
>> 3) Remove wrong CloseHandle calls
>
>I'd prefer if this went in first.  I'm convinced this is the correct
>thing to do everywhere.

I agree.  I thought that I noticed that these handles needed to be
closed so I'd rather that my confusion go away as soon as possible.

>(Hey, I'm happy in that allows me to remove an ugly #ifdef in
>gdbserver/win32-low.c.)
>
>( ( ( If it isn't [the correct thing to do everywhere], then we can
>follow Cygwin's lead, and
>install a top level SEH handler in gdb that handles this exception.
>It could have
>always have been done in gdb the first place, even for Cygwin.  Remember, the
>exception is only thrown iff a debugger is attached, and a debugger
>can easilly not
>pass it to the inferior.  That would mean 0 changes to win32-nat.c debug API
>usage.  Of course, any other inferior that throws that exception will have
>the same problem. ) ) )
>
>> 1) Handle EXCEPTION_INVALID_HANDLE exception as a SIGSYS target signal
>> 2) Add the stoponinvalidhandle
>
>EXCEPTION_INVALID_HANDLE is not quite a SIGSYS.  Plus, a Cygwin inferior should
>never see this, as you shouldn't be using CloseHandle in user apps there anyway.
>Maybe we can take the oportunity to implement more generic Windows exceptions
>support, not just EXCEPTION_INVALID_HANDLE.  Something similar to the "handle"
>command.

That seems like a better way to go to me.

However, even if it isn't the need to do something like:

if (something)
  DoSomething(arg)

should really make you think "Hey, I guess I'd better use a function" which
tests something and then calls DoSomething with the arg.

>> 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.
>>
>
>Only on MinGW gdb, so they wouldn't qualify as "new" failures.  And only on
>tests that load gdb in gdb, or that have a `close' with a broken argument.
>
>>   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'd prefer we don't add such ugliness.

Ditto.

cgf