Re: [PATCH 1/6] gdb/record: Refactor record history

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

Hello Guinevere,

Nice improvement! Just a few comments, mostly nits.

Guinevere Larsen <guinevere@redhat.com> writes:

> This is the first step in a large refactor in how GDB keeps execution
> history.  Rather than using a linked list where multiple entries can
> describe a single instruction, the history will now be stored in an
> std::deque, each instruction being one entry in the deque.
>
> The choice was initially to use an std::vector, but it would become
> unwieldy because it needs all the memory to be consecutive, which is
> hard for 200 thousand entries. Deque was picked because it was a nice
> midpoint between vector (maximum cache cohesion) and linked list
> (maximum ease of finding space to store more).
>
> Each instruction in memory will be now one record_full_instruction
> entry, which for this commit just contains a vector of
> record_full_entry for the effects of the instruction, and the data that
> was stored in the record_full_end entry (that is, the instruction number
> and the signal, if any).
>
> This change introduced a minimal performance improvement (what's
> important is that it isn't a degradation) and a  reduction in the total
> memory footprint of roughly 20% if the entire history is used.

Awesome!

> ---
>  gdb/aarch64-tdep.c     |    2 -
>  gdb/amd64-linux-tdep.c |    3 -
>  gdb/arm-tdep.c         |    2 -
>  gdb/i386-linux-tdep.c  |    3 -
>  gdb/i386-tdep.c        |    4 -
>  gdb/loongarch-tdep.c   |    2 -
>  gdb/moxie-tdep.c       |    2 -
>  gdb/ppc-linux-tdep.c   |    3 -
>  gdb/record-full.c      | 1193 ++++++++++++++++------------------------
>  gdb/record-full.h      |    1 -
>  gdb/riscv-tdep.c       |    3 -
>  gdb/rs6000-tdep.c      |    4 -
>  gdb/s390-linux-tdep.c  |    3 -
>  gdb/s390-tdep.c        |    2 -
>  14 files changed, 482 insertions(+), 745 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 4befaa2720d..a532992b1d8 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -6210,8 +6210,6 @@ aarch64_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>  	       aarch64_record.aarch64_mems[rec_no].len))
>  	    ret = -1;
>  
> -      if (record_full_arch_list_add_end ())
> -	ret = -1;
>      }
>  
>    deallocate_reg_mem (&aarch64_record);
⋮
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 08cce9dbad8..1ce0927f26b 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -14911,8 +14911,6 @@ arm_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>  	    }
>  	}
>  
> -      if (record_full_arch_list_add_end ())
> -	ret = -1;
>      }
>  
>

FWIW the aarch64-tdep.c and arm-tdep.c changes are obvious.

⋮
>  /* Free one record entry, any type.
>     Return entry->type, in case caller wants to know.  */

The "Return ..." sentence can be deleted, since the function returns void now.

> -static inline enum record_full_type
> -record_full_entry_release (struct record_full_entry *rec)
> +static inline void
> +record_full_entry_cleanup (struct record_full_entry rec)
>  {
> -  enum record_full_type type = rec->type;
>  
> -  switch (type) {
> +  switch (rec.type) {
>    case record_full_reg:
> -    record_full_reg_release (rec);
> +    record_full_reg_cleanup (rec);
>      break;
>    case record_full_mem:
> -    record_full_mem_release (rec);
> -    break;
> -  case record_full_end:
> -    record_full_end_release (rec);
> +    record_full_mem_cleanup (rec);
>      break;
>    }
> -  return type;
>  }
⋮
> -/* Free all record entries forward of the given list position.  */
> -
>  static void
> -record_full_list_release_following (struct record_full_entry *rec)
> +record_full_list_release_following (int index)
>  {
> -  struct record_full_entry *tmp = rec->next;
> -
> -  rec->next = NULL;
> -  while (tmp)
> +  for (int i = record_full_list.size (); i >= index; i--)

Shouldn't this loop start with record_full_list.size () - 1, to avoid
accessing member record_full_list[record_full_list.size ()] below, which
AFAIK isn't valid?

Also, I'll just point out one thing which I don't know if it's
intentional or not: before this patch, this function preserved the rec
entry provided as argument. After this patch, it removes the entry
corresponding to the index argument.

>      {
> -      rec = tmp->next;
> -      if (record_full_entry_release (tmp) == record_full_end)
> -	{
> -	  record_full_insn_num--;
> -	  record_full_insn_count--;
> -	}
> -      tmp = rec;
> +      for (auto &entry : record_full_list[i].effects)
> +	record_full_entry_cleanup (entry);
> +      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;
> +}
> +
> +static void
> +record_full_save_instruction ()
> +{
> +  ++record_full_insn_count;
> +  record_full_incomplete_instruction.insn_num = record_full_insn_count;
> +  record_full_incomplete_instruction.effects.shrink_to_fit ();
> +  record_full_list.push_back (record_full_incomplete_instruction);
> +  record_full_next_insn ++;

Extra space before '++'.

> @@ -746,7 +648,7 @@ record_full_message (struct regcache *regcache, enum gdb_signal signal)
>  	 the user says something different, like "deliver this signal"
>  	 during the replay mode).
>  
> -	 User should understand that nothing he does during the replay
> +	 User should understand that nothing they do during the replay
>  	 mode will change the behavior of the child.  If he tries,

s/he tries/they try/

>  	 then that is a user error.
>
⋮
> @@ -1322,21 +1223,6 @@ record_full_wait_1 (struct target_ops *ops,
>  	  record_full_stop_reason = TARGET_STOPPED_BY_NO_REASON;
>  	  status->set_stopped (GDB_SIGNAL_0);
>  
> -	  /* Check breakpoint when forward execute.  */
> -	  if (execution_direction == EXEC_FORWARD)
> -	    {
> -	      tmp_pc = regcache_read_pc (regcache);
> -	      if (record_check_stopped_by_breakpoint (aspace, tmp_pc,
> -						      &record_full_stop_reason))
> -		{
> -		  if (record_debug)
> -		    gdb_printf (gdb_stdlog,
> -				"Process record: break at %s.\n",
> -				paddress (gdbarch, tmp_pc));
> -		  goto replay_out;
> -		}
> -	    }
> -
>  	  /* If GDB is in terminal_inferior mode, it will not get the
>  	     signal.  And in GDB replay mode, GDB doesn't need to be
>  	     in terminal_inferior mode, because inferior will not

The commit message mentions only a change in the data structure used to
store recorded instructions, but this hunk looks like an unrelated
change in the logic: why is it ok to remove this check of whether a
breakpoint has been hit?

I suppose it's correct because the are no testsuite regressions (there
actually are for this patch, but not for the series as a whole). This
change should either be explained in the commit message, or moved to a
separate patch (assuming that's feasible).

Or maybe I'm wrong and this is just part of the big code reorg in this
function? If so, please disregard this comment.

⋮
> -	replay_out:
> +	  if (record_full_next_insn < 0)
> +	    {
> +	      gdb_assert (execution_direction == EXEC_REVERSE);
> +	      record_full_next_insn = 0;
> +	    }
> +	  else if (record_full_next_insn > record_full_list.size ())
> +	    {
> +	      gdb_assert (execution_direction == EXEC_FORWARD);
> +	      record_full_next_insn = record_full_list.size ();
> +	    }
> +	  /* Reset the current instruciton to point to the one to be replayed
> +	     moving forward.  */
> +	  else if (execution_direction == EXEC_REVERSE)
> +	    record_full_next_insn++;
> +
> +	//replay_out:

This commented-out label should be removed.

>  	  if (status->kind () == TARGET_WAITKIND_STOPPED)
>  	    {
> +	      int insn = (execution_direction == EXEC_FORWARD)
> +			 ? record_full_next_insn - 1 : record_full_next_insn;
>  	      if (record_full_get_sig)
>  		status->set_stopped (GDB_SIGNAL_INT);
> -	      else if (record_full_list->u.end.sigval != GDB_SIGNAL_0)
> -		/* FIXME: better way to check */
> -		status->set_stopped (record_full_list->u.end.sigval);
> +	      else if (record_full_list[insn].sigval.has_value ())
> +		status->set_stopped
> +		  (record_full_list[insn].sigval.value ());
>  	      else
>  		status->set_stopped (GDB_SIGNAL_TRAP);
>  	    }
⋮
> @@ -2339,19 +2157,98 @@ netorder32 (uint32_t input)
>    return ret;
>  }
>  
> +static void
> +record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
> +{
> +  uint8_t rectype;
> +  uint32_t regnum, len;
> +  uint64_t addr;
> +  regcache *cache = get_thread_regcache (inferior_thread ());
> +
> +  bfdcore_read (cbfd, osec, &rectype, sizeof (rectype),
> +		bfd_offset);

This line fits in 80 columns.

Also some others below, but they don't make it to the final version of
the function so not that important.

> @@ -2379,124 +2276,50 @@ record_full_restore (struct bfd &cbfd)
>  		"RECORD_FULL_FILE_MAGIC (0x%s)\n",
>  		phex_nz (netorder32 (magic), 4));
>  
> -  /* Restore the entries in recfd into record_full_arch_list_head and
> -     record_full_arch_list_tail.  */
> -  record_full_arch_list_head = NULL;
> -  record_full_arch_list_tail = NULL;
>    record_full_insn_num = 0;
>  
>    try
>      {
> -      regcache *regcache = get_thread_regcache (inferior_thread ());
>

This empty line should also be removed.

> -      while (1)
> +      while (bfd_offset < osec_size)
>  	{
> -	  uint8_t rectype;
> -	  uint32_t regnum, len, signal, count;
> -	  uint64_t addr;
>  

This empty line should also be removed.

> -	  /* We are finished when offset reaches osec_size.  */
> -	  if (bfd_offset >= osec_size)
> -	    break;
> -	  bfdcore_read (&cbfd, osec, &rectype, sizeof (rectype), &bfd_offset);
> +	  record_full_reset_incomplete ();
> +	  uint32_t eff_count = 0;
> +	  uint8_t sigval;
> +	  uint32_t insn_num;
>  
> -	  switch (rectype)
> -	    {
> -	    case record_full_reg: /* reg */
> -	      /* Get register number to regnum.  */
> -	      bfdcore_read (&cbfd, osec, &regnum, sizeof (regnum),
> -			    &bfd_offset);
> -	      regnum = netorder32 (regnum);
> -
> -	      rec = record_full_reg_alloc (regcache, regnum);
> -
> -	      /* Get val.  */
> -	      bfdcore_read (&cbfd, osec, record_full_get_loc (rec),
> -			    rec->u.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,
> -			    (unsigned long) sizeof (regnum),
> -			    rec->u.reg.len);
> -	      break;
> +	  /* First read the generic information for an instruction.  */
> +	  bfdcore_read (&cbfd, osec, &sigval,
> +			 sizeof (uint8_t), &bfd_offset);
> +	  bfdcore_read (&cbfd, osec, &eff_count, sizeof (uint32_t),
> +			&bfd_offset);
> +	  bfdcore_read (&cbfd, osec, &insn_num,
> +			 sizeof (uint32_t), &bfd_offset);

The first and third bfdcore_read calls above fit in 80 columns.

Actually, there are many bfdcore_read and bfdcore_write calls in this
and other patches which are unnecessarily broken into two lines. I will
stop pointing them out. :)

-- 
Thiago