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

Paul Mathieu via Gdb-patches
2020-10-03T00:36:15Z

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
>