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

Kevin Buettner
2010-03-03T23:44:43Z

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.)

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.

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.

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"
\f

/* 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));
+ 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));
}

/* 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;
+ }

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 ();
}
\f
/* 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 ");
+ 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);
}