This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state
Hi Pedro,
Thanks again for your review of these patches. See my replies below...
On Thu, 4 Mar 2010 00:33:24 +0000
Pedro Alves <pedro@codesourcery.com> wrote:
> > - /* 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.
Okay, I think I've fixed this now. I tried "info inferiors" with my
patch from yesterday and I actually saw four inferiors on the list. (!)
Don't know why though. With the current patch, below, I only see one.
> > + 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.
I've fixed this too.
> > 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.
Long term, I'm not sure what to do about this. Short term, I'd like
to leave it as shown in the patch below - which is the same as the
earlier patch that you've looked at.
I tried setting attach_flag in the inferior and making mips_kill throw
an error, but that messes up gdb.base/opaque.exp. Here's the relevant
bit from the log file:
(gdb) PASS: gdb.base/opaque.exp: ptype on opaque struct tagname (statically)
dir
Reinitialize source path to empty? (y or n) y
Source directories searched: $cdir:$cwd
(gdb) dir /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base
Source directories searched: /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base:$cdir:$cwd
(gdb) kill
Kill the program being debugged? (y or n) y
Can't kill this target.
Use `detach' to disconnect from the target board's monitor.
(gdb) file /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: couldn't load /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque into /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/../../gdb/gdb (timed out).
delete breakpoints
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: Delete all breakpoints in delete_breakpoints (timeout)
break main
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) UNRESOLVED: gdb.base/opaque.exp: setting breakpoint at main (timeout)
ERROR: cannot run to breakpoint at main
I do have an explanation for why mips_kill() is trying to interrupt
the target though. When mips_wait_flag is set, the program is running.
When it's not set, the monitor is, presumably, at the monitor prompt,
waiting for a command. That command could come from GDB or from user
interaction. When we're sitting at that prompt, the program hasn't
exactly been killed, but it's probably about as close as we can get
unless we manage to reset the board somehow.
With regard to entering mips_kill() with mips_wait_flag being set...
I agree with you; I don't see how it can happen. I'm still looking
at old code to see if there's a way that it somehow used to work, but
doesn't any longer. So far, it appears that it's always been broken,
but I have some more investigation to do.
So... given that the current code tries to kill the target as much as
is reasonable, i.e. either do nothing when at the monitor prompt, or
interrupt the board to get to the monitor prompt when the board is running
(even though we can't get here when that happens), it seems to me that
this code isn't entirely unreasonable.
Below is my current patch.
* 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. Delete FIXME comment regarding start_remote().
(mips_close): Invoke generic_mourn_inferior().
(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 4 Mar 2010 23:03:07 -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);
}
@@ -1563,7 +1570,9 @@ 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;
+ inferior_appeared (current_inferior (), ptid_get_pid (inferior_ptid));
+ add_thread_silent (inferior_ptid);
/* Try to figure out the processor model if possible. */
deprecated_mips_set_processor_regs_hack ();
@@ -1639,6 +1648,8 @@ mips_close (int quitting)
close_ports ();
}
+
+ generic_mourn_inferior ();
}
/* Detach from the remote board. */
@@ -2140,7 +2151,10 @@ static void
mips_kill (struct target_ops *ops)
{
if (!mips_wait_flag)
- return;
+ {
+ target_mourn_inferior ();
+ return;
+ }
interrupt_count++;
@@ -2173,6 +2187,8 @@ Give up (and stop debugging it)? ")))
serial_send_break (mips_desc);
+ target_mourn_inferior ();
+
#if 0
if (mips_is_open)
{
@@ -2210,19 +2226,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 +3310,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 +3383,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 +3492,5 @@ Use \"on\" to enable the masking and \"o
NULL,
NULL, /* FIXME: i18n: */
&setlist, &showlist);
+ remote_mips_ptid = ptid_build (42000, 0, 42000);
}