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

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

Guinevere Larsen <guinevere@redhat.com> writes:

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

Maybe mention pc in the doc comment above struct
record_full_instruction?

>  
>    /* 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;
> +}
> +

As a suggestion to further C++fy this code (feel free to adopt them or
not), you could make the functions above constructors of
record_full_reg_entry and record_full_mem_entry or static methods
record_full_reg_entry::from_bfd and record_full_mem_entry::from_bfd

Admittedly it wouldn't be a big difference, just slightly better code
organization.

>  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);

Same here, record_full_entry::from_bfd. Except for the call to
record_full_arch_list_add, which would then be moved to the caller.

> @@ -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);
> +}
> +

These could be write_to_bfd methods in record_full_reg_entry and
record_full_mem_entry.

>  static void
>  record_full_write_entry_to_bfd (record_full_entry &entry,
>  				gdb_bfd_ref_ptr obfd,

And this could be a non-virtual method in record_full_entry ...

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

... which would do this part and then call the write_to_bfd virtual
method instead of the switch below.

> @@ -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;
>      }
>  
>  }

-- 
Thiago