Re: [PATCH 1/2] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear

[Simon Marchi <simark at simark dot ca>]

On 2020-01-22 10:14 a.m., Simon Marchi wrote:
> displaced_step_inferior_state::reset and displaced_step_clear appear to
> have the same goal, but they don't do the same thing.
> displaced_step_inferior_state::reset clears more things than
> displaced_step_clear, but it misses free'ing the closure, which
> displaced_step_clear does.
> 
> This patch replaces displaced_step_clear's implementation with just a call to
> displaced_step_inferior_state::reset.  It then changes
> displaced_step_inferior_state::step_closure to be a unique_ptr, to indicate the
> fact that displaced_step_inferior_state owns the closure (and so that it is
> automatically freed when the field is reset).
> 
> It should be possible to get rid of displaced_step_clear entirely, but I'm not
> sure what the best way, give that it's used in scope exit macros.
> 
> The test gdb.base/step-over-syscall.exp caught a problem when doing this, which
> I consider to be a latent bug which my cleanup exposes.  In
> handle_inferior_event, in the TARGET_WAITKIND_FORKED case, if we displaced-step
> over a fork syscall, we make sure to restore the memory that we used as a
> displaced-stepping buffer in the child.  We do so using the
> displaced_step_inferior_state of the parent.  However, we do it after calling
> displaced_step_fixup for the parent, which clears the information in the
> parent's displaced_step_inferior_state.  It worked fine before, because
> displaced_step_clear didn't completely clear the displaced_step_inferior_state
> structure, so the required information (in this case the gdbarch) was
> still available after clearing.
> 
> I fixed it by making GDB restore the child's memory before calling the
> displaced_step_fixup on the parent.  This way, the data in the
> displaced_step_inferior_state structure is still valid when we use it for the
> child.  This is the error you would get in
> gdb.base/step-over-syscall.exp without this fix:
> 
>     /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:3911: internal-error: ULONGEST gdbarch_max_insn_length(gdbarch*): Assertion `gdbarch != NULL' failed.

If there's no objection, I would push these two patches next week.

Simon