RE: [PATCH v3 5/7] gdb/record: extract the PC to record_full_instruction

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----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>
> Subject: [PATCH v3 5/7] gdb/record: extract the PC to record_full_instruction
> 
> This commit makes it so the PC is not saved as part of the
> record_full_instruction effects, but rather gets a special location.
> That is because a couple of commands would really benefit from it being easy
> to find the PC (especially ones from record-btrace that's haven't been
> implemented to record-full yet, such as the ones in PR record/18059), while
> also possibly allowing for one fewer resizing of the effect vector (and saving
> an entire byte in the process).
> 
> This commit also refactored record_full_read_entry_from_bfd and
> record_full_write_entry_to_bfd, to make them methods of
> record_full_reg_entry and record_full_mem_entry, and also creates similar
> methods for record_full_entry and record_full_instruction.
> These could be turned into constructors in a future step of
> c++ification, but it felt like too much change for a single commit.
> ---
>  gdb/record-full.c | 386 ++++++++++++++++++++++++++++------------------
>  1 file changed, 239 insertions(+), 147 deletions(-)
> 
> diff --git a/gdb/record-full.c b/gdb/record-full.c index
> 8cabd6e9438..17696e07c0d 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -143,6 +143,14 @@ struct record_full_mem_entry
> 
>    DISABLE_COPY_AND_ASSIGN (record_full_mem_entry);
> 
> +  /* Create a mem_entry from a bfd file, when restoring a recording.
> +*/
> +  static record_full_mem_entry from_bfd (bfd *cbfd, asection* osec,
> +					 int *bfd_offset);
> +
> +  /* Save this mem entry to a bfd file.  */
> +  void to_bfd (gdb_bfd_ref_ptr obfd, asection *osec, int *bfd_offset,
> +	       gdbarch *gdbarch);
> +

I believe only in one of those to_bfd functions the gdbarch parameter is used.
Did you intentionally keep it the function which don't have use the parameter in to_bfd ?

And it might be a good idea to describe the parameters a bit, too. 😊

>    gdb_byte *get_loc ()
>    {
>      if (len > sizeof (u.buf))
> @@ -252,6 +260,14 @@ struct record_full_reg_entry
> 
>    DISABLE_COPY_AND_ASSIGN (record_full_reg_entry);
> 
> +  /* Create a reg_entry from a bfd file, when restoring a recording.
> +*/
> +  static record_full_reg_entry from_bfd (bfd *cbfd, asection* osec,
> +					 int *bfd_offset);
> +
> +  /* Save this reg entry to a bfd file.  */
> +  void to_bfd (gdb_bfd_ref_ptr obfd, asection *osec, int *bfd_offset,
> +	       gdbarch *gdbarch);
> +
>    gdb_byte *get_loc ()
>    {
>      if (len > sizeof (u.buf))
> @@ -314,6 +330,13 @@ class record_full_entry
> 
>    DISABLE_COPY_AND_ASSIGN (record_full_entry);
> 
> +  /* Create a generic entry from a bfd file, when restoring a
> + recording.  */  static void from_bfd (bfd *cbfd, asection* osec, int
> + *bfd_offset);
> +
> +  /* Save this entry to a bfd file.  */
> +  void to_bfd (gdb_bfd_ref_ptr obfd, asection *osec, int *bfd_offset,
> +	       gdbarch *gdbarch);
> +
>    record_full_reg_entry& reg ()
>    {
>      gdb_assert (type () == record_full_reg); @@ -374,6 +397,7 @@ class
> record_full_entry
>       this one;
>     * sigval: Whether the inferior received a signal while the following
>       instruction was being recorded;
> +   * pc: The Program Counter for where the instruction starts;

"for where" sounds a bit odd, what do you think about:

The Program Counter at the start of the instruction.

>     * effects: A list of record_full_entry structures, each of which
>       describing one effect that the instruction has on the inferior.
> 
> @@ -388,10 +412,18 @@ struct record_full_instruction
>    uint32_t insn_num;
>    std::optional<gdb_signal> sigval;
>    std::vector<record_full_entry> effects;
> +  record_full_reg_entry pc;
> 
>    /* Execute the full instruction.  As a side effect, set
>       record_full_stop_reason.  */
>    void exec_insn (regcache *regcache);
> +
> +  /* Create a full recorded instruction from a bfd.  */  static void
> + from_bfd (bfd *cbfd, asection* osec, int *bfd_offset);
> +
> +  /* Save this instruction to a bfd file.  */
> +  void to_bfd (gdb_bfd_ref_ptr obfd, asection *osec, int *bfd_offset,
> +	       gdbarch *gdbarch);
>  };
> 
>  /* If true, query if PREC cannot record memory @@ -695,7 +727,10 @@
> record_full_arch_list_add_reg (struct regcache *regcache, int regnum)
> 
>    regcache->cooked_read (regnum, rec.get_loc ());
> 
> -  record_full_arch_list_add (rec);
> +  if (regnum == gdbarch_pc_regnum (regcache->arch ()))
> +      record_full_incomplete_instruction.pc = std::move (rec.reg ());
> + else
> +    record_full_arch_list_add (rec);

Nit: indent is only 2 spaces.

>    return 0;
>  }
> @@ -849,6 +884,7 @@ static enum target_stop_reason
> record_full_stop_reason
> 
>  void record_full_instruction::exec_insn (regcache *regcache)  {
> +  pc.execute (regcache);
>    for (auto &entry : effects)
>      if (entry.execute (regcache))
>        record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT; @@ -
> 2207,68 +2243,88 @@ netorder32 (uint32_t input)
>    return ret;
>  }
> 
> -static void
> -record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
> +record_full_reg_entry
> +record_full_reg_entry::from_bfd (bfd *cbfd, asection *osec, int
> +*bfd_offset)
>  {
> -  uint8_t rectype;
> -  uint32_t regnum, len;
> -  uint64_t addr;
> +  uint32_t regnum;
>    regcache *cache = get_thread_regcache (inferior_thread ());
> 
> +  /* Get register number to regnum.  */
> +  bfdcore_read (cbfd, osec, &regnum, sizeof (regnum),
> +		bfd_offset);
> +  regnum = netorder32 (regnum);
> +
> +  record_full_reg_entry reg (cache->arch (), regnum);
> +
> +  /* Get val.  */
> +  bfdcore_read (cbfd, osec, reg.get_loc (),
> +		reg.len, bfd_offset);
> +
> +  if (record_debug)
> +    gdb_printf (gdb_stdlog,
> +		"  Reading register %d (1 "
> +		"plus %lu plus %d bytes)\n",
> +		reg.num,
> +		(unsigned long) sizeof (regnum),
> +		reg.len);
> +  return reg;
> +
> +}
> +
> +record_full_mem_entry
> +record_full_mem_entry::from_bfd (bfd *cbfd, asection *osec, int
> +*bfd_offset) {
> +  uint32_t len;
> +  uint64_t addr;
> +
> +  /* Get len.  */
> +  bfdcore_read (cbfd, osec, &len, sizeof (len), bfd_offset);  len =
> + netorder32 (len);
> +
> +  /* Get addr.  */
> +  bfdcore_read (cbfd, osec, &addr, sizeof (addr),
> +		bfd_offset);
> +  addr = netorder64 (addr);
> +
> +  record_full_mem_entry mem (addr, len);
> +
> +  /* Get val.  */
> +  bfdcore_read (cbfd, osec, mem.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 (),
> +			  mem.addr),
> +		(unsigned long) sizeof (addr),
> +		(unsigned long) sizeof (len),
> +		len);
> +
> +  return mem;
> +}
> +
> +void
> +record_full_entry::from_bfd (bfd *cbfd, asection *osec, int
> +*bfd_offset) {
> +  uint8_t rectype;
> +  record_full_entry rec;
> +
>    bfdcore_read (cbfd, osec, &rectype, sizeof (rectype), bfd_offset);
>    switch (rectype)
>      {
>      case record_full_reg: /* reg */
>        {
> -	/* Get register number to regnum.  */
> -	bfdcore_read (cbfd, osec, &regnum, sizeof (regnum), bfd_offset);
> -	regnum = netorder32 (regnum);
> -
> -	record_full_entry rec (record_full_reg, cache->arch (), regnum);
> -
> -	/* Get val.  */
> -	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.reg ().num,
> -		      (unsigned long) sizeof (regnum),
> -		      rec.reg ().len);
> -
> -	record_full_arch_list_add (rec);
> +	rec.entry = std::move (record_full_reg_entry::from_bfd
> +				(cbfd, osec, bfd_offset));
>  	break;
>        }
> 
>      case record_full_mem: /* mem */
>        {
> -	/* Get len.  */
> -	bfdcore_read (cbfd, osec, &len, sizeof (len), bfd_offset);
> -	len = netorder32 (len);
> -
> -	/* Get addr.  */
> -	bfdcore_read (cbfd, osec, &addr, sizeof (addr),
> -		      bfd_offset);
> -	addr = netorder64 (addr);
> -
> -	record_full_entry rec (record_full_mem, addr, len);
> -
> -	/* Get val.  */
> -	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.mem ().addr),
> -		      (unsigned long) sizeof (addr),
> -		      (unsigned long) sizeof (len),
> -		      len);
> -
> -	record_full_arch_list_add (rec);
> +	rec.entry = std::move (record_full_mem_entry::from_bfd
> +				(cbfd, osec, bfd_offset));
>  	break;
>        }
> 
> @@ -2279,6 +2335,39 @@ record_full_read_entry_from_bfd (bfd *cbfd,
> asection *osec, int *bfd_offset)
>  			    bfd_get_filename (cbfd)));
>        break;
>      }
> +  record_full_arch_list_add (rec);
> +}
> +
> +void
> +record_full_instruction::from_bfd (bfd *cbfd, asection *osec, int
> +*bfd_offset) {
> +  uint32_t eff_count = 0;
> +  uint8_t sigval;
> +  uint32_t insn_num;
> +
> +  /* 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);
> +
> +  record_full_incomplete_instruction.insn_num = netorder32 (insn_num);
> + if (sigval != GDB_SIGNAL_0)
> +    record_full_incomplete_instruction.sigval = (gdb_signal) sigval;
> +
> +  record_full_incomplete_instruction.pc
> +    = record_full_reg_entry::from_bfd (cbfd, osec, bfd_offset);
> +
> +  eff_count = netorder32 (eff_count);
> +
> +  /* This deals with all the side effects.  */ while (eff_count > 0)
> +    {
> +      eff_count--;
> +
> +      record_full_entry::from_bfd (cbfd, osec, bfd_offset);
> +    }
>  }
> 
>  /* Restore the execution log from core file CBFD.  */ @@ -2325,30 +2414,8
> @@ record_full_restore (struct bfd &cbfd)
>        while (bfd_offset < osec_size)
>  	{
>  	  record_full_reset_incomplete ();
> -	  uint32_t eff_count = 0;
> -	  uint8_t sigval;
> -	  uint32_t insn_num;
> -
> -	  /* 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);
> -
> -	  record_full_incomplete_instruction.insn_num = netorder32
> (insn_num);
> -	  if (sigval != GDB_SIGNAL_0)
> -	    record_full_incomplete_instruction.sigval = (gdb_signal) sigval;
> 
> -	  eff_count = netorder32 (eff_count);
> -
> -	  /* This deals with all the side effects.  */
> -	 while (eff_count > 0)
> -	   {
> -	     eff_count--;
> -
> -	     record_full_read_entry_from_bfd (&cbfd, osec, &bfd_offset);
> -	   }
> +	  record_full_instruction::from_bfd (&cbfd, osec, &bfd_offset);
> 
>  	  record_full_save_instruction ();
>  	}
> @@ -2404,71 +2471,109 @@ cmd_record_full_restore (const char *args, int
> from_tty)
>    record_full_open (nullptr, from_tty);  }
> 
> -static void
> -record_full_write_entry_to_bfd (record_full_entry &entry,
> -				gdb_bfd_ref_ptr obfd,
> -				asection *osec, int *bfd_offset,
> -				gdbarch *gdbarch)
> +void
> +record_full_reg_entry::to_bfd (gdb_bfd_ref_ptr obfd,
> +			       asection *osec, int *bfd_offset,
> +			       gdbarch *gdbarch)
> +{
> +  uint32_t regnum;
> +
> +  if (record_debug)
> +    gdb_printf (gdb_stdlog,
> +		"  Writing register %d (1 "
> +		"plus %lu plus %d bytes)\n",
> +		this->num,
> +		(unsigned long) sizeof (regnum),
> +		this->len);
> +
> +  /* Write regnum.  */
> +  regnum = netorder32 (this->num);
> +  bfdcore_write (obfd.get (), osec, &regnum, sizeof (regnum),
> + bfd_offset);
> +
> +  /* Write regval.  */
> +  bfdcore_write (obfd.get (), osec, this->get_loc (), this->len,
> +bfd_offset); }
> +
> +void
> +record_full_mem_entry::to_bfd (gdb_bfd_ref_ptr obfd,
> +			       asection *osec, int *bfd_offset,
> +			       gdbarch *gdbarch)
> +{
> +  uint32_t len;
> +  uint64_t addr;
> +
> +  if (record_debug)
> +    gdb_printf (gdb_stdlog,
> +		"  Writing memory %s (1 plus "
> +		"%lu plus %lu plus %d bytes)\n",
> +		paddress (gdbarch, this->addr),
> +		(unsigned long) sizeof (addr),
> +		(unsigned long) sizeof (len),
> +		this->len);
> +
> +  /* Write memlen.  */
> +  len = netorder32 (this->len);
> +  bfdcore_write (obfd.get (), osec, &len, sizeof (len), bfd_offset);
> +
> +  /* Write memaddr.  */
> +  addr = netorder64 (this->addr);
> +  bfdcore_write (obfd.get (), osec, &addr, sizeof (addr), bfd_offset);
> +
> +  /* Write memval.  */
> +  bfdcore_write (obfd.get (), osec, this->get_loc (), this->len,
> +bfd_offset); }
> +
> +void
> +record_full_entry::to_bfd (gdb_bfd_ref_ptr obfd, asection *osec,
> +			   int *bfd_offset, gdbarch *gdbarch)
>  {
>    /* Save entry.  */
>    uint8_t type;
> -  uint32_t regnum, len;
> -  uint64_t addr;
> 
> -  type = entry.type ();
> +  type = this->type ();
>    bfdcore_write (obfd.get (), osec, &type, sizeof (type), bfd_offset);
> 
>    switch (type)
>      {
>      case record_full_reg: /* reg */
> -      {
> -	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;
> -      }
> +      reg ().to_bfd (obfd, osec, bfd_offset, gdbarch);
> +      break;
> 
>      case record_full_mem: /* mem */
> -      {
> -	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;
> -      }
> +      mem ().to_bfd (obfd, osec, bfd_offset, gdbarch);
> +      break;
>      }
> 
>  }
> 
> +void
> +record_full_instruction::to_bfd (gdb_bfd_ref_ptr obfd, asection *osec,
> +				 int *bfd_offset, gdbarch *gdbarch) {
> +  uint32_t eff_count = (uint32_t) this->effects.size ();
> +  uint32_t insn_num = this->insn_num;
> +  uint8_t sigval = (this->sigval.has_value ())
> +		     ? this->sigval.value ()
> +		     : GDB_SIGNAL_0;
> +
> +  /* Signal.  */
> +  bfdcore_write (obfd.get (), osec, &sigval, sizeof (sigval),
> +bfd_offset);
> +  eff_count = netorder32 (eff_count);
> +  /* Number of effects.  */
> +  bfdcore_write (obfd.get (), osec, &eff_count, sizeof (eff_count),
> +		 bfd_offset);
> +  /* Instruction number.  */
> +  bfdcore_write (obfd.get (), osec, &insn_num, sizeof (uint32_t),
> +		 bfd_offset);
> +
> +  this->pc.to_bfd (obfd, osec, bfd_offset, gdbarch);
> +
> +  for (auto &entry : this->effects)
> +    {
> +      entry.to_bfd (obfd, osec, bfd_offset, gdbarch);
> +    }
> +}
> +
>  /* Save the execution log to a file.  We use a modified elf corefile
>     format, with an extra section for our data.  */
> 
> @@ -2510,6 +2615,7 @@ 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);
> +      save_size += 4 + record_full_list[i].pc.len;
>        for (auto &entry : record_full_list[i].effects)
>  	switch (entry.type ())
>  	  {
> @@ -2551,27 +2657,7 @@ record_full_base_target::save_record (const char
> *recfilename)
>       record list.  */
>    for (int i = 0; i < record_full_list.size (); i++)
>      {
> -      uint32_t eff_count = (uint32_t) record_full_list[i].effects.size ();
> -      uint32_t insn_num = record_full_list[i].insn_num;
> -      uint8_t sigval = (record_full_list[i].sigval.has_value ())
> -			 ? record_full_list[i].sigval.value ()
> -			 : GDB_SIGNAL_0;
> -
> -      /* Signal.  */
> -      bfdcore_write (obfd.get (), osec, &sigval, sizeof (sigval), &bfd_offset);
> -      eff_count = netorder32 (eff_count);
> -      /* Number of effects.  */
> -      bfdcore_write (obfd.get (), osec, &eff_count, sizeof (eff_count),
> -		     &bfd_offset);
> -      /* Instruction number.  */
> -      bfdcore_write (obfd.get (), osec, &insn_num, sizeof (uint32_t),
> -		     &bfd_offset);
> -
> -      for (auto &entry : record_full_list[i].effects)
> -	{
> -	  record_full_write_entry_to_bfd (entry, obfd, osec, &bfd_offset,
> -					  gdbarch);
> -	}
> +      record_full_list[i].to_bfd (obfd, osec, &bfd_offset, gdbarch);
>        /* Execute entry.  */
>        if (i < record_full_next_insn)
>  	record_full_list[i].exec_insn (regcache); @@ -2648,6 +2734,9 @@
> maintenance_print_record_instruction (const char *args, int from_tty)
>    auto to_print = record_full_list.begin () + offset;
> 
>    gdbarch *arch = current_inferior ()->arch ();
> +  struct value_print_options opts;
> +  get_user_print_options (&opts);
> +  opts.raw = true;
> 
>    for (auto &entry : to_print->effects)
>      {
> @@ -2661,9 +2750,6 @@ maintenance_print_record_instruction (const char
> *args, int from_tty)
>  					 entry.get_loc ());
>  	      gdb_printf ("Register %s changed: ",
>  			  gdbarch_register_name (arch, entry.reg ().num));
> -	      struct value_print_options opts;
> -	      get_user_print_options (&opts);
> -	      opts.raw = true;
>  	      value_print (val, gdb_stdout, &opts);
>  	      gdb_printf ("\n");
>  	      break;
> @@ -2682,6 +2768,12 @@ maintenance_print_record_instruction (const
> char *args, int from_tty)
>  	    }
>  	}
>      }
> +  type *regtype = gdbarch_register_type (arch, to_print->pc.num);
> +  value *val = value_from_contents (regtype, to_print->pc.get_loc ());
> +  gdb_printf ("Register %s changed: ",
> +	      gdbarch_register_name (arch, to_print->pc.num));
> +  value_print (val, gdb_stdout, &opts);
> +  gdb_printf ("\n");
>  }
> 
>  INIT_GDB_FILE (record_full)
> --
> 2.54.0
> 

Besides those nits, LGTM:

Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>

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