OpenE2K
/
binutils-gdb
12
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear 

displaced_step_inferior_state::reset and displaced_step_clear appear to
have the same goal, but they don't do the same thing.
displaced_step_inferior_state::reset clears more things than
displaced_step_clear, but it misses free'ing the closure, which
displaced_step_clear does.

This patch replaces displaced_step_clear's implementation with just a call to
displaced_step_inferior_state::reset.  It then changes
displaced_step_inferior_state::step_closure to be a unique_ptr, to indicate the
fact that displaced_step_inferior_state owns the closure (and so that it is
automatically freed when the field is reset).

The test gdb.base/step-over-syscall.exp caught a problem when doing this, which
I consider to be a latent bug which my cleanup exposes.  In
handle_inferior_event, in the TARGET_WAITKIND_FORKED case, if we displaced-step
over a fork syscall, we make sure to restore the memory that we used as a
displaced-stepping buffer in the child.  We do so using the
displaced_step_inferior_state of the parent.  However, we do it after calling
displaced_step_fixup for the parent, which clears the information in the
parent's displaced_step_inferior_state.  It worked fine before, because
displaced_step_clear didn't completely clear the displaced_step_inferior_state
structure, so the required information (in this case the gdbarch) was
still available after clearing.

I fixed it by making GDB restore the child's memory before calling the
displaced_step_fixup on the parent.  This way, the data in the
displaced_step_inferior_state structure is still valid when we use it for the
child.  This is the error you would get in
gdb.base/step-over-syscall.exp without this fix:

    /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:3911: internal-error: ULONGEST gdbarch_max_insn_length(gdbarch*): Assertion `gdbarch != NULL' failed.

gdb/ChangeLog:

	* infrun.c (get_displaced_step_closure_by_addr): Adjust to
	std::unique_ptr.
	(displaced_step_clear): Rename to...
	(displaced_step_reset): ... this.  Just call displaced->reset ().
	(displaced_step_clear_cleanup): Rename to...
	(displaced_step_reset_cleanup): ... this.
	(displaced_step_prepare_throw): Adjust to std::unique_ptr.
	(displaced_step_fixup): Likewise.
	(resume_1): Likewise.
	(handle_inferior_event): Restore child's memory before calling
	displaced_step_fixup on the parent.
	* infrun.h (displaced_step_inferior_state) <reset>: Adjust
	to std::unique_ptr.
	<step_closure>: Change type to std::unique_ptr.
This commit is contained in:
Simon Marchi 2020-02-14 15:11:58 -05:00
parent 5f661e0397
commit d8d83535e6
3 changed files with 45 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
gdb/ChangeLog
View File

@ -1,3 +1,20 @@
2020-02-14 Simon Marchi <simon.marchi@efficios.com>
* infrun.c (get_displaced_step_closure_by_addr): Adjust to
std::unique_ptr.
(displaced_step_clear): Rename to...
(displaced_step_reset): ... this. Just call displaced->reset ().
(displaced_step_clear_cleanup): Rename to...
(displaced_step_reset_cleanup): ... this.
(displaced_step_prepare_throw): Adjust to std::unique_ptr.
(displaced_step_fixup): Likewise.
(resume_1): Likewise.
(handle_inferior_event): Restore child's memory before calling
displaced_step_fixup on the parent.
* infrun.h (displaced_step_inferior_state) <reset>: Adjust
to std::unique_ptr.
<step_closure>: Change type to std::unique_ptr.
2020-02-14 Simon Marchi <simon.marchi@efficios.com> 2020-02-14 Simon Marchi <simon.marchi@efficios.com>
* arm-tdep.c: Include count-one-bits.h. * arm-tdep.c: Include count-one-bits.h.

54
gdb/infrun.c
View File

@ -1540,7 +1540,7 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr)
/* If checking the mode of displaced instruction in copy area. */ /* If checking the mode of displaced instruction in copy area. */
if (displaced->step_thread != nullptr if (displaced->step_thread != nullptr
&& displaced->step_copy == addr) && displaced->step_copy == addr)
return displaced->step_closure; return displaced->step_closure.get ();
return NULL; return NULL;
} }
@ -1596,20 +1596,18 @@ use_displaced_stepping (struct thread_info *tp)
&& !displaced_state->failed_before); && !displaced_state->failed_before);
} }
/* Clean out any stray displaced stepping state. */ /* Simple function wrapper around displaced_step_inferior_state::reset. */
static void
displaced_step_clear (struct displaced_step_inferior_state *displaced)
{
/* Indicate that there is no cleanup pending. */
displaced->step_thread = nullptr;
delete displaced->step_closure; static void
displaced->step_closure = NULL; displaced_step_reset (displaced_step_inferior_state *displaced)
{
displaced->reset ();
} }
/* A cleanup that wraps displaced_step_clear. */ /* A cleanup that wraps displaced_step_reset. We use this instead of, say,
using displaced_step_clear_cleanup SCOPE_EXIT, because it needs to be discardable with "cleanup.release ()". */
= FORWARD_SCOPE_EXIT (displaced_step_clear);
using displaced_step_reset_cleanup = FORWARD_SCOPE_EXIT (displaced_step_reset);
/* Dump LEN bytes at BUF in hex to FILE, followed by a newline. */ /* Dump LEN bytes at BUF in hex to FILE, followed by a newline. */
void void
@ -1691,7 +1689,7 @@ displaced_step_prepare_throw (thread_info *tp)
target_pid_to_str (tp->ptid).c_str ()); target_pid_to_str (tp->ptid).c_str ());
} }
displaced_step_clear (displaced); displaced_step_reset (displaced);
scoped_restore_current_thread restore_thread; scoped_restore_current_thread restore_thread;
@ -1754,12 +1752,12 @@ displaced_step_prepare_throw (thread_info *tp)
succeeds. */ succeeds. */
displaced->step_thread = tp; displaced->step_thread = tp;
displaced->step_gdbarch = gdbarch; displaced->step_gdbarch = gdbarch;
displaced->step_closure = closure; displaced->step_closure.reset (closure);
displaced->step_original = original; displaced->step_original = original;
displaced->step_copy = copy; displaced->step_copy = copy;
{ {
displaced_step_clear_cleanup cleanup (displaced); displaced_step_reset_cleanup cleanup (displaced);
/* Resume execution at the copy. */ /* Resume execution at the copy. */
regcache_write_pc (regcache, copy); regcache_write_pc (regcache, copy);
@ -1862,7 +1860,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
if (displaced->step_thread != event_thread) if (displaced->step_thread != event_thread)
return 0; return 0;
displaced_step_clear_cleanup cleanup (displaced); displaced_step_reset_cleanup cleanup (displaced);
displaced_step_restore (displaced, displaced->step_thread->ptid); displaced_step_restore (displaced, displaced->step_thread->ptid);
@ -1879,7 +1877,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
{ {
/* Fix up the resulting state. */ /* Fix up the resulting state. */
gdbarch_displaced_step_fixup (displaced->step_gdbarch, gdbarch_displaced_step_fixup (displaced->step_gdbarch,
displaced->step_closure, displaced->step_closure.get (),
displaced->step_original, displaced->step_original,
displaced->step_copy, displaced->step_copy,
get_thread_regcache (displaced->step_thread)); get_thread_regcache (displaced->step_thread));
@ -2472,8 +2470,8 @@ resume_1 (enum gdb_signal sig)
pc = regcache_read_pc (get_thread_regcache (tp)); pc = regcache_read_pc (get_thread_regcache (tp));
displaced = get_displaced_stepping_state (tp->inf); displaced = get_displaced_stepping_state (tp->inf);
step = gdbarch_displaced_step_hw_singlestep (gdbarch, step = gdbarch_displaced_step_hw_singlestep
displaced->step_closure); (gdbarch, displaced->step_closure.get ());
} }
} }
@ -5305,6 +5303,15 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
struct regcache *child_regcache; struct regcache *child_regcache;
CORE_ADDR parent_pc; CORE_ADDR parent_pc;
if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
{
struct displaced_step_inferior_state *displaced
= get_displaced_stepping_state (parent_inf);
/* Restore scratch pad for child process. */
displaced_step_restore (displaced, ecs->ws.value.related_pid);
}
/* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
indicating that the displaced stepping of syscall instruction indicating that the displaced stepping of syscall instruction
has been done. Perform cleanup for parent process here. Note has been done. Perform cleanup for parent process here. Note
@ -5315,15 +5322,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
that needs it. */ that needs it. */
start_step_over (); start_step_over ();
if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
{
struct displaced_step_inferior_state *displaced
= get_displaced_stepping_state (parent_inf);
/* Restore scratch pad for child process. */
displaced_step_restore (displaced, ecs->ws.value.related_pid);
}
/* Since the vfork/fork syscall instruction was executed in the scratchpad, /* Since the vfork/fork syscall instruction was executed in the scratchpad,
the child's PC is also within the scratchpad. Set the child's PC the child's PC is also within the scratchpad. Set the child's PC
to the parent's PC value, which has already been fixed up. to the parent's PC value, which has already been fixed up.

4
gdb/infrun.h
View File

@ -290,7 +290,7 @@ struct displaced_step_inferior_state
failed_before = 0; failed_before = 0;
step_thread = nullptr; step_thread = nullptr;
step_gdbarch = nullptr; step_gdbarch = nullptr;
step_closure = nullptr; step_closure.reset ();
step_original = 0; step_original = 0;
step_copy = 0; step_copy = 0;
step_saved_copy.clear (); step_saved_copy.clear ();
@ -310,7 +310,7 @@ struct displaced_step_inferior_state
/* The closure provided gdbarch_displaced_step_copy_insn, to be used /* The closure provided gdbarch_displaced_step_copy_insn, to be used
for post-step cleanup. */ for post-step cleanup. */
displaced_step_closure *step_closure; std::unique_ptr<displaced_step_closure> step_closure;
/* The address of the original instruction, and the copy we /* The address of the original instruction, and the copy we
made. */ made. */