Re: [PATCH 2/2] S390: Fix gdbserver support for TDB

[Pedro Alves <palves@redhat.com>]

On 12/01/2014 06:15 PM, Andreas Arnez wrote:

> But after a quick grep through the Linux kernel it seems that there are
> currently only two regsets for which ENODATA can be returned: the TDB on
> S390 and the iWMMXt state on ARM.

I've pushed the PowerPC transactional memory ptrace support toward modeling
from s390, and the current patches (iterating under review) will return
ENODATA too, but for the exact same scenario, so sounds like we'll be
good there too.

> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 8b72523..42dd521 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -199,6 +199,13 @@ arm_store_wmmxregset (struct regcache *regcache, const void *buf)
>    if (!(arm_hwcap & HWCAP_IWMMXT))
>      return;

What are the conditions the ARM kernel checks to return
ENODATA for this regset?  I'd assume that it'd be the case of
the machine not really supporting iwmmxt, and thus the check
above already catching this.

>  
> +  if (buf == NULL)
> +    {
> +      for (i = 0; i < 22; i++)
> +	supply_register (regcache, arm_num_regs + i, NULL);
> +      return;
> +    }
> +

It probably doesn't hurt to be explicit, but I should note that
registers' are unavailable by default on gdbserver, so a
'if (buf == NULL) return;' probably would do as well:

struct regcache *
init_register_cache (struct regcache *regcache,
		     const struct target_desc *tdesc,
		     unsigned char *regbuf)
...
      regcache->register_status = xcalloc (1, tdesc->num_registers);
      gdb_assert (REG_UNAVAILABLE == 0);


More importantly:

> @@ -4300,7 +4308,8 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
>        void *buf, *data;
>        int nt_type, res;
>
> -      if (regset->size == 0 || regset_disabled (regsets_info, regset))
> +      if (regset->size == 0 || regset_disabled (regsets_info, regset)
> +	  || regset->fill_function == NULL)
>  	{
>  	  regset ++;
>  	  continue;

Doesn't this mean a write attempt to such a read-only regset "succeeds"
silently instead of erroring?

Does the test exercise this?  How does GDB/native behave?

> @@ -316,13 +310,32 @@ s390_store_system_call (struct regcache *regcache, const void *buf)
>    supply_register_by_name (regcache, "system_call", buf);
>  }
>  
> +static void
> +s390_store_tdb (struct regcache *regcache, const void *buf)
> +{
> +  int tdb0 = find_regno (regcache->tdesc, "tdb0");
> +  int tr0 = find_regno (regcache->tdesc, "tr0");
> +  int i;
> +
> +  for (i = 0; i < 4; i++)
> +    supply_register (regcache, tdb0 + i,
> +		     buf ? (const char *) buf + 8 * i : NULL);

'buf != NULL ? (const...'

> +
> +  for (i = 0; i < 16; i++)
> +    supply_register (regcache, tr0 + i,
> +		     buf ? (const char *) buf + 8 * (16 + i) : NULL);
> +}

'buf != NULL ? (const...'

Thanks,
Pedro Alves