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

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

[Simon Marchi]

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.

gdb/ChangeLog:

	* infrun.c (get_displaced_step_closure_by_addr): Adjust to
	std::unique_ptr.
	(displaced_step_clear): Just call displaced->reset ().
	(displaced_step_prepare_throw): Adjust to std::unique_ptr.
	(displaced_step_fixup): Likewise.
	(resume_1): Likewise.
	(handle_inferior_event): Restore child's memory before calling
	displaced_step_fixup on the parent.
	* infrun.h (displaced_step_inferior_state) <reset>: Adjust
	to std::unique_ptr.
	<step_closure>: Change type to std::unique_ptr.
---
 gdb/infrun.c | 36 ++++++++++++++++--------------------
 gdb/infrun.h |  4 ++--
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9c4a07daba97..1fee65730dbc 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1548,7 +1548,7 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr)
   /* If checking the mode of displaced instruction in copy area.  */
   if (displaced->step_thread != nullptr
       && displaced->step_copy == addr)
-    return displaced->step_closure;
+    return displaced->step_closure.get ();
 
   return NULL;
 }
@@ -1606,13 +1606,9 @@ use_displaced_stepping (struct thread_info *tp)
 
 /* Clean out any stray displaced stepping state.  */
 static void
-displaced_step_clear (struct displaced_step_inferior_state *displaced)
+displaced_step_clear (displaced_step_inferior_state *displaced)
 {
-  /* Indicate that there is no cleanup pending.  */
-  displaced->step_thread = nullptr;
-
-  delete displaced->step_closure;
-  displaced->step_closure = NULL;
+  displaced->reset ();
 }
 
 /* A cleanup that wraps displaced_step_clear.  */
@@ -1762,7 +1758,7 @@ displaced_step_prepare_throw (thread_info *tp)
      succeeds.  */
   displaced->step_thread = tp;
   displaced->step_gdbarch = gdbarch;
-  displaced->step_closure = closure;
+  displaced->step_closure.reset (closure);
   displaced->step_original = original;
   displaced->step_copy = copy;
 
@@ -1887,7 +1883,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
     {
       /* Fix up the resulting state.  */
       gdbarch_displaced_step_fixup (displaced->step_gdbarch,
-                                    displaced->step_closure,
+                                    displaced->step_closure.get (),
                                     displaced->step_original,
                                     displaced->step_copy,
                                     get_thread_regcache (displaced->step_thread));
@@ -2480,8 +2476,8 @@ resume_1 (enum gdb_signal sig)
 	  pc = regcache_read_pc (get_thread_regcache (tp));
 
 	  displaced = get_displaced_stepping_state (tp->inf);
-	  step = gdbarch_displaced_step_hw_singlestep (gdbarch,
-						       displaced->step_closure);
+	  step = gdbarch_displaced_step_hw_singlestep
+	    (gdbarch, displaced->step_closure.get ());
 	}
     }
 
@@ -5313,6 +5309,15 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	    struct regcache *child_regcache;
 	    CORE_ADDR parent_pc;
 
+	    if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
+	      {
+		struct displaced_step_inferior_state *displaced
+		  = get_displaced_stepping_state (parent_inf);
+
+		/* Restore scratch pad for child process.  */
+		displaced_step_restore (displaced, ecs->ws.value.related_pid);
+	      }
+
 	    /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
 	       indicating that the displaced stepping of syscall instruction
 	       has been done.  Perform cleanup for parent process here.  Note
@@ -5323,15 +5328,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	       that needs it.  */
 	    start_step_over ();
 
-	    if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
-	      {
-		struct displaced_step_inferior_state *displaced
-		  = get_displaced_stepping_state (parent_inf);
-
-		/* Restore scratch pad for child process.  */
-		displaced_step_restore (displaced, ecs->ws.value.related_pid);
-	      }
-
 	    /* Since the vfork/fork syscall instruction was executed in the scratchpad,
 	       the child's PC is also within the scratchpad.  Set the child's PC
 	       to the parent's PC value, which has already been fixed up.
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 8040b28f0172..c6329c844d9b 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -290,7 +290,7 @@ struct displaced_step_inferior_state
     failed_before = 0;
     step_thread = nullptr;
     step_gdbarch = nullptr;
-    step_closure = nullptr;
+    step_closure.reset ();
     step_original = 0;
     step_copy = 0;
     step_saved_copy.clear ();
@@ -310,7 +310,7 @@ struct displaced_step_inferior_state
 
   /* The closure provided gdbarch_displaced_step_copy_insn, to be used
      for post-step cleanup.  */
-  displaced_step_closure *step_closure;
+  std::unique_ptr<displaced_step_closure> step_closure;
 
   /* The address of the original instruction, and the copy we
      made.  */
-- 
2.25.0

[PATCH 2/2] gdb: make gdbarch_displaced_step_copy_insn return an std::unique_ptr

[Simon Marchi]

This callback dynamically allocates a specialized displaced_step_closure, and
gives the ownership of the object to its caller.  So I think it would make
sense for the callback to return an std::unique_ptr, this is what this patch
implements.

        * gdbarch.sh (displaced_step_copy_insn): Change return type to an
	std::unique_ptr.
        * gdbarch.c: Re-generate.
        * gdbarch.h: Re-generate.
        * infrun.c (displaced_step_prepare_throw): Adjust to std::unique_ptr
	change.
        * aarch64-tdep.c (aarch64_displaced_step_copy_insn): Change return
	type to std::unique_ptr.
        * aarch64-tdep.h (aarch64_displaced_step_copy_insn): Likewise.
        * amd64-tdep.c (amd64_displaced_step_copy_insn): Likewise.
        * amd64-tdep.h (amd64_displaced_step_copy_insn): Likewise.
        * arm-linux-tdep.c (arm_linux_displaced_step_copy_insn): Likewise.
        * i386-linux-tdep.c (i386_linux_displaced_step_copy_insn): Likewise.
        * i386-tdep.c (i386_displaced_step_copy_insn): Likewise.
        * i386-tdep.h (i386_displaced_step_copy_insn): Likewise.
        * rs6000-tdep.c (ppc_displaced_step_copy_insn): Likewise.
        * s390-tdep.c (s390_displaced_step_copy_insn): Likewise.
---
 gdb/aarch64-tdep.c    |  4 ++--
 gdb/aarch64-tdep.h    |  2 +-
 gdb/amd64-tdep.c      |  8 ++++----
 gdb/amd64-tdep.h      |  2 +-
 gdb/arm-linux-tdep.c  | 11 ++++++-----
 gdb/gdbarch.c         |  2 +-
 gdb/gdbarch.h         |  4 ++--
 gdb/gdbarch.sh        |  2 +-
 gdb/i386-linux-tdep.c |  6 +++---
 gdb/i386-tdep.c       |  5 +++--
 gdb/i386-tdep.h       |  2 +-
 gdb/infrun.c          |  8 +++-----
 gdb/rs6000-tdep.c     |  4 ++--
 gdb/s390-tdep.c       |  4 ++--
 14 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 1631178f8c26..aa768004bf78 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2966,7 +2966,7 @@ static const struct aarch64_insn_visitor visitor =
 
 /* Implement the "displaced_step_copy_insn" gdbarch method.  */
 
-struct displaced_step_closure *
+std::unique_ptr<displaced_step_closure>
 aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
 				  CORE_ADDR from, CORE_ADDR to,
 				  struct regcache *regs)
@@ -3020,7 +3020,7 @@ aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
       dsc = NULL;
     }
 
-  return dsc.release ();
+  return dsc;
 }
 
 /* Implement the "displaced_step_fixup" gdbarch method.  */
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 732f78b5ba50..fc397967cd8e 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -106,7 +106,7 @@ const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p);
 extern int aarch64_process_record (struct gdbarch *gdbarch,
                                struct regcache *regcache, CORE_ADDR addr);
 
-struct displaced_step_closure *
+struct std::unique_ptr<displaced_step_closure>
   aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
 				    CORE_ADDR from, CORE_ADDR to,
 				    struct regcache *regs);
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index f5ec40f37e8c..35ddfbaa32df 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1465,7 +1465,7 @@ fixup_displaced_copy (struct gdbarch *gdbarch,
     }
 }
 
-struct displaced_step_closure *
+std::unique_ptr<displaced_step_closure>
 amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
 				CORE_ADDR from, CORE_ADDR to,
 				struct regcache *regs)
@@ -1474,8 +1474,8 @@ amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
   /* Extra space for sentinels so fixup_{riprel,displaced_copy} don't have to
      continually watch for running off the end of the buffer.  */
   int fixup_sentinel_space = len;
-  amd64_displaced_step_closure *dsc
-    = new amd64_displaced_step_closure (len + fixup_sentinel_space);
+  std::unique_ptr<amd64_displaced_step_closure> dsc
+    (new amd64_displaced_step_closure (len + fixup_sentinel_space));
   gdb_byte *buf = &dsc->insn_buf[0];
   struct amd64_insn *details = &dsc->insn_details;
 
@@ -1500,7 +1500,7 @@ amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   /* Modify the insn to cope with the address where it will be executed from.
      In particular, handle any rip-relative addressing.	 */
-  fixup_displaced_copy (gdbarch, dsc, from, to, regs);
+  fixup_displaced_copy (gdbarch, dsc.get (), from, to, regs);
 
   write_memory (to, buf, len);
 
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 4c6d13222216..33ef0c3cea7e 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -87,7 +87,7 @@ enum amd64_regnum
 
 #define AMD64_NUM_REGS		(AMD64_GSBASE_REGNUM + 1)
 
-extern struct displaced_step_closure *amd64_displaced_step_copy_insn
+extern std::unique_ptr<displaced_step_closure> amd64_displaced_step_copy_insn
   (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to,
    struct regcache *regs);
 extern void amd64_displaced_step_fixup (struct gdbarch *gdbarch,
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 363e67161c84..b3ae04fb431f 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -1103,12 +1103,13 @@ arm_catch_kernel_helper_return (struct gdbarch *gdbarch, CORE_ADDR from,
    the program has stepped into a Linux kernel helper routine (which must be
    handled as a special case).  */
 
-static struct displaced_step_closure *
+static std::unique_ptr<displaced_step_closure>
 arm_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
 				    CORE_ADDR from, CORE_ADDR to,
 				    struct regcache *regs)
 {
-  arm_displaced_step_closure *dsc = new arm_displaced_step_closure;
+  std::unique_ptr<arm_displaced_step_closure> dsc
+    (new arm_displaced_step_closure);
 
   /* Detect when we enter an (inaccessible by GDB) Linux kernel helper, and
      stop at the return location.  */
@@ -1118,17 +1119,17 @@ arm_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
         fprintf_unfiltered (gdb_stdlog, "displaced: detected kernel helper "
 			    "at %.8lx\n", (unsigned long) from);
 
-      arm_catch_kernel_helper_return (gdbarch, from, to, regs, dsc);
+      arm_catch_kernel_helper_return (gdbarch, from, to, regs, dsc.get ());
     }
   else
     {
       /* Override the default handling of SVC instructions.  */
       dsc->u.svc.copy_svc_os = arm_linux_copy_svc;
 
-      arm_process_displaced_insn (gdbarch, from, to, regs, dsc);
+      arm_process_displaced_insn (gdbarch, from, to, regs, dsc.get ());
     }
 
-  arm_displaced_init_closure (gdbarch, from, to, dsc);
+  arm_displaced_init_closure (gdbarch, from, to, dsc.get ());
 
   return dsc;
 }
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index cc8569f5c959..011c2d3306e4 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -3930,7 +3930,7 @@ gdbarch_displaced_step_copy_insn_p (struct gdbarch *gdbarch)
   return gdbarch->displaced_step_copy_insn != NULL;
 }
 
-struct displaced_step_closure *
+std::unique_ptr<displaced_step_closure>
 gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 9f32ac23aba6..de031cc8c16d 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1036,8 +1036,8 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
 
 extern int gdbarch_displaced_step_copy_insn_p (struct gdbarch *gdbarch);
 
-typedef struct displaced_step_closure * (gdbarch_displaced_step_copy_insn_ftype) (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs);
-extern struct displaced_step_closure * gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs);
+typedef std::unique_ptr<displaced_step_closure> (gdbarch_displaced_step_copy_insn_ftype) (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs);
+extern std::unique_ptr<displaced_step_closure> gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs);
 extern void set_gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, gdbarch_displaced_step_copy_insn_ftype *displaced_step_copy_insn);
 
 /* Return true if GDB should use hardware single-stepping to execute
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 58f6e1ebdca1..9357636a1f5f 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -816,7 +816,7 @@ V;ULONGEST;max_insn_length;;;0;0
 # If the instruction cannot execute out of line, return NULL.  The
 # core falls back to stepping past the instruction in-line instead in
 # that case.
-M;struct displaced_step_closure *;displaced_step_copy_insn;CORE_ADDR from, CORE_ADDR to, struct regcache *regs;from, to, regs
+M;std::unique_ptr<displaced_step_closure>;displaced_step_copy_insn;CORE_ADDR from, CORE_ADDR to, struct regcache *regs;from, to, regs
 
 # Return true if GDB should use hardware single-stepping to execute
 # the displaced instruction identified by CLOSURE.  If false,
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 6f702b59e7f8..537f8c46876a 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -797,12 +797,12 @@ i386_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
    which does not seem worth it.  The same effect is achieved by patching that
    'nop' instruction there instead.  */
 
-static struct displaced_step_closure *
+static std::unique_ptr<displaced_step_closure>
 i386_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
 				     CORE_ADDR from, CORE_ADDR to,
 				     struct regcache *regs)
 {
-  displaced_step_closure *closure_
+  std::unique_ptr<displaced_step_closure> closure_
     =  i386_displaced_step_copy_insn (gdbarch, from, to, regs);
 
   if (i386_linux_get_syscall_number_from_regcache (regs) != -1)
@@ -810,7 +810,7 @@ i386_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
       /* The closure returned by i386_displaced_step_copy_insn is simply a
 	 buffer with a copy of the instruction. */
       i386_displaced_step_closure *closure
-	= (i386_displaced_step_closure *) closure_;
+	= (i386_displaced_step_closure *) closure_.get ();
 
       /* Fake nop.  */
       closure->buf[0] = 0x90;
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index f120438081de..ff7f4baa4496 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -798,13 +798,14 @@ i386_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
 
 /* Some kernels may run one past a syscall insn, so we have to cope.  */
 
-struct displaced_step_closure *
+std::unique_ptr<displaced_step_closure>
 i386_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			       CORE_ADDR from, CORE_ADDR to,
 			       struct regcache *regs)
 {
   size_t len = gdbarch_max_insn_length (gdbarch);
-  i386_displaced_step_closure *closure = new i386_displaced_step_closure (len);
+  std::unique_ptr<i386_displaced_step_closure> closure
+    (new i386_displaced_step_closure (len));
   gdb_byte *buf = closure->buf.data ();
 
   read_memory (from, buf, len);
diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
index 14bab37205f8..41faf515868d 100644
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -428,7 +428,7 @@ extern void
 
 typedef buf_displaced_step_closure i386_displaced_step_closure;
 
-extern struct displaced_step_closure *i386_displaced_step_copy_insn
+extern std::unique_ptr<displaced_step_closure> i386_displaced_step_copy_insn
   (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to,
    struct regcache *regs);
 extern void i386_displaced_step_fixup (struct gdbarch *gdbarch,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1fee65730dbc..ef52c3e2faff 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1652,7 +1652,6 @@ displaced_step_prepare_throw (thread_info *tp)
   const address_space *aspace = regcache->aspace ();
   CORE_ADDR original, copy;
   ULONGEST len;
-  struct displaced_step_closure *closure;
   int status;
 
   /* We should never reach this function if the architecture does not
@@ -1744,9 +1743,9 @@ displaced_step_prepare_throw (thread_info *tp)
 				 len);
     };
 
-  closure = gdbarch_displaced_step_copy_insn (gdbarch,
-					      original, copy, regcache);
-  if (closure == NULL)
+  displaced->step_closure
+    = gdbarch_displaced_step_copy_insn (gdbarch, original, copy, regcache);
+  if (displaced->step_closure == NULL)
     {
       /* The architecture doesn't know how or want to displaced step
 	 this instruction or instruction sequence.  Fallback to
@@ -1758,7 +1757,6 @@ displaced_step_prepare_throw (thread_info *tp)
      succeeds.  */
   displaced->step_thread = tp;
   displaced->step_gdbarch = gdbarch;
-  displaced->step_closure.reset (closure);
   displaced->step_original = original;
   displaced->step_copy = copy;
 
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index ccffc39508a6..58f828f41dc6 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -855,7 +855,7 @@ typedef buf_displaced_step_closure ppc_displaced_step_closure;
 
 /* We can't displaced step atomic sequences.  */
 
-static struct displaced_step_closure *
+static std::unique_ptr<displaced_step_closure>
 ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			      CORE_ADDR from, CORE_ADDR to,
 			      struct regcache *regs)
@@ -894,7 +894,7 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return closure.release ();
+  return closure;
 }
 
 /* Fix up the state of registers and memory after having single-stepped
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index e01505549e67..780752338efb 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -425,7 +425,7 @@ typedef buf_displaced_step_closure s390_displaced_step_closure;
 
 /* Implementation of gdbarch_displaced_step_copy_insn.  */
 
-static struct displaced_step_closure *
+static std::unique_ptr<displaced_step_closure>
 s390_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			       CORE_ADDR from, CORE_ADDR to,
 			       struct regcache *regs)
@@ -477,7 +477,7 @@ s390_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return closure.release ();
+  return closure;
 }
 
 /* Fix up the state of registers and memory after having single-stepped
-- 
2.25.0

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

[Simon Marchi]

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

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

[Pedro Alves]

On 1/22/20 3:14 PM, 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 reason it needs to be wrapped in a cleanup instead of say SCOPE_EXIT,
is that it needs to be discardable with the "cleanup.release ();" call.
I'm not sure there's a better way.

Renaming displaced_step_clear to displaced_step_reset and adding a comment
may result in clearer code, though.

> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index 8040b28f0172..c6329c844d9b 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -290,7 +290,7 @@ struct displaced_step_inferior_state
>      failed_before = 0;
>      step_thread = nullptr;
>      step_gdbarch = nullptr;
> -    step_closure = nullptr;
> +    step_closure.reset ();

I see people sometimes doing this change and I'm curious.
Is it for clarity?

Anyway, this LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: make gdbarch_displaced_step_copy_insn return an std::unique_ptr

[Pedro Alves]

On 1/22/20 3:14 PM, Simon Marchi wrote:
> This callback dynamically allocates a specialized displaced_step_closure, and
> gives the ownership of the object to its caller.  So I think it would make
> sense for the callback to return an std::unique_ptr, this is what this patch
> implements.
> 
>         * gdbarch.sh (displaced_step_copy_insn): Change return type to an
> 	std::unique_ptr.
>         * gdbarch.c: Re-generate.
>         * gdbarch.h: Re-generate.
>         * infrun.c (displaced_step_prepare_throw): Adjust to std::unique_ptr
> 	change.
>         * aarch64-tdep.c (aarch64_displaced_step_copy_insn): Change return
> 	type to std::unique_ptr.
>         * aarch64-tdep.h (aarch64_displaced_step_copy_insn): Likewise.
>         * amd64-tdep.c (amd64_displaced_step_copy_insn): Likewise.
>         * amd64-tdep.h (amd64_displaced_step_copy_insn): Likewise.
>         * arm-linux-tdep.c (arm_linux_displaced_step_copy_insn): Likewise.
>         * i386-linux-tdep.c (i386_linux_displaced_step_copy_insn): Likewise.
>         * i386-tdep.c (i386_displaced_step_copy_insn): Likewise.
>         * i386-tdep.h (i386_displaced_step_copy_insn): Likewise.
>         * rs6000-tdep.c (ppc_displaced_step_copy_insn): Likewise.
>         * s390-tdep.c (s390_displaced_step_copy_insn): Likewise.

Looks fine.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: make gdbarch_displaced_step_copy_insn return an std::unique_ptr

[Pedro Alves]

BTW, when I see a bunch of std::unique_ptr<footype>
I wonder whether a footype_up typedef wouldn't make things
read a bit clearer.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2020-02-14 2:39 p.m., Pedro Alves wrote:
> On 1/22/20 3:14 PM, 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 reason it needs to be wrapped in a cleanup instead of say SCOPE_EXIT,
> is that it needs to be discardable with the "cleanup.release ();" call.
> I'm not sure there's a better way.

Ok, makes sense.

> Renaming displaced_step_clear to displaced_step_reset and adding a comment
> may result in clearer code, though.

I did this.

> 
>> diff --git a/gdb/infrun.h b/gdb/infrun.h
>> index 8040b28f0172..c6329c844d9b 100644
>> --- a/gdb/infrun.h
>> +++ b/gdb/infrun.h
>> @@ -290,7 +290,7 @@ struct displaced_step_inferior_state
>>      failed_before = 0;
>>      step_thread = nullptr;
>>      step_gdbarch = nullptr;
>> -    step_closure = nullptr;
>> +    step_closure.reset ();
> 
> I see people sometimes doing this change and I'm curious.
> Is it for clarity?

I never really thought about it, I thought it was just the typical
way of clearing a unique_ptr.

But now that I think about it, I prefer calling reset() over just
assigning to nullptr, because it kind of highlights the fact that this
is not a simple pointer.  So it makes it more obvious to me that this
line will free whatever the pointer points to.  But otherwise I don't
mind.

> 
> Anyway, this LGTM.

I'll push the updated patch below.


From aecc33e5f041af66a3a7eae0c28455b9c9a93b12 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 22 Jan 2020 10:14:09 -0500
Subject: [PATCH] gdb: cleanup of
 displaced_step_inferior_state::reset/displaced_step_clear

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

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

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

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

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

gdb/ChangeLog:

	* infrun.c (get_displaced_step_closure_by_addr): Adjust to
	std::unique_ptr.
	(displaced_step_clear): Rename to...
	(displaced_step_reset): ... this.  Just call displaced->reset ().
	(displaced_step_clear_cleanup): Rename to...
	(displaced_step_reset_cleanup): ... this.
	(displaced_step_prepare_throw): Adjust to std::unique_ptr.
	(displaced_step_fixup): Likewise.
	(resume_1): Likewise.
	(handle_inferior_event): Restore child's memory before calling
	displaced_step_fixup on the parent.
	* infrun.h (displaced_step_inferior_state) <reset>: Adjust
	to std::unique_ptr.
	<step_closure>: Change type to std::unique_ptr.
---
 gdb/infrun.c | 52 +++++++++++++++++++++++++---------------------------
 gdb/infrun.h |  4 ++--
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3e846f8e6802..e3e4bdb9b847 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1540,7 +1540,7 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr)
   /* If checking the mode of displaced instruction in copy area.  */
   if (displaced->step_thread != nullptr
       && displaced->step_copy == addr)
-    return displaced->step_closure;
+    return displaced->step_closure.get ();

   return NULL;
 }
@@ -1596,20 +1596,18 @@ use_displaced_stepping (struct thread_info *tp)
 	  && !displaced_state->failed_before);
 }

-/* Clean out any stray displaced stepping state.  */
+/* Simple function wrapper around displaced_step_inferior_state::reset.  */
+
 static void
-displaced_step_clear (struct displaced_step_inferior_state *displaced)
+displaced_step_reset (displaced_step_inferior_state *displaced)
 {
-  /* Indicate that there is no cleanup pending.  */
-  displaced->step_thread = nullptr;
-
-  delete displaced->step_closure;
-  displaced->step_closure = NULL;
+  displaced->reset ();
 }

-/* A cleanup that wraps displaced_step_clear.  */
-using displaced_step_clear_cleanup
-  = FORWARD_SCOPE_EXIT (displaced_step_clear);
+/* A cleanup that wraps displaced_step_reset.  We use this instead of, say,
+   SCOPE_EXIT, because it needs to be discardable with "cleanup.release ()".  */
+
+using displaced_step_reset_cleanup = FORWARD_SCOPE_EXIT (displaced_step_reset);

 /* Dump LEN bytes at BUF in hex to FILE, followed by a newline.  */
 void
@@ -1691,7 +1689,7 @@ displaced_step_prepare_throw (thread_info *tp)
 			    target_pid_to_str (tp->ptid).c_str ());
     }

-  displaced_step_clear (displaced);
+  displaced_step_reset (displaced);

   scoped_restore_current_thread restore_thread;

@@ -1754,12 +1752,12 @@ displaced_step_prepare_throw (thread_info *tp)
      succeeds.  */
   displaced->step_thread = tp;
   displaced->step_gdbarch = gdbarch;
-  displaced->step_closure = closure;
+  displaced->step_closure.reset (closure);
   displaced->step_original = original;
   displaced->step_copy = copy;

   {
-    displaced_step_clear_cleanup cleanup (displaced);
+    displaced_step_reset_cleanup cleanup (displaced);

     /* Resume execution at the copy.  */
     regcache_write_pc (regcache, copy);
@@ -1862,7 +1860,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
   if (displaced->step_thread != event_thread)
     return 0;

-  displaced_step_clear_cleanup cleanup (displaced);
+  displaced_step_reset_cleanup cleanup (displaced);

   displaced_step_restore (displaced, displaced->step_thread->ptid);

@@ -1879,7 +1877,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
     {
       /* Fix up the resulting state.  */
       gdbarch_displaced_step_fixup (displaced->step_gdbarch,
-                                    displaced->step_closure,
+                                    displaced->step_closure.get (),
                                     displaced->step_original,
                                     displaced->step_copy,
                                     get_thread_regcache (displaced->step_thread));
@@ -2472,8 +2470,8 @@ resume_1 (enum gdb_signal sig)
 	  pc = regcache_read_pc (get_thread_regcache (tp));

 	  displaced = get_displaced_stepping_state (tp->inf);
-	  step = gdbarch_displaced_step_hw_singlestep (gdbarch,
-						       displaced->step_closure);
+	  step = gdbarch_displaced_step_hw_singlestep
+	    (gdbarch, displaced->step_closure.get ());
 	}
     }

@@ -5305,6 +5303,15 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	    struct regcache *child_regcache;
 	    CORE_ADDR parent_pc;

+	    if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
+	      {
+		struct displaced_step_inferior_state *displaced
+		  = get_displaced_stepping_state (parent_inf);
+
+		/* Restore scratch pad for child process.  */
+		displaced_step_restore (displaced, ecs->ws.value.related_pid);
+	      }
+
 	    /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
 	       indicating that the displaced stepping of syscall instruction
 	       has been done.  Perform cleanup for parent process here.  Note
@@ -5315,15 +5322,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	       that needs it.  */
 	    start_step_over ();

-	    if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
-	      {
-		struct displaced_step_inferior_state *displaced
-		  = get_displaced_stepping_state (parent_inf);
-
-		/* Restore scratch pad for child process.  */
-		displaced_step_restore (displaced, ecs->ws.value.related_pid);
-	      }
-
 	    /* Since the vfork/fork syscall instruction was executed in the scratchpad,
 	       the child's PC is also within the scratchpad.  Set the child's PC
 	       to the parent's PC value, which has already been fixed up.
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 8040b28f0172..c6329c844d9b 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -290,7 +290,7 @@ struct displaced_step_inferior_state
     failed_before = 0;
     step_thread = nullptr;
     step_gdbarch = nullptr;
-    step_closure = nullptr;
+    step_closure.reset ();
     step_original = 0;
     step_copy = 0;
     step_saved_copy.clear ();
@@ -310,7 +310,7 @@ struct displaced_step_inferior_state

   /* The closure provided gdbarch_displaced_step_copy_insn, to be used
      for post-step cleanup.  */
-  displaced_step_closure *step_closure;
+  std::unique_ptr<displaced_step_closure> step_closure;

   /* The address of the original instruction, and the copy we
      made.  */
-- 
2.25.0

Re: [PATCH 2/2] gdb: make gdbarch_displaced_step_copy_insn return an std::unique_ptr

[Simon Marchi]

On 2020-02-14 2:48 p.m., Pedro Alves wrote:
> BTW, when I see a bunch of std::unique_ptr<footype>
> I wonder whether a footype_up typedef wouldn't make things
> read a bit clearer.

Personally I don't mind the explicit std::unique_ptr<footype>,
unless we're starting to use containers of that type.  I then prefer:

  std::vector<footype_up>

over

  std::vector<std::unique_ptr<footype>>

But I also don't mind using a typedef either, even without using containers,
so I'll do that change as a separate patch on top.

Thanks for the review!

Simon