From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 954043857C71 for ; Fri, 2 Oct 2020 23:55:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 954043857C71 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 E89211E4B5; Fri, 2 Oct 2020 19:55:14 -0400 (EDT) Subject: Re: [PATCH] gdb: add support for handling core dumps on arm-none-eabi To: Paul Mathieu , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: Date: Fri, 2 Oct 2020 19:55:14 -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: Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Fri, 02 Oct 2020 23:55:17 -0000 Some quick comments, just skimming the patch. I won't comment about indentation issue, because I don't know if it has been modified by the email client. I support what Luis has already said, it needs to be documented what this format is, where it is defined/specified, who produces it, etc. On 2020-10-02 1:32 p.m., Paul Mathieu via Gdb-patches wrote: > Core dump files really help debugging crashes. > It is not uncommon for embedded targets to have the ability to generate > memory and CPU register dumps, which can easily be converted into core dump > files. > > This patch adds support for loading core files into gdb on arm-none-eabi > targets. > The patch was originally written by Robin Haberkorn < > robin.haberkorn@googlemail.com> > > gdb/ChangeLog: > 2018-09-29 Robin Haberkorn > 2020-10-02 Paul Mathieu > > * arm-none-tdep.c: Added. Provide CPU registers from a core file > * floating point registers not yet supported (FIXME) > > > --- > gdb/Makefile.in | 2 + > gdb/arm-none-tdep.c | 140 ++++++++++++++++++++++++++++++++++++++++++++ > gdb/configure.tgt | 2 +- > 3 files changed, 143 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 \ > 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..7641a9f7f0 > --- /dev/null > +++ b/gdb/arm-none-tdep.c > @@ -0,0 +1,140 @@ > +/* Native-dependent code for GDB targetting embedded ARM. Maybe "bare-metal ARM" would be more precise? Embedded doesn't mean "no OS", which I think is what you want to mean here. > + > + 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 "command.h" > +#include "gdbarch.h" > +#include "gdbcore.h" > +#include "inferior.h" > +#include "target.h" > +#include "regcache.h" > + > +#include "arch/arm.h" > + > +#if 0 > +#include > +#include > +#ifdef HAVE_SYS_PROCFS_H > +#include > +#endif > +#endif > + > +typedef struct { > + uint32_t reg[18]; > +} gdb_gregset_t; Use: struct gdb_gregset_t { ... }; We are in C++, so when you use it you can still omit the "struct" keyword. > + > +#define ARM_CPSR_GREGNUM 16 > + > +extern int arm_apcs_32; Don't declare this here, there's a suitable declaration in arm-tdep.h. > + > +static void > +arm_supply_gregset (struct regcache *regcache, const gdb_gregset_t *gregs) > +{ > + struct gdbarch *gdbarch = regcache->arch (); > + enum bfd_endian byte_order = type_byte_order (register_type(gdbarch, 0)); > + int regno; > + CORE_ADDR reg_pc; > + gdb_byte pc_buf[ARM_INT_REGISTER_SIZE]; > + > + for (regno = ARM_A1_REGNUM; regno < ARM_PC_REGNUM; regno++) > + regcache->raw_supply (regno, gregs->reg + regno); > + > + if (arm_apcs_32) > + regcache->raw_supply (ARM_PS_REGNUM, gregs->reg + ARM_CPSR_GREGNUM); > + else > + regcache->raw_supply (ARM_PS_REGNUM, gregs->reg + ARM_PC_REGNUM); > + > + reg_pc = extract_unsigned_integer ((const gdb_byte*)(gregs->reg + > ARM_PC_REGNUM), Space before *, and after the cast: (const gdb_byte *) (gregs...) Simon