Re: [PATCH] gdb: add support for handling core dumps on arm-none-eabi

Simon Marchi
2020-10-02T23:55:19Z

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 <robin.haberkorn@googlemail.com>
> 2020-10-02 Paul Mathieu <paulmathieu@google.com>
>
> * 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 <http://www.gnu.org/licenses/>.
> */
> +
> +#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 <fcntl.h>
> +#include <time.h>
> +#ifdef HAVE_SYS_PROCFS_H
> +#include <sys/procfs.h>
> +#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