From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id GKq3L51ELmDCAgAAWB0awg (envelope-from ) for ; Thu, 18 Feb 2021 05:42:37 -0500 Received: by simark.ca (Postfix, from userid 112) id B54CA1EF78; Thu, 18 Feb 2021 05:42:37 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id F149C1E4A3 for ; Thu, 18 Feb 2021 05:42:35 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 73C69384B106; Thu, 18 Feb 2021 10:42:35 +0000 (GMT) Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id 4F4E5384B106 for ; Thu, 18 Feb 2021 10:42:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4F4E5384B106 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x32a.google.com with SMTP id o10so2551177wmc.1 for ; Thu, 18 Feb 2021 02:42:32 -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; bh=lH69kAHDhHdYUpyT8xklIA1acb9snJGeFffjHBuy+w8=; b=NiLQeLMBUV2xk/xmhXu2TuYfZybZo7+BNDN6uw8WURhIYlmhfx9TpsQVXNoa5gGXkg ZGWGv6vBx+oocOpqDVRuE88QW3qcqMGmA/9zUphidTQ3djDD32RlBAgBFMYMaMP9DYw5 E+DZPeEVOu4gG6Lzy/ZA0n4Qt4iFVYsL8/CRQo5ve5Tauz9A+vre50A0Qx03BozPJDs/ FU5uQrFTxcgKw3AAtRvZDzXg2gYUGv2zAsl+Fnql8Ararrt3UjawKx2MBD95qHn6tEuY CvxSEuXwEycQqrQcc89+HlXdweE4uyb4bF6ilmgnBLEJgjvTenQ+1YZU9FHK5R2g6aab cb+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=lH69kAHDhHdYUpyT8xklIA1acb9snJGeFffjHBuy+w8=; b=fSaNkdd3Xif/3CyZ2tgyWmxz5vnwG8N1Lzx/VhE/JxSVA1EGn5gNoto9fLI4tczd30 itTE2lMz4BU+UsJ9RATttdwiAGLtCMMUEw6/xr98mjqcI7DZrxFFmeqL0ZBrekDwaNCv ErQtZg+afUQ/3xUnm91dXl3dRntXKhPDpdYBv7JWTgAeeKh/OOCPkY1tHeGBM+jt7P0u Zt0IQAPMnv4uTRjaHBJ4R/fNWcpDdJ/t1ouCoSTezOyiAm/4ndr/SfYF0qdWI3sB9hF8 fs72LfbXSj7aHdUr6DN5ZF+8uVJiAyuI5ednECFeAFI1bdB3mJERFq1jSDALBg1Lq0WV ptfA== X-Gm-Message-State: AOAM5301L7s8hxd+7B7sixbsZra763G8X//OeZD4YjhbtE1+Dce0Yah2 sGrHKoHrDbliTnk/nHtGx37G5WSti7Y5FQ== X-Google-Smtp-Source: ABdhPJyLmIqgYpgk79dg5XBlIMufmg8As6GA/2gFIBNWbYFAs4XFCaAGRRdqBHxczFeTDGUujU39kQ== X-Received: by 2002:a05:600c:210f:: with SMTP id u15mr2942715wml.126.1613644951255; Thu, 18 Feb 2021 02:42:31 -0800 (PST) Received: from localhost (host86-186-80-154.range86-186.btcentralplus.com. [86.186.80.154]) by smtp.gmail.com with ESMTPSA id f7sm6813219wmh.39.2021.02.18.02.42.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Feb 2021 02:42:30 -0800 (PST) Date: Thu, 18 Feb 2021 10:42:29 +0000 From: Andrew Burgess To: Simon Marchi Subject: Re: [PATCH] gdb/riscv: select rv32 target by default when requested Message-ID: <20210218104229.GY265215@embecosm.com> References: <20210204204426.608948-1-andrew.burgess@embecosm.com> <66f3ed11-4296-afdf-22f0-9157ddf5b6d5@polymtl.ca> <20210208103116.GB265215@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210208103116.GB265215@embecosm.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 10:39:54 up 71 days, 15:24, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Simon, I know you're pretty busy right now. I'm planning to push this commit in the next couple of days. You queries were all about whether we could/should extend the tests to be run in more cases. I still don't think this is would work, but if when you have time you can explain why I'm wrong I'll be happy to update the test case. Thanks, Andrew * Andrew Burgess [2021-02-08 10:31:16 +0000]: > * Simon Marchi [2021-02-05 11:23:21 -0500]: > > > On 2021-02-04 3:44 p.m., Andrew Burgess wrote: > > > GDB for RISC-V always uses target descriptions. When the target > > > doesn't provide a target description then a default is selected. > > > Usually this default is selected based on the properties of the > > > executable being debugged. However, when there is no executable being > > > debugged we currently fallback to the riscv:rv64 target description as > > > the default. This leads to strange behaviour like this: > > > > > > $ gdb > > > (gdb) set architecture riscv:rv32 > > > (gdb) p sizeof ($pc) > > > $1 = 8 > > > > > > Despite the users specifically setting the architecture to riscv:rv32 > > > GDB still thinks that the target has riscv:rv64 register sizes. > > > > > > The above is a bit of a contrived situation. I actually ran into this > > > situation while trying to connect to a running riscv:rv32 target > > > without supplying an executable (the target didn't provide a target > > > description). When I tried to set a register on the target I ran into > > > errors because GDB was passing 8 bytes to the target rather than the > > > expected 4. Even when I manually specified the architecture (as > > > above) I couldn't convince GDB to only send 4 bytes. > > > > > > This patch fixes this issue. Now, when we selected a default target > > > description we will make use of the user selected architecture to > > > guide our choice. In the above example we now get: > > > > > > $ gdb > > > (gdb) set architecture riscv:rv32 > > > (gdb) p sizeof ($pc) > > > $1 = 4 > > > > > > And my real world example of connecting to a remote without an > > > executable works fine. > > > > > > I've used the fact that we can ask GDB about $pc even when no > > > executable is loaded as the basis for a test to cover this situation. > > > > > > gdb/ChangeLog: > > > > > > * riscv-tdep.c (riscv_features_from_gdbarch_info): Rename to... > > > (riscv_features_from_bfd): ...this. Change parameter type to > > > 'bfd*', and update as required. > > > (riscv_find_default_target_description): Update call to > > > riscv_features_from_bfd. Select a default xlen based on > > > info.bfd_arch_info. > > > (riscv_gdbarch_init): Update call to riscv_features_from_bfd. > > > > > > gdb/testsuite/ChangeLog: > > > > > > * gdb.arch/riscv-default-tdesc.exp: New file. > > > --- > > > gdb/ChangeLog | 10 ++++ > > > gdb/riscv-tdep.c | 28 ++++----- > > > gdb/testsuite/ChangeLog | 4 ++ > > > .../gdb.arch/riscv-default-tdesc.exp | 59 +++++++++++++++++++ > > > 4 files changed, 86 insertions(+), 15 deletions(-) > > > create mode 100644 gdb/testsuite/gdb.arch/riscv-default-tdesc.exp > > > > > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > > > index 460746a9bfe..2a7b792893d 100644 > > > --- a/gdb/riscv-tdep.c > > > +++ b/gdb/riscv-tdep.c > > > @@ -3215,13 +3215,11 @@ static const struct frame_unwind riscv_frame_unwind = > > > /*.prev_arch =*/ NULL, > > > }; > > > > > > -/* 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. */ > > > +/* Extract a set of required target features out of ABFD. If ABFD is > > > + nullptr then a RISCV_GDBARCH_FEATURES is returned in its default state. */ > > > > > > static struct riscv_gdbarch_features > > > -riscv_features_from_gdbarch_info (const struct gdbarch_info info) > > > +riscv_features_from_bfd (const bfd *abfd) > > > { > > > struct riscv_gdbarch_features features; > > > > > > @@ -3231,11 +3229,10 @@ riscv_features_from_gdbarch_info (const struct gdbarch_info info) > > > only used at all if the target hasn't given us a description, so this > > > is really a last ditched effort to do something sane before giving > > > up. */ > > > - if (info.abfd != NULL > > > - && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour) > > > + if (abfd != nullptr && bfd_get_flavour (abfd) == bfd_target_elf_flavour) > > > { > > > - unsigned char eclass = elf_elfheader (info.abfd)->e_ident[EI_CLASS]; > > > - int e_flags = elf_elfheader (info.abfd)->e_flags; > > > + unsigned char eclass = elf_elfheader (abfd)->e_ident[EI_CLASS]; > > > + int e_flags = elf_elfheader (abfd)->e_flags; > > > > > > if (eclass == ELFCLASS32) > > > features.xlen = 4; > > > @@ -3273,13 +3270,14 @@ 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); > > > + = riscv_features_from_bfd (info.abfd); > > > > > > - /* 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 the XLEN field is still 0 then we got nothing useful from INFO.BFD, > > > + maybe there was no bfd object. In this case we fall back to a minimal > > > + useful target with no floating point, the x-register size is selected > > > + based on the architecture from INFO. */ > > > if (features.xlen == 0) > > > - features.xlen = 8; > > > + features.xlen = info.bfd_arch_info->bits_per_word == 32 ? 4 : 8; > > > > > > /* Now build a target description based on the feature set. */ > > > return riscv_lookup_target_description (features); > > > @@ -3497,7 +3495,7 @@ riscv_gdbarch_init (struct gdbarch_info info, > > > target, then check that this matches with what the target is > > > providing. */ > > > struct riscv_gdbarch_features abi_features > > > - = riscv_features_from_gdbarch_info (info); > > > + = riscv_features_from_bfd (info.abfd); > > > > > > /* If the ABI_FEATURES xlen is 0 then this indicates we got no useful abi > > > features from the INFO object. In this case we just treat the > > > diff --git a/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp b/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp > > > new file mode 100644 > > > index 00000000000..2b7e8fa63e8 > > > --- /dev/null > > > +++ b/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp > > > @@ -0,0 +1,59 @@ > > > +# Copyright 2021 Free Software Foundation, Inc. > > > +# > > > +# This program is free software; you can redistribute it and/or modify > > > +# it under the terms of the GNU General Public License as published by > > > +# the Free Software Foundation; either version 3 of the License, or > > > +# (at your option) any later version. > > > +# > > > +# This program is distributed in the hope that it will be useful, > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > +# GNU General Public License for more details. > > > +# > > > +# You should have received a copy of the GNU General Public License > > > +# along with this program. If not, see . > > > + > > > +# Check the size of the $pc register as the user changes the selected > > > +# architecture. As no executable is provided then the size of the $pc > > > +# register is defined by the default target description selected by > > > +# GDB. > > > +# > > > +# This test ensures that GDB is selecting an RV32 default if the user > > > +# has forced the current architecture to be riscv:rv32. > > > +# > > > +# In all other cases the default will be RV64. > > > + > > > +if {![istarget "riscv*-*-*"]} { > > > + verbose "Skipping ${gdb_test_file_name}." > > > + return > > > +} > > > > From what I can see, this test does not run anything, it should be > > possible to run it even if the target triplet isn't riscv*-*-*, but as > > long as support for riscv targets was compiled in (for example with > > --enable-targets=all). If so, maybe the check could be adjusted to > > check for that, so that the test would run when I "make check" on my > > x86-64 all targets build? > > I don't think that would work as the default architecture is unlikely > to be RISC-V, which is what the test is really about testing. > > I agree with the principle of limiting architecture specific tests, > but I think this test truly is RISC-V only. > > > > > Although... > > > > > +# Start GDB with no executable. > > > +gdb_start > > > + > > > +# The tests are defined by a list of architecture names to switch too > > > +# and the expected size of $pc. The first list entry is special and > > > +# has an empty architecture string, this checks GDB's default value on > > > +# startup. > > > +foreach data {{"" 8} {"riscv:rv32" 4} {"riscv:rv64" 8} {"riscv" 8} \ > > > + {"auto" 8}} { > > > + set arch [lindex $data 0] > > > + set size [lindex $data 1] > > > + > > > + # Switch architecture. > > > + if { $arch != "" && $arch != "auto" } { > > > + gdb_test "set architecture $arch" \ > > > + "The target architecture is set to \"$arch\"\\." > > > + } elseif { $arch == "auto" } { > > > + gdb_test "set architecture $arch" \ > > > + "The target architecture is set to \"auto\" \\(currently \"riscv\"\\)\\." > > > + } else { > > > + set arch "default architecture" > > > + } > > > + > > > + # Check the size of $pc. > > > + with_test_prefix "$arch" { > > > + gdb_test "p sizeof (\$pc)" " = $size" \ > > > + "size of \$pc register is $size" > > > + } > > > +} > > > > > > > I don't really understand how the test is supposed to work, how can you > > read the $pc if you don't execute any program? > > We're not reading the register, just its size, which only requires GDB > to have a valid gdbarch, which is always the case. > > As part of this test is specifically about what is the default RISC-V > architecture if I waited until I'd passed an executable before asking > then GDB would already have adapted to take on a gdbarch matching the > executable. > > The same basically works for x86-64 and all other architectures that I > tested, with one caveat, $pc handling for x86-64 is a little odd, I > guess because the register isn't really called $pc. As a result it > seems that we can only ask about $pc on x86-64 once the target has > started. But you can ask about any real register before starting, so: > > $ gdb > (gdb) p sizeof ($eip) > $1 = 4 > (gdb) > > Works fine. > >