This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] S390: Fix gdbserver support for TDB
- From: Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Ulrich Weigand <uweigand at de dot ibm dot com>
- Date: Tue, 02 Dec 2014 20:18:10 +0100
- Subject: Re: [PATCH 2/2] S390: Fix gdbserver support for TDB
- Authentication-results: sourceware.org; auth=none
- References: <1417002962-3424-1-git-send-email-arnez at linux dot vnet dot ibm dot com> <1417002962-3424-3-git-send-email-arnez at linux dot vnet dot ibm dot com> <87iohvp77u dot fsf at br87z6lw dot de dot ibm dot com> <547DD8C2 dot 9000403 at redhat 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.