Re: [RFA] remote-sim.c: Add support for multiple sim instances

[Pedro Alves <pedro at codesourcery dot com>]

Pong. ;-)

Comments inline below.

On Friday 02 July 2010 23:30:17, Kevin Buettner wrote:

> Index: remote-sim.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-sim.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 remote-sim.c
> --- remote-sim.c	16 May 2010 21:11:14 -0000	1.96
> +++ remote-sim.c	2 Jul 2010 20:15:59 -0000
> @@ -101,19 +101,125 @@ void simulator_command (char *args, int 
>  /* Forward data declarations */
>  extern struct target_ops gdbsim_ops;
>  
> -static int program_loaded = 0;
> +static const struct inferior_data *sim_inferior_data_key;
>  
> -/* We must keep track of whether the simulator has been opened or not because
> -   GDB can call a target's close routine twice, but sim_close doesn't allow
> -   this.  We also need to record the result of sim_open so we can pass it
> -   back to the other sim_foo routines.  */
> -static SIM_DESC gdbsim_desc = 0;
> -
> -/* This is the ptid we use while we're connected to the simulator.
> -   Its value is arbitrary, as the simulator target don't have a notion
> -   or processes or threads, but we need something non-null to place in
> -   inferior_ptid.  */
> -static ptid_t remote_sim_ptid;
> +/* Simulator-specific, per-inferior state.  */
> +struct sim_inferior_data {
> +  /* Flag which indicates whether or not the program has been loaded.  */
> +  int program_loaded;
> +
> +  /* Simulator descriptor for this inferior.  */
> +  SIM_DESC gdbsim_desc;
> +
> +  /* This is the ptid we use for this particular simulator instance.  Its
> +     value is somewhat arbitrary, as the simulator target don't have a
> +     notion of tasks or threads, but we need something non-null to place
> +     in inferior_ptid.  For simulators which permit multiple instances,
> +     we also need a unique identifier to use for each inferior.  */
> +  ptid_t remote_sim_ptid;
> +};
> +
> +/* Value of the next pid to allocate for an inferior.  */
> +static int next_pid = 42000;

Maybe this should be reset on, say, gdbsim_open.

> +
> +/* Argument list to pass to sim_open() (for creating simulator instances).  */
> +static char **sim_argv = NULL;
> +
> +/* OS-level callback functions for write, flush, etc.  */
> +static host_callback gdb_callback;
> +static int callbacks_initialized = 0;
> +
> +/* Callback for iterate_over_inferiors.  It checks to see if
> +   the sim descriptor of the per-inferior simulator data
> +   passed via ARG is the same as that for the inferior
> +   designated by INF.  Return true if so; false otherwise.  */
> +
> +static int
> +check_for_duplicate_sim_descriptor (struct inferior *inf, void *arg)
> +{
> +  struct sim_inferior_data *sd1, *sd2;
> +
> +  sd1 = arg;
> +  sd2 = inferior_data (inf, sim_inferior_data_key);
> +
> +  return (sd2 != NULL && sd1 != sd2 && sd2->gdbsim_desc != NULL
> +          && sd1->gdbsim_desc == sd2->gdbsim_desc);
> +}

Why doesn't this return true when sd1 == sd2?  Or, is that
something that should never happen?

> +
> +/* Flags indicating whether or not a sim instance is needed.  One of these
> +   flags should be passed to get_sim_inferior_data().  */
> +
> +enum {SIM_INSTANCE_NOT_NEEDED = 0, SIM_INSTANCE_NEEDED = 1};
> +
> +/* Obtain pointer to per-inferior simulator data, allocating it if necessary.
> +   Attempt to open the sim if SIM_INSTANCE_NEEDED is true.  */
> +
> +static struct sim_inferior_data *
> +get_sim_inferior_data (int sim_instance_needed)
> +{
> +  struct inferior *inf = current_inferior ();
> +  struct sim_inferior_data *sim_data
> +    = inferior_data (inf, sim_inferior_data_key);
> +
> +  if (sim_data == NULL)
> +    {
> +      sim_data = XZALLOC(struct sim_inferior_data);
> +      set_inferior_data (inf, sim_inferior_data_key, sim_data);
> +
> +      /* Allocate a ptid for this inferior.  */
> +      sim_data->remote_sim_ptid = ptid_build (next_pid, 0, next_pid);
> +      next_pid++;
> +
> +      /* Initialize the other instance variables.  */
> +      sim_data->program_loaded = 0;
> +      sim_data->gdbsim_desc = NULL;
> +    }
> +
> +  /* Try to allocate a new sim instance, if needed.  */
> +  if (sim_instance_needed && sim_data->gdbsim_desc == 0)
> +    {
> +      struct inferior *idup;
> +      sim_data->gdbsim_desc
> +        = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, sim_argv);
> +      if (sim_data->gdbsim_desc == NULL)
> +	error (_("Unable to create simulator instance for inferior %d."), inf->num);
> +
> +      idup = iterate_over_inferiors (check_for_duplicate_sim_descriptor,
> +                                     sim_data);
> +      if (idup != NULL)
> +	{
> +	  /* We don't close the descriptor due to the fact that it's shared with
> +	     some other inferior.  If we were to close it, that might needlessly
> +	     muck up the other inferior.  Of course, it's possible that the damage
> +	     has already been done...  But not closing it seems like the safest
> +	     option.  Note that it *will* ultimately be closed during cleanup of
> +	     the other inferior.  */
> +	  sim_data->gdbsim_desc = NULL;
> +	  error (_("Inferior %d and inferior %d would have identical simulator state.\n"
> +		   "(This simulator does not support the running of more than one inferior.)"),
> +		 inf->num, idup->num); }
> +        }


These throw an error while leaving the per-inferior data set in
the inferior.  The next_pid global is also always incremented,
even on error.  I think you should build the sim_data and open
the simulator fully, before setting the per-inferior pointer,
and incrementing that counter.

> +
> +  return sim_data;
> +}
> +
> +/* Free the per-inferior simulator data.  */
> +
> +static void
> +sim_inferior_data_cleanup (struct inferior *inf, void *data)
> +{
> +  struct sim_inferior_data *sim_data = data;
> +
> +  if (sim_data != NULL)
> +    {
> +      if (sim_data->gdbsim_desc)
> +	{
> +	  sim_close (sim_data->gdbsim_desc, 0);
> +	  sim_data->gdbsim_desc = NULL;
> +	}
> +      xfree (sim_data);
> +    }
> +}

This is only called when the "struct inferior" object is
deleted.  Remember that the inferior stays around in a
"proto-inferior" state (debugging the executable), after
kill/detach/program exit, for example, but there's no garantee that
the next target you'll use it with will be the sim.  The
user may do "tar remote" at that point, say.
Thus, you should close the sim descriptor and clear the sim
related data from the inferior on other occasions
as well.  IIUC, it is desirable to keep the sim descriptor
open (and the program loaded in the sim), even after
kill/detach/program exit, so you'll want to cleanup
all sim instances and per-inferior sim data when the
gdbsim target is closed.

>  
>  static void
>  dump_mem (char *buf, int len)
> @@ -142,9 +248,6 @@ dump_mem (char *buf, int len)
>      }
>  }
>  
> -static host_callback gdb_callback;
> -static int callbacks_initialized = 0;
> -
>  /* Initialize gdb_callback.  */
>  
>  static void
> @@ -278,6 +381,8 @@ gdbsim_fetch_register (struct target_ops
>  		       struct regcache *regcache, int regno)
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
>  
>    if (regno == -1)
>      {
> @@ -310,7 +415,7 @@ gdbsim_fetch_register (struct target_ops
>  
>  	gdb_assert (regno >= 0 && regno < gdbarch_num_regs (gdbarch));
>  	memset (buf, 0, MAX_REGISTER_SIZE);
> -	nr_bytes = sim_fetch_register (gdbsim_desc,
> +	nr_bytes = sim_fetch_register (sim_data->gdbsim_desc,
>  				       gdbarch_register_sim_regno
>  					 (gdbarch, regno),
>  				       buf,
> @@ -350,6 +455,9 @@ gdbsim_store_register (struct target_ops
>  		       struct regcache *regcache, int regno)
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
> +
>    if (regno == -1)
>      {
>        for (regno = 0; regno < gdbarch_num_regs (gdbarch); regno++)
> @@ -362,7 +470,7 @@ gdbsim_store_register (struct target_ops
>        int nr_bytes;
>  
>        regcache_cooked_read (regcache, regno, tmp);
> -      nr_bytes = sim_store_register (gdbsim_desc,
> +      nr_bytes = sim_store_register (sim_data->gdbsim_desc,
>  				     gdbarch_register_sim_regno
>  				       (gdbarch, regno),
>  				     tmp, register_size (gdbarch, regno));
> @@ -404,6 +512,8 @@ gdbsim_load (char *args, int fromtty)
>  {
>    char **argv;
>    char *prog;
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
>  
>    if (args == NULL)
>        error_no_arg (_("program to load"));
> @@ -422,13 +532,13 @@ gdbsim_load (char *args, int fromtty)
>    /* FIXME: We will print two messages on error.
>       Need error to either not print anything if passed NULL or need
>       another routine that doesn't take any arguments.  */
> -  if (sim_load (gdbsim_desc, prog, NULL, fromtty) == SIM_RC_FAIL)
> +  if (sim_load (sim_data->gdbsim_desc, prog, NULL, fromtty) == SIM_RC_FAIL)
>      error (_("unable to load program"));
>  
>    /* FIXME: If a load command should reset the targets registers then
>       a call to sim_create_inferior() should go here. */
>  
> -  program_loaded = 1;
> +  sim_data->program_loaded = 1;
>  }
>  
>  
> @@ -444,12 +554,14 @@ static void
>  gdbsim_create_inferior (struct target_ops *target, char *exec_file, char *args,
>  			char **env, int from_tty)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
>    int len;
>    char *arg_buf, **argv;
>  
>    if (exec_file == 0 || exec_bfd == 0)
>      warning (_("No executable file specified."));
> -  if (!program_loaded)
> +  if (!sim_data->program_loaded)
>      warning (_("No program loaded."));
>  
>    if (remote_debug)
> @@ -457,7 +569,7 @@ gdbsim_create_inferior (struct target_op
>  		     (exec_file ? exec_file : "(NULL)"),
>  		     args);
>  
> -  if (ptid_equal (inferior_ptid, remote_sim_ptid))
> +  if (ptid_equal (inferior_ptid, sim_data->remote_sim_ptid))
>      gdbsim_kill (target);
>    remove_breakpoints ();
>    init_wait_for_inferior ();
> @@ -475,9 +587,9 @@ gdbsim_create_inferior (struct target_op
>      }
>    else
>      argv = NULL;
> -  sim_create_inferior (gdbsim_desc, exec_bfd, argv, env);
> +  sim_create_inferior (sim_data->gdbsim_desc, exec_bfd, argv, env);
>  
> -  inferior_ptid = remote_sim_ptid;
> +  inferior_ptid = sim_data->remote_sim_ptid;
>    inferior_appeared (current_inferior (), ptid_get_pid (inferior_ptid));
>    add_thread_silent (inferior_ptid);
>  
> @@ -496,7 +608,8 @@ gdbsim_open (char *args, int from_tty)
>  {
>    int len;
>    char *arg_buf;
> -  char **argv;
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
>  
>    if (remote_debug)
>      printf_filtered ("gdbsim_open: args \"%s\"\n", args ? args : "(null)");
> @@ -507,7 +620,7 @@ gdbsim_open (char *args, int from_tty)
>       sim_close to be called if the simulator is already open, but push_target
>       is called after sim_open!  We can't move the call to push_target before
>       the call to sim_open because sim_open may invoke `error'.  */
> -  if (gdbsim_desc != NULL)
> +  if (sim_data->gdbsim_desc != NULL)
>      unpush_target (&gdbsim_ops);

Although the current inferior may not have a gdbsim_desc,
others might.  The question "(gdbsim_desc != NULL)" was
answering before was "is the gdbsim target open?".  That's
no longer true after that change.

>  
>    len = (7 + 1			/* gdbsim */
> @@ -543,14 +656,17 @@ gdbsim_open (char *args, int from_tty)
>        strcat (arg_buf, " ");	/* 1 */
>        strcat (arg_buf, args);
>      }
> -  argv = gdb_buildargv (arg_buf);
> -  make_cleanup_freeargv (argv);
> +  sim_argv = gdb_buildargv (arg_buf);
>  
>    init_callbacks ();
> -  gdbsim_desc = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, argv);
> +  sim_data->gdbsim_desc = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, sim_argv);
>  
> -  if (gdbsim_desc == 0)
> -    error (_("unable to create simulator instance"));
> +  if (sim_data->gdbsim_desc == 0)
> +    {
> +      freeargv (sim_argv);
> +      sim_argv = NULL;
> +      error (_("unable to create simulator instance"));
> +    }

This is leaving sim_data set on the inferior when a 
exception/error is thrown, and the gdbsim target ends
up not opened.  I think it'd be better to build the sim_data
on the side, and one assign it in the inferior when the
open was succesful.

>  
>    push_target (&gdbsim_ops);
>    printf_filtered ("Connected to the simulator.\n");
> @@ -572,21 +688,30 @@ gdbsim_open (char *args, int from_tty)
>  static void
>  gdbsim_close (int quitting)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +
>    if (remote_debug)
>      printf_filtered ("gdbsim_close: quitting %d\n", quitting);
>  
> -  program_loaded = 0;
> +  sim_data->program_loaded = 0;
>  
> -  if (gdbsim_desc != NULL)
> +  if (sim_data->gdbsim_desc != NULL)
>      {
> -      sim_close (gdbsim_desc, quitting);
> -      gdbsim_desc = NULL;
> +      sim_close (sim_data->gdbsim_desc, quitting);
> +      sim_data->gdbsim_desc = NULL;
> +    }

Following up on a comment I made above, when the target is closed,
you no longer have access to any of the inferiors it was
controlling.  Thus, you should do the above for all the
inferiors, not just the current.

> +
> +  if (sim_argv != NULL)
> +    {
> +      freeargv (sim_argv);
> +      sim_argv = NULL;
>      }

Hmm, not sure I understood the lifetime of this object.
I thought it was just a temporary object to pass to
sim_open.  Why isn't it released right after calling
sim_open?  What happens to it when you run more than
one inferior?

>  
>    end_callbacks ();
>    generic_mourn_inferior ();
> -  delete_thread_silent (remote_sim_ptid);
> -  delete_inferior_silent (ptid_get_pid (remote_sim_ptid));
> +  delete_thread_silent (sim_data->remote_sim_ptid);
> +  delete_inferior_silent (ptid_get_pid (sim_data->remote_sim_ptid));
>  }
>  
>  /* Takes a program previously attached to and detaches it.
> @@ -620,7 +745,10 @@ static void
>  gdbsim_resume (struct target_ops *ops,
>  	       ptid_t ptid, int step, enum target_signal siggnal)
>  {
> -  if (!ptid_equal (inferior_ptid, remote_sim_ptid))
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +
> +  if (!ptid_equal (inferior_ptid, sim_data->remote_sim_ptid))
>      error (_("The program is not being run."));

You'll want to be able to resume all inferiors here.  Try
"set schedule-multiple on", and then "continue".  You'll
see that the PTID argument changes from the PID of the
inferior to minus_one_ptid.  That is, if you've set
target_supports_multi_process... hmm, oh, you haven't.

>  
>    if (remote_debug)
> @@ -642,7 +770,10 @@ gdbsim_resume (struct target_ops *ops,
>  static void
>  gdbsim_stop (ptid_t ptid)
>  {
> -  if (!sim_stop (gdbsim_desc))
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
> +
> +  if (!sim_stop (sim_data->gdbsim_desc))
>      {
>        quit ();
>      }

Pedantically, you should stop the inferior specified by
the PTID argument, not the current.  When async is supported,
you may see the core calling this callback with minus_one_ptid,
meaning stop-the-world.
(this only applies when you add support for actually resuming
more than one sim simultaneously)


> @@ -676,13 +807,18 @@ gdb_os_poll_quit (host_callback *p)
>  static void
>  gdbsim_cntrl_c (int signo)
>  {
> -  gdbsim_stop (remote_sim_ptid);
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +
> +  gdbsim_stop (sim_data->remote_sim_ptid);
>  }


and here I think you should be telling gdbsim_stop that
you want all sims stopped [gdbsim_stop (minus_one_ptid)].
(this only applies when you add support for actually resuming
more than one sim simultaneously)


>  
>  static ptid_t
>  gdbsim_wait (struct target_ops *ops,
>  	     ptid_t ptid, struct target_waitstatus *status, int options)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
>    static RETSIGTYPE (*prev_sigint) ();
>    int sigrc = 0;
>    enum sim_stop reason = sim_running;
> @@ -702,11 +838,11 @@ gdbsim_wait (struct target_ops *ops,
>  #else
>    prev_sigint = signal (SIGINT, gdbsim_cntrl_c);
>  #endif
> -  sim_resume (gdbsim_desc, resume_step, resume_siggnal);
> +  sim_resume (sim_data->gdbsim_desc, resume_step, resume_siggnal);
>    signal (SIGINT, prev_sigint);
>    resume_step = 0;
>  
> -  sim_stop_reason (gdbsim_desc, &reason, &sigrc);
> +  sim_stop_reason (sim_data->gdbsim_desc, &reason, &sigrc);
>  
>    switch (reason)
>      {
> @@ -764,15 +900,26 @@ gdbsim_xfer_inferior_memory (CORE_ADDR m
>  			     int write, struct mem_attrib *attrib,
>  			     struct target_ops *target)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +
>    /* If no program is running yet, then ignore the simulator for
>       memory.  Pass the request down to the next target, hopefully
>       an exec file.  */
>    if (!target_has_execution)
>      return 0;
>  
> -  if (!program_loaded)
> +  if (!sim_data->program_loaded)
>      error (_("No program loaded."));
>  
> +  /* Note that we obtained the sim_data pointer above using
> +     SIM_INSTANCE_NOT_NEEDED.  We do this so that we don't needlessly
> +     allocate a sim instance prior to loading a program.   If we
> +     get to this point in the code though, gdbsim_desc should be
> +     non-NULL.  (Note that a sim instance is needed in order to load
> +     the program...)  */
> +  gdb_assert (sim_data->gdbsim_desc != NULL);
> +
>    if (remote_debug)
>      {
>        /* FIXME: Send to something other than STDOUT? */
> @@ -786,11 +933,11 @@ gdbsim_xfer_inferior_memory (CORE_ADDR m
>  
>    if (write)
>      {
> -      len = sim_write (gdbsim_desc, memaddr, myaddr, len);
> +      len = sim_write (sim_data->gdbsim_desc, memaddr, myaddr, len);
>      }
>    else
>      {
> -      len = sim_read (gdbsim_desc, memaddr, myaddr, len);
> +      len = sim_read (sim_data->gdbsim_desc, memaddr, myaddr, len);
>        if (remote_debug && len > 0)
>  	dump_mem (myaddr, len);
>      }
> @@ -800,6 +947,8 @@ gdbsim_xfer_inferior_memory (CORE_ADDR m
>  static void
>  gdbsim_files_info (struct target_ops *target)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NEEDED);
>    const char *file = "nothing";
>  
>    if (exec_bfd)
> @@ -812,7 +961,7 @@ gdbsim_files_info (struct target_ops *ta
>      {
>        printf_filtered ("\tAttached to %s running program %s\n",
>  		       target_shortname, file);
> -      sim_info (gdbsim_desc, 0);
> +      sim_info (sim_data->gdbsim_desc, 0);
>      }
>  }
>  
> @@ -821,12 +970,15 @@ gdbsim_files_info (struct target_ops *ta
>  static void
>  gdbsim_mourn_inferior (struct target_ops *target)
>  {
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +
>    if (remote_debug)
>      printf_filtered ("gdbsim_mourn_inferior:\n");
>  
>    remove_breakpoints ();
>    generic_mourn_inferior ();
> -  delete_thread_silent (remote_sim_ptid);
> +  delete_thread_silent (sim_data->remote_sim_ptid);
>  }
>  
>  /* Pass the command argument through to the simulator verbatim.  The
> @@ -835,7 +987,10 @@ gdbsim_mourn_inferior (struct target_ops
>  void
>  simulator_command (char *args, int from_tty)
>  {
> -  if (gdbsim_desc == NULL)
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +

This command can be run at any random time, even before
"target sim".  I don't think you should be attaching a
sim_inferior_data to the current inferior here
(get_sim_inferior_data attaches one if there isn't one already).
That is, if "inferior_data (inf, sim_inferior_data_key)"
is NULL, then the error below also applies.

> +  if (sim_data->gdbsim_desc == NULL)
>      {
>  
>        /* PREVIOUSLY: The user may give a command before the simulator
> @@ -851,7 +1006,7 @@ simulator_command (char *args, int from_
>        error (_("Not connected to the simulator target"));
>      }
>  
> -  sim_do_command (gdbsim_desc, args);
> +  sim_do_command (sim_data->gdbsim_desc, args);
>  
>    /* Invalidate the register cache, in case the simulator command does
>       something funny. */
> @@ -863,7 +1018,10 @@ simulator_command (char *args, int from_
>  static int
>  gdbsim_thread_alive (struct target_ops *ops, ptid_t ptid)
>  {
> -  if (ptid_equal (ptid, remote_sim_ptid))
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
> +

Here you should be getting at the sim specified by PTID too,
not the current.  otherwise "info threads" is finding
that all threads not of the current inferior are
always dead.  There may be other places with the same
issue.  Basically, if you have a PTID argument, you
should be using that instead of the current inferior.

( At this point, I'll point out that there'll be several
places that will want to map a pid to a
sim_inferior_data object, and it is no longer clear
whether using the inferior data mechanism is a win,
compared to a private list like linux-thread-db.c
maintains.  Note that even current_inferior() does a 
lookup by PID internally.  The per-inferior mechanism
has the small disadvantage that all registered keys
allocate a slot space in the inferior instances' generic
data array, even if not used. )

> +  if (ptid_equal (ptid, sim_data->remote_sim_ptid))
>      /* The simulators' task is always alive.  */
>      return 1;
>  
> @@ -877,8 +1035,10 @@ static char *
>  gdbsim_pid_to_str (struct target_ops *ops, ptid_t ptid)
>  {
>    static char buf[64];
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (SIM_INSTANCE_NOT_NEEDED);
>  
> -  if (ptid_equal (remote_sim_ptid, ptid))
> +  if (ptid_equal (sim_data->remote_sim_ptid, ptid))

Ditto.

>      {
>        xsnprintf (buf, sizeof buf, "Thread <main>");
>        return buf;
> @@ -934,7 +1094,6 @@ _initialize_remote_sim (void)
>    add_com ("sim", class_obscure, simulator_command,
>  	   _("Send a command to the simulator."));
>  
> -  /* Yes, 42000 is arbitrary.  The only sense out of it, is that it
> -     isn't 0.  */
> -  remote_sim_ptid = ptid_build (42000, 0, 42000);
> +  sim_inferior_data_key
> +    = register_inferior_data_with_cleanup (sim_inferior_data_cleanup);
>  }


-- 
Pedro Alves