| Message ID | 20181128224953.22441-1-andrew.burgess@embecosm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > Currently this will supply a suitably sized set of x-registers, and > will probe the kernel to see if the f-registers are readable. If they > are readable then we currently assume that the f-registers are the > same size as the x-registers as I don't know of a good way to probe > the f-register length. This will obviously need fixing in future. The linux kernel currently only supports 64-bit f-registers or none, so just assuming that they are 64-bits if they exist will work for now. The NT_FPREGSET ptrace call is only supported if f-registers exist, so checking for that is fine. My linux kernel patch to add f-register ptrace support adds an elf_fpreg_t type, but it is in asm/elf.h which is currently not included, and you can't use the type if you have a 4.19 or earlier kernel. The elf_gpreg_t definition is coming from sys/procfs.h which duplicates the definition in asm/elf.h, but I didn't add an elf_fpreg_t definition to sys/procfs.h, so I'm not sure what to do about that. We always have the option of using hwcap info, there are bits set in there for available architecture extensions. The hwcap support is apparently broken in some versions of the linux kernel development tree, but is hopefully correct in all supported releases. There are macros you can find in asm/hwcap.h. We would have to figure out how to get these values into gdb, since including asm/hwcap.h directly is probably not wise. Jim
On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > Adds riscv_linux_nat_target::read_description method to find a > suitable target description for the native linux target we are running > on. I tested this on my HiFive Unleashed running a 4.15 kernel with patches for gcc, gdb, and glibc support. It looks good. It correctly detects the FP register support. I get 3 extra failures FAIL: gdb.base/solib-display.exp: NO: continue FAIL: gdb.base/solib-display.exp: IN: continue FAIL: gdb.base/solib-display.exp: SEP: continue which looks a little odd since there is no obvious connection to the target description support, but this is repeatable so something is going on here. Anyways, I'm OK with this for now, we can worry about debugging this problem later. Jim
* Jim Wilson <jimw@sifive.com> [2018-11-28 18:05:21 -0800]: > On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess > <andrew.burgess@embecosm.com> wrote: > > Adds riscv_linux_nat_target::read_description method to find a > > suitable target description for the native linux target we are running > > on. > > I tested this on my HiFive Unleashed running a 4.15 kernel with > patches for gcc, gdb, and glibc support. It looks good. It correctly > detects the FP register support. I get 3 extra failures > FAIL: gdb.base/solib-display.exp: NO: continue > FAIL: gdb.base/solib-display.exp: IN: continue > FAIL: gdb.base/solib-display.exp: SEP: continue > which looks a little odd since there is no obvious connection to the > target description support, but this is repeatable so something is > going on here. Anyways, I'm OK with this for now, we can worry about > debugging this problem later. Thanks for looking at this Jim. I dug a little deeper, and now understand what is going on here. We (or I) currently create a new target description on every call into `riscv_create_target_description', I (incorrectly) thought that `riscv_gdbarch_init' would sort it all out. However, it turns out GDB relies on identical target descriptions being the exact same object. This new set of patches does a little prep-work, and then adds a cache so that calls to `riscv_create_target_description' with the same feature set will return the exact same target description object. With this done, the tests listed above now pass. But why I hear you ask? Well, in the test we we set up a 'display' of a function local static variable. When the 'display' is setup GDB evaluates the expression and captures the block and symbol. Then, later, when we stop we can display the value of the symbol even though (technically) its out of scope (as we have the block cached). In the test in question we actually set up the display, and the restart GDB, however, we still manage to keep displaying the static variable as we have the block cached, and (I guess) as we are running the same program everything is still valid (I hope). Anyway, GDB does have one safety check built in - has the gdbarch changed. If it has then GDB ditches the cached block and symbol, and tries to reevaluate the 'display' string. However, when this happens the function local static variable is out of scope, and the display expression gets deleted. Conclusion, we need to make sure we don't create new gdbarch objects when we don't need to (well obviously) and to do this we need to make sure that we reuse target descriptions. This is still running through testing at my end, so far its looking good, but I thought I'd share in case you also wanted to test. Thanks, Andrew --- Andrew Burgess (4): gdb/riscv: Make some target description functions constant gdb/riscv: Add equality operators to riscv_gdb_features gdb/riscv: Create each unique target description only once gdb/riscv: Add read_description method for riscv_linux_nat_target gdb/ChangeLog | 27 +++++++++++++++++++++++ gdb/arch/riscv.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++- gdb/arch/riscv.h | 15 ++++++++++++- gdb/riscv-linux-nat.c | 38 +++++++++++++++++++++++++++++++++ gdb/riscv-tdep.c | 6 ++---- 5 files changed, 139 insertions(+), 6 deletions(-)
diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c index d51f6e30218..f0705bc763f 100644 --- a/gdb/riscv-linux-nat.c +++ b/gdb/riscv-linux-nat.c @@ -21,6 +21,8 @@ #include "gregset.h" #include "linux-nat.h" #include "riscv-tdep.h" +#include "inferior.h" +#include "target-descriptions.h" #include "elf/common.h" @@ -34,6 +36,9 @@ public: /* Add our register access methods. */ void fetch_registers (struct regcache *regcache, int regnum) override; void store_registers (struct regcache *regcache, int regnum) override; + + /* Read suitable target description. */ + const struct target_desc *read_description () override; }; static riscv_linux_nat_target the_riscv_linux_nat_target; @@ -155,6 +160,39 @@ fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs, regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr); } +/* Return a target description for the current target. */ + +const struct target_desc * +riscv_linux_nat_target::read_description () +{ + struct riscv_gdbarch_features features; + struct iovec iov; + elf_fpregset_t regs; + int tid; + + /* Figuring out xlen is easy. */ + features.xlen = sizeof (elf_greg_t); + + tid = inferior_ptid.lwp (); + + iov.iov_base = ®s; + iov.iov_len = sizeof (regs); + + /* Can we fetch the f-registers? */ + if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET, + (PTRACE_TYPE_ARG3) &iov) == -1) + features.flen = 0; /* No f-registers. */ + else + { + /* TODO: We need a way to figure out the actual length of the + f-registers. We could have 64-bit x-registers, with 32-bit + f-registers. For now, just assumed xlen and flen match. */ + features.flen = features.xlen; + } + + return riscv_create_target_description (features); +} + /* Fetch REGNUM (or all registers if REGNUM == -1) from the target into REGCACHE using PTRACE_GETREGSET. */