Message ID | 20181025110946.GN2929@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 47937 invoked by alias); 25 Oct 2018 11:09:53 -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 47926 invoked by uid 89); 25 Oct 2018 11:09:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=HX-Google-Smtp-Source:AJdET5d X-HELO: mail-wr1-f68.google.com Received: from mail-wr1-f68.google.com (HELO mail-wr1-f68.google.com) (209.85.221.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 Oct 2018 11:09:51 +0000 Received: by mail-wr1-f68.google.com with SMTP id g9-v6so8856381wrq.4 for <gdb-patches@sourceware.org>; Thu, 25 Oct 2018 04:09:50 -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=V56hJnKKThH1PqXUNPt00vJCWpTzp0+rZpJ3kXu37dU=; b=QMBwB+LWqwf+V5o1lVWDOpWaUCzWLtdRLDhoyj/hXV9rFp4V/LOhbK3fG63+erwxeV zuR4N1hrBShGAYtaUn45Y8ibsP89rt0GMPmZ9wICY4/7or7PkKXjic86P8OVZqR24xoC cI/vTJ0mAUtR7iGibWdbRdGaF8Gol4HwgYLoV6J0QyfewTKzY6vGd6fETqAgi/zpa8L6 YZdtSYiWi0RFvXDuv70swEorvjbJUDcysPsIFh0xWFPXx/pE1N0cg2Hw2MLnCOzAnhOv D4f6XX7TqLexp1UjbKfsGUUSuFiuGJK1ceTzW4MAsqRIt7TL6XrMArpKHi6ZmmK5n2v2 03hg== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host86-164-85-193.range86-164.btcentralplus.com. [86.164.85.193]) by smtp.gmail.com with ESMTPSA id o201-v6sm1391824wmg.16.2018.10.25.04.09.47 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 25 Oct 2018 04:09:48 -0700 (PDT) Date: Thu, 25 Oct 2018 12:09:47 +0100 From: Andrew Burgess <andrew.burgess@embecosm.com> To: Andreas Schwab <schwab@suse.de> Cc: Jim Wilson <jimw@sifive.com>, gdb-patches@sourceware.org Subject: Re: [PATCH 4/5] RISC-V: Add native linux support. Message-ID: <20181025110946.GN2929@embecosm.com> References: <CAFyWVaZoF3a=-QjADoUdzq_hz5skHVaxmXtB1X5ZT-a_sns5PA@mail.gmail.com> <20180808233908.8149-1-jimw@sifive.com> <mvm8t2mpahm.fsf@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <mvm8t2mpahm.fsf@suse.de> X-Fortune: Help a swallow land at Capistrano. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Oct. 25, 2018, 11:09 a.m. UTC
* Andreas Schwab <schwab@suse.de> [2018-10-25 12:49:09 +0200]: > On Aug 08 2018, Jim Wilson <jimw@sifive.com> wrote: > > > + if ((regnum == RISCV_CSR_MISA_REGNUM) > > + || (regnum == -1)) > > + { > > + /* TODO: Need to add a ptrace call for this. */ > > + regcache->raw_supply_zeroed (regnum); > > ../../gdb/gdb/regcache.c:337: internal-error: void reg_buffer::assert_regnum(int) const: Assertion `regnum >= 0' failed. Thanks for the report. I pushed the patch below to fix this issue. Thanks, Andrew --- [PATCH] gdb/riscv: Use correct regnum in riscv_linux_nat_target::fetch_registers In riscv_linux_nat_target::fetch_registers, if we are asked to supply all registers (regnum parameter is -1), then we currently end up calling regcache::raw_supply_zeroed with the regnum -1, which is invalid. Instead we should be passing the regnum of the specific register we wish to supply zeroed, in this case RISCV_CSR_MISA_REGNUM. I removed the extra { ... } block in line with the coding standard while editing this area. gdb/ChangeLog: * riscv-linux-nat.c (riscv_linux_nat_target::fetch_registers): Pass correct regnum to raw_supply_zeroed. --- gdb/ChangeLog | 5 +++++ gdb/riscv-linux-nat.c | 6 ++---- 2 files changed, 7 insertions(+), 4 deletions(-)
Comments
On 10/25/2018 12:09 PM, Andrew Burgess wrote: > > I removed the extra { ... } block in line with the coding standard > while editing this area. Actually, this applies here: > Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements: > if (i) > { > /* Return success. */ > return 0; > } > > and not: > > if (i) > /* Return success. */ > return 0; From: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards?highlight=%28coding%29%7C%28conventions%29#Whitespaces Thanks, Pedro Alves
On 10/25/18 4:09 AM, Andrew Burgess wrote: > diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c > index 7dbfe651f2c..c09121d052b 100644 > --- a/gdb/riscv-linux-nat.c > +++ b/gdb/riscv-linux-nat.c > @@ -201,10 +201,8 @@ riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum) > > if ((regnum == RISCV_CSR_MISA_REGNUM) > || (regnum == -1)) > - { > - /* TODO: Need to add a ptrace call for this. */ > - regcache->raw_supply_zeroed (regnum); > - } > + /* TODO: Need to add a ptrace call for this. */ > + regcache->raw_supply_zeroed (RISCV_CSR_MISA_REGNUM); > > /* Access to other CSRs has potential security issues, don't support them for > now. */ Oops, I just replied to Andrew directly on the commit, but probably better to reply on the list: Now that the MISA defaults to 0 if not present, would it better to just remove this and not set it to 0 explicitly? The FreeBSD native target for RISC-V doesn't set MISA to anything at all.
On Thu, Oct 25, 2018 at 10:55 AM John Baldwin <jhb@freebsd.org> wrote: > Now that the MISA defaults to 0 if not present, would it better to just remove > this and not set it to 0 explicitly? The FreeBSD native target for RISC-V > doesn't set MISA to anything at all. There is still the issue of FP register size, which comes from MISA, unless perhaps we can get it from auxvec/hw-cap info. I was going to look into that latter, and if the auxvec/hw-cap stuff works, then remove the remaining MISA support in the riscv-linux-nat.c file. Jim
On 10/25/18 11:17 AM, Jim Wilson wrote: > On Thu, Oct 25, 2018 at 10:55 AM John Baldwin <jhb@freebsd.org> wrote: >> Now that the MISA defaults to 0 if not present, would it better to just remove >> this and not set it to 0 explicitly? The FreeBSD native target for RISC-V >> doesn't set MISA to anything at all. > > There is still the issue of FP register size, which comes from MISA, > unless perhaps we can get it from auxvec/hw-cap info. I was going to > look into that latter, and if the auxvec/hw-cap stuff works, then > remove the remaining MISA support in the riscv-linux-nat.c file. Ok. I do agree that auxvec is probably the right way to handle this, as what really matters is what format the kernel exports. You can find existing uses of auxvec for this on 32-bit arm support where AT_HWCAP flags are tested for both Linux and FreeBSD in the respective tdep.c files to determine which floating point registers are available. You are free to use the same code in a nat.c file as well of course.
On Thu, 25 Oct 2018 12:19:29 PDT (-0700), jhb@FreeBSD.org wrote: > On 10/25/18 11:17 AM, Jim Wilson wrote: >> On Thu, Oct 25, 2018 at 10:55 AM John Baldwin <jhb@freebsd.org> wrote: >>> Now that the MISA defaults to 0 if not present, would it better to just remove >>> this and not set it to 0 explicitly? The FreeBSD native target for RISC-V >>> doesn't set MISA to anything at all. >> >> There is still the issue of FP register size, which comes from MISA, >> unless perhaps we can get it from auxvec/hw-cap info. I was going to >> look into that latter, and if the auxvec/hw-cap stuff works, then >> remove the remaining MISA support in the riscv-linux-nat.c file. > > Ok. I do agree that auxvec is probably the right way to handle this, as what > really matters is what format the kernel exports. You can find existing uses > of auxvec for this on 32-bit arm support where AT_HWCAP flags are tested for > both Linux and FreeBSD in the respective tdep.c files to determine which > floating point registers are available. You are free to use the same code > in a nat.c file as well of course. We have a very simple scheme here: there's a bit for every ISA extension that is set in HWCAP by the kernel when that extension is present as far as userspace is concerned. The code is probably easier to understand https://git.kernel.org/pub/scm/linux/kernel/git/palmer/riscv-linux.git/tree/arch/riscv/kernel/cpufeature.c#n33 We should probably but this in an ABI document somewhere... :)
On Okt 25 2018, Jim Wilson <jimw@sifive.com> wrote: > On Thu, Oct 25, 2018 at 10:55 AM John Baldwin <jhb@freebsd.org> wrote: >> Now that the MISA defaults to 0 if not present, would it better to just remove >> this and not set it to 0 explicitly? The FreeBSD native target for RISC-V >> doesn't set MISA to anything at all. > > There is still the issue of FP register size, which comes from MISA, > unless perhaps we can get it from auxvec/hw-cap info. I was going to > look into that latter, and if the auxvec/hw-cap stuff works, then > remove the remaining MISA support in the riscv-linux-nat.c file. Note you need commit 0ddc77556c to get a correct AT_HWCAP. Andreas.
diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c index 7dbfe651f2c..c09121d052b 100644 --- a/gdb/riscv-linux-nat.c +++ b/gdb/riscv-linux-nat.c @@ -201,10 +201,8 @@ riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum) if ((regnum == RISCV_CSR_MISA_REGNUM) || (regnum == -1)) - { - /* TODO: Need to add a ptrace call for this. */ - regcache->raw_supply_zeroed (regnum); - } + /* TODO: Need to add a ptrace call for this. */ + regcache->raw_supply_zeroed (RISCV_CSR_MISA_REGNUM); /* Access to other CSRs has potential security issues, don't support them for now. */