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

[Guinevere Larsen <guinevere@redhat.com>]

On 4/17/26 9:15 PM, Thiago Jung Bauermann wrote:
> Hello Guinevere,
>
> Nice improvement! Just a few comments, mostly nits.
Thanks for properly reviewing this beast of a change!
>
> 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.
fixed!
>
>> -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?
oh, nice catch. Not only is it invalid, it then makes the code leak 
memory since we're never calling the cleanups. Fixed
>
> 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.
Huh. Yeah, all the more reason to modernize the code, this had totally 
escaped me. Will update to keep the previous behavior.
>
>>       {
>> -      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 '++'.
fixed
>> @@ -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/
fixed
>
>>   	 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.

I thought this was just an unnecessary piece of code, and that I was 
going to need to rework it because of the refactor, and decided to just 
remove it. Now that the refactor is done, I realize I was wrong... and 
considering Linaro CI found some regressions, I'm guessing these might 
be related.

I'll explore this a little bit, but most likely I'll re-add this for v2.

>
> ⋮
>> -	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.
fixed.
>
>>   	  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.
fixed
>
> 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.
fixed.
>
>> -      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.
fixed.
>
>> -	  /* 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. :)
>
Fixed these, and will take a look at future calls as well :)

These went through so many iterations, no wonder I forgot to keep 
reformatting them lol, but thanks for checking!

-- 
Cheers,
Guinevere Larsen
It/she