From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10881 invoked by alias); 24 Sep 2018 20:35:56 -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 10871 invoked by uid 89); 24 Sep 2018 20:35:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 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=jim, crazy, Jim, Fall 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; Mon, 24 Sep 2018 20:35:52 +0000 Received: from John-Baldwins-MacBook-Pro-2.local (ralph.baldwin.cx [66.234.199.215]) by mail.baldwin.cx (Postfix) with ESMTPSA id 1441510AFCD; Mon, 24 Sep 2018 16:35:49 -0400 (EDT) Subject: Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register. To: Andrew Burgess References: <20180919231950.22634-1-jhb@FreeBSD.org> <20180919231950.22634-3-jhb@FreeBSD.org> <0081bdf8-04cb-f6b7-d80a-d9a878d0a3ab@FreeBSD.org> <20180920215146.GW5952@embecosm.com> <4e2b6d40-dc15-6caa-8520-90289ce972da@FreeBSD.org> <20180921092721.GY5952@embecosm.com> Cc: Jim Wilson , gdb-patches@sourceware.org, Palmer Dabbelt From: John Baldwin Message-ID: <17b2d037-8a81-0f1a-5698-d035ef70a242@FreeBSD.org> Date: Mon, 24 Sep 2018 20:35:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180921092721.GY5952@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00813.txt.bz2 On 9/21/18 2:27 AM, Andrew Burgess wrote: > * John Baldwin [2018-09-20 16:01:04 -0700]: > >> On 9/20/18 2:51 PM, Andrew Burgess wrote: >>> * John Baldwin [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). -- John Baldwin                                                                            Â