Message ID | 20180921092721.GY5952@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 66298 invoked by alias); 21 Sep 2018 09:27:28 -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 65323 invoked by uid 89); 21 Sep 2018 09:27:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.5 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=crazy, burgess, Burgess, hart X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Sep 2018 09:27:26 +0000 Received: by mail-wm1-f66.google.com with SMTP id t25-v6so2447380wmi.3 for <gdb-patches@sourceware.org>; Fri, 21 Sep 2018 02:27:25 -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:content-transfer-encoding:in-reply-to :user-agent; bh=xE+0CQ7jDil8GpNYrw4DOYpHOjaKd98yj9S0YXpvGCE=; b=adXwoeF1QEAQHLUCAls2FdHaYI437U70R7L3LCDWCxd9K2zbpci+TSjBT1M4T5Qur2 KsXCDa6E92r3JRuJE69LowEGScVukVzQ42qv3eyixbayaBxTBBNRKqG4RbjCW/Gfw9ih Lb4OH0FFJRUQRCmZtwaS7TQ3WSPJpwEhMwKAV7d2QLWGRcb5YUFzFfZ5Bz4PHbZRWmwv 4LjfCZvv5bP5+/F7bAio7sULWeJ4BRNmFSoMETLzdhM4SDz6vWQtazgSlBHiCV80GeGi +y1+nsoOCgAWPJI/bYFoLc7W8jt6sB7EyN4+vafV2pdIYwt2r4LlQyF1QRwfz2f96V6H SIuw== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host81-158-205-250.range81-158.btcentralplus.com. [81.158.205.250]) by smtp.gmail.com with ESMTPSA id a126-v6sm4524368wmh.1.2018.09.21.02.27.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 21 Sep 2018 02:27:23 -0700 (PDT) Date: Fri, 21 Sep 2018 10:27:21 +0100 From: Andrew Burgess <andrew.burgess@embecosm.com> To: John Baldwin <jhb@FreeBSD.org> Cc: Jim Wilson <jimw@sifive.com>, gdb-patches@sourceware.org, Palmer Dabbelt <palmer@sifive.com> Subject: Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register. Message-ID: <20180921092721.GY5952@embecosm.com> References: <20180919231950.22634-1-jhb@FreeBSD.org> <20180919231950.22634-3-jhb@FreeBSD.org> <CAFyWVaYuQASExDPvPHVOjf4X-Nwe3dGjOkZDbyRs3YKy_HZccA@mail.gmail.com> <0081bdf8-04cb-f6b7-d80a-d9a878d0a3ab@FreeBSD.org> <e6a0ee4f-1230-d48f-d085-a3bfb46527ab@FreeBSD.org> <20180920215146.GW5952@embecosm.com> <4e2b6d40-dc15-6caa-8520-90289ce972da@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <4e2b6d40-dc15-6caa-8520-90289ce972da@FreeBSD.org> X-Fortune: Can I have an IMPULSE ITEM instead? 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
Sept. 21, 2018, 9:27 a.m. UTC
* John Baldwin <jhb@FreeBSD.org> [2018-09-20 16:01:04 -0700]: > On 9/20/18 2:51 PM, Andrew Burgess wrote: > > * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]: > >> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr) > >> { > >> if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO) > >> { > >> - if (riscv_has_feature (gdbarch, 'C')) > >> + enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch); > > > > byte_order is unused. > > Will fix. > > >> + gdb_byte buf[1]; > >> + int status; > >> + > >> + /* Read the opcode byte to determine the instruction length. */ > >> + status = target_read_memory (*pcptr, buf, 1); > > > > This should use target_read_code. I know that we already have some > > (incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix > > those later. > > Ok. > > > However, this causes a testsuite regression for gdb.gdb/unittest.exp. > > You can easily reproduce the issue with: > > > > (gdb) maintenance selftest > > > > We probably need to add a riscv specific case into > > disasm-selftest.c:print_one_insn_test, lots of other targets already > > do. > > Ok. I'll reproduce that and figure out the fix and include it in a V2 > patch. > > One other question is if I drop the change to default MISA to 0, we should > perhaps fix the comment above riscv_read_misa? The comment implies that > it falls back to zero if it can't read the register and it does that for > the !target_has_registers case already. It's not clear from the comment > that targets are required to provide MISA. I'm kind-of mixed about the default 0 patch. The spec says: The misa CSR is an XLEN-bit WARL read-write register reporting the ISA supported by the hart. This register must be readable in any implementation, but a value of zero can be returned to indicate the misa register has not been implemented, requiring that CPU capabilities be determined through a separate non-standard mechanism. So, it doesn't seem to crazy to say that in GDB, if we make not one, but two attempts to find a MISA value, fail on both, then we could assume a default of 0. After all, the default of 0 just means, go figure out an answer for yourself, so we shouldn't be any worse off. Still, it would probably be a good thing if targets did just provide a 0 value for misa on their own as that would be more inline with the spec (I think). Having just looked at riscv_read_misa_reg again, it turns out that it's completely broken anyway. Take a look at how the READ_P parameter is initialised in the caller and then (not) set in function. Further, the one and only caller, checks READ_P /or/ for a return value of 0, so just defaulting to 0 would be fine. At a minimum we should apply this patch: ## START ## ## END ## Jim: Given that we agree that targets should definitely provide a value for misa, at a minimum just returning the constant 0. But, given that GDB already defaults to 0 in some cases anyway. And the spec is quite clear that 0 is the right default value in the absence of anything better, would you be OK with a patch that does return a default of 0? Thanks, Andrew > > -- > John Baldwin > >
Comments
On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > Jim: Given that we agree that targets should definitely provide a > value for misa, at a minimum just returning the constant 0. But, > given that GDB already defaults to 0 in some cases anyway. And the > spec is quite clear that 0 is the right default value in the absence > of anything better, would you be OK with a patch that does return a > default of 0? The patch to decode an instruction to decide whether to use a compressed breakpoint or not solves my main problem. There is also the issue of finding FP register size, but since we only support rv64gc at the moment, it isn't a serious problem. Also, I think the linker kernel may already be passing FP info via auxvec/hwcap, so I think we already have an alternate solution for that which just needs to be implemented. I haven't looked at that yet. So yes, I think it is OK to start defaulting misa to 0. FYI I have a qemu patch, which I may someday finish, that adds XML register support to the RISC-V qemu system-mode port, which allows qemu to provide a correct value of misa. We know that misa accesses already work with embedded targets via OpenOCD. So it is just linux and freebsd that need to worry about misa. The qemu patch is here, though it looks like github is confused by rebasing and the patch isn't readable anymore. https://github.com/riscv/riscv-qemu/pull/160 I'll have to figure out how to fix that. Jim
On 9/21/18 2:27 AM, Andrew Burgess wrote: > * John Baldwin <jhb@FreeBSD.org> [2018-09-20 16:01:04 -0700]: > >> On 9/20/18 2:51 PM, Andrew Burgess wrote: >>> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]: >> One other question is if I drop the change to default MISA to 0, we should >> perhaps fix the comment above riscv_read_misa? The comment implies that >> it falls back to zero if it can't read the register and it does that for >> the !target_has_registers case already. It's not clear from the comment >> that targets are required to provide MISA. > > I'm kind-of mixed about the default 0 patch. The spec says: > > The misa CSR is an XLEN-bit WARL read-write register reporting the > ISA supported by the hart. This register must be readable in any > implementation, but a value of zero can be returned to indicate > the misa register has not been implemented, requiring that CPU > capabilities be determined through a separate non-standard > mechanism. > > So, it doesn't seem to crazy to say that in GDB, if we make not one, > but two attempts to find a MISA value, fail on both, then we could > assume a default of 0. After all, the default of 0 just means, go > figure out an answer for yourself, so we shouldn't be any worse off. > > Still, it would probably be a good thing if targets did just provide a > 0 value for misa on their own as that would be more inline with the > spec (I think). > > Having just looked at riscv_read_misa_reg again, it turns out that > it's completely broken anyway. Take a look at how the READ_P > parameter is initialised in the caller and then (not) set in > function. > > Further, the one and only caller, checks READ_P /or/ for a return > value of 0, so just defaulting to 0 would be fine. At a minimum we > should apply this patch: > > ## START ## > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index 254914c9c7e..52e101e6ab8 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0; > default (false). */ > > static uint32_t > -riscv_read_misa_reg (bool *read_p) > +riscv_read_misa_reg () > { > uint32_t value = 0; > > @@ -334,13 +334,10 @@ riscv_read_misa_reg (bool *read_p) > static bool > riscv_has_feature (struct gdbarch *gdbarch, char feature) > { > - bool have_read_misa = false; > - uint32_t misa; > - > gdb_assert (feature >= 'A' && feature <= 'Z'); > > - misa = riscv_read_misa_reg (&have_read_misa); > - if (!have_read_misa || misa == 0) > + uint32_t misa = riscv_read_misa_reg (); > + if (misa == 0) > misa = gdbarch_tdep (gdbarch)->core_features; > > return (misa & (1 << (feature - 'A'))) != 0; > > ## END ## > > And, better still would be something more like your original patch: > > ## START ## > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index 254914c9c7e..58e4c221a7c 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0; > default (false). */ > > static uint32_t > -riscv_read_misa_reg (bool *read_p) > +riscv_read_misa_reg () > { > uint32_t value = 0; > > @@ -310,18 +310,23 @@ riscv_read_misa_reg (bool *read_p) > { > struct frame_info *frame = get_current_frame (); > > - TRY > - { > - value = get_frame_register_unsigned (frame, > - RISCV_CSR_MISA_REGNUM); > - } > - CATCH (ex, RETURN_MASK_ERROR) > + /* Old cores might have MISA located at a different offset. */ > + int misa_regs[] = > + { RISCV_CSR_MISA_REGNUM, RISCV_CSR_LEGACY_MISA_REGNUM }; > + > + for (int i = 0; i < ARRAY_SIZE (misa_regs); ++i) > { > - /* Old cores might have MISA located at a different offset. */ > - value = get_frame_register_unsigned (frame, > - RISCV_CSR_LEGACY_MISA_REGNUM); > + TRY > + { > + value = get_frame_register_unsigned (frame, misa_regs[i]); > + break; > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + /* Ignore the error. */ > + } > + END_CATCH > } > - END_CATCH > } > > return value; > @@ -334,13 +339,10 @@ riscv_read_misa_reg (bool *read_p) > static bool > riscv_has_feature (struct gdbarch *gdbarch, char feature) > { > - bool have_read_misa = false; > - uint32_t misa; > - > gdb_assert (feature >= 'A' && feature <= 'Z'); > > - misa = riscv_read_misa_reg (&have_read_misa); > - if (!have_read_misa || misa == 0) > + uint32_t misa = riscv_read_misa_reg (); > + if (misa == 0) > misa = gdbarch_tdep (gdbarch)->core_features; > > return (misa & (1 << (feature - 'A'))) != 0; > > ## END ## > > Jim: Given that we agree that targets should definitely provide a > value for misa, at a minimum just returning the constant 0. But, > given that GDB already defaults to 0 in some cases anyway. And the > spec is quite clear that 0 is the right default value in the absence > of anything better, would you be OK with a patch that does return a > default of 0? FWIW, I think the second patch looks fine. I can confirm that the breakpoint patch is sufficient for my limited testing on FreeBSD that I no longer need the MISA patch so I've dropped it from my local series (I'll post a V2 in a bit).
* Jim Wilson <jimw@sifive.com> [2018-09-21 10:25:47 -0700]: > On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess > <andrew.burgess@embecosm.com> wrote: > > Jim: Given that we agree that targets should definitely provide a > > value for misa, at a minimum just returning the constant 0. But, > > given that GDB already defaults to 0 in some cases anyway. And the > > spec is quite clear that 0 is the right default value in the absence > > of anything better, would you be OK with a patch that does return a > > default of 0? > > The patch to decode an instruction to decide whether to use a > compressed breakpoint or not solves my main problem. There is also > the issue of finding FP register size, but since we only support > rv64gc at the moment, it isn't a serious problem. I regularly test embeded RiscV against: rv32im rv32imc rv32imf rv32imfc rv64im rv64imc rv64imfd rv64imfdc with the last one of those being closes to rv64gc. The pass rate is broadly the same against all of these targets, so right now I consider these equally supported for baremetal. I understand Linux support might be different. Thanks, Andrew > Also, I think the > linker kernel may already be passing FP info via auxvec/hwcap, so I > think we already have an alternate solution for that which just needs > to be implemented. I haven't looked at that yet. So yes, I think it > is OK to start defaulting misa to 0. > > FYI I have a qemu patch, which I may someday finish, that adds XML > register support to the RISC-V qemu system-mode port, which allows > qemu to provide a correct value of misa. We know that misa accesses > already work with embedded targets via OpenOCD. So it is just linux > and freebsd that need to worry about misa. > > The qemu patch is here, though it looks like github is confused by > rebasing and the patch isn't readable anymore. > https://github.com/riscv/riscv-qemu/pull/160 > I'll have to figure out how to fix that. > > Jim
On Fri, 28 Sep 2018 02:43:55 PDT (-0700), andrew.burgess@embecosm.com wrote: > * Jim Wilson <jimw@sifive.com> [2018-09-21 10:25:47 -0700]: > >> On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess >> <andrew.burgess@embecosm.com> wrote: >> > Jim: Given that we agree that targets should definitely provide a >> > value for misa, at a minimum just returning the constant 0. But, >> > given that GDB already defaults to 0 in some cases anyway. And the >> > spec is quite clear that 0 is the right default value in the absence >> > of anything better, would you be OK with a patch that does return a >> > default of 0? >> >> The patch to decode an instruction to decide whether to use a >> compressed breakpoint or not solves my main problem. There is also >> the issue of finding FP register size, but since we only support >> rv64gc at the moment, it isn't a serious problem. > > I regularly test embeded RiscV against: > > rv32im rv32imc rv32imf rv32imfc > > rv64im rv64imc rv64imfd rv64imfdc > > with the last one of those being closes to rv64gc. The pass rate is > broadly the same against all of these targets, so right now I consider > these equally supported for baremetal. For those uniniated in RISC-V, "rv64gc" is the same as "rv64imafdc" (the G is short for IMAFD). Importantly it's probably close enough for embedded GDB testing, as all you're missing is the A extension and GDB doesn't really care about atomics. > I understand Linux support might be different. Right now all that's really supported in Linux land is rv64gc. The kernel should also build on rv64imac, rv32gc, and rv32imac but they're much less mature. The upstream glibc port supports rv64gc and rv64imac, but we test those only on rv64gc kernels. We try our best to avoid breaking the other targets, but until we get some better CI up and running I'd expect that non-rv64gc targets do keep falling apart. We're working on it :) On Linux you should be able to look at the HWCAP in the auxvec, which is meant to tell you what user state is available. The code to fill this out is here https://git.kernel.org/pub/scm/linux/kernel/git/palmer/riscv-linux.git/tree/arch/riscv/kernel/cpufeature.c#n26 > > Thanks, > > Andrew > > >> Also, I think the >> linker kernel may already be passing FP info via auxvec/hwcap, so I >> think we already have an alternate solution for that which just needs >> to be implemented. I haven't looked at that yet. So yes, I think it >> is OK to start defaulting misa to 0. >> >> FYI I have a qemu patch, which I may someday finish, that adds XML >> register support to the RISC-V qemu system-mode port, which allows >> qemu to provide a correct value of misa. We know that misa accesses >> already work with embedded targets via OpenOCD. So it is just linux >> and freebsd that need to worry about misa. >> >> The qemu patch is here, though it looks like github is confused by >> rebasing and the patch isn't readable anymore. >> https://github.com/riscv/riscv-qemu/pull/160 >> I'll have to figure out how to fix that. >> >> Jim
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index 254914c9c7e..52e101e6ab8 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0; default (false). */ static uint32_t -riscv_read_misa_reg (bool *read_p) +riscv_read_misa_reg () { uint32_t value = 0; @@ -334,13 +334,10 @@ riscv_read_misa_reg (bool *read_p) static bool riscv_has_feature (struct gdbarch *gdbarch, char feature) { - bool have_read_misa = false; - uint32_t misa; - gdb_assert (feature >= 'A' && feature <= 'Z'); - misa = riscv_read_misa_reg (&have_read_misa); - if (!have_read_misa || misa == 0) + uint32_t misa = riscv_read_misa_reg (); + if (misa == 0) misa = gdbarch_tdep (gdbarch)->core_features; return (misa & (1 << (feature - 'A'))) != 0; ## END ## And, better still would be something more like your original patch: ## START ## diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index 254914c9c7e..58e4c221a7c 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0; default (false). */ static uint32_t -riscv_read_misa_reg (bool *read_p) +riscv_read_misa_reg () { uint32_t value = 0; @@ -310,18 +310,23 @@ riscv_read_misa_reg (bool *read_p) { struct frame_info *frame = get_current_frame (); - TRY - { - value = get_frame_register_unsigned (frame, - RISCV_CSR_MISA_REGNUM); - } - CATCH (ex, RETURN_MASK_ERROR) + /* Old cores might have MISA located at a different offset. */ + int misa_regs[] = + { RISCV_CSR_MISA_REGNUM, RISCV_CSR_LEGACY_MISA_REGNUM }; + + for (int i = 0; i < ARRAY_SIZE (misa_regs); ++i) { - /* Old cores might have MISA located at a different offset. */ - value = get_frame_register_unsigned (frame, - RISCV_CSR_LEGACY_MISA_REGNUM); + TRY + { + value = get_frame_register_unsigned (frame, misa_regs[i]); + break; + } + CATCH (ex, RETURN_MASK_ERROR) + { + /* Ignore the error. */ + } + END_CATCH } - END_CATCH } return value; @@ -334,13 +339,10 @@ riscv_read_misa_reg (bool *read_p) static bool riscv_has_feature (struct gdbarch *gdbarch, char feature) { - bool have_read_misa = false; - uint32_t misa; - gdb_assert (feature >= 'A' && feature <= 'Z'); - misa = riscv_read_misa_reg (&have_read_misa); - if (!have_read_misa || misa == 0) + uint32_t misa = riscv_read_misa_reg (); + if (misa == 0) misa = gdbarch_tdep (gdbarch)->core_features; return (misa & (1 << (feature - 'A'))) != 0;