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

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

[Paul Mathieu]

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.
+
+   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;
+
+#define ARM_CPSR_GREGNUM 16
+
+extern int arm_apcs_32;
+
+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),
+ ARM_INT_REGISTER_SIZE, byte_order);
+  reg_pc = gdbarch_addr_bits_remove (gdbarch, reg_pc);
+  store_unsigned_integer (pc_buf, ARM_INT_REGISTER_SIZE, byte_order,
reg_pc);
+  regcache->raw_supply (ARM_PC_REGNUM, pc_buf);
+}
+
+/* Provide registers to GDB from a core file.
+
+   CORE_REG_SECT points to an array of bytes, which are the contents
+   of a `note' from a core file which BFD thinks might contain
+   register contents.  CORE_REG_SIZE is its size.
+
+   WHICH says which register set corelow suspects this is:
+     0 --- the general-purpose register set, in gregset_t format
+     2 --- the floating-point register set, in fpregset_t format
+
+   REG_ADDR is ignored.  */
+
+static void
+fetch_core_registers (struct regcache *regcache,
+      char *core_reg_sect,
+      unsigned core_reg_size,
+      int which,
+      CORE_ADDR reg_addr)
+{
+  switch (which)
+    {
+    case 0:
+      if (core_reg_size != sizeof (gdb_gregset_t))
+ warning (_("Wrong size gregset in core file."));
+      else
+ {
+  gdb_gregset_t gregset;
+  memcpy (&gregset, core_reg_sect, sizeof (gregset));
+  arm_supply_gregset (regcache, &gregset);
+ }
+      break;
+
+#if 0 // TODO
+    case 2:
+      if (core_reg_size != sizeof (gdb_fpregset_t))
+ warning (_("Wrong size fpregset in core file."));
+      else
+ {
+  gdb_fpregset_t fpregset;
+  memcpy (&fpregset, core_reg_sect, sizeof (fpregset));
+  if (gdbarch_fp0_regnum (regcache->arch ()) >= 0)
+    arm_supply_fpregset (regcache, &fpregset);
+ }
+      break;
+#endif
+
+    default:
+      /* We've covered all the kinds of registers we know about here,
+         so this must be something we wouldn't know what to do with
+         anyway.  Just ignore it.  */
+      break;
+    }
+}
+
+/* Register that we are able to handle ELF core file formats using
+   standard procfs "regset" structures.  */
+
+static struct core_fns arm_none_core_fns =
+{
+  bfd_target_elf_flavour, /* core_flavour */
+  default_check_format, /* check_format */
+  default_core_sniffer, /* core_sniffer */
+  fetch_core_registers, /* core_read_registers */
+  NULL /* next */
+};
+
+void
+_initialize_arm_none_tdep (void)
+{
+  deprecated_add_core_fns (&arm_none_core_fns);
+}
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index a3e11c4b9b..0cf05efdd1 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -189,7 +189,7 @@ arm*-*-symbianelf*)
  ;;
 arm*-*-*)
  # Target: ARM embedded system
- gdb_target_obs="arm-pikeos-tdep.o"
+ gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
  gdb_sim=../sim/arm/libsim.a
  ;;

--

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

[Luis Machado]

Hi,

It seems the e-mail client has messed up the proper formatting 
(spaces/tabs) of the patch. I'll defer formatting review until the patch 
comes through clean of e-mail client interferences.

Also, it would be nice to get rid of dead code (like "#if 0" blocks). If 
it is not functional, then there is little point in including such code.

Who is generating such core files? Is there some way I can access the 
tool to give it a try? How can I test the patch itself?

Some general comments below...

On 10/2/20 2:32 PM, 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.
> +
> +   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;
> +
> +#define ARM_CPSR_GREGNUM 16
> +
> +extern int arm_apcs_32;
> +
> +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),
> + ARM_INT_REGISTER_SIZE, byte_order);
> +  reg_pc = gdbarch_addr_bits_remove (gdbarch, reg_pc);
> +  store_unsigned_integer (pc_buf, ARM_INT_REGISTER_SIZE, byte_order,
> reg_pc);
> +  regcache->raw_supply (ARM_PC_REGNUM, pc_buf);
> +}
> +
> +/* Provide registers to GDB from a core file.
> +
> +   CORE_REG_SECT points to an array of bytes, which are the contents
> +   of a `note' from a core file which BFD thinks might contain
> +   register contents.  CORE_REG_SIZE is its size.
> +
> +   WHICH says which register set corelow suspects this is:
> +     0 --- the general-purpose register set, in gregset_t format
> +     2 --- the floating-point register set, in fpregset_t format
> +
> +   REG_ADDR is ignored.  */
> +
> +static void
> +fetch_core_registers (struct regcache *regcache,
> +      char *core_reg_sect,
> +      unsigned core_reg_size,
> +      int which,
> +      CORE_ADDR reg_addr)
> +{
> +  switch (which)
> +    {
> +    case 0:
> +      if (core_reg_size != sizeof (gdb_gregset_t))
> + warning (_("Wrong size gregset in core file."));
> +      else
> + {
> +  gdb_gregset_t gregset;
> +  memcpy (&gregset, core_reg_sect, sizeof (gregset));
> +  arm_supply_gregset (regcache, &gregset);
> + }

We usually go for one of the extract_* functions instead of memcpy, to 
account for differences in endianness.

It is not safe to assume the host has the same endianness as the target. 
Same thing for other uses of memcpy in the patch.

> +      break;
> +
> +#if 0 // TODO
> +    case 2:
> +      if (core_reg_size != sizeof (gdb_fpregset_t))
> + warning (_("Wrong size fpregset in core file."));
> +      else
> + {
> +  gdb_fpregset_t fpregset;
> +  memcpy (&fpregset, core_reg_sect, sizeof (fpregset));
> +  if (gdbarch_fp0_regnum (regcache->arch ()) >= 0)
> +    arm_supply_fpregset (regcache, &fpregset);
> + }
> +      break;
> +#endif
> +
> +    default:
> +      /* We've covered all the kinds of registers we know about here,
> +         so this must be something we wouldn't know what to do with
> +         anyway.  Just ignore it.  */
> +      break;
> +    }
> +}
> +
> +/* Register that we are able to handle ELF core file formats using
> +   standard procfs "regset" structures.  */
> +
> +static struct core_fns arm_none_core_fns =
> +{
> +  bfd_target_elf_flavour, /* core_flavour */
> +  default_check_format, /* check_format */
> +  default_core_sniffer, /* core_sniffer */
> +  fetch_core_registers, /* core_read_registers */
> +  NULL /* next */
> +};
> +
> +void
> +_initialize_arm_none_tdep (void)
> +{
> +  deprecated_add_core_fns (&arm_none_core_fns);
> +}

This particular function was removed in commit 
6ba0a321037b748f0b1285c44af762021a380bd3. I'm guessing this won't build.

> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index a3e11c4b9b..0cf05efdd1 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -189,7 +189,7 @@ arm*-*-symbianelf*)
>    ;;
>   arm*-*-*)
>    # Target: ARM embedded system
> - gdb_target_obs="arm-pikeos-tdep.o"
> + gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
>    gdb_sim=../sim/arm/libsim.a
>    ;;
> 
> --
> 

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

[Paul Mathieu]

Hi Luis,

Thanks for the quick feedback!

It seems the e-mail client has messed up the proper formatting
> (spaces/tabs) of the patch. I'll defer formatting review until the patch
> comes through clean of e-mail client interferences.


Woops, sorry about this. I'm submitting a new patch, hopefully it will be
treated well by the email gods.


> Also, it would be nice to get rid of dead code (like "#if 0" blocks). If
> it is not functional, then there is little point in including such code.
>

Done.


> Who is generating such core files? Is there some way I can access the
> tool to give it a try? How can I test the patch itself?
>

I made one myself, with a python script that I wrote.
I have a live embedded target that I can use to dump memory regions and CPU
registers over a serial line. My python script then formats the memory
dumps into PT_LOAD segments, and the CPU registers into a NT_PRSTATUS note
segment, and stores it as an ELF32 core file.
I'm happy to share any material that would help testing this, including the
python script, an example of firmware and core file and some embedded
source code.

I addressed the rest of your feedback through essentially re-writing the
patch to not use `deprecated_add_core_fns`
In the new patch, I had to use the GDB_OSABI_LINUX, otherwise my core
wouldn't be recognized. I'm not sure if this can potentially interfere with
the arm-linux implementation. Since it has its own entry in configure.tgt,
I imagine not?

Thanks!

gdb/ChangeLog:
2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
2020-10-02  Paul Mathieu <paulmathieu@google.com>
* arm-none-tdep.c: Source file added. Provide CPU registers from a core
file
* floating point registers not yet supported (FIXME)
---
 gdb/Makefile.in     |   2 +
 gdb/arm-none-tdep.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/configure.tgt   |   2 +-
 3 files changed, 110 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..bdf346bdc3
--- /dev/null
+++ b/gdb/arm-none-tdep.c
@@ -0,0 +1,107 @@
+/* Native-dependent code for GDB targetting embedded ARM.
+
+   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 "osabi.h"
+
+#include "arch/arm.h"
+#include "gdbarch.h"
+#include "regcache.h"
+#include "regset.h"
+
+struct arm_none_reg
+{
+  uint32_t reg[13];
+  uint32_t sp;
+  uint32_t lr;
+  uint32_t pc;
+  uint32_t cpsr;
+  uint32_t orig_r0;
+};
+
+extern int arm_apcs_32;
+
+static void
+arm_none_supply_gregset (const struct regset *regset,
+                         struct regcache *regcache, int regnum,
+                         const void *gregs, size_t len)
+{
+  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
+  gdb_assert (len >= sizeof (arm_none_reg));
+
+  /* Integer registers.  */
+  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
+    if (regnum == -1 || regnum == i)
+      regcache->raw_supply (i, (char *)&gregset->reg[i]);
+
+  if (regnum == -1 || regnum == ARM_SP_REGNUM)
+    regcache->raw_supply (ARM_SP_REGNUM, (char *)&gregset->sp);
+
+  if (regnum == -1 || regnum == ARM_LR_REGNUM)
+    regcache->raw_supply (ARM_LR_REGNUM, (char *)&gregset->lr);
+
+  if (regnum == -1 || regnum == ARM_PC_REGNUM)
+    {
+      CORE_ADDR r_pc
+          = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);
+      regcache->raw_supply (ARM_PC_REGNUM, (char *)&r_pc);
+    }
+
+  if (regnum == -1 || regnum == ARM_PS_REGNUM)
+    {
+      if (arm_apcs_32)
+        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->cpsr);
+      else
+        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->pc);
+    }
+}
+
+static const struct regset arm_none_regset
+    = { nullptr, arm_none_supply_gregset,
+        /* We don't need a collect function because we only use this
reading
+           registers (via iterate_over_regset_sections and
+           fetch_regs/fetch_register).  */
+        nullptr, 0 };
+
+static void
+arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
+                                       iterate_over_regset_sections_cb *cb,
+                                       void *cb_data,
+                                       const struct regcache *regcache)
+{
+  cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg),
&arm_none_regset,
+      NULL, cb_data);
+}
+
+static void arm_none_elf_init_abi (struct gdbarch_info info,
+                                   struct gdbarch *gdbarch);
+static void
+arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  set_gdbarch_iterate_over_regset_sections (
+      gdbarch, arm_none_iterate_over_regset_sections);
+}
+
+void _initialize_arm_none_tdep (void);
+void
+_initialize_arm_none_tdep (void)
+{
+  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
+                          arm_none_elf_init_abi);
+}
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index a3e11c4b9b..0cf05efdd1 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -189,7 +189,7 @@ arm*-*-symbianelf*)
  ;;
 arm*-*-*)
  # Target: ARM embedded system
- gdb_target_obs="arm-pikeos-tdep.o"
+ gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
  gdb_sim=../sim/arm/libsim.a
  ;;

--

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

[Simon Marchi]

On 2020-10-02 5:54 p.m., Paul Mathieu via Gdb-patches wrote:
> Woops, sorry about this. I'm submitting a new patch, hopefully it will be
> treated well by the email gods.

Make sure you send it using git-send-email, and we'll all be happy!

Simon

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

[Simon Marchi]

On 2020-10-02 5:54 p.m., Paul Mathieu via Gdb-patches wrote:
> Woops, sorry about this. I'm submitting a new patch, hopefully it will be
> treated well by the email gods.

Sorry, I replied to this message but I completely missed the rest of
your reply below.

Note that I am unable to apply this patch either because long lines were
again wrapped by your email client.  Please use git-send-email.  You can
still reply in the thread by using --in-reply-to.

>> Who is generating such core files? Is there some way I can access the
>> tool to give it a try? How can I test the patch itself?
>>
>
> I made one myself, with a python script that I wrote.
> I have a live embedded target that I can use to dump memory regions and CPU
> registers over a serial line. My python script then formats the memory
> dumps into PT_LOAD segments, and the CPU registers into a NT_PRSTATUS note
> segment, and stores it as an ELF32 core file.
> I'm happy to share any material that would help testing this, including the
> python script, an example of firmware and core file and some embedded
> source code.

Ok, so that answers the question in my other email, about which tool
produces this format of core.  The answer is this specific python
script, and the format is "Paul's ARM core format" :).

I don't know what Luis thinks about this, but I would be a bit hesitant
to add support in GDB for a core format that's not standard nor the
output of some "well-known" tool (which would be a de facto standard).

Is there a format that already exists in the ARM ecosystem for core
dumps of bare-metal systems, we could base our stuff on?

Alternatively, do you think you could implement GDB's generate-core-file
command for bare-metal ARM?  First, it would make sense for GDB to be
able to produce a core in some format and be able to ingest it again.
And it would act as some kind of documentation / reference
implementation of what GDB expects, and that could become some de facto
standard.  People who would like to have their core readable by GDB
would produce it in this format.

> I addressed the rest of your feedback through essentially re-writing the
> patch to not use `deprecated_add_core_fns`
> In the new patch, I had to use the GDB_OSABI_LINUX, otherwise my core
> wouldn't be recognized. I'm not sure if this can potentially interfere with
> the arm-linux implementation. Since it has its own entry in configure.tgt,
> I imagine not?

Aren't you able to use GDB_OSABI_NONE?  But see comments below.

> gdb/ChangeLog:
> 2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
> 2020-10-02  Paul Mathieu <paulmathieu@google.com>
> * arm-none-tdep.c: Source file added. Provide CPU registers from a core
> file
> * floating point registers not yet supported (FIXME)
> ---
>  gdb/Makefile.in     |   2 +
>  gdb/arm-none-tdep.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/configure.tgt   |   2 +-
>  3 files changed, 110 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 \

Please keep the lists sorted.

Note that so far, everything related to bare-metal for an architecture
was simply in <arch>-tdep.c.  So perhaps it should just be there?  I'll
defer to Luis to decide what is the best organization for the ARM code.

> +static void
> +arm_none_supply_gregset (const struct regset *regset,
> +                         struct regcache *regcache, int regnum,
> +                         const void *gregs, size_t len)
> +{
> +  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
> +  gdb_assert (len >= sizeof (arm_none_reg));
> +
> +  /* Integer registers.  */
> +  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
> +    if (regnum == -1 || regnum == i)
> +      regcache->raw_supply (i, (char *)&gregset->reg[i]);
> +
> +  if (regnum == -1 || regnum == ARM_SP_REGNUM)
> +    regcache->raw_supply (ARM_SP_REGNUM, (char *)&gregset->sp);
> +
> +  if (regnum == -1 || regnum == ARM_LR_REGNUM)
> +    regcache->raw_supply (ARM_LR_REGNUM, (char *)&gregset->lr);
> +
> +  if (regnum == -1 || regnum == ARM_PC_REGNUM)
> +    {
> +      CORE_ADDR r_pc
> +          = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);

We'd typically use one ident (two spaces) before the equal sign.  But
otherwise that's spot on.

> +      regcache->raw_supply (ARM_PC_REGNUM, (char *)&r_pc);
> +    }
> +
> +  if (regnum == -1 || regnum == ARM_PS_REGNUM)
> +    {
> +      if (arm_apcs_32)
> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->cpsr);
> +      else
> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->pc);
> +    }
> +}
> +
> +static const struct regset arm_none_regset
> +    = { nullptr, arm_none_supply_gregset,
> +        /* We don't need a collect function because we only use this
> reading
> +           registers (via iterate_over_regset_sections and
> +           fetch_regs/fetch_register).  */
> +        nullptr, 0 };

See above where `reading` is on a separate line, that's an example of
the patch getting mangled by the email client.

> +void _initialize_arm_none_tdep (void);
> +void
> +_initialize_arm_none_tdep (void)
> +{
> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
> +                          arm_none_elf_init_abi);

I don't think this is what you want to do.  This "declares" that
combination of architecture ARM and osabi Linux exists, and that
arm_none_elf_init_abi should be called when such a combo is detected to
initialize the gdbarch object.

However, such a combo is already registered somewhere else, as you can
guess:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/arm-linux-tdep.c;h=f60cb51763f1c5b14811494b646acf54b62d1f10;hb=HEAD#l2010

So I presume that if you build a GDB configured with
--enable-targets=all (which will include arm-linux-tdep.c), you will hit
this assertion:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/osabi.c;h=e8a813b62f268417edf5f0ad164c9c942bd43b1e;hb=HEAD#l179

What I think you want is to register an osabi sniffer, whose job is to
try to guess an osabi given a bfd.  Apparently that bfd can sometimes
represent the ELF core file, so you can look up sections and see if that
file looks like a core file you can handle.  And if so, return
GDB_OSABI_NONE.

And then, you'd need to register (with gdbarch_register_osabi) osabi
GDB_OSABI_NONE with the arm architecture (same as you did, but with
GDB_OSABI_NONE instead of GDB_OSABI_LINUX) with the callback to call
when this combo is detected:

  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_NONE,
                          arm_none_elf_init_abi);

I'm not sure about that part, but I think it's the right direction.

Now, the question is how to recognize one of these cores?  As an
example, to recognize Cygwin core dumps (which use ELF files as a
container without a recognizable ABI tag, so it's kind of the same
situation as you), we use the .reg section size to determine if the BFD
is a Cygwin core:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/amd64-windows-tdep.c;h=e427c20538961bda2a1a0f033207eebce64c4729;hb=HEAD#l1387

So, you can do that, although I think we all agree that it's not ideal.
If there was some way of identifying that the core is in the format we
recognize (say, if there was some special section with a tag), it would
make it much easier to identify it without having to guess.

Simon

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

[Paul Mathieu]

Thanks Simon for the super helpful feedback!
A few answers inline, and hopefully a properly formatted patch at the
bottom.

> Note that I am unable to apply this patch either because long lines were
> again wrapped by your email client.  Please use git-send-email.  You can
> still reply in the thread by using --in-reply-to.

Couldn't get git send-email to work with my SMTP, but I got some version
of mutt to comply. Hopefully you can apply the patch this time.


> Ok, so that answers the question in my other email, about which tool
> produces this format of core.  The answer is this specific python
> script, and the format is "Paul's ARM core format" :).

It was some team work between what gdb understood "natively" and what
the patch does.
More specifically, this patch only seems to tell gdb to get the
registers from the core file the way it would normally do, and then
supply them almost verbatim to the regcache.
The struct for these registers is nothing special either, you can find
it in the kernel code as well:
https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135


> I don't know what Luis thinks about this, but I would be a bit hesitant
> to add support in GDB for a core format that's not standard nor the
> output of some "well-known" tool (which would be a de facto standard).
>
> Is there a format that already exists in the ARM ecosystem for core
> dumps of bare-metal systems, we could base our stuff on?

I'm not aware of anything that does that. There are tools that generate
memory dumps and CPU register dumps into their own proprietary format,
but nothing that I know that you could call a 'core file'.

Are there other tools besides gdb that deal with core dumps? Maybe that
could help me find other formats.

That being said, the format used here is "well-known" in the sense that
it's the exact same format the linux kernel would use to dump cores on
arm-linux-*


> Alternatively, do you think you could implement GDB's generate-core-file
> command for bare-metal ARM?  First, it would make sense for GDB to be
> able to produce a core in some format and be able to ingest it again.
> And it would act as some kind of documentation / reference
> implementation of what GDB expects, and that could become some de facto
> standard.  People who would like to have their core readable by GDB
> would produce it in this format.

I had never cloned the gdb source repo until 2 days ago. I have no idea
how much work that would be, given that I know next to nothing about the
gdb codebase.

I do agree that this seems like a better way to do it, since there are
already many integrations with a gdb server talking to a live bare-metal
target over a physical debugger.

As long as gdb already supports primitives to grab memory dumps and cpu
registers of a remote target, I'd imagine generating a core file
shouldn't be too much work. I'm quite sure most of that functionality
already exists for extremely similar targets.


>> +void _initialize_arm_none_tdep (void);
>> +void
>> +_initialize_arm_none_tdep (void)
>> +{
>> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
>> +                          arm_none_elf_init_abi);
>
> I don't think this is what you want to do.  This "declares" that
> combination of architecture ARM and osabi Linux exists, and that
> arm_none_elf_init_abi should be called when such a combo is detected to
> initialize the gdbarch object.
>
> However, such a combo is already registered somewhere else, as you can
> guess:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/arm-linux-tdep.c;h=f60cb51763f1c5b14811494b646acf54b62d1f10;hb=HEAD#l2010
>
> So I presume that if you build a GDB configured with
> --enable-targets=all (which will include arm-linux-tdep.c), you will hit
> this assertion:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/osabi.c;h=e8a813b62f268417edf5f0ad164c9c942bd43b1e;hb=HEAD#l179

Ah, I was afraid of that. Thanks for the pointers.


> What I think you want is to register an osabi sniffer, whose job is to
> try to guess an osabi given a bfd.  Apparently that bfd can sometimes
> represent the ELF core file, so you can look up sections and see if that
> file looks like a core file you can handle.  And if so, return
> GDB_OSABI_NONE.
>
> And then, you'd need to register (with gdbarch_register_osabi) osabi
> GDB_OSABI_NONE with the arm architecture (same as you did, but with
> GDB_OSABI_NONE instead of GDB_OSABI_LINUX) with the callback to call
> when this combo is detected:
>
>   gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_NONE,
>                           arm_none_elf_init_abi);
>
> I'm not sure about that part, but I think it's the right direction.
>
> Now, the question is how to recognize one of these cores?  As an
> example, to recognize Cygwin core dumps (which use ELF files as a
> container without a recognizable ABI tag, so it's kind of the same
> situation as you), we use the .reg section size to determine if the BFD
> is a Cygwin core:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/amd64-windows-tdep.c;h=e427c20538961bda2a1a0f033207eebce64c4729;hb=HEAD#l1387
>
> So, you can do that, although I think we all agree that it's not ideal.
> If there was some way of identifying that the core is in the format we
> recognize (say, if there was some special section with a tag), it would
> make it much easier to identify it without having to guess.

Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.

Since the format isn't specified (yet), I can still freely control it. I
imagine I could add some sort of OSABI marker in the core file to mark
it as an arm-none-eabi core.o

My first guess was to set the EI_OSABI e_ident elf header field to
ELFOSABI_NONE, but that happens to be the same enum value as
ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
files. So it wouldn't be a good marker.

Not sure what a better way would be to not abuse the ELF structure and
produce reasonable ELF core dumps (since they already work so well with
gdb).


Paul



gdb/Changelog
2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
2020-10-02  Paul Mathieu <paulmathieu@google.com>
	* arm-none-tdep.c: Source file added. Provide CPU registers from a core
	file
	* floating point registers not yet supported (FIXME)
---
 gdb/Makefile.in     |   2 +
 gdb/arm-none-tdep.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/configure.tgt   |   2 +-
 3 files changed, 109 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..21743c40a0
--- /dev/null
+++ b/gdb/arm-none-tdep.c
@@ -0,0 +1,106 @@
+/* Native-dependent code for GDB targetting bare-metal ARM.
+
+   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 "osabi.h"
+
+#include "arch/arm.h"
+#include "arm-tdep.h"
+#include "gdbarch.h"
+#include "regcache.h"
+#include "regset.h"
+
+struct arm_none_reg
+{
+  uint32_t reg[13];
+  uint32_t sp;
+  uint32_t lr;
+  uint32_t pc;
+  uint32_t cpsr;
+  uint32_t orig_r0;
+};
+
+static void
+arm_none_supply_gregset (const struct regset *regset,
+                         struct regcache *regcache, int regnum,
+                         const void *gregs, size_t len)
+{
+  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
+  gdb_assert (len >= sizeof (arm_none_reg));
+
+  /* Integer registers.  */
+  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
+    if (regnum == -1 || regnum == i)
+      regcache->raw_supply (i, (char *)&gregset->reg[i]);
+
+  if (regnum == -1 || regnum == ARM_SP_REGNUM)
+    regcache->raw_supply (ARM_SP_REGNUM, (char *)&gregset->sp);
+
+  if (regnum == -1 || regnum == ARM_LR_REGNUM)
+    regcache->raw_supply (ARM_LR_REGNUM, (char *)&gregset->lr);
+
+  if (regnum == -1 || regnum == ARM_PC_REGNUM)
+    {
+      CORE_ADDR r_pc
+        = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);
+      regcache->raw_supply (ARM_PC_REGNUM, (char *)&r_pc);
+    }
+
+  if (regnum == -1 || regnum == ARM_PS_REGNUM)
+    {
+      if (arm_apcs_32)
+        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->cpsr);
+      else
+        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->pc);
+    }
+}
+
+static const struct regset arm_none_regset
+    = { nullptr, arm_none_supply_gregset,
+        /* We don't need a collect function because we only use this reading
+           registers (via iterate_over_regset_sections and
+           fetch_regs/fetch_register).  */
+        nullptr, 0 };
+
+static void
+arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
+                                       iterate_over_regset_sections_cb *cb,
+                                       void *cb_data,
+                                       const struct regcache *regcache)
+{
+  cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg), &arm_none_regset,
+      NULL, cb_data);
+}
+
+static void arm_none_elf_init_abi (struct gdbarch_info info,
+                                   struct gdbarch *gdbarch);
+static void
+arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  set_gdbarch_iterate_over_regset_sections (
+      gdbarch, arm_none_iterate_over_regset_sections);
+}
+
+void _initialize_arm_none_tdep (void);
+void
+_initialize_arm_none_tdep (void)
+{
+  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
+                          arm_none_elf_init_abi);
+}
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index a3e11c4b9b..0cf05efdd1 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -189,7 +189,7 @@ arm*-*-symbianelf*)
 	;;
 arm*-*-*)
 	# Target: ARM embedded system
-	gdb_target_obs="arm-pikeos-tdep.o"
+	gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
 	gdb_sim=../sim/arm/libsim.a
 	;;

--
2.28.0.806.g8561365e88-goog

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

[Paul Mathieu]

Cc'ing Robin, the original author of the patch, who can maybe help us
answer a couple questions.

> > Ok, so that answers the question in my other email, about which tool
> > produces this format of core.  The answer is this specific python
> > script, and the format is "Paul's ARM core format" :).
>
> It was some team work between what gdb understood "natively" and what
> the patch does.
> More specifically, this patch only seems to tell gdb to get the
> registers from the core file the way it would normally do, and then
> supply them almost verbatim to the regcache.
> The struct for these registers is nothing special either, you can find
> it in the kernel code as well:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135
>
>
> > I don't know what Luis thinks about this, but I would be a bit hesitant
> > to add support in GDB for a core format that's not standard nor the
> > output of some "well-known" tool (which would be a de facto standard).
> >
> > Is there a format that already exists in the ARM ecosystem for core
> > dumps of bare-metal systems, we could base our stuff on?
>
> I'm not aware of anything that does that. There are tools that generate
> memory dumps and CPU register dumps into their own proprietary format,
> but nothing that I know that you could call a 'core file'.
>
> Are there other tools besides gdb that deal with core dumps? Maybe that
> could help me find other formats.
>
> That being said, the format used here is "well-known" in the sense that
> it's the exact same format the linux kernel would use to dump cores on
> arm-linux-*

Robin, what was the format you used for your core dumps? Did you use a
tool that's already out there?

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

[Simon Marchi]

On 2020-10-03 2:14 p.m., Paul Mathieu via Gdb-patches wrote:
> Thanks Simon for the super helpful feedback!
> A few answers inline, and hopefully a properly formatted patch at the
> bottom.
>
>> Note that I am unable to apply this patch either because long lines were
>> again wrapped by your email client.  Please use git-send-email.  You can
>> still reply in the thread by using --in-reply-to.
>
> Couldn't get git send-email to work with my SMTP, but I got some version
> of mutt to comply. Hopefully you can apply the patch this time.

Thanks, I am now able to apply the patch.  However, you would it to
export the patch using "git-format-patch", so that we get the full patch
including the commit message (since we review the commit messages to
make sure they include all the relevant information).

>> Ok, so that answers the question in my other email, about which tool
>> produces this format of core.  The answer is this specific python
>> script, and the format is "Paul's ARM core format" :).
>
> It was some team work between what gdb understood "natively" and what
> the patch does.
> More specifically, this patch only seems to tell gdb to get the
> registers from the core file the way it would normally do, and then
> supply them almost verbatim to the regcache.
> The struct for these registers is nothing special either, you can find
> it in the kernel code as well:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135

Ok, so that is some useful information to include in the code comments
and / or in the commit log, that the register layout used for bare-metal
core dumps was chosen to be the same as that of the Linux kernel.

>> I don't know what Luis thinks about this, but I would be a bit hesitant
>> to add support in GDB for a core format that's not standard nor the
>> output of some "well-known" tool (which would be a de facto standard).
>>
>> Is there a format that already exists in the ARM ecosystem for core
>> dumps of bare-metal systems, we could base our stuff on?
>
> I'm not aware of anything that does that. There are tools that generate
> memory dumps and CPU register dumps into their own proprietary format,
> but nothing that I know that you could call a 'core file'.
>
> Are there other tools besides gdb that deal with core dumps? Maybe that
> could help me find other formats.

I don't know any, but I don't have much experience in embedded development.

> That being said, the format used here is "well-known" in the sense that
> it's the exact same format the linux kernel would use to dump cores on
> arm-linux-*

That sounds like a good choice to me.  As I said above, I think it
should be mentioned in some comments.

>> Alternatively, do you think you could implement GDB's generate-core-file
>> command for bare-metal ARM?  First, it would make sense for GDB to be
>> able to produce a core in some format and be able to ingest it again.
>> And it would act as some kind of documentation / reference
>> implementation of what GDB expects, and that could become some de facto
>> standard.  People who would like to have their core readable by GDB
>> would produce it in this format.
>
> I had never cloned the gdb source repo until 2 days ago. I have no idea
> how much work that would be, given that I know next to nothing about the
> gdb codebase.

>
> I do agree that this seems like a better way to do it, since there are
> already many integrations with a gdb server talking to a live bare-metal
> target over a physical debugger.
>
> As long as gdb already supports primitives to grab memory dumps and cpu
> registers of a remote target, I'd imagine generating a core file
> shouldn't be too much work. I'm quite sure most of that functionality
> already exists for extremely similar targets.

You can start at function "gcore_command" and pull on that thread.

It seems like you could implement the gdbarch_make_corefile_notes method
for the gdbarch for bare-metal ARM.  To see examples, look for uses of
set_gdbarch_make_corefile_notes.

> Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.
>
> Since the format isn't specified (yet), I can still freely control it. I
> imagine I could add some sort of OSABI marker in the core file to mark
> it as an arm-none-eabi core.o
>
> My first guess was to set the EI_OSABI e_ident elf header field to
> ELFOSABI_NONE, but that happens to be the same enum value as
> ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
> files. So it wouldn't be a good marker.
>
> Not sure what a better way would be to not abuse the ELF structure and
> produce reasonable ELF core dumps (since they already work so well with
> gdb).

I really don't know either.  Technically, it could be as simple as
adding a new section / note of your choice, but I don't know if that
would be an acceptable thing to do.

I sent comments on the second patch you sent (let's call it 1.1), but
not all of them seem to have been fixed, so here they are again.

> gdb/Changelog
> 2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
> 2020-10-02  Paul Mathieu <paulmathieu@google.com>
> 	* arm-none-tdep.c: Source file added. Provide CPU registers from a core
> 	file
> 	* floating point registers not yet supported (FIXME)
> ---
>  gdb/Makefile.in     |   2 +
>  gdb/arm-none-tdep.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/configure.tgt   |   2 +-
>  3 files changed, 109 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 \

Please keep the lists sorted.

Note that so far, everything related to bare-metal for an architecture
was simply in <arch>-tdep.c.  So perhaps it should just be there?  I'll
defer to Luis to decide what is the best organization for the ARM code.

>  	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..21743c40a0
> --- /dev/null
> +++ b/gdb/arm-none-tdep.c
> @@ -0,0 +1,106 @@
> +/* Native-dependent code for GDB targetting bare-metal ARM.

Actually, this should say "Target-dependent", not "native-dependent".
"native" in GDB means things used when GDB runs directly on the target
platform.

> +
> +   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 "osabi.h"
> +
> +#include "arch/arm.h"
> +#include "arm-tdep.h"
> +#include "gdbarch.h"
> +#include "regcache.h"
> +#include "regset.h"
> +
> +struct arm_none_reg
> +{
> +  uint32_t reg[13];
> +  uint32_t sp;
> +  uint32_t lr;
> +  uint32_t pc;
> +  uint32_t cpsr;
> +  uint32_t orig_r0;
> +};

So this is the layout of the registers found in the core?  I would add a
comment on top of it, saying just that.  And that there's no standard
layout for register layouts in ARM bare-metal cores, but it was chosen
to use the same layout as the Linux kernel uses.

> +
> +static void
> +arm_none_supply_gregset (const struct regset *regset,
> +                         struct regcache *regcache, int regnum,
> +                         const void *gregs, size_t len)
> +{
> +  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
> +  gdb_assert (len >= sizeof (arm_none_reg));
> +
> +  /* Integer registers.  */
> +  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
> +    if (regnum == -1 || regnum == i)
> +      regcache->raw_supply (i, (char *)&gregset->reg[i]);

Space after the cast (apply everywhere).

> +static const struct regset arm_none_regset
> +    = { nullptr, arm_none_supply_gregset,
> +        /* We don't need a collect function because we only use this reading
> +           registers (via iterate_over_regset_sections and
> +           fetch_regs/fetch_register).  */
> +        nullptr, 0 };
> +
> +static void
> +arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
> +                                       iterate_over_regset_sections_cb *cb,
> +                                       void *cb_data,
> +                                       const struct regcache *regcache)
> +{
> +  cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg), &arm_none_regset,
> +      NULL, cb_data);
> +}
> +
> +static void arm_none_elf_init_abi (struct gdbarch_info info,
> +                                   struct gdbarch *gdbarch);

You don't need this declaration.

> +static void
> +arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  set_gdbarch_iterate_over_regset_sections (
> +      gdbarch, arm_none_iterate_over_regset_sections);
> +}
> +
> +void _initialize_arm_none_tdep (void);
> +void
> +_initialize_arm_none_tdep (void)

Remove the "void" in the lines above.

> +{
> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
> +                          arm_none_elf_init_abi);

As mentioned previously, this should be GDB_OSABI_NONE, not
GDB_OSABI_LINUX.  Like this, it GDB aborts on startup when configured
with --enable-targets=all.

Simon

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

[Paul Mathieu]

> > Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.
> >
> > Since the format isn't specified (yet), I can still freely control it. I
> > imagine I could add some sort of OSABI marker in the core file to mark
> > it as an arm-none-eabi core.o
> >
> > My first guess was to set the EI_OSABI e_ident elf header field to
> > ELFOSABI_NONE, but that happens to be the same enum value as
> > ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
> > files. So it wouldn't be a good marker.
> >
> > Not sure what a better way would be to not abuse the ELF structure and
> > produce reasonable ELF core dumps (since they already work so well with
> > gdb).
>
> I really don't know either.  Technically, it could be as simple as
> adding a new section / note of your choice, but I don't know if that
> would be an acceptable thing to do.

So, I implemented a basic sniffer, and it seems like the core file is
being recognized as GDB_OSABI_NONE, but gdb crashes while loading it.
The assert & stack trace are as follows:

gdbarch.c:3684: internal-error: void
gdbarch_iterate_over_regset_sections(gdbarch*, void (*)(const char*,
int, int, const regset*, const char*, void*), void*, const regcache*):
Assertion `gdbarch->iterate_over_regset_sections != NULL' failed.

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fd93a9a4537 in __GI_abort () at abort.c:79
#2  0x000055e524ab29b7 in dump_core () at utils.c:204
#3  0x000055e524ab770d in internal_vproblem(internal_problem *, const
char *, int, const char *, typedef __va_list_tag __va_list_tag *) (
    problem=0x55e524f5a3a0 <internal_error_problem>, file=<optimized
out>, line=<optimized out>, fmt=<optimized out>, ap=<optimized out>)
    at utils.c:414
#4  0x000055e524ab78eb in internal_verror (file=<optimized out>,
line=<optimized out>, fmt=<optimized out>, ap=ap@entry=0x7fffb8c40628)
    at utils.c:439
#5  0x000055e524be45c2 in internal_error
(file=file@entry=0x55e524c7e484 "gdbarch.c", line=line@entry=3684,
fmt=<optimized out>)
    at errors.cc:55
#6  0x000055e5248d8c48 in gdbarch_iterate_over_regset_sections
(gdbarch=0x55e5261014f0,
    cb=0x55e5248354e0 <get_core_registers_cb(char const*, int, int,
regset const*, char const*, void*)>, cb_data=0x7fffb8c40730,
    regcache=0x0) at gdbarch.c:3687
#7  0x000055e524834f07 in core_target::fetch_registers
(regno=<optimized out>, regcache=0x55e5261625c0, this=<optimized out>)
    at corelow.c:735
#8  core_target::fetch_registers (this=<optimized out>,
regcache=0x55e5261625c0, regno=<optimized out>) at corelow.c:723
#9  0x000055e524a6ee8d in target_fetch_registers
(regcache=regcache@entry=0x55e5261625c0, regno=regno@entry=15) at
target.h:1334
#10 0x000055e5249df14a in regcache::raw_update (regnum=15,
this=0x55e5261625c0) at regcache.c:583
#11 regcache::raw_update (this=0x55e5261625c0, regnum=<optimized out>)
at regcache.c:572
#12 0x000055e5249df1ea in readable_regcache::raw_read
(this=0x55e5261625c0, regnum=15, buf=0x7fffb8c407c0 "\001") at
regcache.c:597
#13 0x000055e5249e4edf in readable_regcache::cooked_read<unsigned
long, void> (this=this@entry=0x55e5261625c0, regnum=15,
    val=val@entry=0x7fffb8c40818) at regcache.c:769
#14 0x000055e5249e1ce3 in regcache_cooked_read_unsigned
(val=0x7fffb8c40818, regnum=<optimized out>, regcache=0x55e5261625c0)
    at regcache.c:790
#15 regcache_read_pc (regcache=0x55e5261625c0) at regcache.c:1294
#16 0x000055e524907e2e in post_create_inferior
(target=target@entry=0x55e52615b480, from_tty=from_tty@entry=1) at
infcmd.c:303
#17 0x000055e5248360ff in core_target_open (arg=<error reading
variable: value has been optimized out>,
    from_tty=<error reading variable: value has been optimized out>)
at corelow.c:521
#18 0x000055e5249571e0 in catch_command_errors (command=<optimized
out>, arg=<optimized out>, from_tty=<optimized out>) at main.c:457
#19 0x000055e524958ecf in captured_main_1 (context=<optimized out>) at
main.c:1167
#20 0x000055e52495904b in captured_main (data=<optimized out>) at main.c:1268
#21 gdb_main (args=<optimized out>) at main.c:1268
#22 0x000055e524765f0c in main (argc=<optimized out>, argv=<optimized
out>) at gdb.c:32

Seems to me like current_inferior()->gdbarch is returning the
arm-linux gdbarch. Maybe this is because the main .elf executable
itself is recognized as arm-linux? I have virtually no control over
the executable image, so I would not be able to insert the same kind
of note section as I would for a core file to differentiate it from
arm-linux executables.

I'm not sure how to proceed forward, would you have any pointers?

Thanks!
Paul

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

[Luis Machado]

On 10/6/20 1:32 AM, Paul Mathieu via Gdb-patches wrote:
>>> Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.
>>>
>>> Since the format isn't specified (yet), I can still freely control it. I
>>> imagine I could add some sort of OSABI marker in the core file to mark
>>> it as an arm-none-eabi core.o
>>>
>>> My first guess was to set the EI_OSABI e_ident elf header field to
>>> ELFOSABI_NONE, but that happens to be the same enum value as
>>> ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
>>> files. So it wouldn't be a good marker.
>>>
>>> Not sure what a better way would be to not abuse the ELF structure and
>>> produce reasonable ELF core dumps (since they already work so well with
>>> gdb).
>>
>> I really don't know either.  Technically, it could be as simple as
>> adding a new section / note of your choice, but I don't know if that
>> would be an acceptable thing to do.
> 
> So, I implemented a basic sniffer, and it seems like the core file is
> being recognized as GDB_OSABI_NONE, but gdb crashes while loading it.
> The assert & stack trace are as follows:
> 
> gdbarch.c:3684: internal-error: void
> gdbarch_iterate_over_regset_sections(gdbarch*, void (*)(const char*,
> int, int, const regset*, const char*, void*), void*, const regcache*):
> Assertion `gdbarch->iterate_over_regset_sections != NULL' failed.
> 
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007fd93a9a4537 in __GI_abort () at abort.c:79
> #2  0x000055e524ab29b7 in dump_core () at utils.c:204
> #3  0x000055e524ab770d in internal_vproblem(internal_problem *, const
> char *, int, const char *, typedef __va_list_tag __va_list_tag *) (
>      problem=0x55e524f5a3a0 <internal_error_problem>, file=<optimized
> out>, line=<optimized out>, fmt=<optimized out>, ap=<optimized out>)
>      at utils.c:414
> #4  0x000055e524ab78eb in internal_verror (file=<optimized out>,
> line=<optimized out>, fmt=<optimized out>, ap=ap@entry=0x7fffb8c40628)
>      at utils.c:439
> #5  0x000055e524be45c2 in internal_error
> (file=file@entry=0x55e524c7e484 "gdbarch.c", line=line@entry=3684,
> fmt=<optimized out>)
>      at errors.cc:55
> #6  0x000055e5248d8c48 in gdbarch_iterate_over_regset_sections
> (gdbarch=0x55e5261014f0,
>      cb=0x55e5248354e0 <get_core_registers_cb(char const*, int, int,
> regset const*, char const*, void*)>, cb_data=0x7fffb8c40730,
>      regcache=0x0) at gdbarch.c:3687
> #7  0x000055e524834f07 in core_target::fetch_registers
> (regno=<optimized out>, regcache=0x55e5261625c0, this=<optimized out>)
>      at corelow.c:735
> #8  core_target::fetch_registers (this=<optimized out>,
> regcache=0x55e5261625c0, regno=<optimized out>) at corelow.c:723
> #9  0x000055e524a6ee8d in target_fetch_registers
> (regcache=regcache@entry=0x55e5261625c0, regno=regno@entry=15) at
> target.h:1334
> #10 0x000055e5249df14a in regcache::raw_update (regnum=15,
> this=0x55e5261625c0) at regcache.c:583
> #11 regcache::raw_update (this=0x55e5261625c0, regnum=<optimized out>)
> at regcache.c:572
> #12 0x000055e5249df1ea in readable_regcache::raw_read
> (this=0x55e5261625c0, regnum=15, buf=0x7fffb8c407c0 "\001") at
> regcache.c:597
> #13 0x000055e5249e4edf in readable_regcache::cooked_read<unsigned
> long, void> (this=this@entry=0x55e5261625c0, regnum=15,
>      val=val@entry=0x7fffb8c40818) at regcache.c:769
> #14 0x000055e5249e1ce3 in regcache_cooked_read_unsigned
> (val=0x7fffb8c40818, regnum=<optimized out>, regcache=0x55e5261625c0)
>      at regcache.c:790
> #15 regcache_read_pc (regcache=0x55e5261625c0) at regcache.c:1294
> #16 0x000055e524907e2e in post_create_inferior
> (target=target@entry=0x55e52615b480, from_tty=from_tty@entry=1) at
> infcmd.c:303
> #17 0x000055e5248360ff in core_target_open (arg=<error reading
> variable: value has been optimized out>,
>      from_tty=<error reading variable: value has been optimized out>)
> at corelow.c:521
> #18 0x000055e5249571e0 in catch_command_errors (command=<optimized
> out>, arg=<optimized out>, from_tty=<optimized out>) at main.c:457
> #19 0x000055e524958ecf in captured_main_1 (context=<optimized out>) at
> main.c:1167
> #20 0x000055e52495904b in captured_main (data=<optimized out>) at main.c:1268
> #21 gdb_main (args=<optimized out>) at main.c:1268
> #22 0x000055e524765f0c in main (argc=<optimized out>, argv=<optimized
> out>) at gdb.c:32
> 
> Seems to me like current_inferior()->gdbarch is returning the
> arm-linux gdbarch. Maybe this is because the main .elf executable
> itself is recognized as arm-linux? I have virtually no control over
> the executable image, so I would not be able to insert the same kind
> of note section as I would for a core file to differentiate it from
> arm-linux executables.

I think there may be enough hints here and there to be able to tell 
those apart via an heuristic. Isn't there a particular note section or 
flag that you can fetch and tell it is not a Linux executable but a 
bare-metal one?

> 
> I'm not sure how to proceed forward, would you have any pointers?

See the code in gdb/arm-tdep.c:arm_gdbarch_init, around the BFD flag 
checks and the ABI checks. Is that something usable?

> 
> Thanks!
> Paul
> 

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

[Simon Marchi]

On 2020-10-06 8:45 a.m., Luis Machado wrote:
> On 10/6/20 1:32 AM, Paul Mathieu via Gdb-patches wrote:
>> Seems to me like current_inferior()->gdbarch is returning the
>> arm-linux gdbarch. Maybe this is because the main .elf executable
>> itself is recognized as arm-linux? I have virtually no control over
>> the executable image, so I would not be able to insert the same kind
>> of note section as I would for a core file to differentiate it from
>> arm-linux executables.
>
> I think there may be enough hints here and there to be able to tell
> those apart via an heuristic. Isn't there a particular note section or
> flag that you can fetch and tell it is not a Linux executable but a
> bare-metal one?

Just so we are on the same page as to why this is happening, can you try
to step into function "gdbarch_info_fill" while you're executable is
loaded?

What I think is happening is that gdbarch_lookup_osabi returns
GDB_OSABI_UNKNOWN, because no sniffer identified it.

So the default osabi of your build gets used instead:

    /* From the configured default.  */
    #ifdef GDB_OSABI_DEFAULT
      if (info->osabi == GDB_OSABI_UNKNOWN)
        info->osabi = GDB_OSABI_DEFAULT;
    #endif

So indeed, if there was a sniffer identifying the binary as a "none"
osabi, it would probably work.  The problem is, how do you write such a
sniffer?  It's easy to check for a Linux binary, you check for the os
abi ELF note.  But how do you identify a "none" binary?  You would have
to check that's it's not a Linux binary, not FreeBSD binary, not a
Windows binary, etc.

I don't really like the idea of having a "default" osabi that we fall
back on, because it can just silently get it wrong, like in this case.
I think it would make more sense for the fall back to be "none".
Because it's hard to detect a "none" binary, as explained above.

In the mean time, for your testing, you can force the osabi to none by
using "set osabi none" prior to loading anything.

Simon

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

[Paul Mathieu]

On Tue, Oct 6, 2020 at 7:29 AM Simon Marchi <simark@simark.ca> wrote:
> On 2020-10-06 8:45 a.m., Luis Machado wrote:
> > On 10/6/20 1:32 AM, Paul Mathieu via Gdb-patches wrote:
> >> Seems to me like current_inferior()->gdbarch is returning the
> >> arm-linux gdbarch. Maybe this is because the main .elf executable
> >> itself is recognized as arm-linux? I have virtually no control over
> >> the executable image, so I would not be able to insert the same kind
> >> of note section as I would for a core file to differentiate it from
> >> arm-linux executables.
> >
> > I think there may be enough hints here and there to be able to tell
> > those apart via an heuristic. Isn't there a particular note section or
> > flag that you can fetch and tell it is not a Linux executable but a
> > bare-metal one?

I did a quick comparison between two executables.
The first one is taken from the armhf debian distribution.
The second one is for my bare-metal cortex-m4 target.

➜  bin readelf -h screen
ELF Header:
  Magic:   7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00
  Class:                             ELF32
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              DYN (Shared object file)
  Machine:                           ARM
  Version:                           0x1
  Entry point address:               0x6271
  Start of program headers:          52 (bytes into file)
  Start of section headers:          310736 (bytes into file)
  Flags:                             0x5000400, Version5 EABI, hard-float ABI
  Size of this header:               52 (bytes)
  Size of program headers:           32 (bytes)
  Number of program headers:         9
  Size of section headers:           40 (bytes)
  Number of section headers:         28
  Section header string table index: 27

➜  bin readelf -h my_bare_metal_stuff.elf
ELF Header:
  Magic:   7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00
  Class:                             ELF32
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              EXEC (Executable file)
  Machine:                           ARM
  Version:                           0x1
  Entry point address:               0xc018011
  Start of program headers:          52 (bytes into file)
  Start of section headers:          49269716 (bytes into file)
  Flags:                             0x5000400, Version5 EABI, hard-float ABI
  Size of this header:               52 (bytes)
  Size of program headers:           32 (bytes)
  Number of program headers:         39
  Size of section headers:           40 (bytes)
  Number of section headers:         76
  Section header string table index: 75

The ELF header does not help us much, except that the linux executable
is marked as a DYN object, probably because it's dynamically
linked(?). I assume that statically linked executables will then use
the EXEC ELF type, which means I can't use that.
In particular, the OS/ABI section is identical. That's a quirk of the
enum, where SYSV and NONE have the same value. Yikes.

There are also some notes in the linux binary that look nice:

➜  bin readelf --notes screen

Displaying notes found in: .note.gnu.build-id
  Owner                Data size Description
  GNU                  0x00000014 NT_GNU_BUILD_ID (unique build ID bitstring)
    Build ID: 76c649229b0402d6a0d1f65c3925e85fcb18e79d

Displaying notes found in: .note.ABI-tag
  Owner                Data size Description
  GNU                  0x00000010 NT_GNU_ABI_TAG (ABI version tag)
    OS: Linux, ABI: 3.2.0

My own bare-metal executable contains no notes at all.
Again, here, I'm not sure how widespread these notes are, and if linux
really requires them to run the binary or not.
As the note name suggests, it looks like a GNU extension, which
implies that it's not mandatory?

I found one more thing, trying to find differences between the 2 files.

➜  bin readelf --arch-specific screen
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "7-A"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2
  Tag_FP_arch: VFPv3-D16
  Tag_ABI_PCS_wchar_t: 4
  Tag_ABI_FP_rounding: Needed
  Tag_ABI_FP_denormal: Needed
  Tag_ABI_FP_exceptions: Needed
  Tag_ABI_FP_number_model: IEEE 754
  Tag_ABI_align_needed: 8-byte
  Tag_ABI_align_preserved: 8-byte, except leaf SP
  Tag_ABI_enum_size: int
  Tag_ABI_VFP_args: VFP registers
  Tag_CPU_unaligned_access: v6

➜  bin readelf --arch-specific my_bare_metal_stuff.elf
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "8-M.MAIN"
  Tag_CPU_arch: v8-M.mainline
  Tag_CPU_arch_profile: Microcontroller
  Tag_THUMB_ISA_use: Yes
  Tag_FP_arch: FPv5/FP-D16 for ARMv8
  Tag_ABI_PCS_wchar_t: 4
  Tag_ABI_FP_denormal: Needed
  Tag_ABI_FP_exceptions: Needed
  Tag_ABI_FP_number_model: IEEE 754
  Tag_ABI_align_needed: 8-byte
  Tag_ABI_enum_size: small
  Tag_ABI_VFP_args: VFP registers
  Tag_CPU_unaligned_access: v6
  Tag_DSP_extension: Allowed

The Tag_CPU_arch_profile value is different, and looks like I could use that.
Alan, would you say this tag is meaningful & reliable?


> Just so we are on the same page as to why this is happening, can you try
> to step into function "gdbarch_info_fill" while you're executable is
> loaded?
>
> What I think is happening is that gdbarch_lookup_osabi returns
> GDB_OSABI_UNKNOWN, because no sniffer identified it.
>
> So the default osabi of your build gets used instead:
>
>     /* From the configured default.  */
>     #ifdef GDB_OSABI_DEFAULT
>       if (info->osabi == GDB_OSABI_UNKNOWN)
>         info->osabi = GDB_OSABI_DEFAULT;
>     #endif

That looks right. `gdbarch_lookup_osabi` indeed returns GDB_OSABI_UNKNOWN.
Here's what I get in gdb after loading the main file:

(gdb) show osabi
The current OS ABI is "auto" (currently "GNU/Linux").
The default OS ABI is "GNU/Linux".



> So indeed, if there was a sniffer identifying the binary as a "none"
> osabi, it would probably work.  The problem is, how do you write such a
> sniffer?  It's easy to check for a Linux binary, you check for the os
> abi ELF note.

As mentioned above, I looked into ELF file differences between
arm-linux and arm-none and couldn't find anything of substance. Am I
missing something?

> But how do you identify a "none" binary?  You would have
> to check that's it's not a Linux binary, not FreeBSD binary, not a
> Windows binary, etc.

Agreed. There seems to be a sniffer for PikeOS, which looks for a
specific symbol in the symbol table. There is a comment in there
saying that the BFD target is otherwise just standard ELF. I'm afraid
that if I was to use a trick, such as the Tag_CPU_arch_profile
mentioned above, I would prevent some other targets from working
properly.

> I don't really like the idea of having a "default" osabi that we fall
> back on, because it can just silently get it wrong, like in this case.
> I think it would make more sense for the fall back to be "none".
> Because it's hard to detect a "none" binary, as explained above.

I looked into configure.tgt, and it looks like default osabi is only
set for targets that have -linux in them. I'm building gdb with
`./configure --enable-targets=arm-none-eabi`, should that help? In my
case, the resulting gdb program still crashes with the same message.
Interestingly, it also prints out a warning:

warning: A handler for the OS ABI "GNU/Linux" is not built into this
configuration
of GDB.  Attempting to continue with the default armv8-m.main settings.

I think in this case, the fact that it uses linux as a default osabi
could be a bug?

In the general case, a multiarch gdb would require `set osabi none` to
work, but that's already better than nothing, IMO.

> In the mean time, for your testing, you can force the osabi to none by
> using "set osabi none" prior to loading anything.

Yes, that works fine. Thanks!
I'll send a revised patch that uses GDB_OSABI_NONE shortly.

Super thanks for the great feedback!
Paul

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

[Simon Marchi]

On 2020-10-06 12:59 p.m., Paul Mathieu via Gdb-patches wrote:
> There are also some notes in the linux binary that look nice:
>
> ➜  bin readelf --notes screen
>
> Displaying notes found in: .note.gnu.build-id
>   Owner                Data size Description
>   GNU                  0x00000014 NT_GNU_BUILD_ID (unique build ID bitstring)
>     Build ID: 76c649229b0402d6a0d1f65c3925e85fcb18e79d
>
> Displaying notes found in: .note.ABI-tag
>   Owner                Data size Description
>   GNU                  0x00000010 NT_GNU_ABI_TAG (ABI version tag)
>     OS: Linux, ABI: 3.2.0
>
> My own bare-metal executable contains no notes at all.
> Again, here, I'm not sure how widespread these notes are, and if linux
> really requires them to run the binary or not.
> As the note name suggests, it looks like a GNU extension, which
> implies that it's not mandatory?

I thought that the ABI version tag was ubiquitous enough, because of:

  https://refspecs.linuxfoundation.org/LSB_1.2.0/gLSB/noteabitag.html

But I just checked in an Alpine system (which uses musl as the libc,
busybox for the core utils, and pretty much no GNU stuff):

    $ docker run -it alpine
    / # apk add binutils
    fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/main/x86_64/APKINDEX.tar.gz
    fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/community/x86_64/APKINDEX.tar.gz
    (1/3) Installing libgcc (9.3.0-r2)
    (2/3) Installing libstdc++ (9.3.0-r2)
    (3/3) Installing binutils (2.34-r1)
    Executing busybox-1.31.1-r16.trigger
    OK: 17 MiB in 17 packages
    / # readelf -n /bin/busybox
    / #

So the question is not arm-specific.  Here, how would we differentiate
this busybox executable from a bare-metal x86-64 executable, and know
that the busybox one is made to run on Linux and not the other one.

> I found one more thing, trying to find differences between the 2 files.
>
> ➜  bin readelf --arch-specific screen
> Attribute Section: aeabi
> File Attributes
>   Tag_CPU_name: "7-A"
>   Tag_CPU_arch: v7
>   Tag_CPU_arch_profile: Application
>   Tag_ARM_ISA_use: Yes
>   Tag_THUMB_ISA_use: Thumb-2
>   Tag_FP_arch: VFPv3-D16
>   Tag_ABI_PCS_wchar_t: 4
>   Tag_ABI_FP_rounding: Needed
>   Tag_ABI_FP_denormal: Needed
>   Tag_ABI_FP_exceptions: Needed
>   Tag_ABI_FP_number_model: IEEE 754
>   Tag_ABI_align_needed: 8-byte
>   Tag_ABI_align_preserved: 8-byte, except leaf SP
>   Tag_ABI_enum_size: int
>   Tag_ABI_VFP_args: VFP registers
>   Tag_CPU_unaligned_access: v6
>
> ➜  bin readelf --arch-specific my_bare_metal_stuff.elf
> Attribute Section: aeabi
> File Attributes
>   Tag_CPU_name: "8-M.MAIN"
>   Tag_CPU_arch: v8-M.mainline
>   Tag_CPU_arch_profile: Microcontroller
>   Tag_THUMB_ISA_use: Yes
>   Tag_FP_arch: FPv5/FP-D16 for ARMv8
>   Tag_ABI_PCS_wchar_t: 4
>   Tag_ABI_FP_denormal: Needed
>   Tag_ABI_FP_exceptions: Needed
>   Tag_ABI_FP_number_model: IEEE 754
>   Tag_ABI_align_needed: 8-byte
>   Tag_ABI_enum_size: small
>   Tag_ABI_VFP_args: VFP registers
>   Tag_CPU_unaligned_access: v6
>   Tag_DSP_extension: Allowed
>
> The Tag_CPU_arch_profile value is different, and looks like I could use that.
> Alan, would you say this tag is meaningful & reliable?

From what I read, this is just the "ARM profile", meaning the letter in
the ARM processor model number.  Cortex M0 == M (Microcontroller).
Cortex A53 -> A (Application).  So not very reliable, it doesn't
indicate whether or not you're running an OS.

>> Just so we are on the same page as to why this is happening, can you try
>> to step into function "gdbarch_info_fill" while you're executable is
>> loaded?
>>
>> What I think is happening is that gdbarch_lookup_osabi returns
>> GDB_OSABI_UNKNOWN, because no sniffer identified it.
>>
>> So the default osabi of your build gets used instead:
>>
>>     /* From the configured default.  */
>>     #ifdef GDB_OSABI_DEFAULT
>>       if (info->osabi == GDB_OSABI_UNKNOWN)
>>         info->osabi = GDB_OSABI_DEFAULT;
>>     #endif
>
> That looks right. `gdbarch_lookup_osabi` indeed returns GDB_OSABI_UNKNOWN.
> Here's what I get in gdb after loading the main file:
>
> (gdb) show osabi
> The current OS ABI is "auto" (currently "GNU/Linux").
> The default OS ABI is "GNU/Linux".

Thanks.

>> So indeed, if there was a sniffer identifying the binary as a "none"
>> osabi, it would probably work.  The problem is, how do you write such a
>> sniffer?  It's easy to check for a Linux binary, you check for the os
>> abi ELF note.
>
> As mentioned above, I looked into ELF file differences between
> arm-linux and arm-none and couldn't find anything of substance. Am I
> missing something?

I don't think so.  Well, the closest thing is .note.ABI-tag, but since
that it's not _always_ there... note sure we can rely on it.

>> But how do you identify a "none" binary?  You would have
>> to check that's it's not a Linux binary, not FreeBSD binary, not a
>> Windows binary, etc.
>
> Agreed. There seems to be a sniffer for PikeOS, which looks for a
> specific symbol in the symbol table. There is a comment in there
> saying that the BFD target is otherwise just standard ELF. I'm afraid
> that if I was to use a trick, such as the Tag_CPU_arch_profile
> mentioned above, I would prevent some other targets from working
> properly.

Indeed.  I don't think sniffers have a concept of priority at the
moment, but we could think of adding that.  Since the PikeOS sniffer is
more specific, it would have a higher priority than yours, so would run
first.  So you would catch whatever was not identified as PikeOS.

>> I don't really like the idea of having a "default" osabi that we fall
>> back on, because it can just silently get it wrong, like in this case.
>> I think it would make more sense for the fall back to be "none".
>> Because it's hard to detect a "none" binary, as explained above.
>
> I looked into configure.tgt, and it looks like default osabi is only
> set for targets that have -linux in them. I'm building gdb with
> `./configure --enable-targets=arm-none-eabi`, should that help? In my
> case, the resulting gdb program still crashes with the same message.
> Interestingly, it also prints out a warning:
>
> warning: A handler for the OS ABI "GNU/Linux" is not built into this
> configuration
> of GDB.  Attempting to continue with the default armv8-m.main settings.
>
> I think in this case, the fact that it uses linux as a default osabi
> could be a bug?

Not sure.  When you use --enable-targets=arm-none-eabi, I think that it
implicitly still uses --target=x86_64-pc-linux-gnu or something
(assuming you work on x86-64 GNU/Linux).  --enable-targets is used to
add more targets than the default one provided by --target.

You end up with this message because:

1. You built GDB with arm/none and x86-64/linux.  You did not include
   arm/linux.
2. The "default osabi", as set in configure.tgt, is Linux, because your
   --target is something-something-linux.
3. The architecture of your executable is ARM, the osabi GDB tries to
   use is "Linux" (because it's the fall back), and it proceeds to
   complain that you don't have support for arm/linux.

So, not really a bug, it's kind of expected due to your configuration
and how the default fall back osabi is decided.

Simon

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

[Luis Machado]

Hi,

I'll go through the new patch soon. I think this was has already been 
said, but it would be nice to have the core format as close as the Linux 
Kernel one as possible, but then have a different ABI marker or some 
other identifiable tag so we can tell those apart.

If we are adding support for this, we should have in mind it should be 
flexible enough to accommodate other targets and future changes, not 
just a one-off.

On 10/3/20 3:14 PM, Paul Mathieu via Gdb-patches wrote:
> Thanks Simon for the super helpful feedback!
> A few answers inline, and hopefully a properly formatted patch at the
> bottom.
> 
>> Note that I am unable to apply this patch either because long lines were
>> again wrapped by your email client.  Please use git-send-email.  You can
>> still reply in the thread by using --in-reply-to.
> 
> Couldn't get git send-email to work with my SMTP, but I got some version
> of mutt to comply. Hopefully you can apply the patch this time.
> 
> 
>> Ok, so that answers the question in my other email, about which tool
>> produces this format of core.  The answer is this specific python
>> script, and the format is "Paul's ARM core format" :).
> 
> It was some team work between what gdb understood "natively" and what
> the patch does.
> More specifically, this patch only seems to tell gdb to get the
> registers from the core file the way it would normally do, and then
> supply them almost verbatim to the regcache.
> The struct for these registers is nothing special either, you can find
> it in the kernel code as well:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135
> 
> 
>> I don't know what Luis thinks about this, but I would be a bit hesitant
>> to add support in GDB for a core format that's not standard nor the
>> output of some "well-known" tool (which would be a de facto standard).
>>
>> Is there a format that already exists in the ARM ecosystem for core
>> dumps of bare-metal systems, we could base our stuff on?
> 
> I'm not aware of anything that does that. There are tools that generate
> memory dumps and CPU register dumps into their own proprietary format,
> but nothing that I know that you could call a 'core file'.
> 
> Are there other tools besides gdb that deal with core dumps? Maybe that
> could help me find other formats.
> 
> That being said, the format used here is "well-known" in the sense that
> it's the exact same format the linux kernel would use to dump cores on
> arm-linux-*
> 
> 
>> Alternatively, do you think you could implement GDB's generate-core-file
>> command for bare-metal ARM?  First, it would make sense for GDB to be
>> able to produce a core in some format and be able to ingest it again.
>> And it would act as some kind of documentation / reference
>> implementation of what GDB expects, and that could become some de facto
>> standard.  People who would like to have their core readable by GDB
>> would produce it in this format.
> 
> I had never cloned the gdb source repo until 2 days ago. I have no idea
> how much work that would be, given that I know next to nothing about the
> gdb codebase.
> 
> I do agree that this seems like a better way to do it, since there are
> already many integrations with a gdb server talking to a live bare-metal
> target over a physical debugger.
> 
> As long as gdb already supports primitives to grab memory dumps and cpu
> registers of a remote target, I'd imagine generating a core file
> shouldn't be too much work. I'm quite sure most of that functionality
> already exists for extremely similar targets.
> 
> 
>>> +void _initialize_arm_none_tdep (void);
>>> +void
>>> +_initialize_arm_none_tdep (void)
>>> +{
>>> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
>>> +                          arm_none_elf_init_abi);
>>
>> I don't think this is what you want to do.  This "declares" that
>> combination of architecture ARM and osabi Linux exists, and that
>> arm_none_elf_init_abi should be called when such a combo is detected to
>> initialize the gdbarch object.
>>
>> However, such a combo is already registered somewhere else, as you can
>> guess:
>>
>>    https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/arm-linux-tdep.c;h=f60cb51763f1c5b14811494b646acf54b62d1f10;hb=HEAD#l2010
>>
>> So I presume that if you build a GDB configured with
>> --enable-targets=all (which will include arm-linux-tdep.c), you will hit
>> this assertion:
>>
>>    https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/osabi.c;h=e8a813b62f268417edf5f0ad164c9c942bd43b1e;hb=HEAD#l179
> 
> Ah, I was afraid of that. Thanks for the pointers.
> 
> 
>> What I think you want is to register an osabi sniffer, whose job is to
>> try to guess an osabi given a bfd.  Apparently that bfd can sometimes
>> represent the ELF core file, so you can look up sections and see if that
>> file looks like a core file you can handle.  And if so, return
>> GDB_OSABI_NONE.
>>
>> And then, you'd need to register (with gdbarch_register_osabi) osabi
>> GDB_OSABI_NONE with the arm architecture (same as you did, but with
>> GDB_OSABI_NONE instead of GDB_OSABI_LINUX) with the callback to call
>> when this combo is detected:
>>
>>    gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_NONE,
>>                            arm_none_elf_init_abi);
>>
>> I'm not sure about that part, but I think it's the right direction.
>>
>> Now, the question is how to recognize one of these cores?  As an
>> example, to recognize Cygwin core dumps (which use ELF files as a
>> container without a recognizable ABI tag, so it's kind of the same
>> situation as you), we use the .reg section size to determine if the BFD
>> is a Cygwin core:
>>
>>    https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/amd64-windows-tdep.c;h=e427c20538961bda2a1a0f033207eebce64c4729;hb=HEAD#l1387
>>
>> So, you can do that, although I think we all agree that it's not ideal.
>> If there was some way of identifying that the core is in the format we
>> recognize (say, if there was some special section with a tag), it would
>> make it much easier to identify it without having to guess.
> 
> Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.
> 
> Since the format isn't specified (yet), I can still freely control it. I
> imagine I could add some sort of OSABI marker in the core file to mark
> it as an arm-none-eabi core.o
> 
> My first guess was to set the EI_OSABI e_ident elf header field to
> ELFOSABI_NONE, but that happens to be the same enum value as
> ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
> files. So it wouldn't be a good marker.
> 
> Not sure what a better way would be to not abuse the ELF structure and
> produce reasonable ELF core dumps (since they already work so well with
> gdb).
> 
> 
> Paul
> 
> 
> 
> gdb/Changelog
> 2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
> 2020-10-02  Paul Mathieu <paulmathieu@google.com>
> 	* arm-none-tdep.c: Source file added. Provide CPU registers from a core
> 	file
> 	* floating point registers not yet supported (FIXME)
> ---
>   gdb/Makefile.in     |   2 +
>   gdb/arm-none-tdep.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
>   gdb/configure.tgt   |   2 +-
>   3 files changed, 109 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..21743c40a0
> --- /dev/null
> +++ b/gdb/arm-none-tdep.c
> @@ -0,0 +1,106 @@
> +/* Native-dependent code for GDB targetting bare-metal ARM.
> +
> +   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 "osabi.h"
> +
> +#include "arch/arm.h"
> +#include "arm-tdep.h"
> +#include "gdbarch.h"
> +#include "regcache.h"
> +#include "regset.h"
> +
> +struct arm_none_reg
> +{
> +  uint32_t reg[13];
> +  uint32_t sp;
> +  uint32_t lr;
> +  uint32_t pc;
> +  uint32_t cpsr;
> +  uint32_t orig_r0;
> +};
> +
> +static void
> +arm_none_supply_gregset (const struct regset *regset,
> +                         struct regcache *regcache, int regnum,
> +                         const void *gregs, size_t len)
> +{
> +  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
> +  gdb_assert (len >= sizeof (arm_none_reg));
> +
> +  /* Integer registers.  */
> +  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
> +    if (regnum == -1 || regnum == i)
> +      regcache->raw_supply (i, (char *)&gregset->reg[i]);
> +
> +  if (regnum == -1 || regnum == ARM_SP_REGNUM)
> +    regcache->raw_supply (ARM_SP_REGNUM, (char *)&gregset->sp);
> +
> +  if (regnum == -1 || regnum == ARM_LR_REGNUM)
> +    regcache->raw_supply (ARM_LR_REGNUM, (char *)&gregset->lr);
> +
> +  if (regnum == -1 || regnum == ARM_PC_REGNUM)
> +    {
> +      CORE_ADDR r_pc
> +        = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);
> +      regcache->raw_supply (ARM_PC_REGNUM, (char *)&r_pc);
> +    }
> +
> +  if (regnum == -1 || regnum == ARM_PS_REGNUM)
> +    {
> +      if (arm_apcs_32)
> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->cpsr);
> +      else
> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->pc);
> +    }
> +}
> +
> +static const struct regset arm_none_regset
> +    = { nullptr, arm_none_supply_gregset,
> +        /* We don't need a collect function because we only use this reading
> +           registers (via iterate_over_regset_sections and
> +           fetch_regs/fetch_register).  */
> +        nullptr, 0 };
> +
> +static void
> +arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
> +                                       iterate_over_regset_sections_cb *cb,
> +                                       void *cb_data,
> +                                       const struct regcache *regcache)
> +{
> +  cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg), &arm_none_regset,
> +      NULL, cb_data);
> +}
> +
> +static void arm_none_elf_init_abi (struct gdbarch_info info,
> +                                   struct gdbarch *gdbarch);
> +static void
> +arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  set_gdbarch_iterate_over_regset_sections (
> +      gdbarch, arm_none_iterate_over_regset_sections);
> +}
> +
> +void _initialize_arm_none_tdep (void);
> +void
> +_initialize_arm_none_tdep (void)
> +{
> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
> +                          arm_none_elf_init_abi);
> +}
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index a3e11c4b9b..0cf05efdd1 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -189,7 +189,7 @@ arm*-*-symbianelf*)
>   	;;
>   arm*-*-*)
>   	# Target: ARM embedded system
> -	gdb_target_obs="arm-pikeos-tdep.o"
> +	gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
>   	gdb_sim=../sim/arm/libsim.a
>   	;;
> 
> --
> 2.28.0.806.g8561365e88-goog
> 

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

[Alan Hayward]

> On 5 Oct 2020, at 13:58, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> Hi,
> 
> I'll go through the new patch soon. I think this was has already been said, but it would be nice to have the core format as close as the Linux Kernel one as possible, but then have a different ABI marker or some other identifiable tag so we can tell those apart.
> 
> If we are adding support for this, we should have in mind it should be flexible enough to accommodate other targets and future changes, not just a one-off.
> 

Been going back and too on my opinion here.
Should we be defining a format ourselves here - And if we do, who else would
use it. But having something is better than nothing. And if it’s simple and
obvious, based on existing standards, then maybe that’s all that’s needed.

+1 for GDB being able to generate the files. That’s fairly crucial.

+1 for Documentation. Maybe even outside of GDB. But I’m not entirely sure
where that would be.

Where it makes sense, sharing code between arm-linux and arm-none would be
good too.


Alan.

> On 10/3/20 3:14 PM, Paul Mathieu via Gdb-patches wrote:
>> Thanks Simon for the super helpful feedback!
>> A few answers inline, and hopefully a properly formatted patch at the
>> bottom.
>>> Note that I am unable to apply this patch either because long lines were
>>> again wrapped by your email client.  Please use git-send-email.  You can
>>> still reply in the thread by using --in-reply-to.
>> Couldn't get git send-email to work with my SMTP, but I got some version
>> of mutt to comply. Hopefully you can apply the patch this time.
>>> Ok, so that answers the question in my other email, about which tool
>>> produces this format of core.  The answer is this specific python
>>> script, and the format is "Paul's ARM core format" :).
>> It was some team work between what gdb understood "natively" and what
>> the patch does.
>> More specifically, this patch only seems to tell gdb to get the
>> registers from the core file the way it would normally do, and then
>> supply them almost verbatim to the regcache.
>> The struct for these registers is nothing special either, you can find
>> it in the kernel code as well:
>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/uapi/asm/ptrace.h#L135
>>> I don't know what Luis thinks about this, but I would be a bit hesitant
>>> to add support in GDB for a core format that's not standard nor the
>>> output of some "well-known" tool (which would be a de facto standard).
>>> 
>>> Is there a format that already exists in the ARM ecosystem for core
>>> dumps of bare-metal systems, we could base our stuff on?
>> I'm not aware of anything that does that. There are tools that generate
>> memory dumps and CPU register dumps into their own proprietary format,
>> but nothing that I know that you could call a 'core file'.
>> Are there other tools besides gdb that deal with core dumps? Maybe that
>> could help me find other formats.
>> That being said, the format used here is "well-known" in the sense that
>> it's the exact same format the linux kernel would use to dump cores on
>> arm-linux-*
>>> Alternatively, do you think you could implement GDB's generate-core-file
>>> command for bare-metal ARM?  First, it would make sense for GDB to be
>>> able to produce a core in some format and be able to ingest it again.
>>> And it would act as some kind of documentation / reference
>>> implementation of what GDB expects, and that could become some de facto
>>> standard.  People who would like to have their core readable by GDB
>>> would produce it in this format.
>> I had never cloned the gdb source repo until 2 days ago. I have no idea
>> how much work that would be, given that I know next to nothing about the
>> gdb codebase.
>> I do agree that this seems like a better way to do it, since there are
>> already many integrations with a gdb server talking to a live bare-metal
>> target over a physical debugger.
>> As long as gdb already supports primitives to grab memory dumps and cpu
>> registers of a remote target, I'd imagine generating a core file
>> shouldn't be too much work. I'm quite sure most of that functionality
>> already exists for extremely similar targets.
>>>> +void _initialize_arm_none_tdep (void);
>>>> +void
>>>> +_initialize_arm_none_tdep (void)
>>>> +{
>>>> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
>>>> +                          arm_none_elf_init_abi);
>>> 
>>> I don't think this is what you want to do.  This "declares" that
>>> combination of architecture ARM and osabi Linux exists, and that
>>> arm_none_elf_init_abi should be called when such a combo is detected to
>>> initialize the gdbarch object.
>>> 
>>> However, such a combo is already registered somewhere else, as you can
>>> guess:
>>> 
>>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/arm-linux-tdep.c;h=f60cb51763f1c5b14811494b646acf54b62d1f10;hb=HEAD#l2010
>>> 
>>> So I presume that if you build a GDB configured with
>>> --enable-targets=all (which will include arm-linux-tdep.c), you will hit
>>> this assertion:
>>> 
>>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/osabi.c;h=e8a813b62f268417edf5f0ad164c9c942bd43b1e;hb=HEAD#l179
>> Ah, I was afraid of that. Thanks for the pointers.
>>> What I think you want is to register an osabi sniffer, whose job is to
>>> try to guess an osabi given a bfd.  Apparently that bfd can sometimes
>>> represent the ELF core file, so you can look up sections and see if that
>>> file looks like a core file you can handle.  And if so, return
>>> GDB_OSABI_NONE.
>>> 
>>> And then, you'd need to register (with gdbarch_register_osabi) osabi
>>> GDB_OSABI_NONE with the arm architecture (same as you did, but with
>>> GDB_OSABI_NONE instead of GDB_OSABI_LINUX) with the callback to call
>>> when this combo is detected:
>>> 
>>>   gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_NONE,
>>>                           arm_none_elf_init_abi);
>>> 
>>> I'm not sure about that part, but I think it's the right direction.
>>> 
>>> Now, the question is how to recognize one of these cores?  As an
>>> example, to recognize Cygwin core dumps (which use ELF files as a
>>> container without a recognizable ABI tag, so it's kind of the same
>>> situation as you), we use the .reg section size to determine if the BFD
>>> is a Cygwin core:
>>> 
>>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/amd64-windows-tdep.c;h=e427c20538961bda2a1a0f033207eebce64c4729;hb=HEAD#l1387
>>> 
>>> So, you can do that, although I think we all agree that it's not ideal.
>>> If there was some way of identifying that the core is in the format we
>>> recognize (say, if there was some special section with a tag), it would
>>> make it much easier to identify it without having to guess.
>> Having a sniffer for GDB_OSABI_NONE seems like the right way to do this.
>> Since the format isn't specified (yet), I can still freely control it. I
>> imagine I could add some sort of OSABI marker in the core file to mark
>> it as an arm-none-eabi core.o
>> My first guess was to set the EI_OSABI e_ident elf header field to
>> ELFOSABI_NONE, but that happens to be the same enum value as
>> ELFOSABI_SYSV, which is afaik broadly used by the linux kernel for core
>> files. So it wouldn't be a good marker.
>> Not sure what a better way would be to not abuse the ELF structure and
>> produce reasonable ELF core dumps (since they already work so well with
>> gdb).
>> Paul
>> gdb/Changelog
>> 2018-09-29  Robin Haberkorn <robin.haberkorn@googlemail.com>
>> 2020-10-02  Paul Mathieu <paulmathieu@google.com>
>> 	* arm-none-tdep.c: Source file added. Provide CPU registers from a core
>> 	file
>> 	* floating point registers not yet supported (FIXME)
>> ---
>>  gdb/Makefile.in     |   2 +
>>  gdb/arm-none-tdep.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
>>  gdb/configure.tgt   |   2 +-
>>  3 files changed, 109 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..21743c40a0
>> --- /dev/null
>> +++ b/gdb/arm-none-tdep.c
>> @@ -0,0 +1,106 @@
>> +/* Native-dependent code for GDB targetting bare-metal ARM.
>> +
>> +   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 "osabi.h"
>> +
>> +#include "arch/arm.h"
>> +#include "arm-tdep.h"
>> +#include "gdbarch.h"
>> +#include "regcache.h"
>> +#include "regset.h"
>> +
>> +struct arm_none_reg
>> +{
>> +  uint32_t reg[13];
>> +  uint32_t sp;
>> +  uint32_t lr;
>> +  uint32_t pc;
>> +  uint32_t cpsr;
>> +  uint32_t orig_r0;
>> +};
>> +
>> +static void
>> +arm_none_supply_gregset (const struct regset *regset,
>> +                         struct regcache *regcache, int regnum,
>> +                         const void *gregs, size_t len)
>> +{
>> +  const arm_none_reg *gregset = static_cast<const arm_none_reg *> (gregs);
>> +  gdb_assert (len >= sizeof (arm_none_reg));
>> +
>> +  /* Integer registers.  */
>> +  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
>> +    if (regnum == -1 || regnum == i)
>> +      regcache->raw_supply (i, (char *)&gregset->reg[i]);
>> +
>> +  if (regnum == -1 || regnum == ARM_SP_REGNUM)
>> +    regcache->raw_supply (ARM_SP_REGNUM, (char *)&gregset->sp);
>> +
>> +  if (regnum == -1 || regnum == ARM_LR_REGNUM)
>> +    regcache->raw_supply (ARM_LR_REGNUM, (char *)&gregset->lr);
>> +
>> +  if (regnum == -1 || regnum == ARM_PC_REGNUM)
>> +    {
>> +      CORE_ADDR r_pc
>> +        = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);
>> +      regcache->raw_supply (ARM_PC_REGNUM, (char *)&r_pc);
>> +    }
>> +
>> +  if (regnum == -1 || regnum == ARM_PS_REGNUM)
>> +    {
>> +      if (arm_apcs_32)
>> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->cpsr);
>> +      else
>> +        regcache->raw_supply (ARM_PS_REGNUM, (char *)&gregset->pc);
>> +    }
>> +}
>> +
>> +static const struct regset arm_none_regset
>> +    = { nullptr, arm_none_supply_gregset,
>> +        /* We don't need a collect function because we only use this reading
>> +           registers (via iterate_over_regset_sections and
>> +           fetch_regs/fetch_register).  */
>> +        nullptr, 0 };
>> +
>> +static void
>> +arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
>> +                                       iterate_over_regset_sections_cb *cb,
>> +                                       void *cb_data,
>> +                                       const struct regcache *regcache)
>> +{
>> +  cb (".reg", sizeof (arm_none_reg), sizeof (arm_none_reg), &arm_none_regset,
>> +      NULL, cb_data);
>> +}
>> +
>> +static void arm_none_elf_init_abi (struct gdbarch_info info,
>> +                                   struct gdbarch *gdbarch);
>> +static void
>> +arm_none_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>> +{
>> +  set_gdbarch_iterate_over_regset_sections (
>> +      gdbarch, arm_none_iterate_over_regset_sections);
>> +}
>> +
>> +void _initialize_arm_none_tdep (void);
>> +void
>> +_initialize_arm_none_tdep (void)
>> +{
>> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_LINUX,
>> +                          arm_none_elf_init_abi);
>> +}
>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>> index a3e11c4b9b..0cf05efdd1 100644
>> --- a/gdb/configure.tgt
>> +++ b/gdb/configure.tgt
>> @@ -189,7 +189,7 @@ arm*-*-symbianelf*)
>>  	;;
>>  arm*-*-*)
>>  	# Target: ARM embedded system
>> -	gdb_target_obs="arm-pikeos-tdep.o"
>> +	gdb_target_obs="arm-pikeos-tdep.o arm-none-tdep.o"
>>  	gdb_sim=../sim/arm/libsim.a
>>  	;;
>> --
>> 2.28.0.806.g8561365e88-goog

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

[Simon Marchi]

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

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

[Paul Mathieu]

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
>

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

[Simon Marchi]

On 2020-10-02 8:35 p.m., Paul Mathieu wrote:
> 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?

Ah sorry, I didn't see it buried in your response.  It's fine, although
doing proper version helps tracking the different versions.

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

I'm not very well-versed in the domain of core files, but it is my
understanding that the format is quite platform dependent.  The
container is often an ELF file with notes.  But the format of those
notes' content varies from one platform to another.  For example, the
layout of how registers are saved in a Linux core file differs from the
layout of registers in a FreeBSD core.  Similarly, for bare-metal, the
tool you are using to generate the core decides of a certain layout,
which could be different from the layout chosen by another tool.

Therefore, I think it's important to describe what the layout expected
by your code is (what tool/platform produces it).

It is a good start to document it somewhere in the code.  We can then
think if it would be relevant to document it in the user manual as well
(we'll have a better idea when we understand better where these cores
come from).

Simon