* [PATCH 0/2] S390: Fixes for gdbserver on targets with TDB @ 2014-11-26 11:56 Andreas Arnez 2014-11-26 11:56 ` [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 Andreas Arnez 2014-11-26 11:56 ` [PATCH 2/2] S390: Fix gdbserver support for TDB Andreas Arnez 0 siblings, 2 replies; 12+ messages in thread From: Andreas Arnez @ 2014-11-26 11:56 UTC (permalink / raw) To: gdb-patches; +Cc: Ulrich Weigand On S390 targets with the transactional execution feature gdbserver has some problems: It terminates early with a 31-bit inferior, and the transaction diagnostic block (TDB) is always indicated as unavailable, even if the inferior was stopped in a transaction. These two patches fix these issues. OK to apply? Andreas Arnez (2): S390: Fix 'expedite' for s390-te-linux64 S390: Fix gdbserver support for TDB gdb/features/Makefile | 2 +- gdb/gdbserver/linux-low.c | 11 ++++++++- gdb/gdbserver/linux-s390-low.c | 45 ++++++++++++++++++++++++++++-------- gdb/regformats/s390-te-linux64.dat | 2 +- 4 files changed, 47 insertions(+), 13 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 2014-11-26 11:56 [PATCH 0/2] S390: Fixes for gdbserver on targets with TDB Andreas Arnez @ 2014-11-26 11:56 ` Andreas Arnez 2014-12-01 18:34 ` Ulrich Weigand 2014-11-26 11:56 ` [PATCH 2/2] S390: Fix gdbserver support for TDB Andreas Arnez 1 sibling, 1 reply; 12+ messages in thread From: Andreas Arnez @ 2014-11-26 11:56 UTC (permalink / raw) To: gdb-patches; +Cc: Ulrich Weigand Fix a typo in the expedited registers for s390-te-linux64. gdb/ChangeLog: * features/Makefile (s390-te-linux64-expedite): Replace non-existant r14 and r15 by r14l and r15l, respectively. * regformats/s390-te-linux64.dat: Regenerate. --- gdb/features/Makefile | 2 +- gdb/regformats/s390-te-linux64.dat | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/features/Makefile b/gdb/features/Makefile index 7708f5b..dfdb7c9 100644 --- a/gdb/features/Makefile +++ b/gdb/features/Makefile @@ -114,7 +114,7 @@ s390-linux32v2-expedite = r14,r15,pswa s390-linux64-expedite = r14l,r15l,pswa s390-linux64v1-expedite = r14l,r15l,pswa s390-linux64v2-expedite = r14l,r15l,pswa -s390-te-linux64-expedite = r14,r15,pswa +s390-te-linux64-expedite = r14l,r15l,pswa s390x-linux64-expedite = r14,r15,pswa s390x-linux64v1-expedite = r14,r15,pswa s390x-linux64v2-expedite = r14,r15,pswa diff --git a/gdb/regformats/s390-te-linux64.dat b/gdb/regformats/s390-te-linux64.dat index 8910543..28e1b87 100644 --- a/gdb/regformats/s390-te-linux64.dat +++ b/gdb/regformats/s390-te-linux64.dat @@ -2,7 +2,7 @@ # Generated from: s390-te-linux64.xml name:s390_te_linux64 xmltarget:s390-te-linux64.xml -expedite:r14,r15,pswa +expedite:r14l,r15l,pswa 32:pswm 32:pswa 32:r0h -- 1.7.9.5 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 2014-11-26 11:56 ` [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 Andreas Arnez @ 2014-12-01 18:34 ` Ulrich Weigand 0 siblings, 0 replies; 12+ messages in thread From: Ulrich Weigand @ 2014-12-01 18:34 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches Andreas Arnez wrote: > * features/Makefile (s390-te-linux64-expedite): Replace > non-existant r14 and r15 by r14l and r15l, respectively. > * regformats/s390-te-linux64.dat: Regenerate. This is OK. Thanks, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-11-26 11:56 [PATCH 0/2] S390: Fixes for gdbserver on targets with TDB Andreas Arnez 2014-11-26 11:56 ` [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 Andreas Arnez @ 2014-11-26 11:56 ` Andreas Arnez 2014-12-01 18:15 ` Andreas Arnez 1 sibling, 1 reply; 12+ messages in thread From: Andreas Arnez @ 2014-11-26 11:56 UTC (permalink / raw) To: gdb-patches; +Cc: Ulrich Weigand 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. The test case 's390-tdbregs.exp' passes with this patch and fails without. gdb/gdbserver/ChangeLog: * linux-low.c (regsets_fetch_inferior_registers): Treat ENODATA from ptrace as if a zeroed buffer was returned. (regsets_store_inferior_registers): Skip regsets without a fill_function. * 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-low.c | 11 +++++++++- gdb/gdbserver/linux-s390-low.c | 45 +++++++++++++++++++++++++++++++--------- 2 files changed, 45 insertions(+), 11 deletions(-) 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); + } else { char s[256]; @@ -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; diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c index 79fa6c0..4b547aa 100644 --- a/gdb/gdbserver/linux-s390-low.c +++ b/gdb/gdbserver/linux-s390-low.c @@ -290,12 +290,6 @@ s390_fill_gregset (struct regcache *regcache, void *buf) /* Fill and store functions for extended register sets. */ static void -s390_fill_last_break (struct regcache *regcache, void *buf) -{ - /* Last break address is read-only. */ -} - -static void s390_store_last_break (struct regcache *regcache, const void *buf) { const char *p; @@ -316,13 +310,44 @@ 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; + + if (*(const unsigned char *) buf != 0x01) + { + /* Invalid/unrecognized TDB format. Invalidate. */ + for (i = 0; i < 4; i++) + supply_register (regcache, tdb0 + i, NULL); + + for (i = 0; i < 16; i++) + supply_register (regcache, tr0 + i, NULL); + + return; + } + + for (i = 0; i < 4; i++) + supply_register (regcache, tdb0 + i, + (const char *) buf + 8 * i); + + for (i = 0; i < 16; i++) + supply_register (regcache, tr0 + i, + (const char *) buf + 8 * (16 + i)); +} + static struct regset_info s390_regsets[] = { { 0, 0, 0, 0, GENERAL_REGS, s390_fill_gregset, NULL }, - /* Last break address is read-only; do not attempt PTRACE_SETREGSET. */ - { PTRACE_GETREGSET, PTRACE_GETREGSET, NT_S390_LAST_BREAK, 0, - EXTENDED_REGS, s390_fill_last_break, s390_store_last_break }, + /* Last break address is read-only; no fill function. */ + { PTRACE_GETREGSET, -1, NT_S390_LAST_BREAK, 0, EXTENDED_REGS, + NULL, s390_store_last_break }, { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_S390_SYSTEM_CALL, 0, EXTENDED_REGS, s390_fill_system_call, s390_store_system_call }, + /* TDB is read-only. */ + { PTRACE_GETREGSET, -1, NT_S390_TDB, 0, EXTENDED_REGS, + NULL, s390_store_tdb }, { 0, 0, 0, -1, -1, NULL, NULL } }; @@ -485,7 +510,7 @@ s390_arch_setup (void) #endif /* Update target_regsets according to available register sets. */ - for (regset = s390_regsets; regset->fill_function != NULL; regset++) + for (regset = s390_regsets; regset->size >= 0; regset++) if (regset->get_request == PTRACE_GETREGSET) switch (regset->nt_type) { -- 1.7.9.5 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 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 0 siblings, 2 replies; 12+ messages in thread From: Andreas Arnez @ 2014-12-01 18:15 UTC (permalink / raw) To: gdb-patches; +Cc: Ulrich Weigand 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(-) 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; + if (buf == NULL) + { + for (i = 0; i < 22; i++) + supply_register (regcache, arm_num_regs + i, NULL); + return; + } + for (i = 0; i < 16; i++) supply_register (regcache, arm_num_regs + i, (char *) buf + i * 8); diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 01f11b7..062140d 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) + { + /* The regset is currently not "active". For regsets + where this can occur, store_function must be prepared + to deal with a NULL pointer. */ + free (buf); + buf = NULL; + } else { char s[256]; @@ -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; diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c index 79fa6c0..2cb5a73 100644 --- a/gdb/gdbserver/linux-s390-low.c +++ b/gdb/gdbserver/linux-s390-low.c @@ -290,12 +290,6 @@ s390_fill_gregset (struct regcache *regcache, void *buf) /* Fill and store functions for extended register sets. */ static void -s390_fill_last_break (struct regcache *regcache, void *buf) -{ - /* Last break address is read-only. */ -} - -static void s390_store_last_break (struct regcache *regcache, const void *buf) { const char *p; @@ -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); + + for (i = 0; i < 16; i++) + supply_register (regcache, tr0 + i, + buf ? (const char *) buf + 8 * (16 + i) : NULL); +} + static struct regset_info s390_regsets[] = { { 0, 0, 0, 0, GENERAL_REGS, s390_fill_gregset, NULL }, - /* Last break address is read-only; do not attempt PTRACE_SETREGSET. */ - { PTRACE_GETREGSET, PTRACE_GETREGSET, NT_S390_LAST_BREAK, 0, - EXTENDED_REGS, s390_fill_last_break, s390_store_last_break }, + /* Last break address is read-only; no fill function. */ + { PTRACE_GETREGSET, -1, NT_S390_LAST_BREAK, 0, EXTENDED_REGS, + NULL, s390_store_last_break }, { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_S390_SYSTEM_CALL, 0, EXTENDED_REGS, s390_fill_system_call, s390_store_system_call }, + /* TDB is read-only. */ + { PTRACE_GETREGSET, -1, NT_S390_TDB, 0, EXTENDED_REGS, + NULL, s390_store_tdb }, { 0, 0, 0, -1, -1, NULL, NULL } }; @@ -485,7 +498,7 @@ s390_arch_setup (void) #endif /* Update target_regsets according to available register sets. */ - for (regset = s390_regsets; regset->fill_function != NULL; regset++) + for (regset = s390_regsets; regset->size >= 0; regset++) if (regset->get_request == PTRACE_GETREGSET) switch (regset->nt_type) { -- 1.8.4.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-01 18:15 ` Andreas Arnez @ 2014-12-01 18:40 ` Ulrich Weigand 2014-12-02 15:21 ` Pedro Alves 1 sibling, 0 replies; 12+ messages in thread From: Ulrich Weigand @ 2014-12-01 18:40 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches Andreas Arnez wrote: > 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. I agree that this version is preferable to the first one. The patch looks good to me, but I'd like to give other maintainers (in particular for ARM) a chance to comment before approving it. Thanks, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 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 1 sibling, 1 reply; 12+ messages in thread From: Pedro Alves @ 2014-12-02 15:21 UTC (permalink / raw) To: Andreas Arnez, gdb-patches; +Cc: Ulrich Weigand 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-02 15:21 ` Pedro Alves @ 2014-12-02 19:18 ` Andreas Arnez 2014-12-03 18:18 ` Andreas Arnez 2014-12-04 13:39 ` Pedro Alves 0 siblings, 2 replies; 12+ messages in thread From: Andreas Arnez @ 2014-12-02 19:18 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Ulrich Weigand 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-02 19:18 ` Andreas Arnez @ 2014-12-03 18:18 ` Andreas Arnez 2014-12-04 13:33 ` Pedro Alves 2014-12-04 13:39 ` Pedro Alves 1 sibling, 1 reply; 12+ messages in thread From: Andreas Arnez @ 2014-12-03 18:18 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Ulrich Weigand On Tue, Dec 02 2014, Andreas Arnez wrote: > On Tue, Dec 02 2014, Pedro Alves wrote: > >> On 12/01/2014 06:15 PM, Andreas Arnez wrote: >> >>> >>> + 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. Hm, I noticed that this probably deserves some more explanation. While it is true that the registers are marked unavailable when initializing a new regcache, the regcache seems to survive without another initialization between calls to fetch_inferior_registers. I've verified this in my tests, and I've also not seen any code that would perform such a re-initialization. I wonder why that is the case, and whether we would like to change that. If so, the patch could avoid touching ARM code, wouldn't need special treatment of NULL in the TDB store function, and would treat ENODATA like any other error from ptrace, except that the warning would be suppressed. I think this would also improve the behavior of other errors from ptrace, but maybe there's a good reason for falling back to the "last known" register values in this case. Or maybe there's a performance reason for avoiding the re-initialization? For illustration, why don't we do something like the (untested) patch below? -- diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 718ae8c..b0f6a22 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -52,6 +52,8 @@ get_thread_regcache (struct thread_info *thread, int fetch) struct thread_info *saved_thread = current_thread; current_thread = thread; + memset (regcache->register_status, REG_UNAVAILABLE, + regcache->tdesc->num_registers); fetch_inferior_registers (regcache, -1); current_thread = saved_thread; regcache->registers_valid = 1; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-03 18:18 ` Andreas Arnez @ 2014-12-04 13:33 ` Pedro Alves 2014-12-09 16:10 ` Andreas Arnez 0 siblings, 1 reply; 12+ messages in thread From: Pedro Alves @ 2014-12-04 13:33 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches, Ulrich Weigand On 12/03/2014 06:18 PM, Andreas Arnez wrote: > On Tue, Dec 02 2014, Andreas Arnez wrote: >> On Tue, Dec 02 2014, Pedro Alves wrote: >>> 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. > > Hm, I noticed that this probably deserves some more explanation. > > While it is true that the registers are marked unavailable when > initializing a new regcache, the regcache seems to survive without > another initialization between calls to fetch_inferior_registers. I've > verified this in my tests, and I've also not seen any code that would > perform such a re-initialization. Hmm, good find. > > I wonder why that is the case, and whether we would like to change that. "why" is just that nothing ever stumbled on this. The case that required teaching gdbserver about unavailable registers is tracepoint traceframes, in which case the regcache is always a new one: server.c: case 'g': require_running (own_buf); if (current_traceframe >= 0) { struct regcache *regcache = new_register_cache (current_target_desc ()); > If so, the patch could avoid touching ARM code, wouldn't need special > treatment of NULL in the TDB store function, and would treat ENODATA > like any other error from ptrace, except that the warning would be > suppressed. I think this would also improve the behavior of other > errors from ptrace, but maybe there's a good reason for falling back to > the "last known" register values in this case. Or maybe there's a > performance reason for avoiding the re-initialization? > > For illustration, why don't we do something like the (untested) patch > below? > > -- > diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c > index 718ae8c..b0f6a22 100644 > --- a/gdb/gdbserver/regcache.c > +++ b/gdb/gdbserver/regcache.c > @@ -52,6 +52,8 @@ get_thread_regcache (struct thread_info *thread, int fetch) > struct thread_info *saved_thread = current_thread; > > current_thread = thread; > + memset (regcache->register_status, REG_UNAVAILABLE, > + regcache->tdesc->num_registers); This makes sense to me, it's similar to gdb's own handling. See gdb/regcache.c:regcache_raw_read (and regcache_invalidate). Can you check the patch on x86 too, please? You'll need the same #ifdef guard as init_register_cache uses; s390 doesn't build the IPA. > fetch_inferior_registers (regcache, -1); > current_thread = saved_thread; > regcache->registers_valid = 1; Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-04 13:33 ` Pedro Alves @ 2014-12-09 16:10 ` Andreas Arnez 0 siblings, 0 replies; 12+ messages in thread From: Andreas Arnez @ 2014-12-09 16:10 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Ulrich Weigand On Thu, Dec 04 2014, Pedro Alves wrote: > On 12/03/2014 06:18 PM, Andreas Arnez wrote: >> [...] >> >> For illustration, why don't we do something like the (untested) patch >> below? >> >> -- >> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c >> index 718ae8c..b0f6a22 100644 >> --- a/gdb/gdbserver/regcache.c >> +++ b/gdb/gdbserver/regcache.c >> @@ -52,6 +52,8 @@ get_thread_regcache (struct thread_info *thread, int fetch) >> struct thread_info *saved_thread = current_thread; >> >> current_thread = thread; >> + memset (regcache->register_status, REG_UNAVAILABLE, >> + regcache->tdesc->num_registers); > > This makes sense to me, it's similar to gdb's own handling. > See gdb/regcache.c:regcache_raw_read (and regcache_invalidate). > > Can you check the patch on x86 too, please? You'll need the > same #ifdef guard as init_register_cache uses; s390 > doesn't build the IPA. > >> fetch_inferior_registers (regcache, -1); >> current_thread = saved_thread; >> regcache->registers_valid = 1; OK, did that, and it seems to work for me with the #ifdef guard added. See the updated patch set: https://sourceware.org/ml/gdb-patches/2014-12/msg00193.html -- Andreas ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] S390: Fix gdbserver support for TDB 2014-12-02 19:18 ` Andreas Arnez 2014-12-03 18:18 ` Andreas Arnez @ 2014-12-04 13:39 ` Pedro Alves 1 sibling, 0 replies; 12+ messages in thread From: Pedro Alves @ 2014-12-04 13:39 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches, Ulrich Weigand 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-12-09 16:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-26 11:56 [PATCH 0/2] S390: Fixes for gdbserver on targets with TDB Andreas Arnez 2014-11-26 11:56 ` [PATCH 1/2] S390: Fix 'expedite' for s390-te-linux64 Andreas Arnez 2014-12-01 18:34 ` Ulrich Weigand 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 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
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).