This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RFC: remove pop_target
- From: Tom Tromey <tromey at redhat dot com>
- To: gdb-patches at sourceware dot org
- Cc: Tom Tromey <tromey at redhat dot com>
- Date: Tue, 2 Jul 2013 13:35:37 -0600
- Subject: RFC: remove pop_target
This patch fixes the target double-close problem (PR remote/15266),
and in the process removes pop_target entire (PR remote/15256).
The first issue is that pop_target calls target_close. However, it
then calls unpush_target, which also calls target_close. This means
targets must be able to be closed twice. Not only is this strange,
but it also directly contradicts the contract of to_xclose targets.
(We currently have just a single such target, and it is never pushed;
but I plan to add more, and so this latent bug is triggered.)
The second issue is that it seems to me that calling pop_target is
often unsafe. This is what cropped up in 15256, where the remote
target assumed that it could pop_target -- but there was another
target higher on the stack, leading to confusion.
But, it is always just as easy to call unpush_target as it is to call
pop_target; and it is also safer. So, removing pop_target seemed like
an improvement.
Finally, this adds an assertion to target_close to ensure that no
currently-pushed target can be closed.
Built and regtested on x86-64 Fedora 18; both natively and using the
native-gdbserver board file.
PR remote/15256, PR remote/15266:
* bfd-target.c (target_bfd_reopen): Initialize to_magic.
* monitor.c (monitor_detach): Use unpush_target.
* remote-m32r-sdi.c (m32r_detach): Use unpush_target.
* remote-mips.c (mips_detach): Use unpush_target. Don't
call mips_close.
* remote-sim.c (gdbsim_detach): Use unpush_target.
* target.c (pop_target): Remove.
(pop_all_targets_above): Don't call target_close.
(target_close): Assert that the target is unpushed.
* target.h (pop_target): Don't declare.
* tracepoint.c (tfile_open): Use unpush_target.
---
gdb/bfd-target.c | 1 +
gdb/monitor.c | 2 +-
gdb/remote-m32r-sdi.c | 2 +-
gdb/remote-mips.c | 4 +---
gdb/remote-sim.c | 2 +-
gdb/target.c | 17 ++---------------
gdb/target.h | 6 +-----
gdb/tracepoint.c | 4 ++--
8 files changed, 10 insertions(+), 28 deletions(-)
diff --git a/gdb/bfd-target.c b/gdb/bfd-target.c
index a7dee24..8d09831 100644
--- a/gdb/bfd-target.c
+++ b/gdb/bfd-target.c
@@ -96,6 +96,7 @@ target_bfd_reopen (struct bfd *abfd)
t->to_xfer_partial = target_bfd_xfer_partial;
t->to_xclose = target_bfd_xclose;
t->to_data = data;
+ t->to_magic = OPS_MAGIC;
return t;
}
diff --git a/gdb/monitor.c b/gdb/monitor.c
index beca4e4..8968965 100644
--- a/gdb/monitor.c
+++ b/gdb/monitor.c
@@ -877,7 +877,7 @@ monitor_close (void)
static void
monitor_detach (struct target_ops *ops, char *args, int from_tty)
{
- pop_target (); /* calls monitor_close to do the real work. */
+ unpush_target (ops);
if (from_tty)
printf_unfiltered (_("Ending remote %s debugging\n"), target_shortname);
}
diff --git a/gdb/remote-m32r-sdi.c b/gdb/remote-m32r-sdi.c
index 2f910e6..81fea53 100644
--- a/gdb/remote-m32r-sdi.c
+++ b/gdb/remote-m32r-sdi.c
@@ -886,7 +886,7 @@ m32r_detach (struct target_ops *ops, char *args, int from_tty)
m32r_resume (ops, inferior_ptid, 0, GDB_SIGNAL_0);
/* Calls m32r_close to do the real work. */
- pop_target ();
+ unpush_target (ops);
if (from_tty)
fprintf_unfiltered (gdb_stdlog, "Ending remote %s debugging\n",
target_shortname);
diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index 1619622..13ea146 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -1755,9 +1755,7 @@ mips_detach (struct target_ops *ops, char *args, int from_tty)
if (args)
error (_("Argument given to \"detach\" when remotely debugging."));
- pop_target ();
-
- mips_close ();
+ unpush_target (ops);
if (from_tty)
printf_unfiltered ("Ending remote MIPS debugging.\n");
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index fda3735..d534e56 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -821,7 +821,7 @@ gdbsim_detach (struct target_ops *ops, char *args, int from_tty)
if (remote_debug)
printf_filtered ("gdbsim_detach: args \"%s\"\n", args);
- pop_target (); /* calls gdbsim_close to do the real work */
+ unpush_target (ops); /* calls gdbsim_close to do the real work */
if (from_tty)
printf_filtered ("Ending simulator %s debugging\n", target_shortname);
}
diff --git a/gdb/target.c b/gdb/target.c
index 5bf8059..aae3eb6 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1094,25 +1094,10 @@ unpush_target (struct target_ops *t)
}
void
-pop_target (void)
-{
- target_close (target_stack); /* Let it clean up. */
- if (unpush_target (target_stack) == 1)
- return;
-
- fprintf_unfiltered (gdb_stderr,
- "pop_target couldn't find target %s\n",
- current_target.to_shortname);
- internal_error (__FILE__, __LINE__,
- _("failed internal consistency check"));
-}
-
-void
pop_all_targets_above (enum strata above_stratum)
{
while ((int) (current_target.to_stratum) > (int) above_stratum)
{
- target_close (target_stack);
if (!unpush_target (target_stack))
{
fprintf_unfiltered (gdb_stderr,
@@ -3779,6 +3764,8 @@ debug_to_open (char *args, int from_tty)
void
target_close (struct target_ops *targ)
{
+ gdb_assert (!target_is_pushed (targ));
+
if (targ->to_xclose != NULL)
targ->to_xclose (targ);
else if (targ->to_close != NULL)
diff --git a/gdb/target.h b/gdb/target.h
index 00dad64..c4d4912 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1849,9 +1849,7 @@ int target_verify_memory (const gdb_byte *data,
unpush_target: Remove this from the stack of currently used targets,
no matter where it is on the list. Returns 0 if no
- change, 1 if removed from stack.
-
- pop_target: Remove the top thing on the stack of current targets. */
+ change, 1 if removed from stack. */
extern void add_target (struct target_ops *);
@@ -1873,8 +1871,6 @@ extern void target_pre_inferior (int);
extern void target_preopen (int);
-extern void pop_target (void);
-
/* Does whatever cleanup is required to get rid of all pushed targets. */
extern void pop_all_targets (void);
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 054372a..8b70bd3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -4366,8 +4366,8 @@ tfile_open (char *filename, int from_tty)
}
if (ex.reason < 0)
{
- /* Pop the partially set up target. */
- pop_target ();
+ /* Remove the partially set up target. */
+ unpush_target (&tfile_ops);
throw_exception (ex);
}
--
1.8.1.4