| Message ID | 20170412183727.22483-5-jhb@FreeBSD.org |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 73634 invoked by alias); 12 Apr 2017 18:45:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 73424 invoked by uid 89); 12 Apr 2017 18:45:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Apr 2017 18:45:55 +0000 Received: from ralph.baldwin.cx.net (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 7841410A87D for <gdb-patches@sourceware.org>; Wed, 12 Apr 2017 14:37:43 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: gdb-patches@sourceware.org Subject: [PATCH 4/4] Don't throw an error in 'info registers' for unavailable MIPS GP registers. Date: Wed, 12 Apr 2017 11:37:27 -0700 Message-Id: <20170412183727.22483-5-jhb@FreeBSD.org> In-Reply-To: <20170412183727.22483-1-jhb@FreeBSD.org> References: <20170412183727.22483-1-jhb@FreeBSD.org> X-IsSubscribed: yes |
Commit Message
John Baldwin
April 12, 2017, 6:37 p.m. UTC
'info registers' for MIPS throws an error and when it first encounters an unavailable register. This does not match other architectures which annotate unavailable registers and continue to print out the values of subsequent registers. Replace the error by displaying an aligned "<unavailable>". This string is truncated when displaying a 32-bit register. gdb/ChangeLog: * mips-tdep.c (print_gp_register_row): Don't error for unavailable registers. --- gdb/ChangeLog | 5 +++++ gdb/mips-tdep.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)
Comments
On 04/12/2017 01:37 PM, John Baldwin wrote: > index 674b5098b0..4ec0c31341 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame, > > /* OK: get the data in raw format. */ > if (!deprecated_frame_register_read (frame, regnum, raw_buffer)) > - error (_("can't read register %d (%s)"), > - regnum, gdbarch_register_name (gdbarch, regnum)); > + { > + fprintf_filtered (file, "%*.*s ", > + (int) mips_abi_regsize (gdbarch) * 2, > + (int) mips_abi_regsize (gdbarch) * 2, > + "<unavailable>"); > + col++; > + continue; > + } What is the output you're getting in this case?
On Thursday, April 13, 2017 11:37:15 AM Luis Machado wrote: > On 04/12/2017 01:37 PM, John Baldwin wrote: > > index 674b5098b0..4ec0c31341 100644 > > --- a/gdb/mips-tdep.c > > +++ b/gdb/mips-tdep.c > > @@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame, > > > > /* OK: get the data in raw format. */ > > if (!deprecated_frame_register_read (frame, regnum, raw_buffer)) > > - error (_("can't read register %d (%s)"), > > - regnum, gdbarch_register_name (gdbarch, regnum)); > > + { > > + fprintf_filtered (file, "%*.*s ", > > + (int) mips_abi_regsize (gdbarch) * 2, > > + (int) mips_abi_regsize (gdbarch) * 2, > > + "<unavailable>"); > > + col++; > > + continue; > > + } > > What is the output you're getting in this case? On FreeBSD (which doesn't support fir) I now get this: O32: (gdb) info registers zero at v0 v1 a0 a1 a2 a3 R0 00000000 00000000 00000000 00000000 00000001 7fffecac 7fffecb4 004182c0 t0 t1 t2 t3 t4 t5 t6 t7 R8 00000000 00000000 00000061 2e9a1b76 00124f9b 00000001 00001000 00401964 s0 s1 s2 s3 s4 s5 s6 s7 R16 7fffecb4 00000001 7fffecac 00000000 00000000 00000000 00000000 00000000 t8 t9 k0 k1 gp sp s8 ra R24 0000008d 004033d0 00000000 00000000 00420320 7ffeec60 00000000 00401980 sr lo hi bad cause pc 00008413 0000d0d7 000002d1 004033d0 00000024 004033d0 fsr fir 00000000 <unavail N64: (gdb) info registers zero at v0 v1 R0 0000000000000000 0000000000000000 0000000000000000 0000000000000000 a0 a1 a2 a3 R4 0000000000000001 0000007fffffeb40 0000007fffffeb50 0000000120018030 a4 a5 a6 a7 R8 0000000000000000 0000000000000001 0000000000000000 0000000000000001 t0 t1 t2 t3 R12 0000000000000000 0000000000000000 0000000000000000 0000000160051940 s0 s1 s2 s3 R16 0000007fffffeb50 0000000000000001 0000007fffffeb40 0000000000000000 s4 s5 s6 s7 R20 0000000000000000 0000000000000000 0000000000000000 0000000000000000 t8 t9 k0 k1 R24 000000000000008d 00000001200035e8 0000000000000000 0000000000000000 gp sp s8 ra R28 0000000120020090 0000007fffffead8 0000000000000000 00000001200020b4 sr lo hi bad 00000000008084b3 000000000000d0d7 00000000000002d1 0000007fffffeaa8 cause pc 0000000000000024 00000001200035e8 fsr fir 00000000 <unavailable> It was more dire on a target that doesn't supply all registers. For example, I have an out of tree target for FreeBSD kernels and stopped threads in FreeBSD's kernel only supply a subset of GPRs. Without the patch examining registers for a stopped thread looks like this: (kgdb) info registers zero at v0 v1 R0 0000000000000000 can't read register 91 (at) with the patch it looks like this: (kgdb) info registers zero at v0 v1 R0 0000000000000000 <unavailable> <unavailable> <unavailable> a0 a1 a2 a3 R4 <unavailable> <unavailable> <unavailable> <unavailable> a4 a5 a6 a7 R8 <unavailable> <unavailable> <unavailable> <unavailable> t0 t1 t2 t3 R12 <unavailable> <unavailable> <unavailable> <unavailable> s0 s1 s2 s3 R16 00000000ffff00fa 0000000000000000 0000000000000000 0000010000000000 s4 s5 s6 s7 R20 00000000ffff00fa 0000000000000000 0000000000000000 0000010000000000 t8 t9 k0 k1 R24 0000007fffffefa0 <unavailable> <unavailable> <unavailable> gp sp s8 ra R28 0000000000000102 00000000fffd002e <unavailable> 0000007fffffefa0 sr lo hi bad 0000000000000010 <unavailable> <unavailable> <unavailable> cause pc <unavailable> 0000000000000000 fsr fir <unavailable> <unavailable>
On Fri, 14 Apr 2017, John Baldwin wrote: > > What is the output you're getting in this case? > > On FreeBSD (which doesn't support fir) I now get this: If the register is not ever supplied, then you need a target description that does not include it. The rest of code will then handle it correctly. > It was more dire on a target that doesn't supply all registers. For example, > I have an out of tree target for FreeBSD kernels and stopped threads in > FreeBSD's kernel only supply a subset of GPRs. Without the patch examining > registers for a stopped thread looks like this: Why can't the remaining general registers be read or written -- is that a bug in the kernel? That sort of defeats the point of debugging, where you'd expect to be able to poke at any register that is at debuggee's disposal (so not supplying FIR can be considered a bug too). A program's variable could live in such an inaccessible register for example. I'll see if there's anything else I want to comment on in this series next week. Maciej
On Saturday, April 15, 2017 05:02:23 PM Maciej W. Rozycki wrote: > On Fri, 14 Apr 2017, John Baldwin wrote: > > > > What is the output you're getting in this case? > > > > On FreeBSD (which doesn't support fir) I now get this: > > If the register is not ever supplied, then you need a target description > that does not include it. The rest of code will then handle it correctly. No, mips-tdep.c requires fir to be included in the description: static struct gdbarch * mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) { ... /* Check any target description for validity. */ if (tdesc_has_registers (info.target_desc)) { ... valid_p &= tdesc_numbered_register (feature, tdesc_data, mips_regnum.fp_implementation_revision, "fir"); if (!valid_p) { tdesc_data_cleanup (tdesc_data); return NULL; } ... } ... } Thus, any target description that doesn't include fir is rejected. I will change FreeBSD to export fir via ptrace() and core dumps at some point, but it doesn't currently. Note that Linux doesn't supply a valid fir from core dumps either (it just hardcodes it as zero): linux-mips-tdep.c: void mips_supply_fpregset (struct regcache *regcache, const mips_elf_fpregset_t *fpregsetp) { ... char zerobuf[MAX_REGISTER_SIZE]; memset (zerobuf, 0, MAX_REGISTER_SIZE); ... /* FIXME: how can we supply FCRIR? The ABI doesn't tell us. */ regcache_raw_supply (regcache, mips_regnum (gdbarch)->fp_implementation_revision, zerobuf); } > > It was more dire on a target that doesn't supply all registers. For example, > > I have an out of tree target for FreeBSD kernels and stopped threads in > > FreeBSD's kernel only supply a subset of GPRs. Without the patch examining > > registers for a stopped thread looks like this: > > Why can't the remaining general registers be read or written -- is that a > bug in the kernel? > > That sort of defeats the point of debugging, where you'd expect to be > able to poke at any register that is at debuggee's disposal (so not > supplying FIR can be considered a bug too). A program's variable could > live in such an inaccessible register for example. This isn't about the user thread state. When a user thread enters the kernel due to an exception, system call, etc. then all registers are saved and are available to the debugger. This is about debugging kernel threads in the kernel itself. During a context switch, only a subset of registers are explicitly saved in the thread's control block on FreeBSD (generally callee-save registers). Caller-save registers can be found by unwinding the stack.
On Sat, 15 Apr 2017, John Baldwin wrote: > > If the register is not ever supplied, then you need a target description > > that does not include it. The rest of code will then handle it correctly. > > No, mips-tdep.c requires fir to be included in the description: > > static struct gdbarch * > mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > { > ... > /* Check any target description for validity. */ > if (tdesc_has_registers (info.target_desc)) > { > ... > valid_p > &= tdesc_numbered_register (feature, tdesc_data, > mips_regnum.fp_implementation_revision, > "fir"); > > if (!valid_p) > { > tdesc_data_cleanup (tdesc_data); > return NULL; > } > > ... > } > ... > } That needs to be fixed then. Previously there was no need to handle FIR specially. Overall we need to handle configurations without FPU as well. > Note that Linux doesn't supply a valid fir from core dumps either (it just > hardcodes it as zero): > > linux-mips-tdep.c: > > void > mips_supply_fpregset (struct regcache *regcache, > const mips_elf_fpregset_t *fpregsetp) > { > ... > char zerobuf[MAX_REGISTER_SIZE]; > > memset (zerobuf, 0, MAX_REGISTER_SIZE); > ... > > /* FIXME: how can we supply FCRIR? The ABI doesn't tell us. */ > regcache_raw_supply (regcache, > mips_regnum (gdbarch)->fp_implementation_revision, > zerobuf); > } Ack. > > Why can't the remaining general registers be read or written -- is that a > > bug in the kernel? > > > > That sort of defeats the point of debugging, where you'd expect to be > > able to poke at any register that is at debuggee's disposal (so not > > supplying FIR can be considered a bug too). A program's variable could > > live in such an inaccessible register for example. > > This isn't about the user thread state. When a user thread enters the kernel > due to an exception, system call, etc. then all registers are saved and are > available to the debugger. This is about debugging kernel threads in the kernel > itself. During a context switch, only a subset of registers are explicitly > saved in the thread's control block on FreeBSD (generally callee-save registers). > Caller-save registers can be found by unwinding the stack. Fair enough. Such details have to be given in the patch description itself though. I'm somewhat put off by the truncated message in the 32-bit case though -- unless a better proposition comes up, then how about using `xxxxxxxx' and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with some previous effort? What do other target backends do? Maciej
On Saturday, April 15, 2017 11:06:35 PM Maciej W. Rozycki wrote: > On Sat, 15 Apr 2017, John Baldwin wrote: > > > > If the register is not ever supplied, then you need a target description > > > that does not include it. The rest of code will then handle it correctly. > > > > No, mips-tdep.c requires fir to be included in the description: > > > > static struct gdbarch * > > mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > > { > > ... > > /* Check any target description for validity. */ > > if (tdesc_has_registers (info.target_desc)) > > { > > ... > > valid_p > > &= tdesc_numbered_register (feature, tdesc_data, > > mips_regnum.fp_implementation_revision, > > "fir"); > > > > if (!valid_p) > > { > > tdesc_data_cleanup (tdesc_data); > > return NULL; > > } > > > > ... > > } > > ... > > } > > That needs to be fixed then. Previously there was no need to handle FIR > specially. Overall we need to handle configurations without FPU as well. That might be true, but that is a larger patch (and it doesn't help with my kernel target case where only a subset of GPRs are valid). > > > Why can't the remaining general registers be read or written -- is that a > > > bug in the kernel? > > > > > > That sort of defeats the point of debugging, where you'd expect to be > > > able to poke at any register that is at debuggee's disposal (so not > > > supplying FIR can be considered a bug too). A program's variable could > > > live in such an inaccessible register for example. > > > > This isn't about the user thread state. When a user thread enters the kernel > > due to an exception, system call, etc. then all registers are saved and are > > available to the debugger. This is about debugging kernel threads in the kernel > > itself. During a context switch, only a subset of registers are explicitly > > saved in the thread's control block on FreeBSD (generally callee-save registers). > > Caller-save registers can be found by unwinding the stack. > > Fair enough. Such details have to be given in the patch description > itself though. Ok. I can give this as an example in the commit log. > I'm somewhat put off by the truncated message in the 32-bit case though > -- unless a better proposition comes up, then how about using `xxxxxxxx' > and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with > some previous effort? What do other target backends do? I don't disagree with the 32-bit format and am certainly open to other options. Other architectures don't generally use a table and use the full "<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the "stock" method to print registers rather than a gdbarch-specific one): (kgdb) info registers rax <unavailable> rbx 0xfffff800085cb500 -8795952728832 rcx <unavailable> rdx <unavailable> rsi <unavailable> rdi <unavailable> rbp 0xfffffe04674819c0 0xfffffe04674819c0 rsp 0xfffffe0467481968 0xfffffe0467481968 r8 <unavailable> r9 <unavailable> r10 <unavailable> r11 <unavailable> r12 0xffffffff80d43b00 -2133574912 r13 0xfffff801fb2f3000 -8787583881216 r14 0x0 0 r15 0xffffffff80d43b00 -2133574912 rip 0xffffffff8058bb12 0xffffffff8058bb12 <sched_switch+1218> eflags <unavailable> cs 0x20 32 ss 0x28 40 ds <unavailable> es <unavailable> fs <unavailable> gs <unavailable>
On Mon, 17 Apr 2017, John Baldwin wrote: > > That needs to be fixed then. Previously there was no need to handle FIR > > specially. Overall we need to handle configurations without FPU as well. > > That might be true, but that is a larger patch (and it doesn't help with my > kernel target case where only a subset of GPRs are valid). Minimising changes is not our goal though, unlike making them correct. And I think we need to tell apart a situation where a register (FIR) is invalid according to the OS ABI and where a subset of registers may not always be accessible. > > I'm somewhat put off by the truncated message in the 32-bit case though > > -- unless a better proposition comes up, then how about using `xxxxxxxx' > > and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with > > some previous effort? What do other target backends do? > > I don't disagree with the 32-bit format and am certainly open to other > options. Other architectures don't generally use a table and use the full > "<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the > "stock" method to print registers rather than a gdbarch-specific one): > > (kgdb) info registers > rax <unavailable> > rbx 0xfffff800085cb500 -8795952728832 > rcx <unavailable> > rdx <unavailable> > rsi <unavailable> > rdi <unavailable> > rbp 0xfffffe04674819c0 0xfffffe04674819c0 > rsp 0xfffffe0467481968 0xfffffe0467481968 > r8 <unavailable> > r9 <unavailable> > r10 <unavailable> > r11 <unavailable> > r12 0xffffffff80d43b00 -2133574912 > r13 0xfffff801fb2f3000 -8787583881216 > r14 0x0 0 > r15 0xffffffff80d43b00 -2133574912 > rip 0xffffffff8058bb12 0xffffffff8058bb12 <sched_switch+1218> > eflags <unavailable> > cs 0x20 32 > ss 0x28 40 > ds <unavailable> > es <unavailable> > fs <unavailable> > gs <unavailable> Thanks for checking that. NB I find output above quite messy, especially the lack of column alignment, e.g. `r14' vs `r15'. It makes it hard to read for me. I've given your proposal some thought and I'm of mixed minds between two possibilites I have come up with for the condensed format, specifically `xxxxxxxx' vs `<absent>', which I think are both suitable without being messy: (gdb) info registers zero at v0 v1 a0 a1 a2 a3 R0 00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc t0 t1 t2 t3 t4 t5 t6 t7 R8 c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8 s0 s1 s2 s3 s4 s5 s6 s7 R16 78007010 00401360 xxxxxxxx xxxxxxxx xxxxxxxx 004f0000 00000063 0000006c t8 t9 k0 k1 gp sp s8 ra R24 77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c status lo hi badvaddr cause pc 0100a413 008cbcee 00000006 77f83e90 00800024 00401360 fcsr fir restart 00000000 00f30000 00000000 (gdb) vs: (gdb) info registers zero at v0 v1 a0 a1 a2 a3 R0 00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc t0 t1 t2 t3 t4 t5 t6 t7 R8 c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8 s0 s1 s2 s3 s4 s5 s6 s7 R16 78007010 00401360 <absent> <absent> <absent> 004f0000 00000063 0000006c t8 t9 k0 k1 gp sp s8 ra R24 77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c status lo hi badvaddr cause pc 0100a413 008cbcee 00000006 77f83e90 00800024 00401360 fcsr fir restart 00000000 00f30000 00000000 (gdb) The use of angle brackets with the latter variant is consistent with other targets, so I might have just a slight preference for it, but I'll be happy to accept input from other people. Where space is at disposal full `<unavailable>' could be printed, e.g: (gdb) info registers s3 s4 s5 s3: <unavailable> s4: <unavailable> s5: 0x4f0000 (gdb) or: (gdb) info registers zero at v0 v1 R0 0000000000000000 fffffffffffffff0 <unavailable> ffffffffc0000000 a0 a1 a2 a3 R4 <unavailable> 0000000000000001 000000fffffffbb8 000000fffffffbc8 a4 a5 a6 a7 R8 <unavailable> 0000000000000000 8101010101010100 2f2f2f2f2f2f2f2f t0 t1 t2 t3 R12 0000000000000000 0000000000000001 ffffffff8122f5d8 <unavailable> s0 s1 s2 s3 R16 000000fff8006000 0000000120000970 000000fff8006000 0000000000500000 s4 s5 s6 s7 R20 0000000000521ec8 0000000000522608 0000000000000000 0000000000000000 t8 t9 k0 k1 R24 000000000000161a 0000000120000970 0000000000000000 0000000000000000 gp sp s8 ra R28 000000fff8006000 000000fffffffbb0 0000000000000000 <unavailable> status lo hi badvaddr 0000000000109cf3 000000000d35ec75 0000000000000000 <unavailable> cause pc 0000000000800024 0000000120000970 fcsr fir restart 00000000 00f30000 0000000000000000 (gdb) Comments? Thoughts? Maciej
On Tuesday, April 18, 2017 10:33:28 PM Maciej W. Rozycki wrote: > On Mon, 17 Apr 2017, John Baldwin wrote: > > > > That needs to be fixed then. Previously there was no need to handle FIR > > > specially. Overall we need to handle configurations without FPU as well. > > > > That might be true, but that is a larger patch (and it doesn't help with my > > kernel target case where only a subset of GPRs are valid). > > Minimising changes is not our goal though, unlike making them correct. > And I think we need to tell apart a situation where a register (FIR) is > invalid according to the OS ABI and where a subset of registers may not > always be accessible. For FreeBSD/mips in particular I will probably fix the FIR issue by fixing FreeBSD to export FIR. That said, 'info registers' on other architectures is consistent in that they do not throw an error for an unavailable register but annotate it as such. The goal of this patch was to align MIPS with other architectures in terms of that behavior. Other architectures also permit registers to be unavailable without requiring a custom target description FWIW (e.g. the segment registers on x86 are effectively "optional" and not always supplied by a target). > > > I'm somewhat put off by the truncated message in the 32-bit case though > > > -- unless a better proposition comes up, then how about using `xxxxxxxx' > > > and `xxxxxxxxxxxxxxxx' for the 32-bit and 64-bit case respectively as with > > > some previous effort? What do other target backends do? > > > > I don't disagree with the 32-bit format and am certainly open to other > > options. Other architectures don't generally use a table and use the full > > "<unavailable>" string, e.g. on a FreeBSD 64-bit x86 kernel (which uses the > > "stock" method to print registers rather than a gdbarch-specific one): > > > > (kgdb) info registers > > rax <unavailable> > > rbx 0xfffff800085cb500 -8795952728832 > > rcx <unavailable> > > rdx <unavailable> > > rsi <unavailable> > > rdi <unavailable> > > rbp 0xfffffe04674819c0 0xfffffe04674819c0 > > rsp 0xfffffe0467481968 0xfffffe0467481968 > > r8 <unavailable> > > r9 <unavailable> > > r10 <unavailable> > > r11 <unavailable> > > r12 0xffffffff80d43b00 -2133574912 > > r13 0xfffff801fb2f3000 -8787583881216 > > r14 0x0 0 > > r15 0xffffffff80d43b00 -2133574912 > > rip 0xffffffff8058bb12 0xffffffff8058bb12 <sched_switch+1218> > > eflags <unavailable> > > cs 0x20 32 > > ss 0x28 40 > > ds <unavailable> > > es <unavailable> > > fs <unavailable> > > gs <unavailable> > > Thanks for checking that. NB I find output above quite messy, especially > the lack of column alignment, e.g. `r14' vs `r15'. It makes it hard to > read for me. I don't disagree with the note on alignment. That is probably worth fixing in a separate change. > I've given your proposal some thought and I'm of mixed minds between two > possibilites I have come up with for the condensed format, specifically > `xxxxxxxx' vs `<absent>', which I think are both suitable without being > messy: > > [snip] > > The use of angle brackets with the latter variant is consistent with other > targets, so I might have just a slight preference for it, but I'll be > happy to accept input from other people. I like <absent> and prefer it for similar reasons (consistency). > Where space is at disposal full `<unavailable>' could be printed, e.g: > > (gdb) info registers s3 s4 s5 > s3: <unavailable> > s4: <unavailable> > s5: 0x4f0000 > (gdb) > > or: > > (gdb) info registers > zero at v0 v1 > R0 0000000000000000 fffffffffffffff0 <unavailable> ffffffffc0000000 > a0 a1 a2 a3 > R4 <unavailable> 0000000000000001 000000fffffffbb8 000000fffffffbc8 > a4 a5 a6 a7 > R8 <unavailable> 0000000000000000 8101010101010100 2f2f2f2f2f2f2f2f > t0 t1 t2 t3 > R12 0000000000000000 0000000000000001 ffffffff8122f5d8 <unavailable> > s0 s1 s2 s3 > R16 000000fff8006000 0000000120000970 000000fff8006000 0000000000500000 > s4 s5 s6 s7 > R20 0000000000521ec8 0000000000522608 0000000000000000 0000000000000000 > t8 t9 k0 k1 > R24 000000000000161a 0000000120000970 0000000000000000 0000000000000000 > gp sp s8 ra > R28 000000fff8006000 000000fffffffbb0 0000000000000000 <unavailable> > status lo hi badvaddr > 0000000000109cf3 000000000d35ec75 0000000000000000 <unavailable> > cause pc > 0000000000800024 0000000120000970 > fcsr fir restart > 00000000 00f30000 0000000000000000 > (gdb) > > Comments? Thoughts? I'm generally fine with using <absent> in the 32-bit table and <unavailable> otherwise. I can rework the patch to do that and come back with a V2.
On Tuesday, April 18, 2017 03:19:31 PM John Baldwin wrote: > On Tuesday, April 18, 2017 10:33:28 PM Maciej W. Rozycki wrote: > > The use of angle brackets with the latter variant is consistent with other > > targets, so I might have just a slight preference for it, but I'll be > > happy to accept input from other people. > > I like <absent> and prefer it for similar reasons (consistency). FYI, I've posted a V2 which uses <absent>. > > Where space is at disposal full `<unavailable>' could be printed, e.g: > > > > (gdb) info registers s3 s4 s5 > > s3: <unavailable> > > s4: <unavailable> > > s5: 0x4f0000 This already works this way since it uses the generic routines to print register values. Only the table format is affected by the patch.
On Tue, 18 Apr 2017, John Baldwin wrote: > > Minimising changes is not our goal though, unlike making them correct. > > And I think we need to tell apart a situation where a register (FIR) is > > invalid according to the OS ABI and where a subset of registers may not > > always be accessible. > > For FreeBSD/mips in particular I will probably fix the FIR issue by fixing > FreeBSD to export FIR. We'll still have to handle old kernel versions that do not have the fix. > That said, 'info registers' on other architectures > is consistent in that they do not throw an error for an unavailable register > but annotate it as such. The goal of this patch was to align MIPS with > other architectures in terms of that behavior. Other architectures also > permit registers to be unavailable without requiring a custom target > description FWIW (e.g. the segment registers on x86 are effectively > "optional" and not always supplied by a target). Agreed, however you need to be clear in your patch description which of the two bugs present here it is intended to fix. > > Thanks for checking that. NB I find output above quite messy, especially > > the lack of column alignment, e.g. `r14' vs `r15'. It makes it hard to > > read for me. > > I don't disagree with the note on alignment. That is probably worth fixing > in a separate change. Sure, I haven't asked you or indeed anybody to rush fixing it (although obviously I won't mind either). I'll look through v2 of your changes and see if I have any further concerns. Maciej
On 04/18/2017 10:33 PM, Maciej W. Rozycki wrote: > > The use of angle brackets with the latter variant is consistent with other > targets, so I might have just a slight preference for it, but I'll be > happy to accept input from other people. FWIW, I'd find using anything but <unavailable> when that's what GDB means to be making the port be gratuitously different. "<unavailable>" always means the same thing in gdb -- gdb knows the value materially exists, but it can't get at it for some reason (e.g., ptrace does not expose it, a core dump is trimmed, value not collect in a trace frame, etc.). Also, I'm not absolutely sure I'm following the code correctly, but ISTM that the current mips table printing code can already print strings larger than unavailable, like "<invalid float>" in mips_print_fp_register? I also noticed that there's code in print_gp_register_row that handles printing large registers separately: if (mips_float_register_p (gdbarch, regnum)) break; /* End the row: reached FP register. */ /* Large registers are handled separately. */ if (register_size (gdbarch, regnum) > mips_abi_regsize (gdbarch)) { ... /* Print this register on a row by itself. */ supposedly to avoid column misalignment. Maybe <unavailable> registers could be printed on a row by themselves too? (Or the column width could extended if there's an unavailable register, but I'm sort of trying to avoid suggesting a deeper change.) Note that while the mips table format is more compact, the generic format has the advantage that is prints symbol information, like: >> rip 0xffffffff8058bb12 0xffffffff8058bb12 <sched_switch+1218> I definitely agree that it'd be nice to make the generic code always print the columns neatly aligned. (Maybe the ideal would be if the generic code learned about MIPS compact format and users could pick the format they want with a format switch like "info register /c" or some such. But again, that'd be a larger change which I'd not like to require.) Thanks, Pedro Alves
On Thu, 27 Apr 2017, Pedro Alves wrote: > > The use of angle brackets with the latter variant is consistent with other > > targets, so I might have just a slight preference for it, but I'll be > > happy to accept input from other people. > > FWIW, I'd find using anything but <unavailable> when that's what > GDB means to be making the port be gratuitously different. > "<unavailable>" always means the same thing in gdb -- gdb knows > the value materially exists, but it can't get at it for some > reason (e.g., ptrace does not expose it, a core dump is trimmed, value > not collect in a trace frame, etc.). If you think keeping the word the same across ports is essential, then `<unavl>' would be my second choice, at some aesthetical cost. > Also, I'm not absolutely sure I'm following the code correctly, but ISTM > that the current mips table printing code can already print strings larger > than unavailable, like "<invalid float>" in mips_print_fp_register? For obvious reasons FP registers are formatted differently: (gdb) info all-registers zero at v0 v1 a0 a1 a2 a3 R0 00000000 fffffff8 77fde8cc 00000000 77ff0000 77fec458 7ffffbd4 7ffffbdc t0 t1 t2 t3 t4 t5 t6 t7 R8 c34e4bff ea5f612e 81010100 2f2f2f2f 7ffffce1 ffffffff f0000000 7ffff4e8 s0 s1 s2 s3 s4 s5 s6 s7 R16 78007010 00401360 0051e008 00516188 005161c8 004f0000 00000063 0000006c t8 t9 k0 k1 gp sp s8 ra R24 77fdf64c 00401360 7ffff67c 00000000 78007010 7ffffbd0 00000061 77fcc97c status lo hi badvaddr cause pc 0100a413 008cbcee 00000006 77f83e90 00800024 00401360 f0: 0x7ff80000 flt: nan dbl: nan f1: 0x7ff80000 flt: nan f2: 0x7ff80000 flt: nan dbl: nan f3: 0x7ff80000 flt: nan f4: 0x7ff80000 flt: nan dbl: nan f5: 0x7ff80000 flt: nan f6: 0x7ff80000 flt: nan dbl: nan f7: 0x7ff80000 flt: nan f8: 0x7ff80000 flt: nan dbl: nan f9: 0x7ff80000 flt: nan f10: 0x7ff80000 flt: nan dbl: nan f11: 0x7ff80000 flt: nan f12: 0x7ff80000 flt: nan dbl: nan f13: 0x7ff80000 flt: nan f14: 0x7ff80000 flt: nan dbl: nan f15: 0x7ff80000 flt: nan f16: 0x7ff80000 flt: nan dbl: nan f17: 0x7ff80000 flt: nan f18: 0x7ff80000 flt: nan dbl: nan f19: 0x7ff80000 flt: nan f20: 0x7ff80000 flt: nan dbl: nan f21: 0x7ff80000 flt: nan f22: 0x7ff80000 flt: nan dbl: nan f23: 0x7ff80000 flt: nan f24: 0x7ff80000 flt: nan dbl: nan f25: 0x7ff80000 flt: nan f26: 0x7ff80000 flt: nan dbl: nan f27: 0x7ff80000 flt: nan f28: 0x7ff80000 flt: nan dbl: nan f29: 0x7ff80000 flt: nan f30: 0x7ff80000 flt: nan dbl: nan f31: 0x7ff80000 flt: nan fcsr fir restart 00000000 00f30000 00000000 (gdb) so there's enough space for unusual interpretations. > I also noticed that there's code in print_gp_register_row that handles > printing large registers separately: > > if (mips_float_register_p (gdbarch, regnum)) > break; /* End the row: reached FP register. */ > /* Large registers are handled separately. */ > if (register_size (gdbarch, regnum) > mips_abi_regsize (gdbarch)) > { > ... > /* Print this register on a row by itself. */ > > supposedly to avoid column misalignment. Maybe <unavailable> registers could > be printed on a row by themselves too? Hmm, I've investigated the origin of this piece and it has turned out to be quite recent in MIPS port's history, introduced as a hack with commit 0cc93a06b3ed ("Avoid MIPS port breakage on large registers"), <https://sourceware.org/ml/gdb-patches/2007-05/msg00334.html>, to handle the artificial $restart register, which is always printed last (and as such doesn't break output formatting in a considerable way). See the justification downthread and TBH I wouldn't let this change in if I were asked to approve it today. Instead I would request to implement it like I did for $fcsr and $fir with commit 78b86327b530 ("mips-tdep: Make FCRs always 32-bit") -- though there's something fishy going on there anyway as we (still) do not have proper support in place for a debug scenario where the ptrace(2) caller has been built for an ABI different from the debuggee's ABI -- and the problem is on the kernel side. Though by choosing the ptrace(2) syscall number matching the debuggee's ABI (there's one for each) I think it would be avoided; but then there's no libc support for doing that. So this is not a path I want to follow; I'd rather drop this piece of code and handle the case of $restart correctly. > Note that while the mips table format is more compact, the generic > format has the advantage that is prints symbol information, like: > > >> rip 0xffffffff8058bb12 0xffffffff8058bb12 <sched_switch+1218> With 32 GPRs + HI/LO + PC (which are the minimum subset of architectural registers available to user mode software) that does not fit in a standard 80x24 terminal and IMHO overwhelms the reader with a flood of information -- which I believe is also the reason why FPRs are not shown by default. > (Maybe the ideal would be if the generic code learned about MIPS compact > format and users could pick the format they want with a format switch > like "info register /c" or some such. But again, that'd be a larger > change which I'd not like to require.) That sounds to me like an idea worth considering, assuming that the current target-specific defaults are retained. Maciej
On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote: > On Thu, 27 Apr 2017, Pedro Alves wrote: > >>> The use of angle brackets with the latter variant is consistent with other >>> targets, so I might have just a slight preference for it, but I'll be >>> happy to accept input from other people. >> >> FWIW, I'd find using anything but <unavailable> when that's what >> GDB means to be making the port be gratuitously different. >> "<unavailable>" always means the same thing in gdb -- gdb knows >> the value materially exists, but it can't get at it for some >> reason (e.g., ptrace does not expose it, a core dump is trimmed, value >> not collect in a trace frame, etc.). > > If you think keeping the word the same across ports is essential, then > `<unavl>' would be my second choice, at some aesthetical cost. I'm not sure that trying to come up with short variants of these special values manually is the best option. Another output you could see here is "<not saved>". GDB prints that if GDB figures out the register is not saved in a frame (or if the DWARF indicates that, via DW_CFA_undefined). E.g., with the gdb.mi/mi-reg-undefined.exp testcase binary: (gdb) b stop_frame Breakpoint 1 at 0x40059a: file mi-reg-undefined.c, line 21. (gdb) r Breakpoint 1, stop_frame () at mi-reg-undefined.c:21 (gdb) up #1 0x00000000004005aa in first_frame () at mi-reg-undefined.c:26 26 in mi-reg-undefined.c (gdb) info registers rax <not saved> ... We don't have any arch-independent test for this unfortunately. Maybe a two-pass approach where the first pass prints each register to a buffer to find the largest width, and then use that for column width would be more future proof. It's also work better if these magic values are ever translated (i18n). But TBC, I won't object whatever output you decide is best for MIPS, and I certainly don't want to impose extra work on John -- getting rid of that error call is the most important here. I'm just trying to expose a concern so you can have a more informed decision. > >> Also, I'm not absolutely sure I'm following the code correctly, but ISTM >> that the current mips table printing code can already print strings larger >> than unavailable, like "<invalid float>" in mips_print_fp_register? > > For obvious reasons FP registers are formatted differently: OK. It wasn't actually obvious to me that each FP register was getting its own column. >> if (mips_float_register_p (gdbarch, regnum)) >> break; /* End the row: reached FP register. */ >> /* Large registers are handled separately. */ >> if (register_size (gdbarch, regnum) > mips_abi_regsize (gdbarch)) >> { >> ... >> /* Print this register on a row by itself. */ >> >> supposedly to avoid column misalignment. Maybe <unavailable> registers could >> be printed on a row by themselves too? > > Hmm, I've investigated the origin of this piece and it has turned out to > be quite recent in MIPS port's history, introduced as a hack with commit I like how 10 years ago is considered recent. :-) Thanks, Pedro Alves
On Friday, April 28, 2017 02:51:41 PM Pedro Alves wrote: > On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote: > > On Thu, 27 Apr 2017, Pedro Alves wrote: > > > >>> The use of angle brackets with the latter variant is consistent with other > >>> targets, so I might have just a slight preference for it, but I'll be > >>> happy to accept input from other people. > >> > >> FWIW, I'd find using anything but <unavailable> when that's what > >> GDB means to be making the port be gratuitously different. > >> "<unavailable>" always means the same thing in gdb -- gdb knows > >> the value materially exists, but it can't get at it for some > >> reason (e.g., ptrace does not expose it, a core dump is trimmed, value > >> not collect in a trace frame, etc.). > > > > If you think keeping the word the same across ports is essential, then > > `<unavl>' would be my second choice, at some aesthetical cost. > > I'm not sure that trying to come up with short > variants of these special values manually is the best option. > > Another output you could see here is "<not saved>". > GDB prints that if GDB figures out the register is not saved > in a frame (or if the DWARF indicates that, via DW_CFA_undefined). Unfortunately, even "<not saved>" is too long for the 8 character field for a 32-bit register value. One perhaps simple option would be to always use the same table layout for 32-bit vs 64-bit (4 columns x 11 rows). It would mean doubling the lines that 'info registers' shows on 32-bit MIPS, but it would still fit in 24 lines and would probably simplify the implementation. However, as you note, the primary goal is removing the error(), and I can live with any format that is readable.
On Friday, April 28, 2017 09:48:50 AM John Baldwin wrote: > On Friday, April 28, 2017 02:51:41 PM Pedro Alves wrote: > > On 04/27/2017 08:37 PM, Maciej W. Rozycki wrote: > > > On Thu, 27 Apr 2017, Pedro Alves wrote: > > > > > >>> The use of angle brackets with the latter variant is consistent with other > > >>> targets, so I might have just a slight preference for it, but I'll be > > >>> happy to accept input from other people. > > >> > > >> FWIW, I'd find using anything but <unavailable> when that's what > > >> GDB means to be making the port be gratuitously different. > > >> "<unavailable>" always means the same thing in gdb -- gdb knows > > >> the value materially exists, but it can't get at it for some > > >> reason (e.g., ptrace does not expose it, a core dump is trimmed, value > > >> not collect in a trace frame, etc.). > > > > > > If you think keeping the word the same across ports is essential, then > > > `<unavl>' would be my second choice, at some aesthetical cost. > > > > I'm not sure that trying to come up with short > > variants of these special values manually is the best option. > > > > Another output you could see here is "<not saved>". > > GDB prints that if GDB figures out the register is not saved > > in a frame (or if the DWARF indicates that, via DW_CFA_undefined). > > Unfortunately, even "<not saved>" is too long for the 8 character field > for a 32-bit register value. One perhaps simple option would be to always > use the same table layout for 32-bit vs 64-bit (4 columns x 11 rows). It > would mean doubling the lines that 'info registers' shows on 32-bit MIPS, > but it would still fit in 24 lines and would probably simplify the > implementation. > > However, as you note, the primary goal is removing the error(), and I can > live with any format that is readable. Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit table layout vs <insert other option here>?
On Fri, 5 May 2017, John Baldwin wrote: > > However, as you note, the primary goal is removing the error(), and I can > > live with any format that is readable. > > Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit > table layout vs <insert other option here>? I've been busy with binutils stuff recently, sorry. I'll try to have a look again next week. Maciej
On Friday, May 05, 2017 09:08:01 PM Maciej W. Rozycki wrote: > On Fri, 5 May 2017, John Baldwin wrote: > > > > However, as you note, the primary goal is removing the error(), and I can > > > live with any format that is readable. > > > > Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit > > table layout vs <insert other option here>? > > I've been busy with binutils stuff recently, sorry. I'll try to have a > look again next week. Ping?
On Mon, 12 Jun 2017, John Baldwin wrote: > > > > However, as you note, the primary goal is removing the error(), and I can > > > > live with any format that is readable. > > > > > > Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit > > > table layout vs <insert other option here>? > > > > I've been busy with binutils stuff recently, sorry. I'll try to have a > > look again next week. > > Ping? Given the lack of consensus otherwise I think ` <unavl>' is the only way to go. I hope there is consensus about this variant being better than `<unavail'. AFAIK output from `info registers' is not meant for machine consumption and as such does not constitute a protocol. Therefore we can freely change it again anytime if we find a better alternative, as long as it is comprehensible to a human. Thank you for patience and apologies for the long RTT. Maciej
On 6/15/17 7:50 PM, Maciej W. Rozycki wrote: > On Mon, 12 Jun 2017, John Baldwin wrote: > >>>>> However, as you note, the primary goal is removing the error(), and I can >>>>> live with any format that is readable. >>>> >>>> Any other thoughts on "<absent>" vs "<unavl>" vs always using the 64-bit >>>> table layout vs <insert other option here>? >>> >>> I've been busy with binutils stuff recently, sorry. I'll try to have a >>> look again next week. >> >> Ping? > > Given the lack of consensus otherwise I think ` <unavl>' is the only way > to go. I hope there is consensus about this variant being better than > `<unavail'. Yes, I think this is better certainly. > Thank you for patience and apologies for the long RTT. No problem. I'm on vacation-ish so it might be a while before I test and post the updated patch.
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index f1ac925fec..96a187667d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,10 @@ 2017-04-11 John Baldwin <jhb@FreeBSD.org> + * mips-tdep.c (print_gp_register_row): Don't error for unavailable + registers. + +2017-04-11 John Baldwin <jhb@FreeBSD.org> + * mips-tdep.c (print_gp_register_row): Replace printf_filtered with fprintf_filtered. diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index 674b5098b0..4ec0c31341 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -6533,8 +6533,14 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame, /* OK: get the data in raw format. */ if (!deprecated_frame_register_read (frame, regnum, raw_buffer)) - error (_("can't read register %d (%s)"), - regnum, gdbarch_register_name (gdbarch, regnum)); + { + fprintf_filtered (file, "%*.*s ", + (int) mips_abi_regsize (gdbarch) * 2, + (int) mips_abi_regsize (gdbarch) * 2, + "<unavailable>"); + col++; + continue; + } /* pad small registers */ for (byte = 0; byte < (mips_abi_regsize (gdbarch)