[PATCH] Place displaced step data directly in inferior structure

[Pedro Alves]

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