[PATCH] Place displaced step data directly in inferior structure

[Simon Marchi]

This patch moves the per-inferior data related to displaced stepping to
be directly in the inferior structure, rather than in a container on the
side.

On notable difference is that previously, we deleted the state on
inferior exit, which guaranteed a clean state if re-using the inferior
for a new run or attach. We now need to reset the state manually.

gdb/ChangeLog:

* inferior.h (class inferior) <displaced_step_state>: New field.
* infrun.h (struct displaced_step_state): Move here from
infrun.c. Initialize fields.
<inf>: Remove field.
<reset>: New method.
* infrun.c (struct displaced_step_inferior_state): Move to
infrun.h.
(displaced_step_inferior_states): Remove.
(get_displaced_stepping_state): Adust.
(displaced_step_in_progress_any_inferior): Adjust.
(displaced_step_in_progress_thread): Adjust.
(displaced_step_in_progress): Adjust.
(add_displaced_stepping_state): Remove.
(get_displaced_step_closure_by_addr): Adjust.
(remove_displaced_stepping_state): Remove.
(infrun_inferior_exit): Call displaced_step_state.reset.
(use_displaced_stepping): Don't check for NULL.
(displaced_step_prepare_throw): Call
get_displaced_stepping_state.
(displaced_step_fixup): Don't check for NULL.
(prepare_for_detach): Don't check for NULL.
---
gdb/inferior.h | 3 ++
gdb/infrun.c | 129 ++++++-------------------------------------------
gdb/infrun.h | 47 ++++++++++++++++++
3 files changed, 66 insertions(+), 113 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 33c2eac9d3c..1a3f579d8d2 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -503,6 +503,9 @@ public:
this gdbarch. */
struct gdbarch *gdbarch = NULL;

+ /* Data related to displaced stepping. */
+ displaced_step_inferior_state displaced_step_state;
+
/* Per inferior data-pointers required by other GDB modules. */
REGISTRY_FIELDS;
};
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..63dd28237dd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1476,53 +1476,12 @@ step_over_info_valid_p (void)

displaced_step_closure::~displaced_step_closure () = default;

-/* Per-inferior displaced stepping state. */
-struct displaced_step_inferior_state
-{
- /* The process this displaced step state refers to. */
- inferior *inf;
-
- /* True if preparing a displaced step ever failed. If so, we won't
- try displaced stepping for this inferior again. */
- int failed_before;
-
- /* If this is not nullptr, this is the thread carrying out a
- displaced single-step in process PID. This thread's state will
- require fixing up once it has completed its step. */
- thread_info *step_thread;
-
- /* The architecture the thread had when we stepped it. */
- struct gdbarch *step_gdbarch;
-
- /* The closure provided gdbarch_displaced_step_copy_insn, to be used
- for post-step cleanup. */
- struct displaced_step_closure *step_closure;
-
- /* The address of the original instruction, and the copy we
- made. */
- CORE_ADDR step_original, step_copy;
-
- /* Saved contents of copy area. */
- gdb_byte *step_saved_copy;
-};
-
-/* The list of states of processes involved in displaced stepping
- presently. */
-static std::forward_list<displaced_step_inferior_state *>
- displaced_step_inferior_states;
-
/* Get the displaced stepping state of process PID. */

static displaced_step_inferior_state *
get_displaced_stepping_state (inferior *inf)
{
- for (auto *state : displaced_step_inferior_states)
- {
- if (state->inf == inf)
- return state;
- }
-
- return nullptr;
+ return &inf->displaced_step_state;
}

/* Returns true if any inferior has a thread doing a displaced
@@ -1531,9 +1490,11 @@ get_displaced_stepping_state (inferior *inf)
static bool
displaced_step_in_progress_any_inferior ()
{
- for (auto *state : displaced_step_inferior_states)
+ inferior *i;
+
+ ALL_INFERIORS (i)
{
- if (state->step_thread != nullptr)
+ if (i->displaced_step_state.step_thread != nullptr)
return true;
}

@@ -1546,13 +1507,9 @@ displaced_step_in_progress_any_inferior ()
static int
displaced_step_in_progress_thread (thread_info *thread)
{
- struct displaced_step_inferior_state *displaced;
-
gdb_assert (thread != NULL);

- displaced = get_displaced_stepping_state (thread->inf);
-
- return (displaced != NULL && displaced->step_thread == thread);
+ return get_displaced_stepping_state (thread->inf)->step_thread == thread;
}

/* Return true if process PID has a thread doing a displaced step. */
@@ -1560,34 +1517,7 @@ displaced_step_in_progress_thread (thread_info *thread)
static int
displaced_step_in_progress (inferior *inf)
{
- struct displaced_step_inferior_state *displaced;
-
- displaced = get_displaced_stepping_state (inf);
- if (displaced != NULL && displaced->step_thread != nullptr)
- return 1;
-
- return 0;
-}
-
-/* Add a new displaced stepping state for process PID to the displaced
- stepping state list, or return a pointer to an already existing
- entry, if it already exists. Never returns NULL. */
-
-static displaced_step_inferior_state *
-add_displaced_stepping_state (inferior *inf)
-{
- displaced_step_inferior_state *state
- = get_displaced_stepping_state (inf);
-
- if (state != nullptr)
- return state;
-
- state = XCNEW (struct displaced_step_inferior_state);
- state->inf = inf;
-
- displaced_step_inferior_states.push_front (state);
-
- return state;
+ return get_displaced_stepping_state (inf)->step_thread != nullptr;
}

/* If inferior is in displaced stepping, and ADDR equals to starting address
@@ -1597,42 +1527,21 @@ add_displaced_stepping_state (inferior *inf)
struct displaced_step_closure*
get_displaced_step_closure_by_addr (CORE_ADDR addr)
{
- struct displaced_step_inferior_state *displaced
+ displaced_step_inferior_state *displaced
= get_displaced_stepping_state (current_inferior ());

/* If checking the mode of displaced instruction in copy area. */
- if (displaced != NULL
- && displaced->step_thread != nullptr
+ if (displaced->step_thread != nullptr
&& displaced->step_copy == addr)
return displaced->step_closure;

return NULL;
}

-/* Remove the displaced stepping state of process PID. */
-
-static void
-remove_displaced_stepping_state (inferior *inf)
-{
- gdb_assert (inf != nullptr);
-
- displaced_step_inferior_states.remove_if
- ([inf] (displaced_step_inferior_state *state)
- {
- if (state->inf == inf)
- {
- xfree (state);
- return true;
- }
- else
- return false;
- });
-}
-
static void
infrun_inferior_exit (struct inferior *inf)
{
- remove_displaced_stepping_state (inf);
+ inf->displaced_step_state.reset ();
}

/* If ON, and the architecture supports it, GDB will use displaced
@@ -1669,17 +1578,15 @@ use_displaced_stepping (struct thread_info *tp)
{
struct regcache *regcache = get_thread_regcache (tp);
struct gdbarch *gdbarch = regcache->arch ();
- struct displaced_step_inferior_state *displaced_state;
-
- displaced_state = get_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced_state
+ = get_displaced_stepping_state (tp->inf);

return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
&& target_is_non_stop_p ())
|| can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
&& gdbarch_displaced_step_copy_insn_p (gdbarch)
&& find_record_target () == NULL
- && (displaced_state == NULL
- || !displaced_state->failed_before));
+ && !displaced_state->failed_before);
}

/* Clean out any stray displaced stepping state. */
@@ -1741,7 +1648,6 @@ displaced_step_prepare_throw (thread_info *tp)
CORE_ADDR original, copy;
ULONGEST len;
struct displaced_step_closure *closure;
- struct displaced_step_inferior_state *displaced;
int status;

/* We should never reach this function if the architecture does not
@@ -1760,7 +1666,8 @@ displaced_step_prepare_throw (thread_info *tp)
/* We have to displaced step one thread at a time, as we only have
access to a single scratch space per inferior. */

- displaced = add_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced
+ = get_displaced_stepping_state (tp->inf);

if (displaced->step_thread != nullptr)
{
@@ -1953,10 +1860,6 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
= get_displaced_stepping_state (event_thread->inf);
int ret;

- /* Was any thread of this process doing a displaced step? */
- if (displaced == NULL)
- return 0;
-
/* Was this event for the thread we displaced? */
if (displaced->step_thread != event_thread)
return 0;
@@ -3577,7 +3480,7 @@ prepare_for_detach (void)

/* Is any thread of this process displaced stepping? If not,
there's nothing else to do. */
- if (displaced == NULL || displaced->step_thread == nullptr)
+ if (displaced->step_thread == nullptr)
return;

if (debug_infrun)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a701f0ca47f..81b3c326163 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -258,4 +258,51 @@ struct buf_displaced_step_closure : displaced_step_closure
gdb::byte_vector buf;
};

+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ /* Put this object back in its original state. s*/
+ void reset ()
+ {
+ /* These should have been cleaned after the last displaced step
+ operation, so if there are still set, it's a bug. */
+ gdb_assert (step_thread == nullptr);
+ gdb_assert (step_closure == nullptr);
+
+ failed_before = 0;
+ step_gdbarch = nullptr;
+ step_original = 0;
+ step_copy = 0;
+
+ if (step_saved_copy != nullptr)
+ {
+ xfree (step_saved_copy);
+ step_saved_copy = nullptr;
+ }
+ }
+
+ /* True if preparing a displaced step ever failed. If so, we won't
+ try displaced stepping for this inferior again. */
+ int failed_before = 0;
+
+ /* If this is not nullptr, this is the thread carrying out a
+ displaced single-step in process PID. This thread's state will
+ require fixing up once it has completed its step. */
+ thread_info *step_thread = nullptr;
+
+ /* The architecture the thread had when we stepped it. */
+ gdbarch *step_gdbarch = nullptr;
+
+ /* The closure provided gdbarch_displaced_step_copy_insn, to be used
+ for post-step cleanup. */
+ displaced_step_closure *step_closure = nullptr;
+
+ /* The address of the original instruction, and the copy we
+ made. */
+ CORE_ADDR step_original = 0, step_copy = 0;
+
+ /* Saved contents of copy area. */
+ gdb_byte *step_saved_copy = nullptr;
+};
+
#endif /* INFRUN_H */

[Pedro Alves]

Post by Simon Marchi
This patch moves the per-inferior data related to displaced stepping to
be directly in the inferior structure, rather than in a container on the
side.
On notable difference is that previously, we deleted the state on
inferior exit, which guaranteed a clean state if re-using the inferior
for a new run or attach. We now need to reset the state manually.

Thanks. This LGTM. See a couple nits below.

Post by Simon Marchi
/* Returns true if any inferior has a thread doing a displaced
@@ -1531,9 +1490,11 @@ get_displaced_stepping_state (inferior *inf)
static bool
displaced_step_in_progress_any_inferior ()
{
- for (auto *state : displaced_step_inferior_states)
+ inferior *i;
+
+ ALL_INFERIORS (i)

This won't compile in current master. :-)

Use

for (inferior *inf : all_inferiors ())

Post by Simon Marchi
{
- if (state->step_thread != nullptr)
+ if (i->displaced_step_state.step_thread != nullptr)
return true;
}
if (debug_infrun)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a701f0ca47f..81b3c326163 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -258,4 +258,51 @@ struct buf_displaced_step_closure : displaced_step_closure
gdb::byte_vector buf;
};
+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ /* Put this object back in its original state. s*/

Typo: the "s" at the end.

Post by Simon Marchi
+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ /* Put this object back in its original state. s*/
+ void reset ()
+ {
+ /* These should have been cleaned after the last displaced step
+ operation, so if there are still set, it's a bug. */
+ gdb_assert (step_thread == nullptr);
+ gdb_assert (step_closure == nullptr);
+
+ failed_before = 0;
+ step_gdbarch = nullptr;
+ step_original = 0;
+ step_copy = 0;
+
+ if (step_saved_copy != nullptr)
+ {
+ xfree (step_saved_copy);
+ step_saved_copy = nullptr;
+ }
+ }
+
+ /* True if preparing a displaced step ever failed. If so, we won't
+ try displaced stepping for this inferior again. */
+ int failed_before = 0;
+
+ /* If this is not nullptr, this is the thread carrying out a
+ displaced single-step in process PID. This thread's state will
+ require fixing up once it has completed its step. */
+ thread_info *step_thread = nullptr;
+
+ /* The architecture the thread had when we stepped it. */
+ gdbarch *step_gdbarch = nullptr;
+
+ /* The closure provided gdbarch_displaced_step_copy_insn, to be used
+ for post-step cleanup. */
+ displaced_step_closure *step_closure = nullptr;
+
+ /* The address of the original instruction, and the copy we
+ made. */
+ CORE_ADDR step_original = 0, step_copy = 0;
+
+ /* Saved contents of copy area. */
+ gdb_byte *step_saved_copy = nullptr;

I'm fine with this, but I thought I'd mention another possibility,
which is to not initialize the field in-class, but instead add
a ctor that calls reset(), thus keeping the initializations
all in the same place.

Thanks,
Pedro Alves

[Simon Marchi]

Post by Pedro Alves
This won't compile in current master. :-)
Use
for (inferior *inf : all_inferiors ())

Why do you keep breaking things!?

(/s if that wasn't clear)

Post by Pedro Alves

Post by Simon Marchi
{
- if (state->step_thread != nullptr)
+ if (i->displaced_step_state.step_thread != nullptr)
return true;
}
if (debug_infrun)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a701f0ca47f..81b3c326163 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -258,4 +258,51 @@ struct buf_displaced_step_closure : displaced_step_closure
gdb::byte_vector buf;
};
+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ /* Put this object back in its original state. s*/

Typo: the "s" at the end.

Fixed.

Post by Pedro Alves
I'm fine with this, but I thought I'd mention another possibility,
which is to not initialize the field in-class, but instead add
a ctor that calls reset(), thus keeping the initializations
all in the same place.

I agree it would be nice to avoid that duplication. I kept the asserts and
in-class initializations for step_thread and step_closure, because I think
having those asserts can be useful.

I also changed step_saved_copy to be a vector, this makes the job a bit easier
and plugs the leak identified in

https://sourceware.org/ml/gdb-patches/2018-11/msg00220.html

Technically we don't need to clear it... but if we didn't the comment
above "reset" would be a lie :).

Here's the updated patch (currently testing on the buildbot):


From 0220f85e766ae1035734d0e7cd09b76358fe85ca Mon Sep 17 00:00:00 2001
From: Simon Marchi <***@ericsson.com>
Date: Wed, 21 Nov 2018 21:35:16 -0500
Subject: [PATCH] Place displaced step data directly in inferior structure

This patch moves the per-inferior data related to displaced stepping to
be directly in the inferior structure, rather than in a container on the
side.

On notable difference is that previously, we deleted the state on
inferior exit, which guaranteed a clean state if re-using the inferior
for a new run or attach. We now need to reset the state manually.

At the same time, I changed step_saved_copy to be a gdb::byte_vector, so
it is automatically freed on destruction (which should plug the leak
reported here [1]).

[1] https://sourceware.org/ml/gdb-patches/2018-11/msg00202.html

gdb/ChangeLog:

* inferior.h (class inferior) <displaced_step_state>: New field.
* infrun.h (struct displaced_step_state): Move here from
infrun.c. Initialize some fields, add constructor.
<inf>: Remove field.
<reset>: New method.
* infrun.c (struct displaced_step_inferior_state): Move to
infrun.h.
(displaced_step_inferior_states): Remove.
(get_displaced_stepping_state): Adust.
(displaced_step_in_progress_any_inferior): Adjust.
(displaced_step_in_progress_thread): Adjust.
(displaced_step_in_progress): Adjust.
(add_displaced_stepping_state): Remove.
(get_displaced_step_closure_by_addr): Adjust.
(remove_displaced_stepping_state): Remove.
(infrun_inferior_exit): Call displaced_step_state.reset.
(use_displaced_stepping): Don't check for NULL.
(displaced_step_prepare_throw): Call
get_displaced_stepping_state.
(displaced_step_fixup): Don't check for NULL.
(prepare_for_detach): Don't check for NULL.
---
gdb/inferior.h | 3 ++
gdb/infrun.c | 142 +++++++------------------------------------------
gdb/infrun.h | 47 ++++++++++++++++
3 files changed, 70 insertions(+), 122 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 33c2eac9d3c..1a3f579d8d2 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -503,6 +503,9 @@ public:
this gdbarch. */
struct gdbarch *gdbarch = NULL;

+ /* Data related to displaced stepping. */
+ displaced_step_inferior_state displaced_step_state;
+
/* Per inferior data-pointers required by other GDB modules. */
REGISTRY_FIELDS;
};
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..cf216c7804e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1476,53 +1476,12 @@ step_over_info_valid_p (void)

displaced_step_closure::~displaced_step_closure () = default;

-/* Per-inferior displaced stepping state. */
-struct displaced_step_inferior_state
-{
- /* The process this displaced step state refers to. */
- inferior *inf;
-
- /* True if preparing a displaced step ever failed. If so, we won't
- try displaced stepping for this inferior again. */
- int failed_before;
-
- /* If this is not nullptr, this is the thread carrying out a
- displaced single-step in process PID. This thread's state will
- require fixing up once it has completed its step. */
- thread_info *step_thread;
-
- /* The architecture the thread had when we stepped it. */
- struct gdbarch *step_gdbarch;
-
- /* The closure provided gdbarch_displaced_step_copy_insn, to be used
- for post-step cleanup. */
- struct displaced_step_closure *step_closure;
-
- /* The address of the original instruction, and the copy we
- made. */
- CORE_ADDR step_original, step_copy;
-
- /* Saved contents of copy area. */
- gdb_byte *step_saved_copy;
-};
-
-/* The list of states of processes involved in displaced stepping
- presently. */
-static std::forward_list<displaced_step_inferior_state *>
- displaced_step_inferior_states;
-
/* Get the displaced stepping state of process PID. */

static displaced_step_inferior_state *
get_displaced_stepping_state (inferior *inf)
{
- for (auto *state : displaced_step_inferior_states)
- {
- if (state->inf == inf)
- return state;
- }
-
- return nullptr;
+ return &inf->displaced_step_state;
}

/* Returns true if any inferior has a thread doing a displaced
@@ -1531,9 +1490,9 @@ get_displaced_stepping_state (inferior *inf)
static bool
displaced_step_in_progress_any_inferior ()
{
- for (auto *state : displaced_step_inferior_states)
+ for (inferior *i : all_inferiors ())
{
- if (state->step_thread != nullptr)
+ if (i->displaced_step_state.step_thread != nullptr)
return true;
}

@@ -1546,13 +1505,9 @@ displaced_step_in_progress_any_inferior ()
static int
displaced_step_in_progress_thread (thread_info *thread)
{
- struct displaced_step_inferior_state *displaced;
-
gdb_assert (thread != NULL);

- displaced = get_displaced_stepping_state (thread->inf);
-
- return (displaced != NULL && displaced->step_thread == thread);
+ return get_displaced_stepping_state (thread->inf)->step_thread == thread;
}

/* Return true if process PID has a thread doing a displaced step. */
@@ -1560,34 +1515,7 @@ displaced_step_in_progress_thread (thread_info *thread)
static int
displaced_step_in_progress (inferior *inf)
{
- struct displaced_step_inferior_state *displaced;
-
- displaced = get_displaced_stepping_state (inf);
- if (displaced != NULL && displaced->step_thread != nullptr)
- return 1;
-
- return 0;
-}
-
-/* Add a new displaced stepping state for process PID to the displaced
- stepping state list, or return a pointer to an already existing
- entry, if it already exists. Never returns NULL. */
-
-static displaced_step_inferior_state *
-add_displaced_stepping_state (inferior *inf)
-{
- displaced_step_inferior_state *state
- = get_displaced_stepping_state (inf);
-
- if (state != nullptr)
- return state;
-
- state = XCNEW (struct displaced_step_inferior_state);
- state->inf = inf;
-
- displaced_step_inferior_states.push_front (state);
-
- return state;
+ return get_displaced_stepping_state (inf)->step_thread != nullptr;
}

/* If inferior is in displaced stepping, and ADDR equals to starting address
@@ -1597,42 +1525,21 @@ add_displaced_stepping_state (inferior *inf)
struct displaced_step_closure*
get_displaced_step_closure_by_addr (CORE_ADDR addr)
{
- struct displaced_step_inferior_state *displaced
+ displaced_step_inferior_state *displaced
= get_displaced_stepping_state (current_inferior ());

/* If checking the mode of displaced instruction in copy area. */
- if (displaced != NULL
- && displaced->step_thread != nullptr
+ if (displaced->step_thread != nullptr
&& displaced->step_copy == addr)
return displaced->step_closure;

return NULL;
}

-/* Remove the displaced stepping state of process PID. */
-
-static void
-remove_displaced_stepping_state (inferior *inf)
-{
- gdb_assert (inf != nullptr);
-
- displaced_step_inferior_states.remove_if
- ([inf] (displaced_step_inferior_state *state)
- {
- if (state->inf == inf)
- {
- xfree (state);
- return true;
- }
- else
- return false;
- });
-}
-
static void
infrun_inferior_exit (struct inferior *inf)
{
- remove_displaced_stepping_state (inf);
+ inf->displaced_step_state.reset ();
}

/* If ON, and the architecture supports it, GDB will use displaced
@@ -1669,17 +1576,15 @@ use_displaced_stepping (struct thread_info *tp)
{
struct regcache *regcache = get_thread_regcache (tp);
struct gdbarch *gdbarch = regcache->arch ();
- struct displaced_step_inferior_state *displaced_state;
-
- displaced_state = get_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced_state
+ = get_displaced_stepping_state (tp->inf);

return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
&& target_is_non_stop_p ())
|| can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
&& gdbarch_displaced_step_copy_insn_p (gdbarch)
&& find_record_target () == NULL
- && (displaced_state == NULL
- || !displaced_state->failed_before));
+ && !displaced_state->failed_before);
}

/* Clean out any stray displaced stepping state. */
@@ -1734,14 +1639,12 @@ displaced_step_dump_bytes (struct ui_file *file,
static int
displaced_step_prepare_throw (thread_info *tp)
{
- struct cleanup *ignore_cleanups;
regcache *regcache = get_thread_regcache (tp);
struct gdbarch *gdbarch = regcache->arch ();
const address_space *aspace = regcache->aspace ();
CORE_ADDR original, copy;
ULONGEST len;
struct displaced_step_closure *closure;
- struct displaced_step_inferior_state *displaced;
int status;

/* We should never reach this function if the architecture does not
@@ -1760,7 +1663,8 @@ displaced_step_prepare_throw (thread_info *tp)
/* We have to displaced step one thread at a time, as we only have
access to a single scratch space per inferior. */

- displaced = add_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced
+ = get_displaced_stepping_state (tp->inf);

if (displaced->step_thread != nullptr)
{
@@ -1816,10 +1720,8 @@ displaced_step_prepare_throw (thread_info *tp)
}

/* Save the original contents of the copy area. */
- displaced->step_saved_copy = (gdb_byte *) xmalloc (len);
- ignore_cleanups = make_cleanup (free_current_contents,
- &displaced->step_saved_copy);
- status = target_read_memory (copy, displaced->step_saved_copy, len);
+ displaced->step_saved_copy.resize (len);
+ status = target_read_memory (copy, displaced->step_saved_copy.data (), len);
if (status != 0)
throw_error (MEMORY_ERROR,
_("Error accessing memory address %s (%s) for "
@@ -1830,7 +1732,7 @@ displaced_step_prepare_throw (thread_info *tp)
fprintf_unfiltered (gdb_stdlog, "displaced: saved %s: ",
paddress (gdbarch, copy));
displaced_step_dump_bytes (gdb_stdlog,
- displaced->step_saved_copy,
+ displaced->step_saved_copy.data (),
len);
};

@@ -1841,7 +1743,6 @@ displaced_step_prepare_throw (thread_info *tp)
/* The architecture doesn't know how or want to displaced step
this instruction or instruction sequence. Fallback to
stepping over the breakpoint in-line. */
- do_cleanups (ignore_cleanups);
return -1;
}

@@ -1853,7 +1754,8 @@ displaced_step_prepare_throw (thread_info *tp)
displaced->step_original = original;
displaced->step_copy = copy;

- make_cleanup (displaced_step_clear_cleanup, displaced);
+ cleanup *ignore_cleanups
+ = make_cleanup (displaced_step_clear_cleanup, displaced);

/* Resume execution at the copy. */
regcache_write_pc (regcache, copy);
@@ -1931,7 +1833,7 @@ displaced_step_restore (struct displaced_step_inferior_state *displaced,
ULONGEST len = gdbarch_max_insn_length (displaced->step_gdbarch);

write_memory_ptid (ptid, displaced->step_copy,
- displaced->step_saved_copy, len);
+ displaced->step_saved_copy.data (), len);
if (debug_displaced)
fprintf_unfiltered (gdb_stdlog, "displaced: restored %s %s\n",
target_pid_to_str (ptid),
@@ -1953,10 +1855,6 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
= get_displaced_stepping_state (event_thread->inf);
int ret;

- /* Was any thread of this process doing a displaced step? */
- if (displaced == NULL)
- return 0;
-
/* Was this event for the thread we displaced? */
if (displaced->step_thread != event_thread)
return 0;
@@ -3577,7 +3475,7 @@ prepare_for_detach (void)

/* Is any thread of this process displaced stepping? If not,
there's nothing else to do. */
- if (displaced == NULL || displaced->step_thread == nullptr)
+ if (displaced->step_thread == nullptr)
return;

if (debug_infrun)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a701f0ca47f..ef5038c2fb2 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -258,4 +258,51 @@ struct buf_displaced_step_closure : displaced_step_closure
gdb::byte_vector buf;
};

+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ displaced_step_inferior_state ()
+ {
+ reset ();
+ }
+
+ /* Put this object back in its original state. */
+ void reset ()
+ {
+ /* These should have been cleared after the last displaced step operation
+ (or during object construction), so if there are set, it's a bug. */
+ gdb_assert (step_thread == nullptr);
+ gdb_assert (step_closure == nullptr);
+
+ failed_before = 0;
+ step_gdbarch = nullptr;
+ step_original = 0;
+ step_copy = 0;
+ step_saved_copy.clear ();
+ }
+
+ /* True if preparing a displaced step ever failed. If so, we won't
+ try displaced stepping for this inferior again. */
+ int failed_before;
+
+ /* If this is not nullptr, this is the thread carrying out a
+ displaced single-step in process PID. This thread's state will
+ require fixing up once it has completed its step. */
+ thread_info *step_thread = nullptr;
+
+ /* The architecture the thread had when we stepped it. */
+ gdbarch *step_gdbarch;
+
+ /* The closure provided gdbarch_displaced_step_copy_insn, to be used
+ for post-step cleanup. */
+ displaced_step_closure *step_closure = nullptr;
+
+ /* The address of the original instruction, and the copy we
+ made. */
+ CORE_ADDR step_original, step_copy;
+
+ /* Saved contents of copy area. */
+ gdb::byte_vector step_saved_copy;
+};
+
#endif /* INFRUN_H */
--
2.19.1

[Simon Marchi]

Post by Simon Marchi
I agree it would be nice to avoid that duplication. I kept the asserts and
in-class initializations for step_thread and step_closure, because I think
having those asserts can be useful.
I also changed step_saved_copy to be a vector, this makes the job a bit easier
and plugs the leak identified in
https://sourceware.org/ml/gdb-patches/2018-11/msg00220.html
Technically we don't need to clear it... but if we didn't the comment
above "reset" would be a lie :).

Ah damn it, there's a case where the assertion does not hold (gdb.base/step-over-exit.exp).
So here's a simpler version of the patch where we just blindly reset everything to the
default values (similar to what we do today).


From 1587bbc5c67349a1429542989cf33b102109746e Mon Sep 17 00:00:00 2001
From: Simon Marchi <***@ericsson.com>
Date: Wed, 21 Nov 2018 21:35:16 -0500
Subject: [PATCH] Place displaced step data directly in inferior structure

This patch moves the per-inferior data related to displaced stepping to
be directly in the inferior structure, rather than in a container on the
side.

On notable difference is that previously, we deleted the state on
inferior exit, which guaranteed a clean state if re-using the inferior
for a new run or attach. We now need to reset the state manually.

At the same time, I changed step_saved_copy to be a gdb::byte_vector, so
it is automatically freed on destruction (which should plug the leak
reported here [1]).

[1] https://sourceware.org/ml/gdb-patches/2018-11/msg00202.html

gdb/ChangeLog:

* inferior.h (class inferior) <displaced_step_state>: New field.
* infrun.h (struct displaced_step_state): Move here from
infrun.c. Initialize fields, add constructor.
<inf>: Remove field.
<reset>: New method.
* infrun.c (struct displaced_step_inferior_state): Move to
infrun.h.
(displaced_step_inferior_states): Remove.
(get_displaced_stepping_state): Adust.
(displaced_step_in_progress_any_inferior): Adjust.
(displaced_step_in_progress_thread): Adjust.
(displaced_step_in_progress): Adjust.
(add_displaced_stepping_state): Remove.
(get_displaced_step_closure_by_addr): Adjust.
(remove_displaced_stepping_state): Remove.
(infrun_inferior_exit): Call displaced_step_state.reset.
(use_displaced_stepping): Don't check for NULL.
(displaced_step_prepare_throw): Call
get_displaced_stepping_state.
(displaced_step_fixup): Don't check for NULL.
(prepare_for_detach): Don't check for NULL.
---
gdb/inferior.h | 3 ++
gdb/infrun.c | 142 +++++++------------------------------------------
gdb/infrun.h | 44 +++++++++++++++
3 files changed, 67 insertions(+), 122 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 33c2eac9d3c..1a3f579d8d2 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -503,6 +503,9 @@ public:
this gdbarch. */
struct gdbarch *gdbarch = NULL;

+ /* Data related to displaced stepping. */
+ displaced_step_inferior_state displaced_step_state;
+
/* Per inferior data-pointers required by other GDB modules. */
REGISTRY_FIELDS;
};
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46a8985f860..cf216c7804e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1476,53 +1476,12 @@ step_over_info_valid_p (void)

displaced_step_closure::~displaced_step_closure () = default;

-/* Per-inferior displaced stepping state. */
-struct displaced_step_inferior_state
-{
- /* The process this displaced step state refers to. */
- inferior *inf;
-
- /* True if preparing a displaced step ever failed. If so, we won't
- try displaced stepping for this inferior again. */
- int failed_before;
-
- /* If this is not nullptr, this is the thread carrying out a
- displaced single-step in process PID. This thread's state will
- require fixing up once it has completed its step. */
- thread_info *step_thread;
-
- /* The architecture the thread had when we stepped it. */
- struct gdbarch *step_gdbarch;
-
- /* The closure provided gdbarch_displaced_step_copy_insn, to be used
- for post-step cleanup. */
- struct displaced_step_closure *step_closure;
-
- /* The address of the original instruction, and the copy we
- made. */
- CORE_ADDR step_original, step_copy;
-
- /* Saved contents of copy area. */
- gdb_byte *step_saved_copy;
-};
-
-/* The list of states of processes involved in displaced stepping
- presently. */
-static std::forward_list<displaced_step_inferior_state *>
- displaced_step_inferior_states;
-
/* Get the displaced stepping state of process PID. */

static displaced_step_inferior_state *
get_displaced_stepping_state (inferior *inf)
{
- for (auto *state : displaced_step_inferior_states)
- {
- if (state->inf == inf)
- return state;
- }
-
- return nullptr;
+ return &inf->displaced_step_state;
}

/* Returns true if any inferior has a thread doing a displaced
@@ -1531,9 +1490,9 @@ get_displaced_stepping_state (inferior *inf)
static bool
displaced_step_in_progress_any_inferior ()
{
- for (auto *state : displaced_step_inferior_states)
+ for (inferior *i : all_inferiors ())
{
- if (state->step_thread != nullptr)
+ if (i->displaced_step_state.step_thread != nullptr)
return true;
}

@@ -1546,13 +1505,9 @@ displaced_step_in_progress_any_inferior ()
static int
displaced_step_in_progress_thread (thread_info *thread)
{
- struct displaced_step_inferior_state *displaced;
-
gdb_assert (thread != NULL);

- displaced = get_displaced_stepping_state (thread->inf);
-
- return (displaced != NULL && displaced->step_thread == thread);
+ return get_displaced_stepping_state (thread->inf)->step_thread == thread;
}

/* Return true if process PID has a thread doing a displaced step. */
@@ -1560,34 +1515,7 @@ displaced_step_in_progress_thread (thread_info *thread)
static int
displaced_step_in_progress (inferior *inf)
{
- struct displaced_step_inferior_state *displaced;
-
- displaced = get_displaced_stepping_state (inf);
- if (displaced != NULL && displaced->step_thread != nullptr)
- return 1;
-
- return 0;
-}
-
-/* Add a new displaced stepping state for process PID to the displaced
- stepping state list, or return a pointer to an already existing
- entry, if it already exists. Never returns NULL. */
-
-static displaced_step_inferior_state *
-add_displaced_stepping_state (inferior *inf)
-{
- displaced_step_inferior_state *state
- = get_displaced_stepping_state (inf);
-
- if (state != nullptr)
- return state;
-
- state = XCNEW (struct displaced_step_inferior_state);
- state->inf = inf;
-
- displaced_step_inferior_states.push_front (state);
-
- return state;
+ return get_displaced_stepping_state (inf)->step_thread != nullptr;
}

/* If inferior is in displaced stepping, and ADDR equals to starting address
@@ -1597,42 +1525,21 @@ add_displaced_stepping_state (inferior *inf)
struct displaced_step_closure*
get_displaced_step_closure_by_addr (CORE_ADDR addr)
{
- struct displaced_step_inferior_state *displaced
+ displaced_step_inferior_state *displaced
= get_displaced_stepping_state (current_inferior ());

/* If checking the mode of displaced instruction in copy area. */
- if (displaced != NULL
- && displaced->step_thread != nullptr
+ if (displaced->step_thread != nullptr
&& displaced->step_copy == addr)
return displaced->step_closure;

return NULL;
}

-/* Remove the displaced stepping state of process PID. */
-
-static void
-remove_displaced_stepping_state (inferior *inf)
-{
- gdb_assert (inf != nullptr);
-
- displaced_step_inferior_states.remove_if
- ([inf] (displaced_step_inferior_state *state)
- {
- if (state->inf == inf)
- {
- xfree (state);
- return true;
- }
- else
- return false;
- });
-}
-
static void
infrun_inferior_exit (struct inferior *inf)
{
- remove_displaced_stepping_state (inf);
+ inf->displaced_step_state.reset ();
}

/* If ON, and the architecture supports it, GDB will use displaced
@@ -1669,17 +1576,15 @@ use_displaced_stepping (struct thread_info *tp)
{
struct regcache *regcache = get_thread_regcache (tp);
struct gdbarch *gdbarch = regcache->arch ();
- struct displaced_step_inferior_state *displaced_state;
-
- displaced_state = get_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced_state
+ = get_displaced_stepping_state (tp->inf);

return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
&& target_is_non_stop_p ())
|| can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
&& gdbarch_displaced_step_copy_insn_p (gdbarch)
&& find_record_target () == NULL
- && (displaced_state == NULL
- || !displaced_state->failed_before));
+ && !displaced_state->failed_before);
}

/* Clean out any stray displaced stepping state. */
@@ -1734,14 +1639,12 @@ displaced_step_dump_bytes (struct ui_file *file,
static int
displaced_step_prepare_throw (thread_info *tp)
{
- struct cleanup *ignore_cleanups;
regcache *regcache = get_thread_regcache (tp);
struct gdbarch *gdbarch = regcache->arch ();
const address_space *aspace = regcache->aspace ();
CORE_ADDR original, copy;
ULONGEST len;
struct displaced_step_closure *closure;
- struct displaced_step_inferior_state *displaced;
int status;

/* We should never reach this function if the architecture does not
@@ -1760,7 +1663,8 @@ displaced_step_prepare_throw (thread_info *tp)
/* We have to displaced step one thread at a time, as we only have
access to a single scratch space per inferior. */

- displaced = add_displaced_stepping_state (tp->inf);
+ displaced_step_inferior_state *displaced
+ = get_displaced_stepping_state (tp->inf);

if (displaced->step_thread != nullptr)
{
@@ -1816,10 +1720,8 @@ displaced_step_prepare_throw (thread_info *tp)
}

/* Save the original contents of the copy area. */
- displaced->step_saved_copy = (gdb_byte *) xmalloc (len);
- ignore_cleanups = make_cleanup (free_current_contents,
- &displaced->step_saved_copy);
- status = target_read_memory (copy, displaced->step_saved_copy, len);
+ displaced->step_saved_copy.resize (len);
+ status = target_read_memory (copy, displaced->step_saved_copy.data (), len);
if (status != 0)
throw_error (MEMORY_ERROR,
_("Error accessing memory address %s (%s) for "
@@ -1830,7 +1732,7 @@ displaced_step_prepare_throw (thread_info *tp)
fprintf_unfiltered (gdb_stdlog, "displaced: saved %s: ",
paddress (gdbarch, copy));
displaced_step_dump_bytes (gdb_stdlog,
- displaced->step_saved_copy,
+ displaced->step_saved_copy.data (),
len);
};

@@ -1841,7 +1743,6 @@ displaced_step_prepare_throw (thread_info *tp)
/* The architecture doesn't know how or want to displaced step
this instruction or instruction sequence. Fallback to
stepping over the breakpoint in-line. */
- do_cleanups (ignore_cleanups);
return -1;
}

@@ -1853,7 +1754,8 @@ displaced_step_prepare_throw (thread_info *tp)
displaced->step_original = original;
displaced->step_copy = copy;

- make_cleanup (displaced_step_clear_cleanup, displaced);
+ cleanup *ignore_cleanups
+ = make_cleanup (displaced_step_clear_cleanup, displaced);

/* Resume execution at the copy. */
regcache_write_pc (regcache, copy);
@@ -1931,7 +1833,7 @@ displaced_step_restore (struct displaced_step_inferior_state *displaced,
ULONGEST len = gdbarch_max_insn_length (displaced->step_gdbarch);

write_memory_ptid (ptid, displaced->step_copy,
- displaced->step_saved_copy, len);
+ displaced->step_saved_copy.data (), len);
if (debug_displaced)
fprintf_unfiltered (gdb_stdlog, "displaced: restored %s %s\n",
target_pid_to_str (ptid),
@@ -1953,10 +1855,6 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
= get_displaced_stepping_state (event_thread->inf);
int ret;

- /* Was any thread of this process doing a displaced step? */
- if (displaced == NULL)
- return 0;
-
/* Was this event for the thread we displaced? */
if (displaced->step_thread != event_thread)
return 0;
@@ -3577,7 +3475,7 @@ prepare_for_detach (void)

/* Is any thread of this process displaced stepping? If not,
there's nothing else to do. */
- if (displaced == NULL || displaced->step_thread == nullptr)
+ if (displaced->step_thread == nullptr)
return;

if (debug_infrun)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a701f0ca47f..1b243db057c 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -258,4 +258,48 @@ struct buf_displaced_step_closure : displaced_step_closure
gdb::byte_vector buf;
};

+/* Per-inferior displaced stepping state. */
+struct displaced_step_inferior_state
+{
+ displaced_step_inferior_state ()
+ {
+ reset ();
+ }
+
+ /* Put this object back in its original state. */
+ void reset ()
+ {
+ failed_before = 0;
+ step_thread = nullptr;
+ step_gdbarch = nullptr;
+ step_closure = nullptr;
+ step_original = 0;
+ step_copy = 0;
+ step_saved_copy.clear ();
+ }
+
+ /* True if preparing a displaced step ever failed. If so, we won't
+ try displaced stepping for this inferior again. */
+ int failed_before;
+
+ /* If this is not nullptr, this is the thread carrying out a
+ displaced single-step in process PID. This thread's state will
+ require fixing up once it has completed its step. */
+ thread_info *step_thread;
+
+ /* The architecture the thread had when we stepped it. */
+ gdbarch *step_gdbarch;
+
+ /* The closure provided gdbarch_displaced_step_copy_insn, to be used
+ for post-step cleanup. */
+ displaced_step_closure *step_closure;
+
+ /* The address of the original instruction, and the copy we
+ made. */
+ CORE_ADDR step_original, step_copy;
+
+ /* Saved contents of copy area. */
+ gdb::byte_vector step_saved_copy;
+};
+
#endif /* INFRUN_H */
--
2.19.1

[Philippe Waroquiers]

Post by Simon Marchi
At the same time, I changed step_saved_copy to be a gdb::byte_vector, so
it is automatically freed on destruction (which should plug the leak
reported here [1]).
[1] https://sourceware.org/ml/gdb-patches/2018-11/msg00202.html

Re-tested with your patch+valgrind, and the leak is effectively fixed.


The re-testing shown that the 'open_source_file' leak was re-introduced
(which I re-fixed as under the obvious rule) and some definitely leaked
blocks in linespec.c.

I will investigate ...

Philippe