* [PATCH] Place displaced step data directly in inferior structure @ 2018-11-23 18:25 Simon Marchi 2018-11-23 19:01 ` Pedro Alves 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi @ 2018-11-23 18:25 UTC (permalink / raw) To: gdb-patches; +Cc: dblaikie, 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 */ -- 2.19.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-11-23 18:25 [PATCH] Place displaced step data directly in inferior structure Simon Marchi @ 2018-11-23 19:01 ` Pedro Alves 2018-11-23 19:47 ` Simon Marchi 0 siblings, 1 reply; 7+ messages in thread From: Pedro Alves @ 2018-11-23 19:01 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: dblaikie On 11/23/2018 06:24 PM, Simon Marchi wrote: > 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. > > /* 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 ()) > { > - 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. > +/* 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-11-23 19:01 ` Pedro Alves @ 2018-11-23 19:47 ` Simon Marchi 2018-11-23 21:05 ` Simon Marchi 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi @ 2018-11-23 19:47 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: dblaikie, Philippe Waroquiers On 2018-11-23 14:01, Pedro Alves wrote: > 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) >> { >> - 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. > 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 <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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-11-23 19:47 ` Simon Marchi @ 2018-11-23 21:05 ` Simon Marchi 2018-11-24 13:22 ` Philippe Waroquiers 2018-12-30 12:59 ` Philippe Waroquiers 0 siblings, 2 replies; 7+ messages in thread From: Simon Marchi @ 2018-11-23 21:05 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: dblaikie, Philippe Waroquiers On 2018-11-23 2:47 p.m., Simon Marchi wrote: > 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): 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 <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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-11-23 21:05 ` Simon Marchi @ 2018-11-24 13:22 ` Philippe Waroquiers 2018-12-30 12:59 ` Philippe Waroquiers 1 sibling, 0 replies; 7+ messages in thread From: Philippe Waroquiers @ 2018-11-24 13:22 UTC (permalink / raw) To: Simon Marchi, Pedro Alves, gdb-patches; +Cc: dblaikie On Fri, 2018-11-23 at 21:05 +0000, Simon Marchi wrote: > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-11-23 21:05 ` Simon Marchi 2018-11-24 13:22 ` Philippe Waroquiers @ 2018-12-30 12:59 ` Philippe Waroquiers 2019-01-02 22:33 ` Simon Marchi 1 sibling, 1 reply; 7+ messages in thread From: Philippe Waroquiers @ 2018-12-30 12:59 UTC (permalink / raw) To: Simon Marchi, Pedro Alves, gdb-patches; +Cc: dblaikie Isn't/wasn't this ready to be pushed ? Thanks Philippe On Fri, 2018-11-23 at 21:05 +0000, Simon Marchi wrote: > On 2018-11-23 2:47 p.m., Simon Marchi wrote: > > 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): > > 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 <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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Place displaced step data directly in inferior structure 2018-12-30 12:59 ` Philippe Waroquiers @ 2019-01-02 22:33 ` Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2019-01-02 22:33 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: Simon Marchi, Pedro Alves, gdb-patches, dblaikie On 2018-12-30 07:59, Philippe Waroquiers wrote: > Isn't/wasn't this ready to be pushed ? > > Thanks > Philippe Looks like it fell between the cracks. Thanks for reminding me, this is now pushed. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-02 22:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-23 18:25 [PATCH] Place displaced step data directly in inferior structure Simon Marchi 2018-11-23 19:01 ` Pedro Alves 2018-11-23 19:47 ` Simon Marchi 2018-11-23 21:05 ` Simon Marchi 2018-11-24 13:22 ` Philippe Waroquiers 2018-12-30 12:59 ` Philippe Waroquiers 2019-01-02 22:33 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).