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

[Paul Mathieu <paulmathieu@google.com>]

Thanks for the feedback Simon!
I did send a new version of that patch as a reply to Luis' feedback, you
seem to have commented on the first one.
Should I have sent that patch in a different thread?

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.
>

I'm not sure how (and where) this could be documented, are there examples
of such documentation?
Since I couldn't find any prior art for bare metal targets, I went for
something that I assumed was a standard core file format.


> 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.
>

Sure, bare-metal ARM works.


>
> > +
> > +   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.
>

Done in the patch I sent after Luis' review.


>
> > +
> > +#define ARM_CPSR_GREGNUM 16
> > +
> > +extern int arm_apcs_32;
>
> Don't declare this here, there's a suitable declaration in arm-tdep.h.
>

Done.


>
> > +
> > +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...)
>

Done.


>
> Simon
>