Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves <palves@redhat.com>]

On 02/23/2016 07:34 PM, Antoine Tremblay wrote:
> 
> Pedro Alves writes:
> 
>>> +
>>> +  return double_regnum;
>>> +}
>>> +
>>> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
>>> +
>>> +static int
>>> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>>> +				struct agent_expr *ax, int reg)
>>> +{
>>> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
>>> +
>>> +  /* Error.  */
>>> +  if (rawnum < 0)
>>> +    return 1;
>>> +
>>> +  ax_reg_mask (ax, rawnum);
>>
>> Hmm, seems to me that gdb raw -> target raw mapping should be
>> either here, or perhaps even in ax_reg / ax_reg_mask?
>>
> 
> After more investigation, this can't be in ax_reg / ax_reg_mask for
> pseudo registers as this function is solely reponsible to encode the
> right number here.

I don't follow.

ax_reg / ax_reg_mask today obviously work with gdb numbers:

/* Add register REG to the register mask for expression AX.  */
void
ax_reg_mask (struct agent_expr *ax, int reg)
{
  if (reg >= gdbarch_num_regs (ax->gdbarch))
    {
      /* This is a pseudo-register.  */
      if (!gdbarch_ax_pseudo_register_collect_p (ax->gdbarch))
	error (_("'%s' is a pseudo-register; "
		 "GDB cannot yet trace its contents."),
	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
      if (gdbarch_ax_pseudo_register_collect (ax->gdbarch, ax, reg))
	error (_("Trace '%s' failed."),
	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
    }
  else
    ...


This is comparing gdb-side num_regs, and calling
gdbarch_ax_pseudo_register_collect, whose implementations expect
gdb register numbers.  And it calls user_reg_map_regnum_to_name,
which works with gdb register numbers.  Etc.

So it seems to me that we need to make ax_reg and ax_reg_mask
convert gdb -> remote numbers in their else branches.

> 
>> Consider the case of an expression requiring the collection of
>> a _raw_ register, thus not even reaching here.  Looking at
>> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
>> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
>> is computing actually wrong for the target, and things just happen
>> to work because gdbserver ignores them and always collects all registers?
> 
> However yes it should be in ax_reg/ax_reg_mask for non-pseudo registers,
> but this is not the objective of this patch, I suggest that such a
> change be the subject of another patch

Sure, but in that case, drop the gdb -> remote conversion entirely.
If with that things don't work for arm, let's fix ax_reg/ax_reg_mask
_first_.

> maybe coupled with better gdbserver handling of the R action.

I think this coupling would be a mistake.  This can be handled
independently, if at all.

> 
> I will send a v5 with the ax_pseudo_register_collect inside the
> arm_ax_pseudo_register_collect/arm_ax_pseudo_register_push stack function.

Thanks,
Pedro Alves