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

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

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.