[PATCH 3/6] gdb/record: c++ify internal structures of record-full.c

[Guinevere Larsen <guinevere@redhat.com>]

This commit adds a constructor, destructor, and some methods to the
structures record_full_entry, record_full_reg_entry and
record_full_mem_entry. This is a move to disentangle the internal
representation of the data and how record-full manipulates it for
replaying.

Along with this change, record_full_entry is changed to use an
std::variant, since it was basically doing that already, but now we have
the stdlibc++ error checking to make sure we're only accessing elements
we're allowed to.
---
 gdb/record-full.c | 517 +++++++++++++++++++++++++---------------------
 1 file changed, 279 insertions(+), 238 deletions(-)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 95776679f21..f3737fdbc1f 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -48,6 +48,7 @@
 #include "cli/cli-style.h"
 
 #include <signal.h>
+#include <variant>
 
 /* This module implements "target record-full", also known as "process
    record and replay".  This target sits on top of a "normal" target
@@ -90,12 +91,87 @@ struct record_full_mem_entry
   int len;
   /* Set this flag if target memory for this entry
      can no longer be accessed.  */
-  int mem_entry_not_accessible;
+  bool mem_entry_not_accessible;
   union
   {
     gdb_byte *ptr;
     gdb_byte buf[sizeof (gdb_byte *)];
   } u;
+
+  record_full_mem_entry () : addr (0), len (0) { }
+
+  record_full_mem_entry (CORE_ADDR mem_addr, int mem_len)
+  {
+    addr = mem_addr;
+    len = mem_len;
+    if (len > sizeof (u.buf))
+      u.ptr = new gdb_byte[len];
+    mem_entry_not_accessible = false;
+  }
+
+  gdb_byte *get_loc ()
+  {
+    if (len > sizeof (u.buf))
+      return u.ptr;
+    else
+      return u.buf;
+  }
+
+  bool execute (struct regcache *regcache,
+		struct gdbarch *gdbarch)
+  {
+    /* Nothing to do if the memory is flagged not_accessible.  */
+    if (!mem_entry_not_accessible)
+      {
+	gdb::byte_vector buf (len);
+
+	  if (record_debug > 1)
+	    gdb_printf (gdb_stdlog,
+			"Process record: record_full_mem %s to "
+			"inferior addr = %s len = %d.\n",
+			host_address_to_string (this),
+			paddress (gdbarch, addr),
+			len);
+
+	  if (record_read_memory (gdbarch,
+				  addr, buf.data (),
+				  len))
+	    mem_entry_not_accessible = 1;
+	  else
+	    {
+	      if (target_write_memory (addr,
+				       get_loc (),
+				       len))
+		{
+		  mem_entry_not_accessible = 1;
+		  if (record_debug)
+		    warning (_("Process record: error writing memory at "
+			       "addr = %s len = %d."),
+			     paddress (gdbarch, addr),
+			     len);
+		}
+	      else
+		{
+		  memcpy (get_loc (), buf.data (),
+			  len);
+
+		  /* We've changed memory --- check if a hardware
+		     watchpoint should trap.  Note that this
+		     presently assumes the target beneath supports
+		     continuable watchpoints.  On non-continuable
+		     watchpoints target, we'll want to check this
+		     _before_ actually doing the memory change, and
+		     not doing the change at all if the watchpoint
+		     traps.  */
+		  if (hardware_watchpoint_inserted_in_range
+		      (current_inferior ()->aspace.get (),
+		       addr, len))
+		    return true;
+		}
+	    }
+	}
+    return false;
+    }
 };
 
 struct record_full_reg_entry
@@ -107,6 +183,42 @@ struct record_full_reg_entry
     gdb_byte *ptr;
     gdb_byte buf[2 * sizeof (gdb_byte *)];
   } u;
+
+  record_full_reg_entry () : num (0), len (0) { }
+
+  record_full_reg_entry (gdbarch *gdbarch, int regnum)
+  {
+    num = regnum;
+    len = register_size (gdbarch, regnum);
+    if (len > sizeof (u.buf))
+      u.ptr = new gdb_byte[len];
+  }
+
+  gdb_byte *get_loc ()
+  {
+    if (len > sizeof (u.buf))
+      return u.ptr;
+    else
+      return u.buf;
+  }
+
+  bool execute (struct regcache *regcache,
+		struct gdbarch *gdbarch)
+  {
+    gdb::byte_vector buf (len);
+
+    if (record_debug > 1)
+      gdb_printf (gdb_stdlog,
+		  "Process record: record_full_reg %s to "
+		  "inferior num = %d.\n",
+		  host_address_to_string (this),
+		  num);
+
+    regcache->cooked_read (num, buf.data ());
+    regcache->cooked_write (num, get_loc ());
+    memcpy (get_loc (), buf.data (), len);
+    return false;
+  }
 };
 
 enum record_full_type
@@ -115,16 +227,82 @@ enum record_full_type
   record_full_mem
 };
 
-struct record_full_entry
+class record_full_entry
 {
-  enum record_full_type type;
-  union
+  std::variant<record_full_reg_entry, record_full_mem_entry> entry;
+
+public:
+  record_full_entry () : entry (record_full_reg_entry ()) {}
+
+  /* Constructor for a register entry.  Type is here to make it
+     easier to recognize it in the constructor calls, it isn't
+     actually important.  */
+  record_full_entry (record_full_type reg_type, gdbarch *gdbarch,
+		     int regnum)
+  : entry(record_full_reg_entry (gdbarch, regnum))
   {
-    /* reg */
-    struct record_full_reg_entry reg;
-    /* mem */
-    struct record_full_mem_entry mem;
-  } u;
+    gdb_assert (reg_type == record_full_reg);
+  }
+
+  record_full_entry (record_full_type mem_type, CORE_ADDR addr, int len)
+  : entry(record_full_mem_entry (addr, len))
+  {
+    gdb_assert (mem_type == record_full_mem);
+  }
+
+  record_full_reg_entry& reg ()
+  {
+    gdb_assert (type () == record_full_reg);
+    return std::get<record_full_reg_entry> (entry);
+  }
+
+  record_full_mem_entry& mem ()
+  {
+    gdb_assert (type () == record_full_mem);
+    return std::get<record_full_mem_entry> (entry);
+  }
+
+  record_full_type type ()
+  {
+    switch (entry.index ())
+    {
+    case 0:
+      return record_full_reg;
+    case 1:
+      return record_full_mem;
+    }
+    gdb_assert_not_reached ("Impossible variant index");
+  }
+
+  /* Get the pointer to the data stored by this entry.  */
+  gdb_byte *get_loc ()
+  {
+    switch (type ())
+    {
+    case record_full_reg:
+      return reg ().get_loc ();
+    case record_full_mem:
+      return mem ().get_loc ();
+    }
+    gdb_assert_not_reached ("Impossible entry type");
+  }
+
+  /* Execute this entry, swapping the appropriate values from memory or
+     register and the recorded ones.  Returns TRUE if the execution was
+     stopped by a watchpoint.  */
+
+  bool execute (struct regcache *regcache,
+		struct gdbarch *gdbarch)
+  {
+    switch (type ())
+      {
+      case record_full_reg:
+	return reg ().execute (regcache, gdbarch);
+      case record_full_mem:
+	return mem ().execute (regcache, gdbarch);
+      }
+    return false;
+  }
 };
 
 /* This is the main structure that comprises the execution log.
@@ -381,61 +559,30 @@ static struct cmd_list_element *record_full_cmdlist;
 static void record_full_goto_insn (size_t target_insn,
 				   enum exec_direction_kind dir);
 
-/* Initialization and cleanup functions for record_full_reg and
-   record_full_mem entries.  */
-
-/* Init a record_full_reg record entry.  */
-
-static inline struct record_full_entry
-record_full_reg_init (struct regcache *regcache, int regnum)
-{
-  struct record_full_entry rec;
-  struct gdbarch *gdbarch = regcache->arch ();
-
-  rec.type = record_full_reg;
-  rec.u.reg.num = regnum;
-  rec.u.reg.len = register_size (gdbarch, regnum);
-  if (rec.u.reg.len > sizeof (rec.u.reg.u.buf))
-    rec.u.reg.u.ptr = (gdb_byte *) xmalloc (rec.u.reg.len);
-
-  return rec;
-}
-
-/* Cleanup a record_full_reg record entry.  */
+/* Cleanup a record_full_reg_entry.  This would ideally be a
+   destructor for the classes, but I kept running into issues with
+   double free, so this is left as a future improvement.  */
 
 static inline void
 record_full_reg_cleanup (struct record_full_entry rec)
 {
-  gdb_assert (rec.type == record_full_reg);
-  if (rec.u.reg.len > sizeof (rec.u.reg.u.buf))
-    xfree (rec.u.reg.u.ptr);
+  gdb_assert (rec.type () == record_full_reg);
+  auto reg = rec.reg ();
+  if (reg.len > sizeof (reg.u.buf))
+    delete reg.u.ptr;
 }
 
-/* Init a record_full_mem record entry.  */
-
-static inline struct record_full_entry
-record_full_mem_init (CORE_ADDR addr, int len)
-{
-  struct record_full_entry rec;
-
-  rec.type = record_full_mem;
-  rec.u.mem.addr = addr;
-  rec.u.mem.len = len;
-  if (rec.u.mem.len > sizeof (rec.u.mem.u.buf))
-    rec.u.mem.u.ptr = (gdb_byte *) xmalloc (len);
-  rec.u.mem.mem_entry_not_accessible = 0;
-
-  return rec;
-}
-
-/* Cleanup a record_full_mem record entry.  */
+/* Cleanup a record_full_mem_entry.  This would ideally be a
+   destructor for the classes, but I kept running into issues with
+   double free, so this is left as a future improvement.  */
 
 static inline void
 record_full_mem_cleanup (struct record_full_entry rec)
 {
-  gdb_assert (rec.type == record_full_mem);
-  if (rec.u.mem.len > sizeof (rec.u.mem.u.buf))
-    xfree (rec.u.mem.u.ptr);
+  gdb_assert (rec.type () == record_full_mem);
+  auto mem = rec.mem ();
+  if (mem.len > sizeof (mem.u.buf))
+    delete mem.u.ptr;
 }
 
 /* Free one record entry, any type.
@@ -444,8 +591,8 @@ record_full_mem_cleanup (struct record_full_entry rec)
 static inline void
 record_full_entry_cleanup (struct record_full_entry rec)
 {
-
-  switch (rec.type) {
+  switch (rec.type ())
+  {
   case record_full_reg:
     record_full_reg_cleanup (rec);
     break;
@@ -518,33 +665,12 @@ record_full_arch_list_add (struct record_full_entry &rec)
   record_full_incomplete_instruction.effects.push_back (rec);
 }
 
-/* Return the value storage location of a record entry.  */
-static inline gdb_byte *
-record_full_get_loc (struct record_full_entry *rec)
-{
-  switch (rec->type) {
-  case record_full_mem:
-    if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
-      return rec->u.mem.u.ptr;
-    else
-      return rec->u.mem.u.buf;
-  case record_full_reg:
-    if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
-      return rec->u.reg.u.ptr;
-    else
-      return rec->u.reg.u.buf;
-  default:
-    gdb_assert_not_reached ("unexpected record_full_entry type");
-    return NULL;
-  }
-}
-
 /* Record the value of a register NUM to record_full_arch_list.  */
 
 int
 record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
 {
-  struct record_full_entry rec;
+  struct record_full_entry rec (record_full_reg, regcache->arch (), regnum);
 
   if (record_debug > 1)
     gdb_printf (gdb_stdlog,
@@ -552,9 +678,7 @@ record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
 		"record list.\n",
 		regnum);
 
-  rec = record_full_reg_init (regcache, regnum);
-
-  regcache->cooked_read (regnum, record_full_get_loc (&rec));
+  regcache->cooked_read (regnum, rec.get_loc ());
 
   record_full_arch_list_add (rec);
 
@@ -567,7 +691,7 @@ record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
 int
 record_full_arch_list_add_mem (CORE_ADDR addr, int len)
 {
-  struct record_full_entry rec;
+  struct record_full_entry rec (record_full_mem, addr, len);
 
   if (record_debug > 1)
     gdb_printf (gdb_stdlog,
@@ -578,10 +702,8 @@ record_full_arch_list_add_mem (CORE_ADDR addr, int len)
   if (!addr)	/* FIXME: Why?  Some arch must permit it...  */
     return 0;
 
-  rec = record_full_mem_init (addr, len);
-
   if (record_read_memory (current_inferior ()->arch (), addr,
-			  record_full_get_loc (&rec), len))
+			  rec.get_loc (), len))
     {
       record_full_mem_cleanup (rec);
       return -1;
@@ -713,98 +835,14 @@ record_full_gdb_operation_disable_set (void)
 static enum target_stop_reason record_full_stop_reason
   = TARGET_STOPPED_BY_NO_REASON;
 
-/* Execute one instruction from the record log.  Each instruction in
-   the log will be represented by an arbitrary sequence of register
-   entries and memory entries, followed by an 'end' entry.  */
-
-static inline void
-record_full_exec_entry (struct regcache *regcache,
-			struct gdbarch *gdbarch,
-			struct record_full_entry *entry)
-{
-  switch (entry->type)
-    {
-    case record_full_reg: /* reg */
-      {
-	gdb::byte_vector reg (entry->u.reg.len);
-
-	if (record_debug > 1)
-	  gdb_printf (gdb_stdlog,
-		      "Process record: record_full_reg %s to "
-		      "inferior num = %d.\n",
-		      host_address_to_string (entry),
-		      entry->u.reg.num);
-
-	regcache->cooked_read (entry->u.reg.num, reg.data ());
-	regcache->cooked_write (entry->u.reg.num, record_full_get_loc (entry));
-	memcpy (record_full_get_loc (entry), reg.data (), entry->u.reg.len);
-      }
-      break;
-
-    case record_full_mem: /* mem */
-      {
-	/* Nothing to do if the entry is flagged not_accessible.  */
-	if (!entry->u.mem.mem_entry_not_accessible)
-	  {
-	    gdb::byte_vector mem (entry->u.mem.len);
-
-	    if (record_debug > 1)
-	      gdb_printf (gdb_stdlog,
-			  "Process record: record_full_mem %s to "
-			  "inferior addr = %s len = %d.\n",
-			  host_address_to_string (entry),
-			  paddress (gdbarch, entry->u.mem.addr),
-			  entry->u.mem.len);
-
-	    if (record_read_memory (gdbarch,
-				    entry->u.mem.addr, mem.data (),
-				    entry->u.mem.len))
-	      entry->u.mem.mem_entry_not_accessible = 1;
-	    else
-	      {
-		if (target_write_memory (entry->u.mem.addr,
-					 record_full_get_loc (entry),
-					 entry->u.mem.len))
-		  {
-		    entry->u.mem.mem_entry_not_accessible = 1;
-		    if (record_debug)
-		      warning (_("Process record: error writing memory at "
-				 "addr = %s len = %d."),
-			       paddress (gdbarch, entry->u.mem.addr),
-			       entry->u.mem.len);
-		  }
-		else
-		  {
-		    memcpy (record_full_get_loc (entry), mem.data (),
-			    entry->u.mem.len);
-
-		    /* We've changed memory --- check if a hardware
-		       watchpoint should trap.  Note that this
-		       presently assumes the target beneath supports
-		       continuable watchpoints.  On non-continuable
-		       watchpoints target, we'll want to check this
-		       _before_ actually doing the memory change, and
-		       not doing the change at all if the watchpoint
-		       traps.  */
-		    if (hardware_watchpoint_inserted_in_range
-			(current_inferior ()->aspace.get (),
-			 entry->u.mem.addr, entry->u.mem.len))
-		      record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
-		  }
-	      }
-	  }
-      }
-      break;
-    }
-}
-
 static inline void
 record_full_exec_insn (struct regcache *regcache,
 		       struct gdbarch *gdbarch,
 		       record_full_instruction &insn)
 {
   for (auto &entry : insn.effects)
-    record_full_exec_entry (regcache, gdbarch, &entry);
+    if (entry.execute (regcache, gdbarch))
+      record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 }
 
 static void record_full_restore (struct bfd &cbfd);
@@ -2165,21 +2203,19 @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
 		      bfd_offset);
 	regnum = netorder32 (regnum);
 
-	record_full_entry rec;
-
-	rec = record_full_reg_init (cache, regnum);
+	record_full_entry rec (record_full_reg, cache->arch (), regnum);
 
 	/* Get val.  */
-	bfdcore_read (cbfd, osec, record_full_get_loc (&rec),
-		      rec.u.reg.len, bfd_offset);
+	bfdcore_read (cbfd, osec, rec.get_loc (),
+		      rec.reg ().len, bfd_offset);
 
 	if (record_debug)
 	  gdb_printf (gdb_stdlog,
 		      "  Reading register %d (1 "
 		      "plus %lu plus %d bytes)\n",
-		      rec.u.reg.num,
+		      rec.reg ().num,
 		      (unsigned long) sizeof (regnum),
-		      rec.u.reg.len);
+		      rec.reg ().len);
 
 	record_full_arch_list_add (rec);
 	break;
@@ -2196,11 +2232,10 @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
 		      bfd_offset);
 	addr = netorder64 (addr);
 
-	record_full_entry rec;
-	rec = record_full_mem_init (addr, len);
+	record_full_entry rec (record_full_mem, addr, len);
 
 	/* Get val.  */
-	bfdcore_read (cbfd, osec, record_full_get_loc (&rec),
+	bfdcore_read (cbfd, osec, rec.get_loc (),
 		      len, bfd_offset);
 
 	if (record_debug)
@@ -2208,7 +2243,7 @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
 		      "  Reading memory %s (1 plus "
 		      "%lu plus %lu plus %d bytes)\n",
 		      paddress (get_current_arch (),
-				rec.u.mem.addr),
+				rec.mem ().addr),
 		      (unsigned long) sizeof (addr),
 		      (unsigned long) sizeof (len),
 		      len);
@@ -2363,57 +2398,62 @@ record_full_write_entry_to_bfd (record_full_entry &entry,
   uint32_t regnum, len;
   uint64_t addr;
 
-  type = entry.type;
+  type = entry.type ();
   bfdcore_write (obfd.get (), osec, &type, sizeof (type), bfd_offset);
 
-  switch (entry.type)
+  switch (type)
     {
     case record_full_reg: /* reg */
-      if (record_debug)
-	gdb_printf (gdb_stdlog,
-		    "  Writing register %d (1 "
-		    "plus %lu plus %d bytes)\n",
-		    entry.u.reg.num,
-		    (unsigned long) sizeof (regnum),
-		    entry.u.reg.len);
-
-      /* Write regnum.  */
-      regnum = netorder32 (entry.u.reg.num);
-      bfdcore_write (obfd.get (), osec, &regnum,
-		     sizeof (regnum), bfd_offset);
-
-      /* Write regval.  */
-      bfdcore_write (obfd.get (), osec,
-		     record_full_get_loc (&entry),
-		     entry.u.reg.len, bfd_offset);
-      break;
+      {
+	auto reg = entry.reg ();
+	if (record_debug)
+	  gdb_printf (gdb_stdlog,
+		      "  Writing register %d (1 "
+		      "plus %lu plus %d bytes)\n",
+		      reg.num,
+		      (unsigned long) sizeof (regnum),
+		      reg.len);
+
+	/* Write regnum.  */
+	regnum = netorder32 (reg.num);
+	bfdcore_write (obfd.get (), osec, &regnum,
+		       sizeof (regnum), bfd_offset);
+
+	/* Write regval.  */
+	bfdcore_write (obfd.get (), osec,
+		       entry.get_loc (),
+		       reg.len, bfd_offset);
+	break;
+      }
 
     case record_full_mem: /* mem */
-      if (record_debug)
-	gdb_printf (gdb_stdlog,
-		    "  Writing memory %s (1 plus "
-		    "%lu plus %lu plus %d bytes)\n",
-		    paddress (gdbarch,
-			      entry.u.mem.addr),
-		    (unsigned long) sizeof (addr),
-		    (unsigned long) sizeof (len),
-		    entry.u.mem.len);
-
-      /* Write memlen.  */
-      len = netorder32 (entry.u.mem.len);
-      bfdcore_write (obfd.get (), osec, &len, sizeof (len),
-		     bfd_offset);
-
-      /* Write memaddr.  */
-      addr = netorder64 (entry.u.mem.addr);
-      bfdcore_write (obfd.get (), osec, &addr,
-		     sizeof (addr), bfd_offset);
-
-      /* Write memval.  */
-      bfdcore_write (obfd.get (), osec,
-		     record_full_get_loc (&entry),
-		     entry.u.mem.len, bfd_offset);
-      break;
+      {
+	auto mem = entry.mem ();
+	if (record_debug)
+	  gdb_printf (gdb_stdlog,
+		      "  Writing memory %s (1 plus "
+		      "%lu plus %lu plus %d bytes)\n",
+		      paddress (gdbarch, mem.addr),
+		      (unsigned long) sizeof (addr),
+		      (unsigned long) sizeof (len),
+		      mem.len);
+
+	/* Write memlen.  */
+	len = netorder32 (mem.len);
+	bfdcore_write (obfd.get (), osec, &len, sizeof (len),
+		       bfd_offset);
+
+	/* Write memaddr.  */
+	addr = netorder64 (mem.addr);
+	bfdcore_write (obfd.get (), osec, &addr,
+		       sizeof (addr), bfd_offset);
+
+	/* Write memval.  */
+	bfdcore_write (obfd.get (), osec,
+		       entry.get_loc (),
+		       mem.len, bfd_offset);
+	break;
+      }
     }
 
 }
@@ -2461,13 +2501,13 @@ record_full_base_target::save_record (const char *recfilename)
       /* Number of effects of an instruction.  */
       save_size += sizeof (uint32_t) + sizeof (uint8_t) + sizeof (uint32_t);
       for (auto &entry : record_full_list[i].effects)
-	switch (entry.type)
+	switch (entry.type ())
 	  {
 	  case record_full_reg:
-	    save_size += 1 + 4 + entry.u.reg.len;
+	    save_size += 1 + 4 + entry.reg ().len;
 	    break;
 	  case record_full_mem:
-	    save_size += 1 + 4 + 8 + entry.u.mem.len;
+	    save_size += 1 + 4 + 8 + entry.mem ().len;
 	    break;
 	  }
     }
@@ -2603,16 +2643,16 @@ maintenance_print_record_instruction (const char *args, int from_tty)
 
   for (auto entry : to_print->effects)
     {
-      switch (entry.type)
+      switch (entry.type ())
 	{
 	  case record_full_reg:
 	    {
-	      type *regtype = gdbarch_register_type (arch, entry.u.reg.num);
+	      type *regtype = gdbarch_register_type (arch, entry.reg ().num);
 	      value *val
 		  = value_from_contents (regtype,
-					 record_full_get_loc (&entry));
+					 entry.get_loc ());
 	      gdb_printf ("Register %s changed: ",
-			  gdbarch_register_name (arch, entry.u.reg.num));
+			  gdbarch_register_name (arch, entry.reg ().num));
 	      struct value_print_options opts;
 	      get_user_print_options (&opts);
 	      opts.raw = true;
@@ -2622,11 +2662,12 @@ maintenance_print_record_instruction (const char *args, int from_tty)
 	    }
 	  case record_full_mem:
 	    {
-	      gdb_byte *b = record_full_get_loc (&entry);
+	      record_full_mem_entry& mem = entry.mem ();
+	      gdb_byte *b = entry.get_loc ();
 	      gdb_printf ("%d bytes of memory at address %s changed from:",
-			  entry.u.mem.len,
-			  print_core_address (arch, entry.u.mem.addr));
-	      for (int i = 0; i < entry.u.mem.len; i++)
+			  mem.len,
+			  print_core_address (arch, mem.addr));
+	      for (int i = 0; i < mem.len; i++)
 		gdb_printf (" %02x", b[i]);
 	      gdb_printf ("\n");
 	      break;
-- 
2.53.0