[PATCH 5/6] gdb/record: extract the PC to record_full_instruction

[Guinevere Larsen <guinevere@redhat.com>]

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, removing the parts specific to mem and
reg entries, to avoid obscure code paths or writing the code to do the
same thing again.
---
 gdb/record-full.c | 268 ++++++++++++++++++++++++++++------------------
 1 file changed, 165 insertions(+), 103 deletions(-)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index c9e66fac578..297b5b76ae7 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -250,6 +250,16 @@ class record_full_entry
     gdb_assert (mem_type == record_full_mem);
   }
 
+  void set_entry (record_full_reg_entry reg)
+  {
+    entry = reg;
+  }
+
+  void set_entry (record_full_mem_entry mem)
+  {
+    entry = mem;
+  }
+
   record_full_reg_entry& reg ()
   {
     gdb_assert (type () == record_full_reg);
@@ -325,6 +335,7 @@ 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.  */
@@ -684,7 +695,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 = rec.reg ();
+  else
+    record_full_arch_list_add (rec);
 
   return 0;
 }
@@ -841,6 +855,7 @@ static enum target_stop_reason record_full_stop_reason
 
 void record_full_instruction::exec_insn (regcache *regcache, gdbarch *gdbarch)
 {
+  pc.execute (regcache, gdbarch);
   for (auto &entry : effects)
     if (entry.execute (regcache, gdbarch))
       record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
@@ -2184,13 +2199,73 @@ netorder32 (uint32_t input)
   return ret;
 }
 
+static record_full_reg_entry
+record_full_read_reg_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
+{
+  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;
+
+}
+
+static record_full_mem_entry
+record_full_read_mem_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;
+}
+
 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 ());
+  record_full_entry rec;
 
   bfdcore_read (cbfd, osec, &rectype, sizeof (rectype),
 		bfd_offset);
@@ -2198,57 +2273,13 @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
     {
     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.set_entry (record_full_read_reg_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.set_entry (record_full_read_mem_from_bfd (cbfd, osec, bfd_offset));
 	break;
       }
 
@@ -2259,6 +2290,7 @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset)
 			    bfd_get_filename (cbfd)));
       break;
     }
+  record_full_arch_list_add (rec);
 }
 
 /* Restore the execution log from core file CBFD.  */
@@ -2323,6 +2355,9 @@ record_full_restore (struct bfd &cbfd)
 	  if (sigval != GDB_SIGNAL_0)
 	    record_full_incomplete_instruction.sigval = (gdb_signal) sigval;
 
+	  record_full_incomplete_instruction.pc
+	    = record_full_read_reg_from_bfd (&cbfd, osec, &bfd_offset);
+
 	  eff_count = netorder32 (eff_count);
 
 	  /* This deals with all the side effects.  */
@@ -2387,6 +2422,67 @@ cmd_record_full_restore (const char *args, int from_tty)
   record_full_open (nullptr, from_tty);
 }
 
+static void
+record_full_write_reg_to_bfd (record_full_reg_entry &reg,
+			      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",
+		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,
+		 reg.get_loc (),
+		 reg.len, bfd_offset);
+}
+
+static void
+record_full_write_mem_to_bfd (record_full_mem_entry &mem,
+			      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, 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,
+		 mem.get_loc (),
+		 mem.len, bfd_offset);
+}
+
 static void
 record_full_write_entry_to_bfd (record_full_entry &entry,
 				gdb_bfd_ref_ptr obfd,
@@ -2395,8 +2491,6 @@ record_full_write_entry_to_bfd (record_full_entry &entry,
 {
   /* Save entry.  */
   uint8_t type;
-  uint32_t regnum, len;
-  uint64_t addr;
 
   type = entry.type ();
   bfdcore_write (obfd.get (), osec, &type, sizeof (type), bfd_offset);
@@ -2404,56 +2498,14 @@ record_full_write_entry_to_bfd (record_full_entry &entry,
   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;
-      }
+      record_full_write_reg_to_bfd (entry.reg (), 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;
-      }
+      record_full_write_mem_to_bfd (entry.mem (), obfd, osec,
+				    bfd_offset, gdbarch);
+      break;
     }
 
 }
@@ -2499,6 +2551,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 ())
 	  {
@@ -2557,6 +2610,9 @@ record_full_base_target::save_record (const char *recfilename)
       bfdcore_write (obfd.get (), osec, &insn_num,
 		     sizeof (uint32_t), &bfd_offset);
 
+      record_full_write_reg_to_bfd (record_full_list[i].pc, obfd, osec,
+				    &bfd_offset, gdbarch);
+
       for (auto &entry : record_full_list[i].effects)
 	{
 	  record_full_write_entry_to_bfd (entry, obfd, osec, &bfd_offset,
@@ -2639,6 +2695,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)
     {
@@ -2652,9 +2711,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;
@@ -2673,6 +2729,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.53.0