Re: [patch v4 11/24] record-btrace: supply register target methods

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Wed, 03 Jul 2013 11:14:21 +0200, Markus Metzger wrote:
> Supply target methods to allow reading the PC.  Forbid anything else.
> 
> 2013-07-03  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* record-btrace.c (record_btrace_fetch_registers,
> 	record_btrace_store_registers,
> 	record_btrace_to_prepare_to_store): New.
> 	(init_record_btrace_ops): Add the above.
> 
> 
> ---
>  gdb/record-btrace.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 95 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 5e41b20..e9c0801 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -32,6 +32,7 @@
>  #include "ui-out.h"
>  #include "symtab.h"
>  #include "filenames.h"
> +#include "regcache.h"
>  
>  /* The target_ops of record-btrace.  */
>  static struct target_ops record_btrace_ops;
> @@ -752,6 +753,97 @@ record_btrace_is_replaying (void)
>    return 0;
>  }
>  
> +/* The to_fetch_registers method of target record-btrace.  */
> +
> +static void
> +record_btrace_fetch_registers (struct target_ops *ops,
> +			       struct regcache *regcache, int regno)
> +{
> +  struct btrace_insn_iterator *replay;
> +  struct thread_info *tp;
> +
> +  tp = find_thread_ptid (inferior_ptid);
> +  if (tp == NULL)
> +    return;

Are you aware when it can happen?  If not then:
  gdb_assert (tp != NULL);


> +
> +  replay = tp->btrace.replay;
> +  if (replay != NULL)
> +    {
> +      const struct btrace_insn *insn;
> +      struct gdbarch *gdbarch;
> +      int pcreg;
> +
> +      gdbarch = get_regcache_arch (regcache);
> +      pcreg = gdbarch_pc_regnum (gdbarch);
> +      if (pcreg < 0)
> +	return;
> +
> +      /* We can only provide the PC register.  */
> +      if (regno >= 0 && regno != pcreg)
> +	return;
> +
> +      insn = btrace_insn_get (replay);
> +      if (insn == NULL)
> +	return;

Shouldn't here be rather an error?

> +
> +      regcache_raw_supply (regcache, regno, &insn->pc);
> +    }
> +  else
> +    {
> +      struct target_ops *t;
> +
> +      for (t = ops->beneath; t != NULL; t = t->beneath)
> +	if (t->to_fetch_registers != NULL)
> +	  {
> +	    t->to_fetch_registers (t, regcache, regno);
> +	    break;
> +	  }
> +    }
> +}
> +
> +/* The to_store_registers method of target record-btrace.  */
> +
> +static void
> +record_btrace_store_registers (struct target_ops *ops,
> +			       struct regcache *regcache, int regno)
> +{
> +  struct target_ops *t;
> +
> +  if (record_btrace_is_replaying ())
> +    return;

Currently I get:
	(gdb) p $rax
	$1 = <unavailable>
	(gdb) p $rax=1
	$2 = <unavailable>

I would find more appropriate an error() here so that we get:
	(gdb) p $rax
	$1 = <unavailable>
	(gdb) p $rax=1
	Some error message.

With gdbserver trace one gets:
	(gdb) print globalc
	$1 = <unavailable>
	(gdb) print globalc=1
	Cannot access memory at address 0x602120
which is not so convenient as it comes from gdbserver E01 response:
gdb_write_memory -> if (current_traceframe >= 0) return EIO;
as I checked.


> +
> +  if (may_write_registers == 0)
> +    error (_("Writing to registers is not allowed (regno %d)"), regno);

Here should be rather:
  gdb_assert (may_write_registers == 0);

as target_store_registers() would not pass the call here otherwise.


> +
> +  for (t = ops->beneath; t != NULL; t = t->beneath)
> +    if (t->to_store_registers != NULL)
> +      {
> +	t->to_store_registers (t, regcache, regno);
> +	return;
> +      }
> +
> +  noprocess ();
> +}
> +
> +/* The to_prepare_to_store method of target record-btrace.  */
> +
> +static void
> +record_btrace_prepare_to_store (struct target_ops *ops,
> +				struct regcache *regcache)
> +{
> +  struct target_ops *t;
> +
> +  if (record_btrace_is_replaying ())
> +    return;
> +
> +  for (t = ops->beneath; t != NULL; t = t->beneath)
> +    if (t->to_prepare_to_store != NULL)
> +      {
> +	t->to_prepare_to_store (t, regcache);
> +	return;
> +      }
> +}
> +
>  /* Initialize the record-btrace target ops.  */
>  
>  static void
> @@ -779,6 +871,9 @@ init_record_btrace_ops (void)
>    ops->to_call_history_from = record_btrace_call_history_from;
>    ops->to_call_history_range = record_btrace_call_history_range;
>    ops->to_record_is_replaying = record_btrace_is_replaying;
> +  ops->to_fetch_registers = record_btrace_fetch_registers;
> +  ops->to_store_registers = record_btrace_store_registers;
> +  ops->to_prepare_to_store = record_btrace_prepare_to_store;
>    ops->to_stratum = record_stratum;
>    ops->to_magic = OPS_MAGIC;
>  }
> -- 
> 1.7.1