[RFA 2] gdb/win32-nat.c: do not call CloseHandle on process and thread handles

["Pierre Muller" <muller@ics.u-strasbg.fr>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Christopher Faylor
> Sent: Saturday, November 24, 2007 6:44 AM
> To: gdb-patches@sourceware.org
> Subject: Re: [RFA] gdb/win32-nat.c: do not call CloseHandle on process
> and thread handles
> 
> On Fri, Nov 23, 2007 at 05:21:14PM +0100, Pierre Muller wrote:
> >  Following Christopher and Pedro's comments,
> >I added a new variable called fake_create_used
> >to record if the current_process_handle is
> >coming from OpenProcess called in fake_create_process,
> >in which case we should close the handle, or
> >coming from the CREATE_PROCESS_DEBUG_EVENT,
> >which is closed by the system.
> >
> >Pierre
> >
> >ChangeLog entry:
> >
> >2007-11-23  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> >	*win32-nat.c (fake_create_used): New static variable.
> >	(win32_init_thread_list): Remove call to CloseHandle for thread.
> >	(win32_delete_thread): Ditto.
> >	(fake_create_process): Set fake_create_used if OpenProcess call
> >	is successful.
> >	(get_win32_debug_event): Do not close process handle.
> >	(do_initial_win32_stuff): Set fake_create_used to zero.
> >	(win32_mourn_inferior): Call CloseHandle for
> current_process_handle
> >	if fake_create_process is set.
> >	(win32_kill_inferior): Do not close process and main_thread
> handles.
> 
> I'd prefer if you called the flag variable "open_process_used" since I
> think that's more descriptive.
 I changed this variable name.
 
> Also, rather than using printf_unfiltered, shouldn't you be using
> error,
> like the rest of the code?

   I don't really know where error would bring us back, that is why I did
not dare
to use it, but I also changed that part according to your suggestion.
   I tried to manually trigger the error code, and got an
error indeed, but I could still do a  'continue' after gdb 
which is probably not right either...

> And, the GNU C coding standard seems to imply that braces around single
> line statements after an `if' are not required.  Most of the code in
> win32-nat.c follows this convention so I would appreciate if you would
> eliminate this use of braces where appropriate.
> 
> Other than that, the patch looks good.
> 
> cgf


adapted ChangeLog entry:

2007-11-26  Pierre Muller  <muller@ics.u-strasbg.fr>

	*win32-nat.c (open_process_used): New static variable.
	(win32_init_thread_list): Remove call to CloseHandle for thread.
	(win32_delete_thread): Ditto.
	(fake_create_process): Set open_process_used if OpenProcess call
	is successful.
	(get_win32_debug_event): Do not close process handle.
	(do_initial_win32_stuff): Set open_process_used to zero.
	(win32_mourn_inferior): Call CloseHandle for current_process_handle
	if open_process_used is set.
	(win32_kill_inferior): Do not close process and main_thread handles.

Index: gdb/win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.140
diff -u -p -r1.140 win32-nat.c
--- gdb/win32-nat.c     24 Nov 2007 12:13:28 -0000      1.140
+++ gdb/win32-nat.c     26 Nov 2007 08:45:33 -0000
@@ -141,6 +141,7 @@ static DWORD main_thread_id;                /* Thread
 static int exception_count = 0;
 static int event_count = 0;
 static int saw_create;
+static int open_process_used = 0;

 /* User options. */
 static int new_console = 0;
@@ -325,7 +326,6 @@ win32_init_thread_list (void)
     {
       thread_info *here = th->next;
       th->next = here->next;
-      (void) CloseHandle (here->h);
       xfree (here);
     }
   thread_head.next = NULL;
@@ -350,7 +350,6 @@ win32_delete_thread (DWORD id)
     {
       thread_info *here = th->next;
       th->next = here->next;
-      CloseHandle (here->h);
       xfree (here);
     }
 }
@@ -1171,6 +1170,14 @@ fake_create_process (void)
 {
   current_process_handle = OpenProcess (PROCESS_ALL_ACCESS, FALSE,
                                        current_event.dwProcessId);
+  if (current_process_handle != NULL)
+    open_process_used = 1;
+  else
+    {
+      error (_("OpenProcess call failed, GetLastError = %lud\n"),
+       GetLastError ());
+      /*  We can not debug anything in that case.  */
+    }
   main_thread_id = current_event.dwThreadId;
   current_thread = win32_add_thread (main_thread_id,
                                     current_event.u.CreateThread.hThread);
@@ -1299,7 +1306,8 @@ get_win32_debug_event (int pid, struct t
                 thread event.  Caused when attached process does not have
                 a main thread. */
              retval = ourstatus->value.related_pid = fake_create_process
();
-             saw_create++;
+             if (retval)
+               saw_create++;
            }
          break;
        }
@@ -1333,7 +1341,6 @@ get_win32_debug_event (int pid, struct t
       CloseHandle (current_event.u.CreateProcessInfo.hFile);
       if (++saw_create != 1)
        {
-         CloseHandle (current_event.u.CreateProcessInfo.hProcess);
          break;
        }

@@ -1356,7 +1363,6 @@ 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);
       retval = main_thread_id;
       break;

@@ -1488,6 +1494,7 @@ do_initial_win32_stuff (DWORD pid)
   last_sig = TARGET_SIGNAL_0;
   event_count = 0;
   exception_count = 0;
+  open_process_used = 0;
   debug_registers_changed = 0;
   debug_registers_used = 0;
   for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++)
@@ -1886,9 +1893,6 @@ win32_create_inferior (char *exec_file,
     error (_("Error creating process %s, (error %d)."),
           exec_file, (unsigned) GetLastError ());

-  CloseHandle (pi.hThread);
-  CloseHandle (pi.hProcess);
-
   if (useshell && shell[0] != '\0')
     saw_create = -1;
   else
@@ -1904,6 +1908,11 @@ win32_mourn_inferior (void)
 {
   (void) win32_continue (DBG_CONTINUE, -1);
   i386_cleanup_dregs();
+  if (open_process_used)
+    {
+      CHECK (CloseHandle (current_process_handle));
+      open_process_used = 0;
+    }
   unpush_target (&win32_ops);
   generic_mourn_inferior ();
 }
@@ -1960,11 +1969,6 @@ win32_kill_inferior (void)
        break;
     }

-  CHECK (CloseHandle (current_process_handle));
-
-  /* this may fail in an attached process so don't check. */
-  if (current_thread && current_thread->h)
-    (void) CloseHandle (current_thread->h);
   target_mourn_inferior ();    /* or just win32_mourn_inferior? */
 }