From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id UvEFMqhdel+DcwAAWB0awg (envelope-from ) for ; Sun, 04 Oct 2020 19:41:28 -0400 Received: by simark.ca (Postfix, from userid 112) id BF6D21EE0F; Sun, 4 Oct 2020 19:41:28 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=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 2A3EB1E58E for ; Sun, 4 Oct 2020 19:41:27 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A549C385781B; Sun, 4 Oct 2020 23:41:26 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 5C55A385781B for ; Sun, 4 Oct 2020 23:41:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5C55A385781B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 87DB31E58E; Sun, 4 Oct 2020 19:41:23 -0400 (EDT) Subject: Re: [PATCH v2] gdb: add support for handling core dumps on arm-none-eabi To: Paul Mathieu , gdb-patches@sourceware.org References: <20201003181451.GA2211174@google.com> From: Simon Marchi Message-ID: Date: Sun, 4 Oct 2020 19:41:22 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20201003181451.GA2211174@google.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-10-03 2:14 p.m., Paul Mathieu via Gdb-patches wrote: > Thanks Simon for the super helpful feedback! > A few answers inline, and hopefully a properly formatted patch at the > bottom. > >> Note that I am unable to apply this patch either because long lines were >> again wrapped by your email client. Please use git-send-email. You can >> still reply in the thread by using --in-reply-to. > > Couldn't get git send-email to work with my SMTP, but I got some version > of mutt to comply. Hopefully you can apply the patch this time. Thanks, I am now able to apply the patch. However, you would it to export the patch using "git-format-patch", so that we get the full patch including the commit message (since we review the commit messages to make sure they include all the relevant information). >> Ok, so that answers the question in my other email, about which tool >> produces this format of core. The answer is this specific python >> script, and the format is "Paul's ARM core format" :). > > It was some team work between what gdb understood "natively" and what > the patch does. > More specifically, this patch only seems to tell gdb to get the > registers from the core file the way it would normally do, and then > supply them almost verbatim to the regcache. > The struct for these registers is nothing special either, you can find > it in the kernel code as well: > https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135 Ok, so that is some useful information to include in the code comments and / or in the commit log, that the register layout used for bare-metal core dumps was chosen to be the same as that of the Linux kernel. >> I don't know what Luis thinks about this, but I would be a bit hesitant >> to add support in GDB for a core format that's not standard nor the >> output of some "well-known" tool (which would be a de facto standard). >> >> Is there a format that already exists in the ARM ecosystem for core >> dumps of bare-metal systems, we could base our stuff on? > > I'm not aware of anything that does that. There are tools that generate > memory dumps and CPU register dumps into their own proprietary format, > but nothing that I know that you could call a 'core file'. > > Are there other tools besides gdb that deal with core dumps? Maybe that > could help me find other formats. I don't know any, but I don't have much experience in embedded development. > That being said, the format used here is "well-known" in the sense that > it's the exact same format the linux kernel would use to dump cores on > arm-linux-* That sounds like a good choice to me. As I said above, I think it should be mentioned in some comments. >> Alternatively, do you think you could implement GDB's generate-core-file >> command for bare-metal ARM? First, it would make sense for GDB to be >> able to produce a core in some format and be able to ingest it again. >> And it would act as some kind of documentation / reference >> implementation of what GDB expects, and that could become some de facto >> standard. People who would like to have their core readable by GDB >> would produce it in this format. > > I had never cloned the gdb source repo until 2 days ago. I have no idea > how much work that would be, given that I know next to nothing about the > gdb codebase. > > I do agree that this seems like a better way to do it, since there are > already many integrations with a gdb server talking to a live bare-metal > target over a physical debugger. > > As long as gdb already supports primitives to grab memory dumps and cpu > registers of a remote target, I'd imagine generating a core file > shouldn't be too much work. I'm quite sure most of that functionality > already exists for extremely similar targets. You can start at function "gcore_command" and pull on that thread. It seems like you could implement the gdbarch_make_corefile_notes method for the gdbarch for bare-metal ARM. To see examples, look for uses of set_gdbarch_make_corefile_notes. > Having a sniffer for GDB_OSABI_NONE seems like the right way to do this. > > Since the format isn't specified (yet), I can still freely control it. I > imagine I could add some sort of OSABI marker in the core file to mark > it as an arm-none-eabi core.o > > My first guess was to set the EI_OSABI e_ident elf header field to > ELFOSABI_NONE, but that happens to be the same enum value as > ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core > files. So it wouldn't be a good marker. > > Not sure what a better way would be to not abuse the ELF structure and > produce reasonable ELF core dumps (since they already work so well with > gdb). I really don't know either. Technically, it could be as simple as adding a new section / note of your choice, but I don't know if that would be an acceptable thing to do. I sent comments on the second patch you sent (let's call it 1.1), but not all of them seem to have been fixed, so here they are again. > gdb/Changelog > 2018-09-29 Robin Haberkorn > 2020-10-02 Paul Mathieu > * arm-none-tdep.c: Source file added. Provide CPU registers from a core > file > * floating point registers not yet supported (FIXME) > --- > gdb/Makefile.in | 2 + > gdb/arm-none-tdep.c | 106 ++++++++++++++++++++++++++++++++++++++++++++ > gdb/configure.tgt | 2 +- > 3 files changed, 109 insertions(+), 1 deletion(-) > create mode 100644 gdb/arm-none-tdep.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index dbede7a9cf..7f0e3ea0b0 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -720,6 +720,7 @@ ALL_TARGET_OBS = \ > arm-obsd-tdep.o \ > arm-pikeos-tdep.o \ > arm-symbian-tdep.o \ > + arm-none-tdep.o \ Please keep the lists sorted. Note that so far, everything related to bare-metal for an architecture was simply in -tdep.c. So perhaps it should just be there? I'll defer to Luis to decide what is the best organization for the ARM code. > arm-tdep.o \ > arm-wince-tdep.o \ > avr-tdep.o \ > @@ -2150,6 +2151,7 @@ ALLDEPFILES = \ > arm-nbsd-tdep.c \ > arm-obsd-tdep.c \ > arm-symbian-tdep.c \ > + arm-none-tdep.c \ > arm-tdep.c \ > avr-tdep.c \ > bfin-linux-tdep.c \ > diff --git a/gdb/arm-none-tdep.c b/gdb/arm-none-tdep.c > new file mode 100644 > index 0000000000..21743c40a0 > --- /dev/null > +++ b/gdb/arm-none-tdep.c > @@ -0,0 +1,106 @@ > +/* Native-dependent code for GDB targetting bare-metal ARM. Actually, this should say "Target-dependent", not "native-dependent". "native" in GDB means things used when GDB runs directly on the target platform. > + > + Copyright (C) 2020 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + 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 . */ > + > +#include "defs.h" > +#include "osabi.h" > + > +#include "arch/arm.h" > +#include "arm-tdep.h" > +#include "gdbarch.h" > +#include "regcache.h" > +#include "regset.h" > + > +struct arm_none_reg > +{ > + uint32_t reg[13]; > + uint32_t sp; > + uint32_t lr; > + uint32_t pc; > + uint32_t cpsr; > + uint32_t orig_r0; > +}; So this is the layout of the registers found in the core? I would add a comment on top of it, saying just that. And that there's no standard layout for register layouts in ARM bare-metal cores, but it was chosen to use the same layout as the Linux kernel uses. > + > +static void > +arm_none_supply_gregset (const struct regset *regset, > + struct regcache *regcache, int regnum, > + const void *gregs, size_t len) > +{ > + const arm_none_reg *gregset = static_cast (gregs); > + gdb_assert (len >= sizeof (arm_none_reg)); > + > + /* Integer registers. */ > + for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++) > + if (regnum == -1 || regnum == i) > + regcache->raw_supply (i, (char *)&gregset->reg[i]); Space after the cast (apply everywhere). > +static const struct regset arm_none_regset > + = { nullptr, arm_none_supply_gregset, > + /* We don't need a collect function because we only use this reading > + registers (via iterate_over_regset_sections and > + fetch_regs/fetch_register). */ > + nullptr, 0 }; > + > +static void > +arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch, > + iterate_over_regset_sections_cb *cb, > + void *cb_data, > + const struct regcache *regcache) > +{ > + cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg), &arm_none_regset, > + NULL, cb_data); > +} > + > +static void arm_none_elf_init_abi (struct gdbarch_info info, > + struct gdbarch *gdbarch); You don't need this declaration. > +static void > +arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > +{ > + set_gdbarch_iterate_over_regset_sections ( > + gdbarch, arm_none_iterate_over_regset_sections); > +} > + > +void _initialize_arm_none_tdep (void); > +void > +_initialize_arm_none_tdep (void) Remove the "void" in the lines above. > +{ > + gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX, > + arm_none_elf_init_abi); As mentioned previously, this should be GDB_OSABI_NONE, not GDB_OSABI_LINUX. Like this, it GDB aborts on startup when configured with --enable-targets=all. Simon