From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66841 invoked by alias); 8 Aug 2018 15:58:41 -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 66815 invoked by uid 89); 8 Aug 2018 15:58:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f68.google.com Received: from mail-wm0-f68.google.com (HELO mail-wm0-f68.google.com) (74.125.82.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 08 Aug 2018 15:58:37 +0000 Received: by mail-wm0-f68.google.com with SMTP id f21-v6so3256616wmc.5 for ; Wed, 08 Aug 2018 08:58:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=A1KBadL9xLP3csad5zxOGVUC+91Lyg0DBeoxc05k/cU=; b=U3ihJwUFbDrz7a552eCrC6o21GfXOtIIAvbAeY6UdKdZKVJmoOUP0QAh4qdkFvGeT/ E1xquoh6LBoX0k2+BtKAgVZeYhzTHjSUWQr5yZ21MeDYPpUKsPhX6OicUGmObPuMsfh9 glLq/GCK7AKeQYnCPmpdjb1LeeFRmxWRqlWpnyduVVkXWnFgiQ84p2KYqSy+oz8/sEUf ofkAlP/KBcag5UsP2gdHbHO5QGLCXVceHrLqUTWZePAPoU9C1Z6eDpCeeXVgGDfSfm3u umOPPEPgl2+Ccx7/Jpr+h0V7xW44TLfd2h/9jTMl4f6czkHvE93KMmi6RdcofZ3AcHaT +rZA== Return-Path: Received: from localhost (host81-140-215-41.range81-140.btcentralplus.com. [81.140.215.41]) by smtp.gmail.com with ESMTPSA id m200-v6sm7415604wma.32.2018.08.08.08.58.33 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 08 Aug 2018 08:58:34 -0700 (PDT) Date: Wed, 08 Aug 2018 15:58:00 -0000 From: Andrew Burgess To: Jim Wilson Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 4/5] RISC-V: Add native linux support. Message-ID: <20180808155833.GO3155@embecosm.com> References: <20180808021710.7779-1-jimw@sifive.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180808021710.7779-1-jimw@sifive.com> X-Fortune: Bad user karma. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00164.txt.bz2 * Jim Wilson [2018-08-07 19:17:10 -0700]: > Add initial native support for riscv*-linux*. > > gdb/ > * riscv-linux-nat.c: New file. This looks good with a couple of minor nits... > --- > gdb/riscv-linux-nat.c | 277 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 277 insertions(+) > create mode 100644 gdb/riscv-linux-nat.c > > diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c > new file mode 100644 > index 0000000000..b525220759 > --- /dev/null > +++ b/gdb/riscv-linux-nat.c > @@ -0,0 +1,277 @@ > +/* Native-dependent code for GNU/Linux RISC-V. > + Copyright (C) 2018 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "defs.h" > +#include "regcache.h" > +#include "gregset.h" > +#include "linux-nat.h" > +#include "elf.h" > +#include "riscv-tdep.h" > + > +#include > + > +class riscv_linux_nat_target final : public linux_nat_target This probably needs a comment. > +{ > +public: > + /* Add our register access methods. */ > + void fetch_registers (struct regcache *regcache, int regnum) override; > + void store_registers (struct regcache *regcache, int regnum) override; > +}; > + > +static riscv_linux_nat_target the_riscv_linux_nat_target; > + > +/* Copy general purpose register REGNUM (or all gp regs if REGNUM == -1) > + from regset GREGS into REGCACHE. */ > + > +void > +supply_gregset_regnum (struct regcache *regcache, const prgregset_t *gregs, > + int regnum) Can this be static? > +{ > + int i; > + const elf_greg_t *regp = *gregs; > + > + if (regnum == -1) > + { > + /* We only support the integer registers and PC here. */ > + for (i = RISCV_ZERO_REGNUM + 1; i < RISCV_PC_REGNUM; i++) > + regcache->raw_supply (i, regp + i); > + > + /* GDB stores PC in reg 32. Linux kernel stores it in reg 0. */ > + regcache->raw_supply (32, regp + 0); > + > + /* Fill the inaccessible zero register with zero. */ > + regcache->raw_supply_zeroed (0); > + } > + else if (regnum == RISCV_ZERO_REGNUM) > + regcache->raw_supply_zeroed (0); > + else if (regnum > RISCV_ZERO_REGNUM && regnum < RISCV_PC_REGNUM) > + regcache->raw_supply (regnum, regp + regnum); > + else if (regnum == RISCV_PC_REGNUM) > + regcache->raw_supply (32, regp + 0); > +} > + > +/* Copy all general purpose registers from regset GREGS into REGCACHE. */ > + > +void > +supply_gregset (struct regcache *regcache, const prgregset_t *gregs) > +{ > + supply_gregset_regnum (regcache, gregs, -1); > +} > + > +/* Copy floating point register REGNUM (or all fp regs if REGNUM == -1) > + from regset FPREGS into REGCACHE. */ > + > +void > +supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs, > + int regnum) Can this be static? > +{ > + int i; > + > + if (regnum == -1) > + { > + /* We only support the FP registers and FCSR here. */ > + for (i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++) > + regcache->raw_supply (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]); > + > + regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr); > + } > + else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM) > + regcache->raw_supply (regnum, > + &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]); > + else if (regnum == RISCV_CSR_FCSR_REGNUM) > + regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr); > +} > + > +/* Copy all floating point registers from regset FPREGS into REGCACHE. */ > + > +void > +supply_fpregset (struct regcache *regcache, const prfpregset_t *fpregs) > +{ > + supply_fpregset_regnum (regcache, fpregs, -1); > +} > + > +/* Copy general purpose register REGNUM (or all gp regs if REGNUM == -1) > + from REGCACHE into regset GREGS. */ > + > +void > +fill_gregset (const struct regcache *regcache, prgregset_t *gregs, int regnum) > +{ > + elf_greg_t *regp = *gregs; > + > + if (regnum == -1) > + { > + /* We only support the integer registers and PC here. */ > + for (int i = RISCV_ZERO_REGNUM + 1; i < RISCV_PC_REGNUM; i++) > + regcache->raw_collect (i, regp + i); > + > + regcache->raw_collect (32, regp + 0); > + } > + else if (regnum == RISCV_ZERO_REGNUM) > + /* Nothing to do here. */ > + ; > + else if (regnum > RISCV_ZERO_REGNUM && regnum < RISCV_PC_REGNUM) > + regcache->raw_collect (regnum, regp + regnum); > + else if (regnum == RISCV_PC_REGNUM) > + regcache->raw_collect (32, regp + 0); > +} > + > +/* Copy floating point register REGNUM (or all fp regs if REGNUM == -1) > + from REGCACHE into regset FPREGS. */ > + > +void > +fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs, > + int regnum) > +{ > + if (regnum == -1) > + { > + /* We only support the FP registers and FCSR here. */ > + for (int i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++) > + regcache->raw_collect (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]); > + > + regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr); > + } > + else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM) > + regcache->raw_collect (regnum, > + &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]); > + else if (regnum == RISCV_CSR_FCSR_REGNUM) > + regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr); > +} > + > +/* Fetch REGNUM (or all registers if REGNUM == -1) from the target > + into REGCACHE using PTRACE_GETREGSET. */ > + > +void > +riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum) > +{ > + int tid; > + > + tid = get_ptrace_pid (regcache->ptid()); > + > + if ((regnum >= RISCV_ZERO_REGNUM && regnum <= RISCV_PC_REGNUM) > + || (regnum == -1)) > + { > + struct iovec iov; > + elf_gregset_t regs; > + > + iov.iov_base = ®s; > + iov.iov_len = sizeof (regs); > + > + if (ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't get registers")); > + else > + supply_gregset_regnum (regcache, ®s, regnum); > + } > + > + if ((regnum >= RISCV_FIRST_FP_REGNUM > + && regnum <= RISCV_LAST_FP_REGNUM) > + || (regnum == RISCV_CSR_FCSR_REGNUM) > + || (regnum == -1)) > + { > + struct iovec iov; > + elf_fpregset_t regs; > + > + iov.iov_base = ®s; > + iov.iov_len = sizeof (regs); > + > + if (ptrace (PTRACE_GETREGSET, tid, NT_PRFPREG, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't get registers")); > + else > + supply_fpregset_regnum (regcache, ®s, regnum); > + } > + > + if (regnum == RISCV_CSR_MISA_REGNUM) Should this also have a 'regnum == -1' check? > + { > + /* TODO: Need to add a ptrace call for this. */ > + regcache->raw_supply_zeroed (regnum); > + } > + > + /* Access to other CSRs has potential security issues, don't support them for > + now. */ Should we add a warning if regnum is NOT -1, but IS for a CSR? Would this warning end up triggering all the time? Maybe if it does it should just trigger once? I'm just thinking if a user specifically asks for a csr register, it might be nice if GDB would say "can't" instead of silently failing... > +} > + > +/* Store REGNUM (or all registers if REGNUM == -1) to the target > + from REGCACHE using PTRACE_SETREGSET. */ > + > +void > +riscv_linux_nat_target::store_registers (struct regcache *regcache, int regnum) > +{ > + int tid; > + > + tid = get_ptrace_pid (regcache->ptid ()); > + > + if ((regnum >= RISCV_ZERO_REGNUM && regnum <= RISCV_PC_REGNUM) > + || (regnum == -1)) > + { > + struct iovec iov; > + elf_gregset_t regs; > + > + iov.iov_base = ®s; > + iov.iov_len = sizeof (regs); > + > + if (ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't get registers")); > + else > + { > + fill_gregset (regcache, ®s, regnum); > + > + if (ptrace (PTRACE_SETREGSET, tid, NT_PRSTATUS, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't set registers")); > + } > + } > + > + if ((regnum >= RISCV_FIRST_FP_REGNUM > + && regnum <= RISCV_LAST_FP_REGNUM) > + || (regnum == RISCV_CSR_FCSR_REGNUM) > + || (regnum == -1)) > + { > + struct iovec iov; > + elf_fpregset_t regs; > + > + iov.iov_base = ®s; > + iov.iov_len = sizeof (regs); > + > + if (ptrace (PTRACE_GETREGSET, tid, NT_PRFPREG, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't get registers")); > + else > + { > + fill_fpregset (regcache, ®s, regnum); > + > + if (ptrace (PTRACE_SETREGSET, tid, NT_PRFPREG, > + (PTRACE_TYPE_ARG3) &iov) == -1) > + perror_with_name (_("Couldn't set registers")); > + } > + } > + > + /* Access to CSRs has potential security issues, don't support them for > + now. */ As above, it might be nice if GDB produced some warning when the user tries to write to an unsupported register... > +} > + > +/* Initialize RISC-V Linux native support. */ > + > +void > +_initialize_riscv_linux_nat (void) > +{ > + /* Register the target. */ > + linux_target = &the_riscv_linux_nat_target; > + add_inf_child_target (&the_riscv_linux_nat_target); > +} > -- > 2.17.1 > With the above issues fixed, this looks fine. Thanks, Andrew