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

[Guinevere Larsen <guinevere@redhat.com>]

On 5/26/26 6:00 AM, Schimpe, Christina wrote:
>> -----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 ?
Yes, good point!
> 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. 😊
Thiago suggested that, since I'm already messing with the class, I may 
as well invert the name and logic of the variable. I guess my thinking 
was that, since he already seen my previous version this wouldn't be 
much of a change to review, but you make a good point, will keep it in 
mind for the future
>
>>     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 ?
Mainly because I wanted to treat "len == 0" as effectively 
uninitialized, so it didn't seem important, but I see that I didn't 
actually follow through with it that much so I suppose I should at least 
initialize mem_entry_accessible. u seems unnecessary to setup, since 
we'll do 0-sized reads (which will return the local buffer and not a 
random pointer)....
>
>> +
>> +  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 ?

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.

>
> 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.

>
>> +    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.

There's a good amount of places that would need changing for adding 
const to get_loc, I think, including bfdcore_read and bfdcore_write, so 
it feels like its too much for this patch. reg and mem throw errors 
about the get discarding qualifiers.

I did add const to "type" though, thanks for pointing that out.

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

-- 
Cheers,
Guinevere Larsen
It/she