[PATCH v2] Support for corefiles for arm-none-eabi target.
Simon Marchi
simark@simark.ca
Wed Oct 21 02:51:35 GMT 2020
Hi Fredrik,
Note that for the patch to be accepted, it will need a commit message
that explains sufficiently well what's happening, as well as a ChangeLog
entry:
https://sourceware.org/gdb/wiki/ContributionChecklist#ChangeLog
https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog
You will need to add your new files to the ALL_TARGET_OBS list in
gdb/Makefile.in, so that they get built when GDB is configured with
--enable-targets=all.
I posted a patch that makes gdbarch_make_corefile_notes return a
gdb::unique_xmalloc_ptr<char>:
https://sourceware.org/pipermail/gdb-patches/2020-October/172712.html
It will probably be merged soon, so you can probably rebase yours on top
and imitate the changes I did in linux-tdep.c and fbsd-tdep.c.
Here's a first round of comments below.
On 2020-10-20 5:58 p.m., Fredrik Hederstierna wrote:
> diff --git a/gdb/arm-none-tdep.c b/gdb/arm-none-tdep.c
> new file mode 100644
> index 0000000000..9d83a8b913
> --- /dev/null
> +++ b/gdb/arm-none-tdep.c
> @@ -0,0 +1,368 @@
> +/* none on ARM target support.
> +
> + 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 "target.h"
> +#include "value.h"
> +#include "gdbtypes.h"
> +#include "gdbcore.h"
> +#include "frame.h"
> +#include "regcache.h"
> +#include "solib-svr4.h"
> +#include "osabi.h"
> +#include "regset.h"
> +#include "trad-frame.h"
> +#include "tramp-frame.h"
> +#include "breakpoint.h"
> +#include "auxv.h"
> +
> +#include "aarch32-tdep.h"
> +#include "arch/arm.h"
> +#include "arm-tdep.h"
> +#include "arm-none-tdep.h"
> +#include "glibc-tdep.h"
> +#include "arch-utils.h"
> +#include "inferior.h"
> +#include "gdbthread.h"
> +#include "symfile.h"
> +
> +#include "cli/cli-utils.h"
> +#include "stap-probe.h"
> +#include "parser-defs.h"
> +#include "user-regs.h"
> +#include <ctype.h>
> +
> +#include "elf-bfd.h"
> +#include "coff/internal.h"
> +#include "elf/arm.h"
> +
> +#include "elf/common.h"
I have the feeling that a bunch of these includes are not necessary.
Can you reduce the the essential?
> +
> +/* ---- based on arm-linux-tdep.h ---- */
I don't think it's useful to say it's based on this other file.
Instead, you might want to say that the register layout was
designed based on the layout used by Linux.
> +
> +#define ARM_NONE_SIZEOF_NWFPE (8 * ARM_FP_REGISTER_SIZE \
> + + 2 * ARM_INT_REGISTER_SIZE \
> + + 8 + ARM_INT_REGISTER_SIZE)
> +
> +/* The index to access CSPR in user_regs defined in GLIBC. */
> +#define ARM_CPSR_GREGNUM 16
> +
> +/* Support for register format used by the NWFPE FPA emulator. Each
> + register takes three words, where either the first one, two, or
> + three hold a single, double, or extended precision value (depending
> + on the corresponding tag). The register set is eight registers,
> + followed by the fpsr and fpcr, followed by eight tag bytes, and a
> + final word flag which indicates whether NWFPE has been
> + initialized. */
> +
> +#define NWFPE_FPSR_OFFSET (8 * ARM_FP_REGISTER_SIZE)
> +#define NWFPE_FPCR_OFFSET (NWFPE_FPSR_OFFSET + ARM_INT_REGISTER_SIZE)
> +#define NWFPE_TAGS_OFFSET (NWFPE_FPCR_OFFSET + ARM_INT_REGISTER_SIZE)
> +#define NWFPE_INITFLAG_OFFSET (NWFPE_TAGS_OFFSET + 8)
What's NWFPE? From what I can read, it's floating point emulation in
the Linux kernels, is that right? If so, is it relevant to have here?
> diff --git a/gdb/arm-none-tdep.h b/gdb/arm-none-tdep.h
> new file mode 100644
> index 0000000000..7927c0aff3
> --- /dev/null
> +++ b/gdb/arm-none-tdep.h
> @@ -0,0 +1,60 @@
> +/* none on ARM target support, prototypes.
> +
> + 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/>. */
> +
> +#ifndef ARM_NONE_TDEP_H
> +#define ARM_NONE_TDEP_H
> +
> +#include "gdbarch.h"
> +#include "target.h"
> +#include "bfd.h"
> +
> +struct regset;
> +struct regcache;
> +
> +void arm_none_supply_gregset (const struct regset *regset,
> + struct regcache *regcache,
> + int regnum, const void *gregs_buf, size_t len);
> +void arm_none_collect_gregset (const struct regset *regset,
> + const struct regcache *regcache,
> + int regnum, void *gregs_buf, size_t len);
> +
> +void arm_none_supply_nwfpe_register (struct regcache *regcache, int regno,
> + const gdb_byte *regs);
> +void arm_none_collect_nwfpe_register (const struct regcache *regcache, int regno,
> + gdb_byte *regs);
> +
> +void arm_none_supply_nwfpe (const struct regset *regset,
> + struct regcache *regcache,
> + int regnum, const void *regs_buf, size_t len);
> +void arm_none_collect_nwfpe (const struct regset *regset,
> + const struct regcache *regcache,
> + int regnum, void *regs_buf, size_t len);
> +
> +void
> +arm_none_iterate_over_regset_sections (struct gdbarch *gdbarch,
> + iterate_over_regset_sections_cb *cb,
> + void *cb_data,
> + const struct regcache *regcache);
> +
> +const struct target_desc *
> +arm_none_core_read_description (struct gdbarch *gdbarch,
> + struct target_ops *target,
> + bfd *abfd);
For all functions declared above, if they are only used in
arm-none-tdep.c, make them static to arm-none-tdep.c.
> +
> +#endif /* ARM_NONE_TDEP_H */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index a214f22d7a..be47cc2228 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -50,6 +50,8 @@
> #include "arch/arm.h"
> #include "arch/arm-get-next-pcs.h"
> #include "arm-tdep.h"
> +#include "none-tdep.h"
> +#include "arm-none-tdep.h"
> #include "gdb/sim-arm.h"
>
> #include "elf-bfd.h"
> @@ -9453,6 +9455,12 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> /* Virtual tables. */
> set_gdbarch_vbit_in_delta (gdbarch, 1);
>
> + /* Default none core file support, can be overridden by osabi. */
Our convention is to have two spaces after periods, even the last one:
/* Default none core file support, can be overridden by osabi. */
Apply everywhere.
> +
> /* Hook in the ABI-specific overrides, if they have been registered. */
> gdbarch_init_osabi (info, gdbarch);
>
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index d865ecdcb6..f4a1540464 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -64,7 +64,8 @@ arc*-*-*)
>
> arm*-*-*)
> cpu_obs="aarch32-tdep.o arch/aarch32.o arch/arm.o \
> - arch/arm-get-next-pcs.o arm-tdep.o";;
> + arch/arm-get-next-pcs.o arm-tdep.o \
> + none-tdep.o arm-none-tdep.o";;
>
> hppa*-*-*)
> # Target: HP PA-RISC
> @@ -798,4 +799,7 @@ for t in x ${gdb_target_obs}; do
> if test "$t" = linux-tdep.o; then
> gdb_have_gcore=true
> fi
> + if test "$t" = arm-none-tdep.o; then
> + gdb_have_gcore=true
> + fi
Are you sure you want this? gcore is a standalone program to generate a
core given a pid, I don't think it applies in our bare-metal scenario,
does it?
> +/* Callback for iterate_over_regset_sections that records a single
> + regset in the corefile note section. */
> +
> +static void
> +none_collect_regset_section_cb (const char *sect_name, int supply_size,
> + int collect_size, const struct regset *regset,
> + const char *human_name, void *cb_data)
Make sure the right whitespaces are used throughout. It should use as
many tabs as possible (one tab is 8 columnms) and then spaces until the
desired column.
> +{
> + struct none_collect_regset_section_cb_data *data
> + = (struct none_collect_regset_section_cb_data *) cb_data;
> + bool variable_size_section = (regset != NULL
> + && regset->flags & REGSET_VARIABLE_SIZE);
> +
> + if (!variable_size_section)
> + gdb_assert (supply_size == collect_size);
> +
> + if (data->abort_iteration)
> + return;
> +
> + gdb_assert (regset && regset->collect_regset);
When comparing pointers, we use explicit comparison, so:
regset == nullptr
regset != nullptr
We also use nullptr instead of NULL for new code, since we have switched
to C++. You'll find plenty of old code that doesn't follow this, but we
try to be strict for the new or modified code.
> +/* Fetch the siginfo data for the specified thread, if it exists. If
> + there is no data, or we could not read it, return an empty
> + buffer. */
> +
> +static gdb::byte_vector
> +none_get_siginfo_data (thread_info *thread, struct gdbarch *gdbarch)
> +{
> + struct type *siginfo_type;
> + LONGEST bytes_read;
> +
> + if (!gdbarch_get_siginfo_type_p (gdbarch))
> + return gdb::byte_vector ();
> +
> + scoped_restore_current_thread save_current_thread;
> + switch_to_thread (thread);
> +
> + siginfo_type = gdbarch_get_siginfo_type (gdbarch);
> +
> + gdb::byte_vector buf (TYPE_LENGTH (siginfo_type));
> +
> + bytes_read = target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL,
> + buf.data (), 0, TYPE_LENGTH (siginfo_type));
> + if (bytes_read != TYPE_LENGTH (siginfo_type))
> + buf.clear ();
> +
> + return buf;
> +}
Is siginfo relevant for a bare-metal target?
> +struct none_corefile_thread_data
> +{
> + struct gdbarch *gdbarch;
> + bfd *obfd;
> + char *note_data;
> + int *note_size;
> + enum gdb_signal stop_signal;
> +};
> +
> +/* Records the thread's register state for the corefile note
> + section. */
> +
> +static void
> +none_corefile_thread (struct thread_info *info,
> + struct none_corefile_thread_data *args)
> +{
> + struct regcache *regcache;
> +
> + regcache = get_thread_arch_regcache (info->inf->process_target (),
> + info->ptid, args->gdbarch);
> +
> + target_fetch_registers (regcache, -1);
> + gdb::byte_vector siginfo_data = none_get_siginfo_data (info, args->gdbarch);
> +
> + args->note_data = none_collect_thread_registers
> + (regcache, info->ptid, args->obfd, args->note_data,
> + args->note_size, args->stop_signal);
> +
> + /* Don't return anything if we got no register information above,
> + such a core file is useless. */
> + if (args->note_data != NULL)
> + if (!siginfo_data.empty ())
> + args->note_data = elfcore_write_note (args->obfd,
> + args->note_data,
> + args->note_size,
> + "CORE", NT_SIGINFO,
> + siginfo_data.data (),
> + siginfo_data.size ());
> +}
> +
> +/* Find the signalled thread. In case there's more than one signalled
> + thread, prefer the current thread, if it is signalled. If no
> + thread was signalled, default to the current thread, unless it has
> + exited, in which case return NULL. */
> +
> +static thread_info *
> +find_signalled_thread ()
> +{
> + thread_info *curr_thr = inferior_thread ();
> + if (curr_thr->state != THREAD_EXITED
> + && curr_thr->suspend.stop_signal != GDB_SIGNAL_0)
> + return curr_thr;
> +
> + for (thread_info *thr : current_inferior ()->non_exited_threads ())
> + if (thr->suspend.stop_signal != GDB_SIGNAL_0)
> + return thr;
> +
> + /* Default to the current thread, unless it has exited. */
> + if (curr_thr->state != THREAD_EXITED)
> + return curr_thr;
> +
> + return nullptr;
> +}
> +
> +/* Fills the "to_make_corefile_note" target vector. Builds the note
> + section for a corefile, and returns it in a malloc buffer. */
> +
> +char *
> +none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
> + none_collect_thread_registers_ftype collect)
> +{
> + struct none_corefile_thread_data thread_args;
> + char *note_data = NULL;
> +
> + /* Process information. */
> + if (get_exec_file (0))
> + {
> + const char *fname = lbasename (get_exec_file (0));
> + char *psargs = xstrdup (fname);
> +
> + if (get_inferior_args ())
> + psargs = reconcat (psargs, psargs, " ", get_inferior_args (),
> + (char *) NULL);
> +
> + note_data = elfcore_write_prpsinfo (obfd, note_data, note_size,
> + fname, psargs);
> + xfree (psargs);
> + }
> +
> + if (!note_data)
> + return NULL;
> +
> + /* Thread register information. */
> + try
> + {
> + update_thread_list ();
> + }
> + catch (const gdb_exception_error &e)
> + {
> + exception_print (gdb_stderr, e);
> + }
> +
> + /* Like the kernel, prefer dumping the signalled thread first.
> + "First thread" is what tools use to infer the signalled
> + thread. */
> + thread_info *signalled_thr = find_signalled_thread ();
> +
> + thread_args.gdbarch = gdbarch;
> + thread_args.obfd = obfd;
> + thread_args.note_data = note_data;
> + thread_args.note_size = note_size;
> + if (signalled_thr != nullptr)
> + thread_args.stop_signal = signalled_thr->suspend.stop_signal;
> + else
> + thread_args.stop_signal = GDB_SIGNAL_0;
> +
> + if (signalled_thr != nullptr)
> + none_corefile_thread (signalled_thr, &thread_args);
> + for (thread_info *thr : current_inferior ()->non_exited_threads ())
> + {
> + if (thr == signalled_thr)
> + continue;
> +
> + none_corefile_thread (thr, &thread_args);
> + }
> +
> + note_data = thread_args.note_data;
> + if (!note_data)
> + return NULL;
> +
> + /* Auxillary vector. */
> + gdb::optional<gdb::byte_vector> auxv =
> + target_read_alloc (current_top_target (), TARGET_OBJECT_AUXV, NULL);
> + if (auxv && !auxv->empty ())
> + {
> + note_data = elfcore_write_note (obfd, note_data, note_size,
> + "CORE", NT_AUXV, auxv->data (),
> + auxv->size ());
> + if (!note_data)
> + return NULL;
> + }
As mentioned earlier, I don't think auxv is relevant for a bare-metal
target, I don't know of any bare-metal target using this concept. Do
you use it?
I'd suggest just sticking to the features you use and have actually
tested (it's not clear to me what in this file you actually use for your
use case). More things can be added later as needed.
> +
> + /* make_cleanup (xfree, note_data); */
> + return note_data;
> +}
> +
> +static char *
> +none_make_corefile_notes_1 (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
> +{
Our convention is that the _1 version of the function is the helper, so
it would be the other way around.
> + return none_make_corefile_notes (gdbarch, obfd, note_size,
> + none_collect_thread_registers);
> +}
> +
> +/* Setup default core file support for none targets.
> + Can be overridden later by OSABI. */
> +
> +void
> +none_init_corefile (struct gdbarch_info info,
> + struct gdbarch *gdbarch)
Function comments go to the header file. Here, you just write:
/* See none-tdep.h. */
> +{
> + /* Default core file support. */
> + set_gdbarch_make_corefile_notes (gdbarch, none_make_corefile_notes_1);
> +}
> +
> +/* Provide a prototype to silence -Wmissing-prototypes. */
> +extern initialize_file_ftype _initialize_none_tdep;
> +
> +void
> +_initialize_none_tdep (void)
> +{
> + /* No special actions. */
> +}
Remove if not needed.
> diff --git a/gdb/none-tdep.h b/gdb/none-tdep.h
> new file mode 100644
> index 0000000000..4aea7004d8
> --- /dev/null
> +++ b/gdb/none-tdep.h
> @@ -0,0 +1,39 @@
> +/* Target-dependent code for none, architecture independent.
> +
> + 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/>. */
> +
> +#ifndef NONE_TDEP_H
> +#define NONE_TDEP_H
> +
> +#include "bfd.h"
> +
> +struct regcache;
> +
> +typedef char *(*none_collect_thread_registers_ftype) (const struct regcache *,
> + ptid_t,
> + bfd *, char *, int *,
> + enum gdb_signal);
> +char *
> +none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
> + none_collect_thread_registers_ftype collect);
For declarations, the return type doesn't go on its own line, sorry if I
lead you into error.
Thanks,
Simon
More information about the Gdb-patches
mailing list