Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 03 March 2010 23:44:22, Kevin Buettner wrote:
> On Tue, 2 Mar 2010 23:37:46 +0000
> Pedro Alves <pedro@codesourcery.com> wrote:
> 
> > Looks mostly okay, but are you missing a target_thread_alive
> > implementation?
> 
> Yes, I was.
> 
> Thanks for the review.  Thanks, too, for the pointers to your work on
> this file.
> 
> > See these:
> > 
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00475.html
> 
> This looks like an earlier version of msg00531, below.  (Missing
> from it are the thread_alive and pid_to_str implementations.)
> 
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00479.html
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00531.html
> > 
> > Could you take a look and see if the other changes make
> > sense to you too?
> 
> I've incorporated much of msg0051, "[02/02] remote-mips.c, always a
> thread (take 2)", into my current patch.  (So, yes, the other changes
> in your patch did make sense to me.)  I'll include your name in the
> ChangeLog entry when it eventually gets committed.
> 
> I'd appreciate it if you could look over the changes to mips_close(),
> below, since my current version differs from what you proposed in your
> patch.  (I cribbed off of remote-sim.c for those changes.)

Hmm, remote-sim.c shouldn't be calling delete_inferior_silent,
that's a bit of bitrot, but, I can see how it isn't
causing problems in the remote-sim.c's case.  You'll need
to change this copied bit in remote-mips.c.  See below.
Sorry about that.  :-(

> 
> Also new from my last patch, in addition to your changes, is the
> fact that mips_mourn_inferior() no longer calls generic_mourn_inferior().
> That's now done in mips_close() which is called when the target is
> popped.
> 
> > You may also be interested in this:
> > 
> > http://sourceware.org/ml/gdb-patches/2008-08/msg00530.html
> 
> This looks interesting, but I haven't tried it yet.  I'd like to get
> the current patch and my other pending changes in first.
> 
> Below is my current patch, incorporating your work, plus a few
> additional changes of my own.  Thanks again for looking this over.

Thank you.

> 
> Kevin
> 
> 	* remote-mips.c (gdbthread.h): Include.
> 	(remote_mips_ptid): Declare.
> 	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
> 	(common_open): Set inferior_ptid, add it as an inferior, and
> 	as a thread too.  Set up the inferior's address spaces.
> 	(mips_close): Invoke generic_mourn_inferior().  Delete the thread
> 	and the inferior.  Delete FIXME comment regarding common_open().
> 	(mips_kill): Make sure that target_mourn_inferior is invoked.
> 	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
> 	it's now invoked from mips_close().
> 	(mips_load): Don't null out inferior_ptid.  Don't call
> 	clear_symtab_users().
> 	(mips_thread_alive, mips_pid_to_str): New functions.
> 	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
> 	to_thread_alive and to_pid_to_str operations.

Looks almost good to me, with comments below.

> 
> Index: remote-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-mips.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 remote-mips.c
> --- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
> +++ remote-mips.c	3 Mar 2010 22:57:34 -0000
> @@ -35,6 +35,7 @@
>  #include "regcache.h"
>  #include <ctype.h>
>  #include "mips-tdep.h"
> +#include "gdbthread.h"
>  
>  
>  /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
> @@ -440,6 +441,11 @@ struct lsi_error lsi_error_table[] =
>     of warnings returned by PMON when hardware breakpoints are used.  */
>  static int monitor_warnings;
>  
> +/* This is the ptid we use while we're connected to the remote.  Its
> +   value is arbitrary, as the remote-mips target doesn't have a notion of
> +   processes or threads, but we need something non-null to place in
> +   inferior_ptid.  */
> +static ptid_t remote_mips_ptid;
>  
>  static void
>  close_ports (void)
> @@ -483,7 +489,8 @@ mips_error (char *string,...)
>    close_ports ();
>  
>    printf_unfiltered ("Ending remote MIPS debugging.\n");
> -  target_mourn_inferior ();
> +  if (!ptid_equal (inferior_ptid, null_ptid))
> +    target_mourn_inferior ();
>  
>    deprecated_throw_reason (RETURN_ERROR);
>  }
> @@ -1468,6 +1475,7 @@ common_open (struct target_ops *ops, cha
>    char *remote_name = 0;
>    char *local_name = 0;
>    char **argv;
> +  struct inferior *inf;
>  
>    if (name == 0)
>      error (
> @@ -1563,7 +1571,11 @@ device is attached to the target board (
>    /* Switch to using remote target now.  */
>    push_target (ops);
>  
> -  /* FIXME: Should we call start_remote here?  */
> +  inferior_ptid = remote_mips_ptid;
> +  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));

Sorry, I missed this before.  Been staring at too many different
GDB versions in this regard.  :-)  You should be using
inferior_appeared instead of add_inferior_silent, and
exit_inferior_silent instead of delete_inferior_silent.  Ever
since GDB gained multi-exec support, there should always
be an inferior in GDBs inferior list, even after the "process"
exits.  With the current patch, after opening a connection, 
you'll be seeing two inferiors in "info inferiors", but you
should only see one, and you should still see it after
the connection having been closed.  This also gets
rid of the need to much with the aspaces below.

> +  inf->aspace = current_program_space->aspace;
> +  inf->pspace = current_program_space;
> +  add_thread_silent (inferior_ptid);
>  
>    /* Try to figure out the processor model if possible.  */
>    deprecated_mips_set_processor_regs_hack ();
> @@ -1639,6 +1651,10 @@ mips_close (int quitting)
>  
>        close_ports ();
>      }
> +
> +  generic_mourn_inferior ();
> +  delete_thread_silent (remote_mips_ptid);
> +  delete_inferior_silent (ptid_get_pid (remote_mips_ptid));

Note that generic_mourn_inferior already
calls exit_inferior and that already deletes all the
inferior's threads, so, if the output exit_inferior gives
is okay for you, _and_ inferior_ptid is never null_ptid
when you get here, you can simply get rid of the
delete_thread_silent and delete_inferior_silent calls.
Otherwise, you could also call discard_all_inferiors
instead (see remote.c:remote_close) -- despite the
name, which is stale, it doesn't really discard
the inferiors, it calls exit_inferior on them.


>  }
>  
>  /* Detach from the remote board.  */
> @@ -2140,7 +2156,10 @@ static void
>  mips_kill (struct target_ops *ops)
>  {
>    if (!mips_wait_flag)
> -    return;
> +    {
> +      target_mourn_inferior ();
> +      return;
> +    }

Hmmm, so, "kill" will just disconnect without
actually killing the target.  But, when mips_wait_flag
is set, mips_kill will just try to interrupt
the target.  Actually, I can't see how to get here
with mips_wait_flag set.  Bah.  This code is
sanatized (I think) by that other patch I pointed you at.
Anyway, if there's no way to actually kill the
target, this is fine.  But you do have one other
option:  that is, to forbid "kill" (make mips_kill
throw an error), and set attach_flag in the inferior,
so that gdb offers to "detach" it instead of "kill"ing
it on "(gdb) quit".  Users would have to "detach" instead.

>  
>    interrupt_count++;
>  
> @@ -2173,6 +2192,8 @@ Give up (and stop debugging it)? ")))
>  
>    serial_send_break (mips_desc);
>  
> +  target_mourn_inferior ();
> +
>  #if 0
>    if (mips_is_open)
>      {
> @@ -2210,19 +2231,17 @@ Can't pass arguments to remote MIPS boar
>  
>    init_wait_for_inferior ();
>  
> -  /* FIXME: Should we set inferior_ptid here?  */
> -
>    regcache_write_pc (get_current_regcache (), entry_pt);
>  }
>  
> -/* Clean up after a process.  Actually nothing to do.  */
> +/* Clean up after a process. The bulk of the work is done in mips_close(),
> +   which is called when unpushing the target.  */
>  
>  static void
>  mips_mourn_inferior (struct target_ops *ops)
>  {
>    if (current_ops != NULL)
>      unpush_target (current_ops);
> -  generic_mourn_inferior ();
>  }
>  
>  /* We can write a breakpoint and read the shadow contents in one
> @@ -3296,18 +3315,36 @@ mips_load (char *file, int from_tty)
>      }
>    if (exec_bfd)
>      regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
> +}
>  
> -  inferior_ptid = null_ptid;	/* No process now */
> -
> -/* This is necessary because many things were based on the PC at the time that
> -   we attached to the monitor, which is no longer valid now that we have loaded
> -   new code (and just changed the PC).  Another way to do this might be to call
> -   normal_stop, except that the stack may not be valid, and things would get
> -   horribly confused... */
> +/* Check to see if a thread is still alive.  */
> + 
> +static int
> +mips_thread_alive (struct target_ops *ops, ptid_t ptid)
> +{
> +  if (ptid_equal (ptid, remote_mips_ptid))
> +    /* The monitor's task is always alive.  */
> +    return 1;
>  
> -  clear_symtab_users ();
> +  return 0;
>  }
>  
> +/* Convert a thread ID to a string.  Returns the string in a static
> +   buffer.  */
> +
> +static char *
> +mips_pid_to_str (struct target_ops *ops, ptid_t ptid)
> +{
> +  static char buf[64];
> +
> +  if (ptid_equal (ptid, remote_mips_ptid))
> +    {
> +      xsnprintf (buf, sizeof buf, "Thread <main>");
> +      return buf;
> +    }
> +
> +  return normal_pid_to_str (ptid);
> +}
>  
>  /* Pass the command argument as a packet to PMON verbatim.  */
>  
> @@ -3351,6 +3388,8 @@ _initialize_remote_mips (void)
>    mips_ops.to_load = mips_load;
>    mips_ops.to_create_inferior = mips_create_inferior;
>    mips_ops.to_mourn_inferior = mips_mourn_inferior;
> +  mips_ops.to_thread_alive = mips_thread_alive;
> +  mips_ops.to_pid_to_str = mips_pid_to_str;
>    mips_ops.to_log_command = serial_log_command;
>    mips_ops.to_stratum = process_stratum;
>    mips_ops.to_has_all_memory = default_child_has_all_memory;
> @@ -3458,4 +3497,5 @@ Use \"on\" to enable the masking and \"o
>  			   NULL,
>  			   NULL, /* FIXME: i18n: */
>  			   &setlist, &showlist);
> +  remote_mips_ptid = ptid_build (42000, 0, 42000);
>  }
> 
> 


-- 
Pedro Alves