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

  On Wed, Nov 26 2014, Andreas Arnez wrote:

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 01f11b7..5d1d9e1 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4263,6 +4263,14 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
>  	      free (buf);
>  	      continue;
>  	    }
> +	  else if (errno == ENODATA)
> +	    {
> +	      /* ENODATA may be returned if the regset is currently
> +		 not "active".  Provide a zeroed buffer and assume
> +		 that the store_function deals with this
> +		 appropriately.  */
> +	      memset (buf, 0, regset->size);
> +	    }

Well, providing a zeroed buffer may not actually the best thing to do
here.  It is just coincidence that the regset I targeted this for (the
TDB on S390) can be recognized as invalid if the buffer contains all
zeroes.  What really should be done is to enrich the store_function
interface such that "no data" can be distinguished from "zero data".
The reason I wrote it this way is to avoid impacting other platforms.
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.

Here's an updated version of the patch.  Instead of clearing the buffer,
this version passes a NULL pointer to the store function.  And the store
function for iWMMXt is adjusted accordingly.  Note that I've tested this
on S390, but not on ARM.


-- >8 --
Subject: [PATCH v2] S390: Fix gdbserver support for TDB

This makes gdbserver actually provide values for the TDB registers
when the inferior was stopped in a transaction.  The changes in
linux-low.c are needed for treating an unavailable TDB correctly and
without warnings.  In particular, ENODATA from ptrace is now handled
by passing a NULL pointer to the store function.  Since this may also
occur on ARM for the iWMMXt state, the patch ensures that the
respective store function can handle that.

The test case 's390-tdbregs.exp' passes with this patch and fails
without.

gdb/gdbserver/ChangeLog:

	* linux-low.c (regsets_fetch_inferior_registers): Upon ENODATA
	from ptrace, pass a NULL pointer to the store function.
	(regsets_store_inferior_registers): Skip regsets without a
	fill_function.
	* linux-arm-low.c (arm_store_wmmxregset): Accept a NULL pointer as
	buffer and then mark the registers as unavailable.
	* linux-s390-low.c (s390_fill_last_break): Remove.
	(s390_store_tdb): New.
	(s390_regsets): Set fill_function to NULL for NT_S390_LAST_BREAK.
	Add regset for NT_S390_TDB.
	(s390_arch_setup): Use regset's size instead of fill_function for
	loop end condition.
---
 gdb/gdbserver/linux-arm-low.c  |  7 +++++++
 gdb/gdbserver/linux-low.c      | 11 ++++++++++-
 gdb/gdbserver/linux-s390-low.c | 33 +++++++++++++++++++++++----------
 3 files changed, 40 insertions(+), 11 deletions(-)
  

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
  

On Tue, Dec 02 2014, Pedro Alves wrote:

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

Someone with more ARM background should probably answer this, but
according to elf_set_personality in arch/arm/kernel/elf.c it seems that
ENODATA is also returned if the binary is neither EABI nor softfloat,
even if HWCAP_IWMXXT is set.

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

In general, if a prior call to fetch_inferior_registers has filled the
regset already, I'd expect the store function to reset the registers to
"unavailable" again.  Otherwise we may have stale left-overs from
before.

In this particular case this situation can probably only occur if the
ELF personality is changed from EABI/softfloat to non-EABI/softfloat,
e.g. by an "execve".  Again, someone with more ARM knowledge may jump in
here.

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

Good point.  Both gdbserver and GDB silently "succeed".  I did not
consider this a significant issue so far...  Any suggestions how to
improve this behavior?

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

OK, I'll change that.

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

OK.
  

On 12/02/2014 07:18 PM, Andreas Arnez wrote:
> On Tue, Dec 02 2014, Pedro Alves wrote:
> 
>> 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.
> 
> Someone with more ARM background should probably answer this, but
> according to elf_set_personality in arch/arm/kernel/elf.c it seems that
> ENODATA is also returned if the binary is neither EABI nor softfloat,
> even if HWCAP_IWMXXT is set.

Looks like you're right.


>> 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?
> 
> Good point.  Both gdbserver and GDB silently "succeed".  I did not
> consider this a significant issue so far...  Any suggestions how to
> improve this behavior?

Hmm, thinking further, I think the main issue is that gdbserver
may get a single G packet to write the whole register buffer
to the target, and it'll get that whether the user explicitly
tried to change one of the read-only registers, or any other regular
register.  I think we'd have to augment the G packet with a way that
says "these values are don't care, I'm not actually writing
to these registers".  The read/'g' packet uses 'x' for unavailable
registers, maybe we could reuse that for writes/'G', something like
"G 00112233xxxx0022", meaning the registers at the offsets where we
have 'x's shouldn't be written.

Anyway, if GDB behaves the same, let's not worry about this now.

Thanks,
Pedro Alves