diff mbox series

[v3,3/7] gdb/record: c++ify internal structures of record-full.c

Message ID 20260515163706.3355686-4-guinevere@redhat.com
State New
Headers
Series refactor the internals of record-full |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Guinevere Larsen May 15, 2026, 4:37 p.m. UTC 
  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.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
 gdb/record-full.c | 601 +++++++++++++++++++++++++---------------------
 1 file changed, 321 insertions(+), 280 deletions(-)

Comments

Guinevere Larsen May 29, 2026, 4:38 p.m. UTC | #1 
On 5/29/26 9:09 AM, Schimpe, Christina wrote:
>>>> +    addr = other.addr;
>>>> +    len = other.len;
>>>> +    memcpy (u.buf, other.u.buf, sizeof (u.buf));
>>> Shouldn't we check for self-assignment here ?
>> I don't think it would be possible to self-assign here, since this is a
>> move operator. We shouldn't be able to create a copy anywhere, so I
>> think it shouldn't be a large thing.
>> I can add an assert, since if it ever happens this is a large bug.
> Yeah, I just noticed it since I've been looking at a sample implementation
> for the move assignment operator which does check for self-assignment:
>
> https://learn.microsoft.com/en-us/cpp/cpp/move-constructors-and-move-assignment-operators-cpp?view=msvc-170#example-complete-move-constructor-and-assignment-operator
>
> and then compared a bit with other classes which have DISABLE_COPY_AND_ASSIGN
> and the move assignment operator.
Right, makes sense. Since in this case I would see self-assignment as a 
bug, I added an assert, but if we ever have a different opinion as a 
project, it's easy enough to make it an if condition.
>
>>> Also, like the destructor, I believe we must delete u.ptr here:
>>> ~~~
>>>     if (len > sizeof (u.buf))
>>>         delete[] u.ptr;
>>> ~~~
>>>
>>> Same feedback for record_full_mem_entry &operator=
>> The idea of a move operation (constructor or assignment) is that we
>> won't need to allocate new memory, we'll just pass the pointer to the
>> new holder and that's it. If the pointer is deleted here, since it was
>> copied over in the memcpy call, when the next object calls its
>> destructor, we'll get a double free.
> Arg, this might have been confusing.  My feedback on this is not exactly at the right line.
> I'd add this delete before those lines:
>
>>>> +    addr = other.addr;
>>>> +    len = other.len;
>>>> +    memcpy (u.buf, other.u.buf, sizeof (u.buf));
> When the object is finally destructed, I don't expect a double free, since we only free
> the data of the moved object. In the move operator we delete the data of the original
> object.
>
> In the example link I shared above, also the existing data are deleted.
> But it might be that I'm missing something here still.

Oh right, that makes a lot more sense, I thought the suggestion was to 
free the other.u.ptr lol. Sorry for the misunderstanding!

In the case of this code, I would expect that we're always assigning 
this to the "essentially uninitialized" variable, so again I think an 
assert that len == 0 is a better check. I'll run stuff locally to ensure 
that my understanding is correct, and if it isn't I'll add the free you 
suggested.
diff mbox series

Patch

diff --git a/gdb/record-full.c b/gdb/record-full.c
index e4dd07dc300..24e94eaf5da 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -51,6 +51,7 @@ 
 #include <optional>
 #include <deque>
 #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
@@ -93,12 +94,113 @@  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_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_accessible = true;
+  }
+
+  record_full_mem_entry (record_full_mem_entry &&other)
+  {
+    addr = other.addr;
+    len = other.len;
+    memcpy (u.buf, other.u.buf, sizeof (u.buf));
+    mem_entry_accessible = other.mem_entry_accessible;
+    /* With len == 0, OTHER is guaranteed to not try to free the
+       memory when it is destructed.  */
+    other.len = 0;
+  }
+
+  ~record_full_mem_entry ()
+  {
+    if (len > sizeof (u.buf))
+      delete[] u.ptr;
+  }
+
+  record_full_mem_entry &operator= (record_full_mem_entry &&other)
+  {
+    addr = other.addr;
+    len = other.len;
+    memcpy (u.buf, other.u.buf, sizeof (u.buf));
+    mem_entry_accessible = other.mem_entry_accessible;
+    /* With len == 0, OTHER is guaranteed to not try to free the
+       memory when it is destructed.  */
+    other.len = 0;
+    return *this;
+  }
+
+  DISABLE_COPY_AND_ASSIGN (record_full_mem_entry);
+
+  gdb_byte *get_loc ()
+  {
+    if (len > sizeof (u.buf))
+      return u.ptr;
+    else
+      return u.buf;
+  }
+
+  bool execute (gdbarch *gdbarch)
+  {
+    /* Nothing to do if the memory is flagged not_accessible.  */
+    if (!mem_entry_accessible)
+      return false;
+
+    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_accessible = false;
+    else
+      {
+	if (target_write_memory (addr,
+				 get_loc (),
+				 len))
+	  {
+	    mem_entry_accessible = false;
+	    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
@@ -110,6 +212,70 @@  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];
+  }
+
+  record_full_reg_entry (record_full_reg_entry &&other)
+  {
+    num = other.num;
+    len = other.len;
+    memcpy (u.buf, other.u.buf, sizeof (u.buf));
+    /* With len == 0, OTHER is guaranteed to not try to free the
+       memory when it is destructed.  */
+    other.len = 0;
+  }
+
+  ~record_full_reg_entry ()
+  {
+    if (len > sizeof (u.buf))
+      delete[] u.ptr;
+  }
+
+  record_full_reg_entry &operator=(record_full_reg_entry &&other)
+  {
+    num = other.num;
+    len = other.len;
+    memcpy (u.buf, other.u.buf, sizeof (u.buf));
+    /* With len == 0, OTHER is guaranteed to not try to free the
+       memory when it is destructed.  */
+    other.len = 0;
+    return *this;
+  }
+
+  DISABLE_COPY_AND_ASSIGN (record_full_reg_entry);
+
+  gdb_byte *get_loc ()
+  {
+    if (len > sizeof (u.buf))
+      return u.ptr;
+    else
+      return u.buf;
+  }
+
+  bool execute (regcache *regcache)
+  {
+    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);
+    regcache->cooked_write (num, get_loc ());
+    memcpy (get_loc (), buf.data (), len);
+    return false;
+  }
 };
 
 enum record_full_type
@@ -118,16 +284,88 @@  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_entry (record_full_entry &&other)
+  : entry (std::move (other.entry))
+  {
+  }
+
+  DISABLE_COPY_AND_ASSIGN (record_full_entry);
+
+  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 (regcache *regcache)
+  {
+    switch (type ())
+      {
+      case record_full_reg:
+	return reg ().execute (regcache);
+      case record_full_mem:
+	return mem ().execute (regcache->arch ());
+      }
+    return false;
+  }
 };
 
 /* This is the main structure that comprises the execution log.
@@ -384,91 +622,12 @@  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 record_full_entry
-record_full_reg_init (struct regcache *regcache, int regnum)
-{
-  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.  */
-
-static inline void
-record_full_reg_cleanup (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);
-}
-
-/* Init a record_full_mem record entry.  */
-
-static inline record_full_entry
-record_full_mem_init (CORE_ADDR addr, int len)
-{
-  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.  */
-
-static inline void
-record_full_mem_cleanup (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);
-}
-
-/* Free one record entry, any type.  */
-
-static inline void
-record_full_entry_cleanup (record_full_entry rec)
-{
-
-  switch (rec.type) {
-  case record_full_reg:
-    record_full_reg_cleanup (rec);
-    break;
-  case record_full_mem:
-    record_full_mem_cleanup (rec);
-    break;
-  }
-}
-
 static void
 record_full_reset_history ()
 {
   record_full_insn_count = 0;
   record_full_next_insn = 0;
 
-  for (auto &insn : record_full_list)
-    {
-      for (auto &entry : insn.effects)
-	record_full_entry_cleanup (entry);
-    }
-
   record_full_list.clear ();
 }
 
@@ -476,11 +635,7 @@  static void
 record_full_list_release_following (int index)
 {
   for (int i = record_full_list.size () - 1; i > index; i--)
-    {
-      for (auto &entry : record_full_list[i].effects)
-	record_full_entry_cleanup (entry);
-      record_full_list.pop_back ();
-    }
+    record_full_list.pop_back ();
   /* Set the next instruction to be past the end of the log so we
      start recording if the user moves forward again.  */
   record_full_next_insn = index;
@@ -509,9 +664,6 @@  record_full_list_release_first (void)
   if (record_full_list.empty ())
     return;
 
-  for (auto &entry : record_full_list[0].effects)
-    record_full_entry_cleanup (entry);
-
   record_full_list.pop_front ();
   --record_full_next_insn;
 }
@@ -521,28 +673,7 @@  record_full_list_release_first (void)
 static void
 record_full_arch_list_add (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_full_incomplete_instruction.effects.push_back (std::move (rec));
 }
 
 /* Record the value of a register NUM to record_full_arch_list.  */
@@ -550,7 +681,7 @@  record_full_get_loc (struct record_full_entry *rec)
 int
 record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
 {
-  record_full_entry rec;
+  record_full_entry rec (record_full_reg, regcache->arch (), regnum);
 
   if (record_debug > 1)
     gdb_printf (gdb_stdlog,
@@ -558,9 +689,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);
 
@@ -573,7 +702,7 @@  record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
 int
 record_full_arch_list_add_mem (CORE_ADDR addr, int len)
 {
-  record_full_entry rec;
+  record_full_entry rec (record_full_mem, addr, len);
 
   if (record_debug > 1)
     gdb_printf (gdb_stdlog,
@@ -584,14 +713,9 @@  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))
-    {
-      record_full_mem_cleanup (rec);
-      return -1;
-    }
+			  rec.get_loc (), len))
+    return -1;
 
   record_full_arch_list_add (rec);
 
@@ -719,98 +843,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 (regcache *regcache,
-			gdbarch *gdbarch,
-			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 (regcache *regcache,
 		       gdbarch *gdbarch,
 		       record_full_instruction &insn)
 {
   for (auto &entry : insn.effects)
-    record_full_exec_entry (regcache, gdbarch, &entry);
+    if (entry.execute (regcache))
+      record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 }
 
 static void record_full_restore (struct bfd &cbfd);
@@ -2185,21 +2225,19 @@  record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
 	bfdcore_read (cbfd, osec, &regnum, sizeof (regnum), 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;
@@ -2216,19 +2254,17 @@  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), len,
-		      bfd_offset);
+	bfdcore_read (cbfd, osec, rec.get_loc (), len, bfd_offset);
 
 	if (record_debug)
 	  gdb_printf (gdb_stdlog,
 		      "  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);
@@ -2380,52 +2416,56 @@  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;
+      }
     }
 
 }
@@ -2473,13 +2513,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;
 	  }
     }
@@ -2612,18 +2652,18 @@  maintenance_print_record_instruction (const char *args, int from_tty)
 
   gdbarch *arch = current_inferior ()->arch ();
 
-  for (auto entry : to_print->effects)
+  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;
@@ -2633,11 +2673,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;