* [PATCH v3 0/3] gdbserver: Fix support for S390 TDB @ 2014-12-09 13:59 Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 3/3] S390: Fix gdbserver support for TDB Andreas Arnez ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Andreas Arnez @ 2014-12-09 13:59 UTC (permalink / raw) To: gdb-patches This is a follow-up to patch 2 of the series called "S390: Fixes for gdbserver on targets with TDB". The last version of that patch was here: https://sourceware.org/ml/gdb-patches/2014-12/msg00021.html This new version is split up in three patches and contains the following changes: * Invalidate the register cache whenever we are about to fetch the register values from the inferior. * Instead of passing NULL to the regset store function upon ENODATA from ptrace, just suppress its invocation, since the registers are now "unavailable" by default. Consequently no longer handle NULL in s390_store_tdb or arm_store_wmmxregset. * Suppress calling the register store function upon other errors from ptrace as well. * Rephrase the while()-loops as for()-loops in regsets_fetch_- and -_store_inferior_registers, for improved readability. Tested on S390 and i386. Andreas Arnez (3): gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers gdbserver: Prevent stale/random values in register cache S390: Fix gdbserver support for TDB gdb/gdbserver/linux-low.c | 38 +++++++++++++------------------------- gdb/gdbserver/linux-s390-low.c | 31 +++++++++++++++++++++---------- gdb/gdbserver/regcache.c | 5 +++++ 3 files changed, 39 insertions(+), 35 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] S390: Fix gdbserver support for TDB 2014-12-09 13:59 [PATCH v3 0/3] gdbserver: Fix support for S390 TDB Andreas Arnez @ 2014-12-09 13:59 ` Andreas Arnez 2014-12-10 19:16 ` Ulrich Weigand 2014-12-09 13:59 ` [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 1/3] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers Andreas Arnez 2 siblings, 1 reply; 9+ messages in thread From: Andreas Arnez @ 2014-12-09 13:59 UTC (permalink / raw) To: gdb-patches 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): Suppress the warning upon ENODATA from ptrace. (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 | 5 +++-- gdb/gdbserver/linux-s390-low.c | 31 +++++++++++++++++++++---------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index c1b53ff..6ace4b0 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, this process mode. */ disable_regset (regsets_info, regset); } - else + else if (errno != ENODATA) { char s[256]; sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d", @@ -4293,7 +4293,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) continue; buf = xmalloc (regset->size); diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c index 79fa6c0..40fb61c 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,30 @@ 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, (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 +496,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] 9+ messages in thread
* Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB 2014-12-09 13:59 ` [PATCH v3 3/3] S390: Fix gdbserver support for TDB Andreas Arnez @ 2014-12-10 19:16 ` Ulrich Weigand 2014-12-11 17:04 ` Andreas Arnez 0 siblings, 1 reply; 9+ messages in thread From: Ulrich Weigand @ 2014-12-10 19:16 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches Andreas Arnez wrote: > @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, > this process mode. */ > disable_regset (regsets_info, regset); > } > - else > + else if (errno != ENODATA) > { > char s[256]; > sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d", It would be better to keep the comment explaining in what situations the kernel can return ENODATA that you had in a previous iteration of the patch set. > @@ -4293,7 +4293,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) > continue; This (and the related s390_fill_last_break change) is really an independent change; maybe do it as a separate patch? For consistency, we might likewise want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers). Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB 2014-12-10 19:16 ` Ulrich Weigand @ 2014-12-11 17:04 ` Andreas Arnez 2014-12-11 20:16 ` Ulrich Weigand 0 siblings, 1 reply; 9+ messages in thread From: Andreas Arnez @ 2014-12-11 17:04 UTC (permalink / raw) To: Ulrich Weigand; +Cc: gdb-patches On Wed, Dec 10 2014, Ulrich Weigand wrote: > Andreas Arnez wrote: > >> @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, >> this process mode. */ >> disable_regset (regsets_info, regset); >> } >> - else >> + else if (errno != ENODATA) >> { >> char s[256]; >> sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d", > > It would be better to keep the comment explaining in what situations the kernel > can return ENODATA that you had in a previous iteration of the patch set. OK, I will add this comment in the next version: else if (errno == ENODATA) { /* ENODATA may be returned if the regset is currently not "active". This can happen in normal operation, so suppress the warning in this case. */ } else ... > >> @@ -4293,7 +4293,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) >> continue; > > This (and the related s390_fill_last_break change) is really an independent > change; maybe do it as a separate patch? For consistency, we might likewise > want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers). OK, I will split the change to allow regsets with a NULL store_function and the exploitation for last_break out into a separate patch ("gdbserver: Support read-only regsets"). And for consistency I can also allow regsets with a NULL store_function. In that case I *think* we should suppress the invocation of ptrace with the regset's get_request in regsets_store_inferior_registers(). In other words, instead of read-modify-write we would then only do the write. Agreed? In the original patch I omitted support for write-only regsets because I do not see a use case for this; and if there will be a use case in the future, I am not sure that the approach described above will really be appropriate. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB 2014-12-11 17:04 ` Andreas Arnez @ 2014-12-11 20:16 ` Ulrich Weigand 0 siblings, 0 replies; 9+ messages in thread From: Ulrich Weigand @ 2014-12-11 20:16 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches Andreas Arnez wrote: > On Wed, Dec 10 2014, Ulrich Weigand wrote: > > This (and the related s390_fill_last_break change) is really an independent > > change; maybe do it as a separate patch? For consistency, we might likewise > > want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers). > > OK, I will split the change to allow regsets with a NULL store_function > and the exploitation for last_break out into a separate patch > ("gdbserver: Support read-only regsets"). > > And for consistency I can also allow regsets with a NULL store_function. > In that case I *think* we should suppress the invocation of ptrace with > the regset's get_request in regsets_store_inferior_registers(). In > other words, instead of read-modify-write we would then only do the > write. Agreed? Ah, yes, the situation isn't symmetrical; I didn't think of that. > In the original patch I omitted support for write-only regsets because I > do not see a use case for this; and if there will be a use case in the > future, I am not sure that the approach described above will really be > appropriate. Given that, it's probably best to indeed wait until we have a use case (if ever), so I withdraw my suggestion. Thanks, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache 2014-12-09 13:59 [PATCH v3 0/3] gdbserver: Fix support for S390 TDB Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 3/3] S390: Fix gdbserver support for TDB Andreas Arnez @ 2014-12-09 13:59 ` Andreas Arnez 2014-12-10 19:14 ` Ulrich Weigand 2014-12-09 13:59 ` [PATCH v3 1/3] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers Andreas Arnez 2 siblings, 1 reply; 9+ messages in thread From: Andreas Arnez @ 2014-12-09 13:59 UTC (permalink / raw) To: gdb-patches When fetch_inferior_registers does not update all registers, this patch assures that no stale register values remain in the register cache. On Linux platforms using the regsets interface, when one of the ptrace calls used for fetching the register values returns an error, this patch also avoids copying the random data returned from ptrace into the register cache. All unfetched registers are marked "unavailable" instead. gdb/gdbserver/ChangeLog: * linux-low.c (regsets_fetch_inferior_registers): Do not invoke the regset's store function when ptrace returned an error. * regcache.c (get_thread_regcache): Invalidate register cache before fetching inferior's registers. --- gdb/gdbserver/linux-low.c | 11 ++++++----- gdb/gdbserver/regcache.c | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 164b0f6..c1b53ff 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -4255,8 +4255,6 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, /* If we get EIO on a regset, do not try it again for this process mode. */ disable_regset (regsets_info, regset); - free (buf); - continue; } else { @@ -4266,9 +4264,12 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, perror (s); } } - else if (regset->type == GENERAL_REGS) - saw_general_regs = 1; - regset->store_function (regcache, buf); + else + { + if (regset->type == GENERAL_REGS) + saw_general_regs = 1; + regset->store_function (regcache, buf); + } free (buf); } if (saw_general_regs) diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 718ae8c..4ea16a6 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch) struct thread_info *saved_thread = current_thread; current_thread = thread; +#ifndef IN_PROCESS_AGENT + /* Invalidate all registers, to prevent stale left-overs. */ + memset (regcache->register_status, REG_UNAVAILABLE, + regcache->tdesc->num_registers); +#endif fetch_inferior_registers (regcache, -1); current_thread = saved_thread; regcache->registers_valid = 1; -- 1.7.9.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache 2014-12-09 13:59 ` [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache Andreas Arnez @ 2014-12-10 19:14 ` Ulrich Weigand 2014-12-11 17:05 ` Andreas Arnez 0 siblings, 1 reply; 9+ messages in thread From: Ulrich Weigand @ 2014-12-10 19:14 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches Andreas Arnez wrote: > @@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch) > struct thread_info *saved_thread = current_thread; > > current_thread = thread; > +#ifndef IN_PROCESS_AGENT > + /* Invalidate all registers, to prevent stale left-overs. */ > + memset (regcache->register_status, REG_UNAVAILABLE, > + regcache->tdesc->num_registers); > +#endif > fetch_inferior_registers (regcache, -1); > current_thread = saved_thread; > regcache->registers_valid = 1; The whole get_thread_regcache routine is already under #ifndef IN_PROCESS_AGENT, so the ifdef seems redundant here. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache 2014-12-10 19:14 ` Ulrich Weigand @ 2014-12-11 17:05 ` Andreas Arnez 0 siblings, 0 replies; 9+ messages in thread From: Andreas Arnez @ 2014-12-11 17:05 UTC (permalink / raw) To: Ulrich Weigand; +Cc: gdb-patches On Wed, Dec 10 2014, Ulrich Weigand wrote: > Andreas Arnez wrote: > >> @@ -52,6 +52,11 @@ get_thread_regcache (struct thread_info *thread, int fetch) >> struct thread_info *saved_thread = current_thread; >> >> current_thread = thread; >> +#ifndef IN_PROCESS_AGENT >> + /* Invalidate all registers, to prevent stale left-overs. */ >> + memset (regcache->register_status, REG_UNAVAILABLE, >> + regcache->tdesc->num_registers); >> +#endif >> fetch_inferior_registers (regcache, -1); >> current_thread = saved_thread; >> regcache->registers_valid = 1; > > The whole get_thread_regcache routine is already under #ifndef IN_PROCESS_AGENT, > so the ifdef seems redundant here. Correct ;-) Will be removed in the next version. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/3] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers 2014-12-09 13:59 [PATCH v3 0/3] gdbserver: Fix support for S390 TDB Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 3/3] S390: Fix gdbserver support for TDB Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache Andreas Arnez @ 2014-12-09 13:59 ` Andreas Arnez 2 siblings, 0 replies; 9+ messages in thread From: Andreas Arnez @ 2014-12-09 13:59 UTC (permalink / raw) To: gdb-patches Replace the while-loops in linux-low.c that iterate over regsets by for-loops. This makes it clearer what is iterated over. Also, since "continue" now moves on to the next iteration without having to increment the regset pointer first, the code is slightly reduced. In case of EIO the old code did not increment the regset pointer, but iterated over the same (now disabled) regset again. This extra iteration is now avoided. gdb/gdbserver/ChangeLog: * linux-low.c (regsets_fetch_inferior_registers): Rephrase while-loop as for-loop. (regsets_store_inferior_registers): Likewise. --- gdb/gdbserver/linux-low.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 01f11b7..164b0f6 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -4221,19 +4221,14 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, int pid; struct iovec iov; - regset = regsets_info->regsets; - pid = lwpid_of (current_thread); - while (regset->size >= 0) + for (regset = regsets_info->regsets; regset->size >= 0; regset++) { void *buf, *data; int nt_type, res; if (regset->size == 0 || regset_disabled (regsets_info, regset)) - { - regset ++; - continue; - } + continue; buf = xmalloc (regset->size); @@ -4274,7 +4269,6 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, else if (regset->type == GENERAL_REGS) saw_general_regs = 1; regset->store_function (regcache, buf); - regset ++; free (buf); } if (saw_general_regs) @@ -4292,19 +4286,14 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info, int pid; struct iovec iov; - regset = regsets_info->regsets; - pid = lwpid_of (current_thread); - while (regset->size >= 0) + for (regset = regsets_info->regsets; regset->size >= 0; regset++) { void *buf, *data; int nt_type, res; if (regset->size == 0 || regset_disabled (regsets_info, regset)) - { - regset ++; - continue; - } + continue; buf = xmalloc (regset->size); @@ -4350,8 +4339,6 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info, /* If we get EIO on a regset, do not try it again for this process mode. */ disable_regset (regsets_info, regset); - free (buf); - continue; } else if (errno == ESRCH) { @@ -4369,7 +4356,6 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info, } else if (regset->type == GENERAL_REGS) saw_general_regs = 1; - regset ++; free (buf); } if (saw_general_regs) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-12-11 20:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-09 13:59 [PATCH v3 0/3] gdbserver: Fix support for S390 TDB Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 3/3] S390: Fix gdbserver support for TDB Andreas Arnez 2014-12-10 19:16 ` Ulrich Weigand 2014-12-11 17:04 ` Andreas Arnez 2014-12-11 20:16 ` Ulrich Weigand 2014-12-09 13:59 ` [PATCH v3 2/3] gdbserver: Prevent stale/random values in register cache Andreas Arnez 2014-12-10 19:14 ` Ulrich Weigand 2014-12-11 17:05 ` Andreas Arnez 2014-12-09 13:59 ` [PATCH v3 1/3] gdbserver: Rephrase loops in regsets_fetch/store_inferior_registers Andreas Arnez
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).