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

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

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