From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126785 invoked by alias); 22 Feb 2016 00:56:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 126774 invoked by uid 89); 22 Feb 2016 00:56:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=inactive, 004, read-only, gprs X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 22 Feb 2016 00:56:05 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id 7FE0240000 for ; Mon, 22 Feb 2016 01:56:56 +0100 (CET) Received: from [192.168.0.17] (87-206-68-155.dynamic.chello.pl [87.206.68.155]) by hogfather.0x04.net (Postfix) with ESMTPSA id B2A44580105 for ; Mon, 22 Feb 2016 01:56:02 +0100 (CET) Subject: Re: [PATCH] gdbserver/s390: Enable high GPRs, VX, TDB with 31-bit gdbserver. References: <1455307062-8075-1-git-send-email-koriakin@0x04.net> To: gdb-patches@sourceware.org From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56CA5CA1.5070509@0x04.net> Date: Mon, 22 Feb 2016 00:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1455307062-8075-1-git-send-email-koriakin@0x04.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00634.txt.bz2 On 12/02/16 20:57, Marcin Kościelnicki wrote: > Currently, 31-bit gdbserver doesn't support collecting/supplying high > GPRs, VX registers, and TDB data. This is not much of a problem now, > since machines that have them usually have a 64-bit gdbserver that can > be used to debug 31-bit targets just fine. However, with fast > tracepoints, it's not possible to use a 64-bit gdbserver with a 31-bit > IPA (and thus a 31-bit target), so 31-bit gdbserver has to be used > for 31-bit targets. Thus, this patch is needed to allow collecting > high GPRs and VX registers on 31-bit targets via fast tracepoints. > > gdb/gdbserver/ChangeLog: > > * linux-s390-low.c (s390_num_regs_3264): Define on 31-bit too. > (s390_regmap_3264) [!__s390x__]: New global. > (s390_collect_ptrace_register): Skip map entries containing -1. > (s390_supply_ptrace_register): Ditto. > (s390_fill_gprs_high): New function. > (s390_store_gprs_high): New function. > (s390_regsets): Add NT_S390_HIGH_GPRS. > (s390_get_hwcap): Enable on 31-bit. > (have_hwcap_s390_high_gprs): Enable on 31-bit. > (s390_arch_setup): Enable detection of high GPRs, TDB, VX on 31-bit. > Detect NT_S390_HIGH_GPRS. > (s390_usrregs_info_3264): Enable on 31-bit. > (s390_regs_info): Enable regs_info_3264 on 31-bit. > (initialize_low_arch): Initialize s390_regsets_info_3264 on 31-bit. > --- > As mentioned in the s390 fast tracepoint thread, here comes a patch for > proper high GPR support (and VX+TDB while we're at it) in 31-bit > gdbserver. It's mostly removing ifdefs for __s390x__ - the only > nontrivial part is the new regmap (with -1s for $rXh since they are not > in the basic register set), and the new regset. > > Tests have been run (RHEL 7.2, configured for s390-ibm-linux-gnu on a 64-bit > kernel)), and it's a mixed bag. A few new failures are present: > > native-gdbserver: gdb.base/store.exp: up struct 1 u; print new u, expecting {s = \{1}} > native-gdbserver: gdb.base/store.exp: up struct 1 u; set u to s_1 > native-gdbserver: gdb.base/store.exp: up struct 2 u; print new u, expecting {s = \{1, 2}} > native-gdbserver: gdb.base/store.exp: up struct 2 u; set u to s_2 > native-gdbserver: gdb.base/store.exp: upvar charest l; print new l, expecting 4 ..004. > native-gdbserver: gdb.base/store.exp: upvar charest l; set l to 4 > native-gdbserver: gdb.base/store.exp: upvar int l; print new l, expecting 4 > native-gdbserver: gdb.base/store.exp: upvar int l; set l to 4 > native-gdbserver: gdb.base/store.exp: upvar long l; print new l, expecting 4 > native-gdbserver: gdb.base/store.exp: upvar long l; set l to 4 > native-gdbserver: gdb.base/store.exp: upvar longest l; print new l, expecting 4 > native-gdbserver: gdb.base/store.exp: upvar longest l; set l to 4 > native-gdbserver: gdb.base/store.exp: upvar short l; print new l, expecting 4 > native-gdbserver: gdb.base/store.exp: upvar short l; set l to 4 > > However, these failures are already present on 64-bit gdbserver with > -m31 target, so they're not really new. On the other hand, if -mzarch > is included in CFLAGS, this patch fixes a few failures: > > native-gdbserver: gdb.base/gdb1090.exp: print s24 > native-gdbserver: gdb.base/store.exp: up struct 4 u; print old u, expecting {s = \{0, 0, 0, 0}} > native-gdbserver: gdb.base/store.exp: var longest l; print old l, expecting -1 > native-gdbserver: gdb.base/store.exp: var struct 4 u; print old u, expecting {s = \{0, 0, 0, 0}} > > So, I suppose we should look into the store.exp failures, there's a bug > in there somewhere... Ping. I've looked at store.exp. Basically, it attempts a store to a register variable (in %r12) of an inactive frame. Since it's a 31-bit target, only low 32 bits of %r12 are stored (which is just fine, since it's a 32-bit variable or smaller). If there are no high GPRs, the unwinder returns a proper memory lvalue, and everything works fine. However, if there are high GPRs, unwinding %r12 results in a memory reference cast to a 64-bit int (s390_unwind_pseudo_register) - since that's not an lvalue, the store fails. I'm not sure how that could be fixed, any ideas? > > gdb/gdbserver/ChangeLog | 18 +++++++ > gdb/gdbserver/linux-s390-low.c | 105 +++++++++++++++++++++++++++++++---------- > 2 files changed, 97 insertions(+), 26 deletions(-) > > diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog > index ca82f91..d5162df 100644 > --- a/gdb/gdbserver/ChangeLog > +++ b/gdb/gdbserver/ChangeLog > @@ -1,5 +1,23 @@ > 2016-02-12 Marcin Kościelnicki > > + * linux-s390-low.c (s390_num_regs_3264): Define on 31-bit too. > + (s390_regmap_3264) [!__s390x__]: New global. > + (s390_collect_ptrace_register): Skip map entries containing -1. > + (s390_supply_ptrace_register): Ditto. > + (s390_fill_gprs_high): New function. > + (s390_store_gprs_high): New function. > + (s390_regsets): Add NT_S390_HIGH_GPRS. > + (s390_get_hwcap): Enable on 31-bit. > + (have_hwcap_s390_high_gprs): Enable on 31-bit. > + (s390_arch_setup): Enable detection of high GPRs, TDB, VX on 31-bit. > + Detect NT_S390_HIGH_GPRS. > + (s390_usrregs_info_3264): Enable on 31-bit. > + (s390_regs_info): Enable regs_info_3264 on 31-bit. > + (initialize_low_arch): Initialize s390_regsets_info_3264 on 31-bit. > + > + > +2016-02-12 Marcin Kościelnicki > + > * tracepoint.c (x_tracepoint_action_download): Change > write_inferior_data_ptr to write_inferior_data_pointer. > (cmd_qtstart): Likewise. > diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c > index 63728aa..116042e 100644 > --- a/gdb/gdbserver/linux-s390-low.c > +++ b/gdb/gdbserver/linux-s390-low.c > @@ -132,9 +132,9 @@ static int s390_regmap[] = { > PT_ORIGGPR2, > }; > > -#ifdef __s390x__ > #define s390_num_regs_3264 68 > > +#ifdef __s390x__ > static int s390_regmap_3264[] = { > PT_PSWMASK, PT_PSWADDR, > > @@ -161,6 +161,33 @@ static int s390_regmap_3264[] = { > > PT_ORIGGPR2, > }; > +#else > +static int s390_regmap_3264[] = { > + PT_PSWMASK, PT_PSWADDR, > + > + -1, PT_GPR0, -1, PT_GPR1, > + -1, PT_GPR2, -1, PT_GPR3, > + -1, PT_GPR4, -1, PT_GPR5, > + -1, PT_GPR6, -1, PT_GPR7, > + -1, PT_GPR8, -1, PT_GPR9, > + -1, PT_GPR10, -1, PT_GPR11, > + -1, PT_GPR12, -1, PT_GPR13, > + -1, PT_GPR14, -1, PT_GPR15, > + > + PT_ACR0, PT_ACR1, PT_ACR2, PT_ACR3, > + PT_ACR4, PT_ACR5, PT_ACR6, PT_ACR7, > + PT_ACR8, PT_ACR9, PT_ACR10, PT_ACR11, > + PT_ACR12, PT_ACR13, PT_ACR14, PT_ACR15, > + > + PT_FPC, > + > + PT_FPR0_HI, PT_FPR1_HI, PT_FPR2_HI, PT_FPR3_HI, > + PT_FPR4_HI, PT_FPR5_HI, PT_FPR6_HI, PT_FPR7_HI, > + PT_FPR8_HI, PT_FPR9_HI, PT_FPR10_HI, PT_FPR11_HI, > + PT_FPR12_HI, PT_FPR13_HI, PT_FPR14_HI, PT_FPR15_HI, > + > + PT_ORIGGPR2, > +}; > #endif > > > @@ -180,12 +207,12 @@ static void > s390_collect_ptrace_register (struct regcache *regcache, int regno, char *buf) > { > int size = register_size (regcache->tdesc, regno); > + const struct regs_info *regs_info = (*the_low_target.regs_info) (); > + struct usrregs_info *usr = regs_info->usrregs; > + int regaddr = usr->regmap[regno]; > + > if (size < sizeof (long)) > { > - const struct regs_info *regs_info = (*the_low_target.regs_info) (); > - struct usrregs_info *usr = regs_info->usrregs; > - int regaddr = usr->regmap[regno]; > - > memset (buf, 0, sizeof (long)); > > if ((regno ^ 1) < usr->num_regs > @@ -218,7 +245,7 @@ s390_collect_ptrace_register (struct regcache *regcache, int regno, char *buf) > else > collect_register (regcache, regno, buf); > } > - else > + else if (regaddr != -1) > collect_register (regcache, regno, buf); > } > > @@ -227,12 +254,12 @@ s390_supply_ptrace_register (struct regcache *regcache, > int regno, const char *buf) > { > int size = register_size (regcache->tdesc, regno); > + const struct regs_info *regs_info = (*the_low_target.regs_info) (); > + struct usrregs_info *usr = regs_info->usrregs; > + int regaddr = usr->regmap[regno]; > + > if (size < sizeof (long)) > { > - const struct regs_info *regs_info = (*the_low_target.regs_info) (); > - struct usrregs_info *usr = regs_info->usrregs; > - int regaddr = usr->regmap[regno]; > - > if ((regno ^ 1) < usr->num_regs > && usr->regmap[regno ^ 1] == regaddr) > { > @@ -274,7 +301,7 @@ s390_supply_ptrace_register (struct regcache *regcache, > else > supply_register (regcache, regno, buf); > } > - else > + else if (regaddr != -1) > supply_register (regcache, regno, buf); > } > > @@ -301,6 +328,28 @@ s390_fill_gregset (struct regcache *regcache, void *buf) > > /* Fill and store functions for extended register sets. */ > > +#ifndef __s390x__ > +static void > +s390_fill_gprs_high (struct regcache *regcache, void *buf) > +{ > + int r0h = find_regno (regcache->tdesc, "r0h"); > + int i; > + > + for (i = 0; i < 16; i++) > + collect_register (regcache, r0h + 2 * i, (char *) buf + 4 * i); > +} > + > +static void > +s390_store_gprs_high (struct regcache *regcache, const void *buf) > +{ > + int r0h = find_regno (regcache->tdesc, "r0h"); > + int i; > + > + for (i = 0; i < 16; i++) > + supply_register (regcache, r0h + 2 * i, (const char *) buf + 4 * i); > +} > +#endif > + > static void > s390_store_last_break (struct regcache *regcache, const void *buf) > { > @@ -378,6 +427,10 @@ s390_store_vxrs_high (struct regcache *regcache, const void *buf) > > static struct regset_info s390_regsets[] = { > { 0, 0, 0, 0, GENERAL_REGS, s390_fill_gregset, NULL }, > +#ifndef __s390x__ > + { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_S390_HIGH_GPRS, 0, > + EXTENDED_REGS, s390_fill_gprs_high, s390_store_gprs_high }, > +#endif > /* Last break address is read-only; no fill function. */ > { PTRACE_GETREGSET, -1, NT_S390_LAST_BREAK, 0, EXTENDED_REGS, > NULL, s390_store_last_break }, > @@ -440,7 +493,6 @@ s390_set_pc (struct regcache *regcache, CORE_ADDR newpc) > } > } > > -#ifdef __s390x__ > static unsigned long > s390_get_hwcap (const struct target_desc *tdesc) > { > @@ -468,7 +520,6 @@ s390_get_hwcap (const struct target_desc *tdesc) > > return 0; > } > -#endif > > static int > s390_check_regset (int pid, int regset, int regsize) > @@ -485,11 +536,9 @@ s390_check_regset (int pid, int regset, int regsize) > return 0; > } > > -#ifdef __s390x__ > /* For a 31-bit inferior, whether the kernel supports using the full > 64-bit GPRs. */ > static int have_hwcap_s390_high_gprs = 0; > -#endif > > static void > s390_arch_setup (void) > @@ -517,8 +566,8 @@ s390_arch_setup (void) > > /* On a 64-bit host, check the low bit of the (31-bit) PSWM > -- if this is one, we actually have a 64-bit inferior. */ > -#ifdef __s390x__ > { > +#ifdef __s390x__ > unsigned int pswm; > struct regcache *regcache = new_register_cache (tdesc); > > @@ -550,7 +599,9 @@ s390_arch_setup (void) > > /* For a 31-bit inferior, check whether the kernel supports > using the full 64-bit GPRs. */ > - else if (s390_get_hwcap (tdesc) & HWCAP_S390_HIGH_GPRS) > + else > +#endif > + if (s390_get_hwcap (tdesc) & HWCAP_S390_HIGH_GPRS) > { > have_hwcap_s390_high_gprs = 1; > if (have_regset_tdb) > @@ -571,18 +622,22 @@ s390_arch_setup (void) > tdesc = tdesc_s390_linux64; > } > } > -#endif > > /* Update target_regsets according to available register sets. */ > for (regset = s390_regsets; regset->size >= 0; regset++) > if (regset->get_request == PTRACE_GETREGSET) > switch (regset->nt_type) > { > +#ifndef __s390x__ > + case NT_S390_HIGH_GPRS: > + regset->size = have_hwcap_s390_high_gprs ? 64 : 0; > + break; > +#endif > case NT_S390_LAST_BREAK: > - regset->size = have_regset_last_break? 8 : 0; > + regset->size = have_regset_last_break ? 8 : 0; > break; > case NT_S390_SYSTEM_CALL: > - regset->size = have_regset_system_call? 4 : 0; > + regset->size = have_regset_system_call ? 4 : 0; > break; > case NT_S390_TDB: > regset->size = have_regset_tdb ? 256 : 0; > @@ -637,7 +692,6 @@ static struct regs_info regs_info = > &s390_regsets_info > }; > > -#ifdef __s390x__ > static struct usrregs_info s390_usrregs_info_3264 = > { > s390_num_regs_3264, > @@ -657,20 +711,21 @@ static struct regs_info regs_info_3264 = > &s390_usrregs_info_3264, > &s390_regsets_info_3264 > }; > -#endif > > static const struct regs_info * > s390_regs_info (void) > { > -#ifdef __s390x__ > if (have_hwcap_s390_high_gprs) > { > +#ifdef __s390x__ > const struct target_desc *tdesc = current_process ()->tdesc; > > if (register_size (tdesc, 0) == 4) > return ®s_info_3264; > - } > +#else > + return ®s_info_3264; > #endif > + } > return ®s_info; > } > > @@ -732,7 +787,5 @@ initialize_low_arch (void) > init_registers_s390x_tevx_linux64 (); > > initialize_regsets_info (&s390_regsets_info); > -#ifdef __s390x__ > initialize_regsets_info (&s390_regsets_info_3264); > -#endif > } >