[PATCH] run > file for win32

[PATCH] run > file for win32

[Christopher Faylor]

I've just adapted some patches from Tak Ota to win32-nat.c.

Tak's patch didn't seem to be complete but the concept was so nice that
I took some time in the last few days and finished things.

The result is that you can use shell meta characters in gdb now, so
things like 'run < foo > bar' now work.

Since this is a departure from previous behavior, I turned this is off
by default.  I added a 'set shell' command to control whether it is used
or not.

Hmm.  I forgot to document this, didn't I?  I'll submit that patch later.

Thanks you for your contribution, Tak!  I've wanted this in the windows
version of gdb for a long time.

cgf

2002-02-15  Christopher Faylor  <cgf@redhat.com>
            Tak Ota <Takaaki.Ota@am.sony.com>

	* win32-nat.c (get_image_name): New function.
	(handle_load_dll): Use get_image_name function.
	(get_child_debug_event): Avoid registering debug events until possibly
	execed process is started.
	(child_create_inferior): Allow invocation via shell so that command
	line redirection, etc.  works ok.
	(_initialize_inftarg): Add new command: "set shell" to control whether
	a shell is used to start a process.

Re: [PATCH] run > file for win32

[Pierre Muller]

At 03:39 16/02/2002 , vous avez écrit:
>I've just adapted some patches from Tak Ota to win32-nat.c.
>
>Tak's patch didn't seem to be complete but the concept was so nice that
>I took some time in the last few days and finished things.
>
>The result is that you can use shell meta characters in gdb now, so
>things like 'run < foo > bar' now work.
>
>Since this is a departure from previous behavior, I turned this is off
>by default.  I added a 'set shell' command to control whether it is used
>or not.

Christopher, 
after examining your patch, I think there is a mistake inside.

Your patch uses a shell to start the debuggee and thus uses the flag
DEBUG_PROCESS instead of DEBUG_ONLY_THIS_PROCESS.

This means that you need to sort the events and only handle the events of the 
the second process created.

  First question: 
   - is it sure that the shell will never call any process before 
running the executable that it has as argument?

To sort the events and only handles those of the process that you want to debug you use
saw_create variable.

But this variable is incremented each time a process is run.

   Starting value is -1
    shell starts value is 0
    debuggee starts value is 1 so the events are handled, here its OK.
but if the debuggee starts another process then saw_create is incremented past 1 and
no event is recorded anymore => the program is stuck....

The following patch seems to overcome this bug.


2002-01-08  Pierre Muller  <muller@ics.u-strasbg.fr>

         * win32-nat.c (main_process_id): New variable. Used to only handle the 
         events from the debuggee.
         (get_child_debug_event): Set main_process_id on the 
         CREATE_PROCESS_DEBUG_EVENT. 
         Use main_process_id to check if a given event should be handled 
         by the debugger.


Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.52
diff -u -p -r1.52 win32-nat.c
--- win32-nat.c 2002/02/19 08:49:41     1.52
+++ win32-nat.c 2002/02/20 09:52:33
@@ -124,6 +124,7 @@ 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;

  /* Counts of things. */
  static int exception_count = 0;
@@ -1122,9 +1123,9 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "CREATE_THREAD_DEBUG_EVENT"));
-      if (saw_create != 1)
-       break;
-      /* Record the existence of this thread */
+     if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
+       break;
+     /* Record the existence of this thread */
        th = child_add_thread (current_event.dwThreadId,
                              current_event.u.CreateThread.hThread);
        if (info_verbose)
@@ -1139,8 +1140,9 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "EXIT_THREAD_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
+
        child_delete_thread (current_event.dwThreadId);
        th = &dummy_thread_info;
        break;
@@ -1159,6 +1161,7 @@ get_child_debug_event (int pid, struct t

        current_process_handle = current_event.u.CreateProcessInfo.hProcess;
        main_thread_id = current_event.dwThreadId;
+      main_process_id = current_event.dwProcessId;
        /* Add the main thread */
  #if 0
        th = child_add_thread (current_event.dwProcessId,
@@ -1174,7 +1177,7 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "EXIT_PROCESS_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
        ourstatus->kind = TARGET_WAITKIND_EXITED;
        ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
@@ -1188,7 +1191,7 @@ get_child_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 ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
        catch_errors (handle_load_dll, NULL, (char *) "", RETURN_MASK_ALL);
        registers_changed ();    /* mark all regs invalid */
@@ -1202,7 +1205,7 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "UNLOAD_DLL_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
        catch_errors (handle_unload_dll, NULL, (char *) "", RETURN_MASK_ALL);
        registers_changed ();    /* mark all regs invalid */
@@ -1215,7 +1218,7 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "EXCEPTION_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
        if (handle_exception (ourstatus))
         retval = current_event.dwThreadId;
@@ -1226,14 +1229,14 @@ get_child_debug_event (int pid, struct t
                      (unsigned) current_event.dwProcessId,
                      (unsigned) current_event.dwThreadId,
                      "OUTPUT_DEBUG_STRING_EVENT"));
-      if (saw_create != 1)
+      if ((saw_create < 1) || (current_event.dwProcessId != main_process_id))
         break;
        if (handle_output_debug_string (ourstatus))
         retval = main_thread_id;
        break;

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

-  if (!retval || saw_create != 1)
+  if (!retval)
      CHECK (child_continue (continue_status, -1));
    else
      {



Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99

Re: [PATCH] run > file for win32

[Christopher Faylor]

On Wed, Feb 20, 2002 at 10:58:11AM +0100, Pierre Muller wrote:
>At 03:39 16/02/2002 , vous avez ?crit:
>>I've just adapted some patches from Tak Ota to win32-nat.c.
>>
>>Tak's patch didn't seem to be complete but the concept was so nice that
>>I took some time in the last few days and finished things.
>>
>>The result is that you can use shell meta characters in gdb now, so
>>things like 'run < foo > bar' now work.
>>
>>Since this is a departure from previous behavior, I turned this is off
>>by default.  I added a 'set shell' command to control whether it is used
>>or not.
>
>Christopher, 
>after examining your patch, I think there is a mistake inside.
>
>Your patch uses a shell to start the debuggee and thus uses the flag
>DEBUG_PROCESS instead of DEBUG_ONLY_THIS_PROCESS.
>
>This means that you need to sort the events and only handle the events of the 
>the second process created.
>
>  First question: 
>   - is it sure that the shell will never call any process before 
>running the executable that it has as argument?

No.  This is just an initial implementation.  I added hooks for doing it
better in the future.  That's the reason this isn't on by default.  I'm
sorry that I didn't make that clear.

>To sort the events and only handles those of the process that you want
>to debug you use saw_create variable.

IMO, the right way to handle this is to inspect the name of each process
as it is created.  That presents some interesting difficulties, however.
It means that gdb has to understand how the shell will find the executable,
however.

Alternatively, gdb could detect the transition from shell -> program where
the pid stays the same.  Hmm.  I think that would be the most foolproof.

I'll try to come up with some kind of solution to this that uses your method
plus the above.  It will probably take a couple of days, though.

cgf

>But this variable is incremented each time a process is run.
>
>   Starting value is -1
>    shell starts value is 0
>    debuggee starts value is 1 so the events are handled, here its OK.
>but if the debuggee starts another process then saw_create is incremented past 1 and
>no event is recorded anymore => the program is stuck....
>
>The following patch seems to overcome this bug.
>
>
>2002-01-08  Pierre Muller  <muller@ics.u-strasbg.fr>
>
>         * win32-nat.c (main_process_id): New variable. Used to only handle the 
>         events from the debuggee.
>         (get_child_debug_event): Set main_process_id on the 
>         CREATE_PROCESS_DEBUG_EVENT. 
>         Use main_process_id to check if a given event should be handled 
>         by the debugger.

Re: [PATCH] run > file for win32

[Pierre Muller]

> >To sort the events and only handles those of the process that you want
> >to debug you use saw_create variable.
>
>IMO, the right way to handle this is to inspect the name of each process
>as it is created.  That presents some interesting difficulties, however.
>It means that gdb has to understand how the shell will find the executable,
>however.
>
>Alternatively, gdb could detect the transition from shell -> program where
>the pid stays the same.  Hmm.  I think that would be the most foolproof.

But that is not the case... the windows pid does change when you pass from the 
shell to the debuggee. My patch is even based on that change.

>I'll try to come up with some kind of solution to this that uses your method
>plus the above.  It will probably take a couple of days, though.

As the win32 API clearly says that the lpImageName field of the
CREATE_PROCESS_DEBUG_EVENT can be null,
this is not an option.

May I suggest using GetFileInformationByHandle
function?

  If you open the file in the child_create_inferior function
and leave it open until the program is killed or exits,
then the nFileIndexHigh,nFileIndexLow and dwVolumeSerialNumber
identifies uniquely the file.
Using the handle given by the CREATE_PROCESS_DEBUG_EVENT
and calling again GetFileInformationByHandle
should be able to check if the correct file is used.

Remarks:
   -- if a different file (in some other dir) is found,
it will need to be handled somehow...

   -- Even with my patch, there is still a 
problem: the shell does not exit (because 
there is one more ContinueDebugEvent needed to 
the shell exit also, that is at least the feeling I had...)
So after runnig the same program 10 times, you have got 10 zomby shells.

  -- I don't know if this is a problem with my configuration,
but the getenv("SHELL") returns NULL in _initialize_inftarg 
while I do have SHELL=/bin/bash
in the bash that I use as shell...

  --  This reminds me of one more question:
do all shells accept  "exec" command and -c option ?
  
   -- We need to be very careful about the different handles that I given
by the different events, failing to close some might 
result in problems later.

  Finally, wouldn't it be wise to set the default value of useshell to 0
until this problem is fixed?
Currently the variable is set to 1 in _initialize_inftarg
if a shell is found.



Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99

Re: [PATCH] run > file for win32

[Christopher Faylor]

On Wed, Feb 20, 2002 at 05:40:24PM +0100, Pierre Muller wrote:
>
>> >To sort the events and only handles those of the process that you want
>> >to debug you use saw_create variable.
>>
>>IMO, the right way to handle this is to inspect the name of each process
>>as it is created.  That presents some interesting difficulties, however.
>>It means that gdb has to understand how the shell will find the executable,
>>however.
>>
>>Alternatively, gdb could detect the transition from shell -> program where
>>the pid stays the same.  Hmm.  I think that would be the most foolproof.
>
>But that is not the case... the windows pid does change when you pass from the 
>shell to the debuggee. My patch is even based on that change.

I was talking about the cygwin pid.  It doesn't change across an exec, just like
UNIX.

>>I'll try to come up with some kind of solution to this that uses your method
>>plus the above.  It will probably take a couple of days, though.
>
>As the win32 API clearly says that the lpImageName field of the
>CREATE_PROCESS_DEBUG_EVENT can be null, this is not an option.

We have methods for dealing with this, though.  They aren't implemented
for this case but they are for the situation where we're attaching a
process.

I'd like to eventually record the name of the debugged process using
these methods.

>May I suggest using GetFileInformationByHandle function?
>
>If you open the file in the child_create_inferior function and leave it
>open until the program is killed or exits,

The problem is, as I mentioned, we don't know what the file is named.
The fact that you are debugging 'foo.exe' doesn't mean that the actual
file is ./foo.exe.

Detecting when windows pid changes and the cygwin pid stays the same
seems to be a foolproof method for dealing with this.

>-- I don't know if this is a problem with my configuration, but the
>getenv("SHELL") returns NULL in _initialize_inftarg while I do have
>SHELL=/bin/bash in the bash that I use as shell...

If you are saying that getenv isn't working, then there is certainly
a problem somewhere.

>  --  This reminds me of one more question:
>do all shells accept  "exec" command and -c option ?

If they don't then the user will have to use the "set shell off" option.  
fork-child.c seems to assume that -c is universal, however.

>-- We need to be very careful about the different handles that I given
>by the different events, failing to close some might result in problems
>later.

I kinda thought I was careful about this.

>  Finally, wouldn't it be wise to set the default value of useshell to 0
>until this problem is fixed?
>Currently the variable is set to 1 in _initialize_inftarg
>if a shell is found.

Actually, I'm going to turn it off by default even when this is fixed.

cgf

Re: [PATCH] run > file for win32

[muller]

At 11:58 20/02/02 -0500, Christopher Faylor wrote:
>On Wed, Feb 20, 2002 at 05:40:24PM +0100, Pierre Muller wrote:
>>
>>> >To sort the events and only handles those of the process that you want
>>> >to debug you use saw_create variable.
>>>
>>>IMO, the right way to handle this is to inspect the name of each process
>>>as it is created.  That presents some interesting difficulties, however.
>>>It means that gdb has to understand how the shell will find the executable,
>>>however.
>>>
>>>Alternatively, gdb could detect the transition from shell -> program where
>>>the pid stays the same.  Hmm.  I think that would be the most foolproof.
>>
>>But that is not the case... the windows pid does change when you pass
from the 
>>shell to the debuggee. My patch is even based on that change.
>
>I was talking about the cygwin pid.  It doesn't change across an exec,
just like
>UNIX.
OK, but it must also work for non-cygwin executables.

>>>I'll try to come up with some kind of solution to this that uses your
method
>>>plus the above.  It will probably take a couple of days, though.
>>
>>As the win32 API clearly says that the lpImageName field of the
>>CREATE_PROCESS_DEBUG_EVENT can be null, this is not an option.
>
>We have methods for dealing with this, though.  They aren't implemented
>for this case but they are for the situation where we're attaching a
>process.
>
>I'd like to eventually record the name of the debugged process using
>these methods.
>
>>May I suggest using GetFileInformationByHandle function?
>>
>>If you open the file in the child_create_inferior function and leave it
>>open until the program is killed or exits,
>
>The problem is, as I mentioned, we don't know what the file is named.
>The fact that you are debugging 'foo.exe' doesn't mean that the actual
>file is ./foo.exe.

I am not sure this is true,
when I tried it, the name of the executable was always a
fully qualified name, it probably expanded before.
Its already so if you type 'info file'.

>Detecting when windows pid changes and the cygwin pid stays the same
>seems to be a foolproof method for dealing with this.
>
>>-- I don't know if this is a problem with my configuration, but the
>>getenv("SHELL") returns NULL in _initialize_inftarg while I do have
>>SHELL=/bin/bash in the bash that I use as shell...
>
>If you are saying that getenv isn't working, then there is certainly
>a problem somewhere.
 I will try to investigatee that a little.

>>  --  This reminds me of one more question:
>>do all shells accept  "exec" command and -c option ?
>
>If they don't then the user will have to use the "set shell off" option.  
>fork-child.c seems to assume that -c is universal, however.
But maybe without 'exec'?

>>-- We need to be very careful about the different handles that I given
>>by the different events, failing to close some might result in problems
>>later.
>
>I kinda thought I was careful about this.

I didn't look closely. Its probably perfectly allright
if you already tought about this possible problem.

>>  Finally, wouldn't it be wise to set the default value of useshell to 0
>>until this problem is fixed?
>>Currently the variable is set to 1 in _initialize_inftarg
>>if a shell is found.
>
>Actually, I'm going to turn it off by default even when this is fixed.

Its probably the wisest solution.
But I just committed ingdb.texinfo that its on by default,
so don't forget to change that also.