From patchwork Fri Sep 21 09:27:21 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 29498 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: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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 ; 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: 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 To: John Baldwin Cc: Jim Wilson , gdb-patches@sourceware.org, Palmer Dabbelt 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> <0081bdf8-04cb-f6b7-d80a-d9a878d0a3ab@FreeBSD.org> <20180920215146.GW5952@embecosm.com> <4e2b6d40-dc15-6caa-8520-90289ce972da@FreeBSD.org> MIME-Version: 1.0 Content-Disposition: inline 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 * 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]: > >> @@ -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 > > 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;