[PATCH v2] gdb: add support for handling core dumps on arm-none-eabi
Simon Marchi
simark@simark.ca
Sun Oct 4 23:41:22 GMT 2020
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
More information about the Gdb-patches
mailing list