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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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.

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

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.

> +  {
> +    /* 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.

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.

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

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

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

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

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

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

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

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

>  
>    record_full_arch_list_add (rec);
>  

-- 
Thiago