[RFC] win32-nat.c: Improve handling of 'set shell on'

[RFC] win32-nat.c: Improve handling of 'set shell on'

[Pierre Muller]

  I recently discovered that if you use
'set shell on' on cygwin native, there are 
some specific problems that appear.

  I ran the testsuite with a single line 
modified:
-static int useshell = 0;               /* use shell for subprocesses */
+static int useshell = 1;               /* use shell for subprocesses */
Which made the use of shell to start the subprocess
the default.
  The testuite results gave this:

--- gdb-cvs.sum 2008-01-08 11:09:16.684999100 +0100
+++ gdb-useshell.sum    2008-01-08 12:22:11.434999100 +0100
<<<< Middle cut >>>
                === gdb Summary ===

-# of expected passes           10522
-# of unexpected failures       514
+# of expected passes           10505
+# of unexpected failures       534
 # of expected failures         59
 # of known failures            31
 # of unresolved testcases      38

 Showing that you get 20 unexpected failures
more with 'use shell'
 I finally traced the problem down to the fact that
saw_create can sometimes be messed up by 
executables that launch other subprocesses.
 After adding two new variables holding the main process
id and the shell process id, I changed the code to check that the processed
of the 
event reported matches the main_process_id, otherwise
only checking for an EXIT_PROCESS_DEBUG_EVENT for shell_process_id,
even though I do not know if it would be acceptable
that the calling shell exits before the main process.

 After that change the testsuite results are much closer
to the results I get without using 'set shell on':
                === gdb Summary ===

-# of expected passes           10522
-# of unexpected failures       514
+# of expected passes           10517
+# of unexpected failures       519
 # of expected failures         59
 # of known failures            31
 # of unresolved testcases      38

Only 5 more failures instead of 20.

I send below the patch as I used it to check the testsuite results,
the second change, which sets the use of shell as a default,
would of course not be in my final patch...

Pierre Muller

PS: Is it possible to run the testsuite with a custom
script added ? This would facilitate testing of
other such options like 'set new-group'?
  The testsuite uses --nx option which disables
reading of .gdbinit file, but is it possible to 
add some other default option?
like "-x MySpecficOptionsFile".

 Can that be set? How?
Which tests would fail because of this?


ChangeLog entry:

2008-01-09  Pierre Muller  <muller@ics.u-strasbg.fr>

	* win32-nat.c (main_process_id, shell_process_id) New variables.
	// Not to be committed: Set default value of useshell to 1
	(fake_create_process): Set main_process_id.
	(get_win32_debug_event): only handle event if 
	current_event.dwProcessId is equal to main_process_id,
	and EXIT_PROCESS_DEBUG_EVENT event if
	current_event.dwProcessId is equal to shell_process_id.
	(do_initial_win32_stuff) Set shell_process_id or
	main_process_id depending on saw_create value.
	(win32_mourn_inferior) wait until EXIT_PROCESS_DEBUG_EVENT
	for shell_process_id is seen if shell_process_id is set.
	(win32_kill_inferior) wait for EXIT_PROCESS_DEBUG_EVENT
	for main_process_id and shell_process_id if shell_process_id is set.

Index: gdb/win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.146
diff -u -p -r1.146 win32-nat.c
--- gdb/win32-nat.c     6 Jan 2008 06:59:14 -0000       1.146
+++ gdb/win32-nat.c     9 Jan 2008 08:38:57 -0000
@@ -137,6 +137,8 @@ static DEBUG_EVENT current_event;   /* The
 static HANDLE current_process_handle;  /* Currently executing process */
 static thread_info *current_thread;    /* Info on currently selected thread
*/
 static DWORD main_thread_id;           /* Thread ID of the main thread */
+static DWORD main_process_id;          /* Process ID of debugged process */
+static DWORD shell_process_id;         /* Process ID of the shell */

 /* Counts of things. */
 static int exception_count = 0;
@@ -154,7 +156,7 @@ static int debug_exec = 0;          /* show exec
 static int debug_events = 0;           /* show events from kernel */
 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 useshell = 1;               /* use shell for subprocesses */

 /* This vector maps GDB's idea of a register's number into an address
    in the win32 exception context vector.
@@ -1176,6 +1178,7 @@ fake_create_process (void)
       /*  We can not debug anything in that case.  */
     }
   main_thread_id = current_event.dwThreadId;
+  main_process_id = current_event.dwProcessId;
   current_thread = win32_add_thread (main_thread_id,
                                     current_event.u.CreateThread.hThread);
   return main_thread_id;
@@ -1295,19 +1298,23 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "CREATE_THREAD_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if (saw_create < 1)
        {
          if (!saw_create && attach_flag)
            {
              /* Kludge around a Windows bug where first event is a create
                 thread event.  Caused when attached process does not have
                 a main thread. */
+
              retval = ourstatus->value.related_pid = fake_create_process
();
-            if (retval)
-              saw_create++;
+             if (retval)
+               saw_create++;
            }
          break;
        }
+      if (current_event.dwProcessId != main_process_id)
+       break;
+
       /* Record the existence of this thread */
       th = win32_add_thread (current_event.dwThreadId,
                             current_event.u.CreateThread.hThread);
@@ -1323,6 +1330,8 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "EXIT_THREAD_DEBUG_EVENT"));
+      if (current_event.dwProcessId != main_process_id)
+       break;
       if (current_event.dwThreadId != main_thread_id)
        {
          win32_delete_thread (current_event.dwThreadId);
@@ -1339,6 +1348,7 @@ get_win32_debug_event (int pid, struct t
       if (++saw_create != 1)
        break;

+      main_process_id = current_event.dwProcessId;
       current_process_handle = current_event.u.CreateProcessInfo.hProcess;
       if (main_thread_id)
        win32_delete_thread (main_thread_id);
@@ -1354,8 +1364,16 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "EXIT_PROCESS_DEBUG_EVENT"));
-      if (saw_create != 1)
-       break;
+      if (current_event.dwProcessId != main_process_id)
+       {
+         if (saw_create > 1)
+           --saw_create;
+         if (current_event.dwProcessId == shell_process_id)
+           shell_process_id = 0;
+         break;
+       }
+      if (shell_process_id)
+       main_process_id = 0;
       ourstatus->kind = TARGET_WAITKIND_EXITED;
       ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
       retval = main_thread_id;
@@ -1367,7 +1385,7 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwThreadId,
                     "LOAD_DLL_DEBUG_EVENT"));
       CloseHandle (current_event.u.LoadDll.hFile);
-      if (saw_create != 1)
+      if (current_event.dwProcessId != main_process_id)
        break;
       catch_errors (handle_load_dll, NULL, (char *) "", RETURN_MASK_ALL);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
@@ -1380,7 +1398,7 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "UNLOAD_DLL_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if (current_event.dwProcessId != main_process_id)
        break;
       catch_errors (handle_unload_dll, NULL, (char *) "", RETURN_MASK_ALL);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
@@ -1393,7 +1411,7 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "EXCEPTION_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if (current_event.dwProcessId != main_process_id)
        break;
       switch (handle_exception (ourstatus))
        {
@@ -1415,13 +1433,13 @@ get_win32_debug_event (int pid, struct t
                     (unsigned) current_event.dwProcessId,
                     (unsigned) current_event.dwThreadId,
                     "OUTPUT_DEBUG_STRING_EVENT"));
-      if (saw_create != 1)
+      if (current_event.dwProcessId != main_process_id)
        break;
       retval = handle_output_debug_string (ourstatus);
       break;

     default:
-      if (saw_create != 1)
+      if (current_event.dwProcessId != main_process_id)
        break;
       printf_unfiltered ("gdb: kernel event for pid=%ld tid=%ld\n",
                         (DWORD) current_event.dwProcessId,
@@ -1431,7 +1449,7 @@ get_win32_debug_event (int pid, struct t
       break;
     }

-  if (!retval || saw_create != 1)
+  if (!retval || saw_create < 1)
     {
       if (continue_status == -1)
        win32_resume (ptid, 0, 1);
@@ -1486,6 +1504,14 @@ do_initial_win32_stuff (DWORD pid)
   extern int stop_after_trap;
   int i;

+  main_process_id = 0;
+  main_thread_id = 0;
+  shell_process_id = 0;
+
+  if (saw_create == -1)
+    shell_process_id = pid;
+  else
+    main_process_id = pid;
   last_sig = TARGET_SIGNAL_0;
   event_count = 0;
   exception_count = 0;
@@ -1893,7 +1919,39 @@ win32_create_inferior (char *exec_file,
 static void
 win32_mourn_inferior (void)
 {
+  /* If shell_process_id is set, we need to wait until
+     EXIT_PROCESS_DEBUG_EVENT for the shell process comes.  */
+  if (shell_process_id)
+    {
+      for (;;)
+       {
+         if (!win32_continue (DBG_CONTINUE, -1))
+           break;
+         if (!WaitForDebugEvent (&current_event, INFINITE))
+           break;
+         if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
+           {
+             DEBUG_EVENTS (("gdb: kernel event for pid=%d tid=%d
code=%s)\n",
+                           (unsigned) current_event.dwProcessId,
+                           (unsigned) current_event.dwThreadId,
+                           "EXIT_PROCESS_DEBUG_EVENT"));
+             if (current_event.dwProcessId == shell_process_id)
+               shell_process_id = 0;
+             if (current_event.dwProcessId == main_process_id)
+               main_process_id = 0;
+             if (main_process_id == 0 && shell_process_id == 0)
+               break;
+           }
+         else
+           DEBUG_EVENTS (("gdb: kernel event for pid=%d tid=%d code=%d)\n",
+                         (unsigned) current_event.dwProcessId,
+                         (unsigned) current_event.dwThreadId,
+                         (unsigned) current_event.dwDebugEventCode));
+        }
+    };
+
   (void) win32_continue (DBG_CONTINUE, -1);
+
   i386_cleanup_dregs();
   if (open_process_used)
     {
@@ -1956,7 +2014,25 @@ win32_kill_inferior (void)
       if (!WaitForDebugEvent (&current_event, INFINITE))
        break;
       if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
-       break;
+       {
+         DEBUG_EVENTS (("gdb: kernel event for pid=%d tid=%d code=%s)\n",
+                       (unsigned) current_event.dwProcessId,
+                       (unsigned) current_event.dwThreadId,
+                       "EXIT_PROCESS_DEBUG_EVENT"));
+
+         if (current_event.dwProcessId == shell_process_id)
+           shell_process_id = 0;
+         if (current_event.dwProcessId == main_process_id)
+           main_process_id = 0;
+         if (main_process_id == 0 && shell_process_id == 0)
+           break;
+       }
+      else
+       DEBUG_EVENTS (("gdb: kernel event for pid=%d tid=%d code=%d)\n",
+                     (unsigned) current_event.dwProcessId,
+                     (unsigned) current_event.dwThreadId,
+                     (unsigned) current_event.dwDebugEventCode));
+
     }

   target_mourn_inferior ();    /* or just win32_mourn_inferior? */

Re: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Daniel Jacobowitz]

On Wed, Jan 09, 2008 at 10:04:35AM +0100, Pierre Muller wrote:
> PS: Is it possible to run the testsuite with a custom
> script added ? This would facilitate testing of
> other such options like 'set new-group'?
>   The testsuite uses --nx option which disables
> reading of .gdbinit file, but is it possible to 
> add some other default option?
> like "-x MySpecficOptionsFile".

Try runtest GDBFLAGS="-nx -ex 'set new-group 1'".

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Eli Zaretskii]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Wed, 9 Jan 2008 10:04:35 +0100
> 
>   I recently discovered that if you use
> 'set shell on' on cygwin native, there are 
> some specific problems that appear.
> 
>   I ran the testsuite with a single line 
> modified:
> -static int useshell = 0;               /* use shell for subprocesses */
> +static int useshell = 1;               /* use shell for subprocesses */
> Which made the use of shell to start the subprocess
> the default.

Is this really appropriate for the native Windows port of GDB as well
as for Cygwin?  If not, we should set this only conditionally, I
think.

Re: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Joel Brobecker]

> >   I ran the testsuite with a single line 
> > modified:
> > -static int useshell = 0;               /* use shell for subprocesses */
> > +static int useshell = 1;               /* use shell for subprocesses */
> > Which made the use of shell to start the subprocess
> > the default.
> 
> Is this really appropriate for the native Windows port of GDB as well
> as for Cygwin?  If not, we should set this only conditionally, I
> think.

I understood that this part of the patch was NOT to be committed
(from a // comment in the ChangeLog IIRC). After having explained
how the patch was tested, I would have excluded that hunk from
the patch, IMO.

-- 
Joel

Re: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Christopher Faylor]

On Wed, Jan 09, 2008 at 08:54:51PM +0200, Eli Zaretskii wrote:
>> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
>> Date: Wed, 9 Jan 2008 10:04:35 +0100
>> 
>>   I recently discovered that if you use
>> 'set shell on' on cygwin native, there are 
>> some specific problems that appear.
>> 
>>   I ran the testsuite with a single line 
>> modified:
>> -static int useshell = 0;               /* use shell for subprocesses */
>> +static int useshell = 1;               /* use shell for subprocesses */
>> Which made the use of shell to start the subprocess
>> the default.
>
>Is this really appropriate for the native Windows port of GDB as well
>as for Cygwin?

No.  In fact, this really doesn't work all that well at all, AFAIK.  The
current code represents my initial attempt to get it working but there
were problems which led me to believe that I needed to add some help in
Cygwin itself.  I think that the main reason for wanting to use the
shell would be to interpret redirection and quoting characters and, last
I checked, that was not right.  That's why it defaults to false.

cgf

Re: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Eli Zaretskii]

> Date: Wed, 9 Jan 2008 20:52:43 -0500
> From: Christopher Faylor <cgf-use-the-mailinglist-please@sourceware.org>
> 
> I think that the main reason for wanting to use the shell would be
> to interpret redirection and quoting characters

Yes, that's the main reason.

RE: [RFC] win32-nat.c: Improve handling of 'set shell on'

[Pierre Muller]

Joel is right,
  I just sent the patch with that line so that
by just applying the patch, 
someone could really test the effect of the patch.
  If you remove that line, the default stays to
not use shell, and thus running the testsuite
does not show any change, as there are no tests
using 'set shell on'.

  In fact, as Daniel explained in his first 
answer to this thread, it is possible to
run the testsuite with
  make check "RUNTESTFLAGS=GDBFLAGS=\"--nx -ex 'set shell on'\""
which removes the need of changing the default value
to useshell static variable.

  Anyhow, this is only a RFC.
Christopher, do you want me to resend another RFC
or will you give your comments on that one?

Pierre