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

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Freitag, 15. Mai 2026 18:37
> To: gdb-patches@sourceware.org
> Cc: Guinevere Larsen <guinevere@redhat.com>; Thiago Jung Bauermann
> <thiago.bauermann@linaro.org>
> Subject: [PATCH v3 3/7] gdb/record: c++ify internal structures of record-full.c
> 
> 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(-)
> 
> 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;

Shouldn't we update the comment then, too ?
Nit: you could consider moving this into a separate patch. I would make the review a bit easier IMO.

The same applies for each structure/class that you refactor in this patch. 
But this is rather an optional suggestion, too. 😊 

>    union
>    {
>      gdb_byte *ptr;
>      gdb_byte buf[sizeof (gdb_byte *)];
>    } u;
> +
> +  record_full_mem_entry () : addr (0), len (0) { }

Any specific reason why you don't init mem_entry_accessible and u here ?

> +
> +  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));

Shouldn't we check for self-assignment here ?

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=

> +    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 ()

I don't think this is too important, but since I saw it: 
This should be const I think.

IIUC we should then also use a const reference in loops which call get_loc (), 
like this one:

  for (auto &entry : to_print->effects)

And the same applies for type, reg and mem. 

> +  {
> +    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;
> --
> 2.54.0
> 

Christina
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928