Can we get rid of go32_stop? (and its normal_stop call?)

Can we get rid of go32_stop? (and its normal_stop call?)

[Pedro Alves]

Hi Eli,

go32_stop contains a call to normal_stop, that I've been
meaning to eliminate for a while --- it is the only call left
outside of infrun.c/infcmd.c.  Looking at the go32-nat.c code, I
think go32_stop has a couple of issues:

- go32_stop is registered as target_stop callback, but,
target_stop is only useful if the target supports
asynchronous execution control --- djgpp doesn't support it.

- That means (it looks to me) that the only call to go32_stop that
can happen, is from go32_create_inferior, which has this bit:

  if (prog_has_started)
    {
      go32_stop (inferior_ptid);
      go32_kill_inferior (ops);
    }

AFAICS, this is doing cleanup from a previous run.  It seems
weird to call go32_kill_inferior here, since go32_create_inferior
will only ever be called if the previous inferior is gone already,
because the "run" command always kills the previous process.  If the
previous process exited by itself, then go32_mourn_inferior is called,
which itself calls go32_kill_inferior.  AFAICS, go32_kill_inferior
is always called either when you kill the program or when it exits
cleanly, which makes me wonder if we can't remove that bit pasted
above, and, make one of go32_kill_inferior or go32_mourn_inferior
do the cleanup that go32_stop is doing, clear the prog_has_started
flag, and in the process also get rid of the normal_stop call.  It
also looks strange to have mourn_inferior call kill_inferior, and
not the other way around, so I've moved the cleaning up to
go32_mourn_inferior.

This is completely untested --- I just remembered this since you
mentioned go32_stop in the other email, and thought I'd ask
and see if I'm in the right direction.

-- 
Pedro Alves

2009-05-01  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c (go32_stop): Delete.
	(go32_kill_inferior): Rewrite to only call go32_mourn_inferior.
	(go32_create_inferior): Don't call go32_stop or
	go32_kill_inferior.
	(go32_mourn_inferior): Inline go32_stop and go32_kill_inferior
	here.
	(init_go32_ops): Don't register go32_stop.

---
 gdb/go32-nat.c |   41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c	2009-05-01 14:58:43.000000000 +0100
+++ src/gdb/go32-nat.c	2009-05-01 15:04:14.000000000 +0100
@@ -185,7 +185,6 @@ static int go32_xfer_memory (CORE_ADDR m
 			     struct mem_attrib *attrib,
 			     struct target_ops *target);
 static void go32_files_info (struct target_ops *target);
-static void go32_stop (ptid_t);
 static void go32_kill_inferior (struct target_ops *ops);
 static void go32_create_inferior (struct target_ops *ops, char *exec_file,
 				  char *args, char **env, int from_tty);
@@ -571,25 +570,9 @@ go32_files_info (struct target_ops *targ
 }
 
 static void
-go32_stop (ptid_t ptid)
-{
-  normal_stop ();
-  cleanup_client ();
-  ptid = inferior_ptid;
-  inferior_ptid = null_ptid;
-  delete_thread_silent (ptid);
-  prog_has_started = 0;
-}
-
-static void
 go32_kill_inferior (struct target_ops *ops)
 {
-  redir_cmdline_delete (&child_cmd);
-  resume_signal = -1;
-  resume_is_step = 0;
-  if (!ptid_equal (inferior_ptid, null_ptid))
-    delete_thread_silent (inferior_ptid);
-  unpush_target (&go32_ops);
+  go32_mourn_inferior (ops);
 }
 
 static void
@@ -607,11 +590,6 @@ go32_create_inferior (struct target_ops 
   if (exec_file == 0)
     exec_file = get_exec_file (1);
 
-  if (prog_has_started)
-    {
-      go32_stop (inferior_ptid);
-      go32_kill_inferior (ops);
-    }
   resume_signal = -1;
   resume_is_step = 0;
 
@@ -685,6 +663,14 @@ go32_create_inferior (struct target_ops 
 static void
 go32_mourn_inferior (struct target_ops *ops)
 {
+  ptid_t ptid;
+
+  redir_cmdline_delete (&child_cmd);
+  resume_signal = -1;
+  resume_is_step = 0;
+
+  cleanup_client ();
+
   /* We need to make sure all the breakpoint enable bits in the DR7
      register are reset when the inferior exits.  Otherwise, if they
      rerun the inferior, the uncleared bits may cause random SIGTRAPs,
@@ -693,7 +679,13 @@ go32_mourn_inferior (struct target_ops *
      at all times, but it doesn't, probably under an assumption that
      the OS cleans up when the debuggee exits.  */
   i386_cleanup_dregs ();
-  go32_kill_inferior (ops);
+
+  ptid = inferior_ptid;
+  inferior_ptid = null_ptid;
+  delete_thread_silent (ptid);
+  prog_has_started = 0;
+
+  unpush_target (ops);
   generic_mourn_inferior ();
 }
 
@@ -910,7 +902,6 @@ init_go32_ops (void)
   go32_ops.to_create_inferior = go32_create_inferior;
   go32_ops.to_mourn_inferior = go32_mourn_inferior;
   go32_ops.to_can_run = go32_can_run;
-  go32_ops.to_stop = go32_stop;
   go32_ops.to_thread_alive = go32_thread_alive;
   go32_ops.to_pid_to_str = go32_pid_to_str;
   go32_ops.to_stratum = process_stratum;

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 1 May 2009 15:14:27 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
> - go32_stop is registered as target_stop callback, but,
> target_stop is only useful if the target supports
> asynchronous execution control --- djgpp doesn't support it.

You are probably right about that.  But what will happen if to_stop is
not registered and target_stop is somehow called nonetheless?

>   if (prog_has_started)
>     {
>       go32_stop (inferior_ptid);
>       go32_kill_inferior (ops);
>     }
> 
> AFAICS, this is doing cleanup from a previous run.  It seems
> weird to call go32_kill_inferior here, since go32_create_inferior
> will only ever be called if the previous inferior is gone already,
> because the "run" command always kills the previous process.  If the
> previous process exited by itself, then go32_mourn_inferior is called,
> which itself calls go32_kill_inferior.

I'm less sure about this part.  Let me tell you something that I'm not
sure you know about how DJGPP debugging works; apologies if I'm
preaching to the choir.

When the DJGPP port of GDB is debugging a DJGPP program natively,
there are no 2 separate processes, the debuggee and GDB itself, as on
other systems.  (This is DOS, where there can only be one active
process at any given time, remember?)  Instead, GDB and the debuggee
live in the same process.  What go32_create_inferior does (in the
functions it calls from the DJGPP debug support library libdbg.a) is
load the debuggee's executable file, set it up for execution as the
stub loader (a short real-mode program prepended to each DJGPP
executable) normally would, and do a lot of preparations for swapping
between GDB's and debuggee's internal state, primarily the wrt
exception handlers.  Then running the debuggee simply means longjmp
into it where its PC is and let it run until it stops for some reason.
When it stops, GDB catches the exception that stopped it and longjmp's
back into its own code.  All the possible exit points of the debuggee
are watched; for example, the normal exit point is recognized because
a DOS program issues a special system call to exit.  If one of those
exit points is hit, we mourn the inferior and clean up after it.
Cleaning up is very important, even if the process exits normally,
because otherwise we might leave behind traces of previous execution,
and in several cases GDB itself might be left hosed, because all the
exception handlers were not restored.

So far so good.  But the plot thickens if abnormal events happen, such
as an exception inside the debuggee or just a Ctrl-C pressed by the
user at a wrong time.  We might get back to GDB before we had a chance
to clean up after the debuggee.  In this situation, can we still be
sure that go32_kill_inferior or go32_mourn_inferior would be called?
If not, then making sure they are called and the necessary cleanup is
done would not hurt anything.

> AFAICS, go32_kill_inferior is always called either when you kill the
> program or when it exits cleanly

That may be so as far as the normal flow of control is concerned.  But
given that there could be a lot of longjmp-ing around, I'm less sure.
Maybe I'm paranoiac.

> It also looks strange to have mourn_inferior call kill_inferior, and
> not the other way around, so I've moved the cleaning up to
> go32_mourn_inferior.

This is okay.

So maybe we should make these changes, but leave alone the call to
go32_kill_inferior inside go32_create_inferior, conditioned on the
prog_has_started flag, just in case.

Thanks.

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Pedro Alves]

On Friday 01 May 2009 16:47:11, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 1 May 2009 15:14:27 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>
> > 
> > - go32_stop is registered as target_stop callback, but,
> > target_stop is only useful if the target supports
> > asynchronous execution control --- djgpp doesn't support it.
> 
> You are probably right about that.  But what will happen if to_stop is
> not registered and target_stop is somehow called nonetheless?

Nothing at all.  The default implementation of to_stop
is target_ignore.

> >   if (prog_has_started)
> >     {
> >       go32_stop (inferior_ptid);
> >       go32_kill_inferior (ops);
> >     }
> > 
> > AFAICS, this is doing cleanup from a previous run.  It seems
> > weird to call go32_kill_inferior here, since go32_create_inferior
> > will only ever be called if the previous inferior is gone already,
> > because the "run" command always kills the previous process.  If the
> > previous process exited by itself, then go32_mourn_inferior is called,
> > which itself calls go32_kill_inferior.
> 

> I'm less sure about this part.  Let me tell you something that I'm not
> sure you know about how DJGPP debugging works; apologies if I'm
> preaching to the choir.

Not at all.  I knew that there are no 2 separate processes involved, but,
your description is very enlightening.  IMO, it would be nice to
paste your description below at the top of go32-nat.c.

> When the DJGPP port of GDB is debugging a DJGPP program natively,
> there are no 2 separate processes, the debuggee and GDB itself, as on
> other systems.  (This is DOS, where there can only be one active
> process at any given time, remember?)  Instead, GDB and the debuggee
> live in the same process.  What go32_create_inferior does (in the
> functions it calls from the DJGPP debug support library libdbg.a) is
> load the debuggee's executable file, set it up for execution as the
> stub loader (a short real-mode program prepended to each DJGPP
> executable) normally would, and do a lot of preparations for swapping
> between GDB's and debuggee's internal state, primarily the wrt
> exception handlers.  Then running the debuggee simply means longjmp
> into it where its PC is and let it run until it stops for some reason.
> When it stops, GDB catches the exception that stopped it and longjmp's
> back into its own code.  All the possible exit points of the debuggee
> are watched; for example, the normal exit point is recognized because
> a DOS program issues a special system call to exit.  If one of those
> exit points is hit, we mourn the inferior and clean up after it.
> Cleaning up is very important, even if the process exits normally,
> because otherwise we might leave behind traces of previous execution,
> and in several cases GDB itself might be left hosed, because all the
> exception handlers were not restored.
> 
> So far so good.  But the plot thickens if abnormal events happen, such
> as an exception inside the debuggee or just a Ctrl-C pressed by the
> user at a wrong time.  We might get back to GDB before we had a chance
> to clean up after the debuggee.  

I see.  To be clear, in this case, is the debuggee considered
dead and gone?  No further debugging possible?

> In this situation, can we still be 
> sure that go32_kill_inferior or go32_mourn_inferior would be called?

I think so.  Currently, this "just-in-case" cleanup, is done on
go32_create_inferior, so only on the next "run".  If for some reason, some
abnormal event happens, and the user is dropped into GDB's shell, the
next "run" will clean up.  But, notice one thing --- the go32_ops target
will still be pushed on the stack, and so the current target still is
considered to have execution (target_has_execution is true, and
inferior_ptid will still point at  SOME_PID).  So, when the user does "run",
GDB will ask if the user wants to kill the current program, and
will refuse to go ahead if the user says "no".  If the user says
"yes", then, target_kill is called to kill the current program
(which ends up in go32_kill_inferior), all before reaching
go32_create_inferior.

 This is run_command->kill_if_already_running->target_kill.

So, if we do the said cleaning up on go32_mourn_inferior, and
make sure that go32_mourn_inferior is called from go32_kill_inferior,
I think you'll get the same ammount of "cleanup after the fact" as we
have currently.  In fact, you'll have more --- if something goes
wild, the user will be able to do full cleaning up with the "kill"
command.  Currently, "kill" doesn't call `cleanup_client', after
my changes, it will --- I'm assuming that to be a good thing.

> If not, then making sure they are called and the necessary cleanup is
> done would not hurt anything.
> 
> > AFAICS, go32_kill_inferior is always called either when you kill the
> > program or when it exits cleanly
> 
> That may be so as far as the normal flow of control is concerned.  But
> given that there could be a lot of longjmp-ing around, I'm less sure.
> Maybe I'm paranoiac.

:-)  I think that with my proposed change, we get the same ammount
of paranoia (even more), but, this needs to be at least minimally
tested, of course.

> So maybe we should make these changes, but leave alone the call to
> go32_kill_inferior inside go32_create_inferior, conditioned on the
> prog_has_started flag, just in case.

I don't think that's required, for the reasons described above.

BTW, have you ever had any success running djgpp from inside an
emulator on linux, say, 'dosemu'?

-- 
Pedro Alves

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 1 May 2009 17:24:36 +0100
> 
> IMO, it would be nice to paste your description below at the top of
> go32-nat.c.

Okay, I will do it some time soon.

Btw, this single-process mode is also the reason why we need to play
games with redir_to_child and redir_to_debugger: the file handles are
also shared, so redirection on the "run" command line needs special
handling (and there's no shell involved in interpreting it).

> > So far so good.  But the plot thickens if abnormal events happen, such
> > as an exception inside the debuggee or just a Ctrl-C pressed by the
> > user at a wrong time.  We might get back to GDB before we had a chance
> > to clean up after the debuggee.  
> 
> I see.  To be clear, in this case, is the debuggee considered
> dead and gone?  No further debugging possible?

That's the best we can do, yes.  The debuggee is trashed.

> But, notice one thing --- the go32_ops target
> will still be pushed on the stack, and so the current target still is
> considered to have execution (target_has_execution is true, and
> inferior_ptid will still point at  SOME_PID).  So, when the user does "run",
> GDB will ask if the user wants to kill the current program, and
> will refuse to go ahead if the user says "no".  If the user says
> "yes", then, target_kill is called to kill the current program
> (which ends up in go32_kill_inferior), all before reaching
> go32_create_inferior.
> 
>  This is run_command->kill_if_already_running->target_kill.

Then we probably can remove that conditional call to
go32_kill_inferior.  This is a very old code, perhaps at some past
time it was necessary, I don't know.  Given your description, it seems
we don't need it anymore.

> Currently, "kill" doesn't call `cleanup_client', after
> my changes, it will --- I'm assuming that to be a good thing.

Yes.

> :-)  I think that with my proposed change, we get the same ammount
> of paranoia (even more), but, this needs to be at least minimally
> tested, of course.

I patched my snapshot with your changes, let me run with it for some
time and see if anything goes wrong.  You can commit the changes in
the meantime.

Thanks.

Out of curiosity: why was normal_stop sentenced to death?

> BTW, have you ever had any success running djgpp from inside an
> emulator on linux, say, 'dosemu'?

I never had time to set that up.  Maybe DJ did.

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Eli Zaretskii]

> Date: Fri, 01 May 2009 19:54:21 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 1 May 2009 17:24:36 +0100
> > 
> > IMO, it would be nice to paste your description below at the top of
> > go32-nat.c.
> 
> Okay, I will do it some time soon.

Done with the following patch:

2009-05-01  Eli Zaretskii  <eliz@gnu.org>

	* go32-nat.c: Add comments about dirty secrets of DJGPP debugging.

Index: gdb/go32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/go32-nat.c,v
retrieving revision 1.72
diff -u -r1.72 go32-nat.c
--- gdb/go32-nat.c	1 May 2009 08:14:00 -0000	1.72
+++ gdb/go32-nat.c	1 May 2009 17:44:32 -0000
@@ -18,6 +18,70 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+/* To whomever it may concern, here's a general description of how
+   debugging in DJGPP works, and the special quirks GDB does to
+   support that.
+
+   When the DJGPP port of GDB is debugging a DJGPP program natively,
+   there aren't 2 separate processes, the debuggee and GDB itself, as
+   on other systems.  (This is DOS, where there can only be one active
+   process at any given time, remember?)  Instead, GDB and the
+   debuggee live in the same process.  So when GDB calls
+   go32_create_inferior below, and that function calls edi_init from
+   the DJGPP debug support library libdbg.a, we load the debuggee's
+   executable file into GDB's address space, set it up for execution
+   as the stub loader (a short real-mode program prepended to each
+   DJGPP executable) normally would, and do a lot of preparations for
+   swapping between GDB's and debuggee's internal state, primarily wrt
+   the exception handlers.  This swapping happens every time we resume
+   the debuggee or switch back to GDB's code, and it includes:
+
+    . swapping all the segment registers
+    . swapping the PSP (the Program Segment Prefix)
+    . swapping the signal handlers
+    . swapping the exception handlers
+    . swapping the FPU status
+    . swapping the 3 standard file handles (more about this below)
+
+   Then running the debuggee simply means longjmp into it where its PC
+   is and let it run until it stops for some reason.  When it stops,
+   GDB catches the exception that stopped it and longjmp's back into
+   its own code.  All the possible exit points of the debuggee are
+   watched; for example, the normal exit point is recognized because a
+   DOS program issues a special system call to exit.  If one of those
+   exit points is hit, we mourn the inferior and clean up after it.
+   Cleaning up is very important, even if the process exits normally,
+   because otherwise we might leave behind traces of previous
+   execution, and in several cases GDB itself might be left hosed,
+   because all the exception handlers were not restored.
+
+   Swapping of the standard handles (in redir_to_child and
+   redir_to_debugger) is needed because, since both GDB and the
+   debuggee live in the same process, as far as the OS is concerned,
+   the share the same file table.  This means that the standard
+   handles 0, 1, and 2 point to the same file table entries, and thus
+   are connected to the same devices.  Therefore, if the debugger
+   redirects its standard output, the standard output of the debuggee
+   is also automagically redirected to the same file/device!
+   Similarly, if the debuggee redirects its stdout to a file, you
+   won't be able to see debugger's output (it will go to the same file
+   where the debuggee has its output); and if the debuggee closes its
+   standard input, you will lose the ability to talk to debugger!
+
+   For this reason, every time the debuggee is about to be resumed, we
+   call redir_to_child, which redirects the standard handles to where
+   the debuggee expects them to be.  When the debuggee stops and GDB
+   regains control, we call redir_to_debugger, which redirects those 3
+   handles back to where GDB expects.
+
+   Note that only the first 3 handles are swapped, so if the debuggee
+   redirects or closes any other handles, GDB will not notice.  In
+   particular, the exit code of a DJGPP program forcibly closes all
+   file handles beyond the first 3 ones, so when the debuggee exits,
+   GDB currently loses its stdaux and stdprn streams.  Fortunately,
+   GDB does not use those as of this writing, and will never need
+   to.  */
+
 #include <fcntl.h>
 
 #include "defs.h"

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Pedro Alves]

On Friday 01 May 2009 17:54:21, Eli Zaretskii wrote:

> I patched my snapshot with your changes, let me run with it for some
> time and see if anything goes wrong.  You can commit the changes in
> the meantime.

Thanks, that's great.

> Out of curiosity: why was normal_stop sentenced to death?

It relies on hidden global state as input parameters, e.g.,
get_last_target_status(), stopped_by_random_signal, stop_stack_dummy,
that breakpoints hadn't been removed yet, etc., so it's not exactly
correct to call it at random times.  Also importantly (and that's
what I was after) not having it being called from outside of the core
of inferior control makes it easier to work on cleaning up and
refactoring bits of infrun.c.

-- 
Pedro Alves

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[Pedro Alves]

On Friday 01 May 2009 18:50:33, Eli Zaretskii wrote:
> Done with the following patch:
> 
> 2009-05-01  Eli Zaretskii  <eliz@gnu.org>
> 
>         * go32-nat.c: Add comments about dirty secrets of DJGPP debugging.

Thank you!

-- 
Pedro Alves

Re: Can we get rid of go32_stop? (and its normal_stop call?)

[DJ Delorie]

> BTW, have you ever had any success running djgpp from inside an
> emulator on linux, say, 'dosemu'?

This is my primary DJGPP environment at the moment.  Works fine.