From: Andreas Arnez <arnez@linux.vnet.ibm.com>
To: Pedro Alves <palves@redhat.com>
Cc: gdb-patches@sourceware.org, Ulrich Weigand <uweigand@de.ibm.com>
Subject: Re: [PATCH 2/2] S390: Fix gdbserver support for TDB
Date: Tue, 02 Dec 2014 19:18:00 -0000 [thread overview]
Message-ID: <8761dtq2rx.fsf@br87z6lw.de.ibm.com> (raw)
In-Reply-To: <547DD8C2.9000403@redhat.com> (Pedro Alves's message of "Tue, 02 Dec 2014 15:20:34 +0000")
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.
next prev parent reply other threads:[~2014-12-02 19:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-26 11:56 [PATCH 0/2] S390: Fixes for gdbserver on targets with TDB Andreas Arnez
2014-11-26 11:56 ` [PATCH 2/2] S390: Fix gdbserver support for TDB Andreas Arnez
2014-12-01 18:15 ` Andreas Arnez
2014-12-01 18:40 ` Ulrich Weigand
2014-12-02 15:21 ` Pedro Alves
2014-12-02 19:18 ` Andreas Arnez [this message]
2014-12-03 18:18 ` Andreas Arnez
2014-12-04 13:33 ` Pedro Alves
2014-12-09 16:10 ` Andreas Arnez
2014-12-04 13:39 ` Pedro Alves
2014-11-26 11:56 ` [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 Andreas Arnez
2014-12-01 18:34 ` Ulrich Weigand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8761dtq2rx.fsf@br87z6lw.de.ibm.com \
--to=arnez@linux.vnet.ibm.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
--cc=uweigand@de.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).