From patchwork Wed Dec 5 13:31:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 30548 Received: (qmail 87861 invoked by alias); 5 Dec 2018 13:32:01 -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 87842 invoked by uid 89); 5 Dec 2018 13:32:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=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=U*jimw, wilson, jimw@sifive.com, Wilson X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Dec 2018 13:31:58 +0000 Received: by mail-wr1-f66.google.com with SMTP id r10so19673810wrs.10 for ; Wed, 05 Dec 2018 05:31:57 -0800 (PST) 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=8jA+a2vOfZtay7/hls8fe85FvKGeshGKPZ2wL2JHEd0=; b=f0bpjmMo2w7Cam/uE2rSf5jMgYbpZZm2N7klwWTlGD52fJyWP88Ncp0SbHlHAbX1tI wHgAWk6+X+HSCxu0z36rgtwV73hgcZf1+JwET3Lb4O5l1HI6us9hkPD54idVo3KjPXkv YtXIhSVXhiNtLgIBwN2zj9krrqJNV6mucykoE2OjVsflZDMyv9kKVIFwpm0B7mZWXeXi ieetkys+Tzyuv3ZtlazCr93wqMq1Dca4twtVg/A3PF7dZ32viaxo0oScgvx3sn9MLpI+ HONAtveW3FUyIM+myEhTA0+jOSWc90bDiCKs2ghSXP+U3X5BfOIhOLO21dRXFqRQ8t/i i3fQ== Return-Path: Received: from localhost (host86-156-236-210.range86-156.btcentralplus.com. [86.156.236.210]) by smtp.gmail.com with ESMTPSA id o17sm11157750wmg.35.2018.12.05.05.31.54 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 05 Dec 2018 05:31:55 -0800 (PST) Date: Wed, 5 Dec 2018 13:31:53 +0000 From: Andrew Burgess To: Jim Wilson Cc: gdb-patches@sourceware.org, Palmer Dabbelt , John Baldwin Subject: Re: [PATCH] gdb/riscv: Improve logic for when h/w float abi should be used Message-ID: <20181205133153.GR18841@embecosm.com> References: <20181204143803.18231-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: -- I have seen the FUN -- X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Jim Wilson [2018-12-04 09:59:23 -0800]: > On Tue, Dec 4, 2018 at 6:38 AM Andrew Burgess > wrote: > > If the current BFD is not an ELF then we don't currently have a > > mechanism for figuring out if the file was compiled for float or not. > > In this case we (currently) disable h/w float abi. We might want to > > improve this case in the future. > > The linker only supports ELF, and gives an error if the target is not > ELF. You can use objcopy to convert to other formats, but that really > only works for raw formats which gdb probably can't use. Thanks for the feedback. I pushed this patch with a slightly updated commit message. Andrew --- gdb/riscv: Improve logic for when h/w float abi should be used Currently, if the target announces that it has floating point registers in its target description then GDB assumes that the hardware float ABI should be used. However, there's nothing stopping a user compiling a program for the soft-float abi, and then trying to run this on a target with hardware floating point registers. This commit adjusts the logic that decides if GDB should use the hardware float abi. The primary decision now is based on what the ELF currently being executed says in its headers. If the file was compiled for h/w float abi, then GDB uses h/w float abi, otherwise s/w float is used. If the current BFD is not an ELF then we don't currently have a mechanism for figuring out if the file was compiled for float or not. In this case we disable the h/w float abi. This shouldn't be a problem as, right now, the RISC-V linker can only produce ELFs. If there is NO current BFD (can this happen?) then we will enable h/w float abi if the target has floating point hardware, otherwise, s/w float abi is used. This commit also adds some sanity checking that the features requested in the BFD (xlen and flen) match the target description. For testing I ran the testsuite on a target that returns a target description containing both integer and floating point registers, but used a compiler that didn't have floating point support. Before this commit I would see failures on may tests that made inferior calls using floating point arguments, after this commit, all of these issues are resolved. One example from the testsuite is gdb.base/infcall-nested-structs.exp. gdb/ChangeLog: * riscv-tdep.c (riscv_features_from_gdbarch_info): New function. (riscv_find_default_target_description): Use new function to extract feature from gdbarch_info. (riscv_gdbarch_init): Add error checks for xlen and flen between target description and bfd headers. Be smarter about when we think the hardware floating point abi should be used. --- gdb/ChangeLog | 9 +++++++++ gdb/riscv-tdep.c | 61 +++++++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index 30c777db6f2..d812b7c8693 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -2789,20 +2789,16 @@ static const struct frame_unwind riscv_frame_unwind = /*.prev_arch =*/ NULL, }; -/* Find a suitable default target description. Use the contents of INFO, - specifically the bfd object being executed, to guide the selection of a - suitable default target description. */ +/* Extract a set of required target features out of INFO, specifically the + bfd being executed is examined to see what target features it requires. + IF there is no current bfd, or the bfd doesn't indicate any useful + features then a RISCV_GDBARCH_FEATURES is returned in its default state. */ -static const struct target_desc * -riscv_find_default_target_description (const struct gdbarch_info info) +static struct riscv_gdbarch_features +riscv_features_from_gdbarch_info (const struct gdbarch_info info) { struct riscv_gdbarch_features features; - /* Setup some arbitrary defaults. */ - features.xlen = 8; - features.flen = 0; - features.hw_float_abi = false; - /* Now try to improve on the defaults by looking at the binary we are going to execute. We assume the user knows what they are doing and that the target will match the binary. Remember, this code path is @@ -2847,6 +2843,26 @@ riscv_find_default_target_description (const struct gdbarch_info info) binfo->bits_per_word); } + return features; +} + +/* Find a suitable default target description. Use the contents of INFO, + specifically the bfd object being executed, to guide the selection of a + suitable default target description. */ + +static const struct target_desc * +riscv_find_default_target_description (const struct gdbarch_info info) +{ + /* Extract desired feature set from INFO. */ + struct riscv_gdbarch_features features + = riscv_features_from_gdbarch_info (info); + + /* If the XLEN field is still 0 then we got nothing useful from INFO. In + this case we fall back to a minimal useful target, 8-byte x-registers, + with no floating point. */ + if (features.xlen == 0) + features.xlen = 8; + /* Now build a target description based on the feature set. */ return riscv_create_target_description (features); } @@ -2981,7 +2997,6 @@ riscv_gdbarch_init (struct gdbarch_info info, int bitsize = tdesc_register_bitsize (feature_fpu, "ft0"); features.flen = (bitsize / 8); - features.hw_float_abi = true; if (riscv_debug_gdbarch) fprintf_filtered @@ -2991,7 +3006,6 @@ riscv_gdbarch_init (struct gdbarch_info info, else { features.flen = 0; - features.hw_float_abi = false; if (riscv_debug_gdbarch) fprintf_filtered @@ -3015,6 +3029,29 @@ riscv_gdbarch_init (struct gdbarch_info info, return NULL; } + /* Have a look at what the supplied (if any) bfd object requires of the + target, then check that this matches with what the target is + providing. */ + struct riscv_gdbarch_features info_features + = riscv_features_from_gdbarch_info (info); + if (info_features.xlen != 0 && info_features.xlen != features.xlen) + error (_("bfd requires xlen %d, but target has xlen %d"), + info_features.xlen, features.xlen); + if (info_features.flen != 0 && info_features.flen != features.flen) + error (_("bfd requires flen %d, but target has flen %d"), + info_features.flen, features.flen); + + /* If the xlen from INFO_FEATURES is 0 then this indicates either there + is no bfd object, or nothing useful could be extracted from it, in + this case we enable hardware float abi if the target has floating + point registers. + + If the xlen from INFO_FEATURES is not 0, and the flen in + INFO_FEATURES is also not 0, then this indicates that the supplied + bfd does require hardware floating point abi. */ + if (info_features.xlen == 0 || info_features.flen != 0) + features.hw_float_abi = (features.flen > 0); + /* Find a candidate among the list of pre-declared architectures. */ for (arches = gdbarch_list_lookup_by_info (arches, &info); arches != NULL;