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

[Guinevere Larsen <guinevere@redhat.com>]

On 4/18/26 12:45 AM, Thiago Jung Bauermann wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> 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.
> Always nice to see these C++ification patches. Some comments below.
>
>> ---
>>   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;
> In general I think bools that have "not" in their name are
> confusing. IMHO it's more straightforward to think about
> mem_entry_accessible than about mem_entry_not_accessible.
>
> The code was already like this before your patch, but if you agree with
> my point this could be a good opportunity to change it.
Yeah, fair point. I will update it
>
>>     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)
> Two nits: The above fits in one line. Also, I suggest removing the
> "struct" keywords.
Fixed.
> A bit less of a nit: the regcache includes a gdbarch, so I think it's
> better to use it from there rather than pass it as a separate argument.
Huh, turns out, the regcache is not even needed in this function, I must 
have not looked closely at the code I copy pasted
>
>> +  {
>> +    /* Nothing to do if the memory is flagged not_accessible.  */
>> +    if (!mem_entry_not_accessible)
> This is a matter of personal preference, but in cases like this I like
> to do an early return:
>
>    if (mem_entry_not_acessible)
>      return false;
>
> Then the rest of the function doesn't need to be fully inside the if
> block. I think it's easier to read, and also saves one level of
> indentation.
Yeah, I think this makes sense. Went with this option.
>
> Also, the if condition above is a good illustration of my comment about
> the variable name. It took me a moment to realize what
> !mem_entry_not_accessible means.
>
>> +      {
>> +	gdb::byte_vector buf (len);
>> +
>> +	  if (record_debug > 1)
> This function isn't indented correctly, starting with this if which
> isn't in the right column.
fixed
>
>> +	    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;
> This is a bool now, so it's better to use true/false rather than 0/1.
fixed
>
>> +	  else
>> +	    {
>> +	      if (target_write_memory (addr,
>> +				       get_loc (),
>> +				       len))
>> +		{
>> +		  mem_entry_not_accessible = 1;
> This is a bool now, so it's better to use true/false rather than 0/1.
fixed
>
>> +		  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)
> Same comments here as in the execute prototype for the mem entry.
this is the opposite, no gdbarch needed :)
>
>> +  {
>> +    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 ());
> It's better to pass simply "buf" here rather than "buf.data ()". This
> way, the cooked_read called will be the one which gets an array_view and
> it will check that buf is big enough to get the register contents.
Good point, fixed
>
>> +    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;
> Instead of having this variant and the switches in all the methods, have
> you considered making this a base class with virtual methods?  Then
> record_full_reg_entry and record_full_mem_entry would implement them as
> appropriate.

I have considered it. The reason I didn't go with it is because 
polymorphism would need pointers as opposed to references or just 
variables, and so the effects vector in record_full_instruction would 
need to hold pointers to the base class, still requiring us to manually 
handle memory and things. So in the end it felt like it would net more 
complexity rather than less... or maybe a similar amount of complexity, 
but one that would be more confusing to follow in my opinion.

>
>> +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);
>> +  }
> For this method, record_full_mem_entry would just have an
> gdb_assert_not_reached.
>
>> +
>> +  record_full_mem_entry& mem ()
>> +  {
>> +    gdb_assert (type () == record_full_mem);
>> +    return std::get<record_full_mem_entry> (entry);
>> +  }
> Likewise for this method, record_full_reg_entry would just have an
> gdb_assert_not_reached.
>
>> +  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;
>> +  }
> The methods above look like dynamic dispatch, but implemented by hand. :)
>
>>   };
>>   
>>   /* 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.  */
> Even if for now it's not a destructor, this could be a method in
> record_full_reg_entry.

good point, I've implemented it.

>
>>   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.  */
> Likewise, this could be a method in record_full_reg_entry.
done too.
>>   
>>   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;
> And this could be a (virtual?) method in record_full_entry.
also done (non-virtual for the previously explained reasons)
>
>> @@ -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 ());
> Just a suggestion, feel free to adopt or ignore: if get_loc () returned
> a gdb::array_view, this cooked_read could be the one with bounds checking.
this seems like a non-trivial change, so considering how there should be 
no way to get that wrong with the current code I'll pass on it.
>
>>   
>>     record_full_arch_list_add (rec);
>>   


-- 
Cheers,
Guinevere Larsen
It/she