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

[Simon Marchi <simark@simark.ca>]

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