[RFA,v2] arm-pikeos: software single step

  Hi 
On Tue, Sep 11, 2018 at 09:40:57AM -0600, Tom Tromey wrote:
> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> +  long max_number_of_symbols
> Joel> +    = bfd_get_symtab_upper_bound (abfd) / sizeof (asymbol *);
> Joel> +  if (max_number_of_symbols <= 0)
> Joel> +    return GDB_OSABI_UNKNOWN;
> 
> Joel> +  std::vector<asymbol *> symbol_table (max_number_of_symbols);
> Joel> +  number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table.data ());
> Joel> +  if (number_of_symbols <= 0)
> Joel> +    return GDB_OSABI_UNKNOWN;
> Joel> +  gdb_assert (number_of_symbols <= max_number_of_symbols);
> Joel> +  symbol_table.resize (number_of_symbols);
> 
> I looked, and some spots doing this just use xmalloc and manage it
> manually.  machoread though uses gdb::def_vector; which is nice since it
> doesn't clear the memory.

Indeed; I think the std::vector approach I had also had that property,
if I understand the spec correctly. Before I could see this suggestion,
I thought gdb::unique_xmalloc_ptr kind of conveyed the idea that
this was more of a C object (due to the use with BFD), than a C++
object, so used that instead and found the code to be slightly
smaller. The approach has its drawbacks as well, so maybe if people
have a preference for using the gdb::def_vector, I can do that too.

> No idea why it isn't working for you, the patch looks ok to me.
> Sorry about this.  If you'd rather get it in, I can remove the cleanup later.

Thanks. I didn't want to do that, because it would have put the burden
on you, and new code shouldn't add constructs we are actively trying
to remove.

Luckily, I found that the issue was due to some instability of
the target, and could repeat the same errors with AdaCore version
of GDB. Our testsuite framework is capable of dealing with that,
by shutting down and restarting the development environment before
re-trying the testcase. This is actually activated by default, but
like the idiot that I am, I had explicitly turned it off!

The testing is running as we speak, but because it's one testcase
at a time, I will not have the results until tomorrow. But so far,
so good, so fingers crossed...

In the menatime, attached is the patch I am testing.

gdb/ChangeLog:

        * defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
        * osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
        * arm-pikeos-tdep.c: New file.
        * configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
        embedded system.
        * Makefile.in (ALL_TARGET_OBS): Add arm-pikeos-tdep.o.

Note the discussion with Pedro about getting the vendor to enhance
the stub to tell GDB that hardware single stepping is not supported
(https://www.sourceware.org/ml/gdb-patches/2018-09/msg00345.html).
We'll work with the vendor, and so this patch would be applied as
a stop gap measure. But it's unclear if and when the stub would be
enhanced, allowing us to remove this patch.

Thanks!
  

> The testing is running as we speak, but because it's one testcase
> at a time, I will not have the results until tomorrow. But so far,
> so good, so fingers crossed...
> 
> In the menatime, attached is the patch I am testing.
> 
> gdb/ChangeLog:
> 
>         * defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
>         * osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
>         * arm-pikeos-tdep.c: New file.
>         * configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
>         embedded system.
>         * Makefile.in (ALL_TARGET_OBS): Add arm-pikeos-tdep.o.

The testing was clean (pfew!).

> Note the discussion with Pedro about getting the vendor to enhance
> the stub to tell GDB that hardware single stepping is not supported
> (https://www.sourceware.org/ml/gdb-patches/2018-09/msg00345.html).
> We'll work with the vendor, and so this patch would be applied as
> a stop gap measure. But it's unclear if and when the stub would be
> enhanced, allowing us to remove this patch.

For the record, as mentioned later in the thread above, the enhancement
request should be passed to Sysgo soon.

In the meantime, is it OK to push this patch as a stopgap measure?
Or would people rather I use a different idiom for managing the memory
where the symbols are stored?

Thank you!

> >From bfa3fa1394b92b6651127f7ca6f7f1b68db6a9ca Mon Sep 17 00:00:00 2001
> From: Jerome Guitton <guitton@adacore.com>
> Date: Mon, 10 Sep 2018 13:04:41 +0200
> Subject: [PATCH] arm-pikeos: software single step
> 
> On ARM, PikeOS does not support hardware single step, causing various
> semi-random errors when trying to next/step over some user code. So
> this patch changes this target to use software-single-step instead.
> 
> The challenge is that, up to now, the PikeOS target was in all respects
> identical to a baremetal target as far as GDB was concerned, meaning
> we were using the baremetal osabi for this target too. This is no longer
> possible, and we need to introduce a new OSABI variant. Unfortunately,
> there isn't anything in the object file that would allow us to
> differentiate between the two platforms. So we have to rely on a
> heuristic instead, where we look for some known symbols that are
> required in a PikeOS application (these symbols are expected to be
> defined by the default linker script, and correspond to routines used
> to allocate the application stack).
> 
> gdb/ChangeLog:
> 
>         * defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
>         * osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
>         * arm-pikeos-tdep.c: New file.
>         * configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
>         embedded system.
>         * Makefile.in (ALL_TARGET_OBS): Add arm-pikeos-tdep.o.
> 
> Tested on arm-pikeos and arm-elf using AdaCore's testsuite.
> We also evaluated it on armhf-linux as a cross platform.
> ---
>  gdb/Makefile.in       |    1 +
>  gdb/arm-pikeos-tdep.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/configure.tgt     |    1 +
>  gdb/defs.h            |    1 +
>  gdb/osabi.c           |    1 +
>  5 files changed, 96 insertions(+)
>  create mode 100644 gdb/arm-pikeos-tdep.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index d49f3ee..5d6e217 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -670,6 +670,7 @@ ALL_TARGET_OBS = \
>  	arm-linux-tdep.o \
>  	arm-nbsd-tdep.o \
>  	arm-obsd-tdep.o \
> +	arm-pikeos-tdep.o \
>  	arm-symbian-tdep.o \
>  	arm-tdep.o \
>  	arm-wince-tdep.o \
> diff --git a/gdb/arm-pikeos-tdep.c b/gdb/arm-pikeos-tdep.c
> new file mode 100644
> index 0000000..4662a28
> --- /dev/null
> +++ b/gdb/arm-pikeos-tdep.c
> @@ -0,0 +1,92 @@
> +/* Copyright (C) 2016-2018 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 "objfiles.h"
> +#include "arm-tdep.h"
> +#include "osabi.h"
> +
> +/* The gdbarch_register_osabi handler for ARM PikeOS; it performs
> +   the gdbarch initialization for that platform.  */
> +
> +static void
> +arm_pikeos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  /* Single stepping.  */
> +  set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
> +}
> +
> +/* The ARM PikeOS OSABI sniffer (see gdbarch_register_osabi_sniffer).
> +   Returns GDB_OSABI_PIKEOS if the given BFD is a PikeOS binary,
> +   GDB_OSABI_UNKNOWN otherwise.  */
> +
> +static enum gdb_osabi
> +arm_pikeos_osabi_sniffer (bfd *abfd)
> +{
> +  long number_of_symbols;
> +  long i;
> +  int pikeos_stack_found = 0;
> +  int pikeos_stack_size_found = 0;
> +
> +  /* The BFD target of PikeOS is really just standard elf, so we
> +     cannot use it to detect this variant.  The only common thing that
> +     may be found in PikeOS modules are symbols _vm_stack/__p4_stack and
> +     _vm_stack_size/__p4_stack_end. They are used to specify the stack
> +     location and size; and defined by the default linker script.
> +
> +     OS ABI sniffers are called before the minimal symtabs are
> +     created. So inspect the symbol table using BFD.  */
> +
> +  long storage_needed = bfd_get_symtab_upper_bound (abfd);
> +  if (storage_needed <= 0)
> +    return GDB_OSABI_UNKNOWN;
> +
> +  gdb::unique_xmalloc_ptr<asymbol *> symbol_table
> +    ((asymbol **) xmalloc (storage_needed));
> +  number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table.get ());
> +
> +  if (number_of_symbols <= 0)
> +    return GDB_OSABI_UNKNOWN;
> +
> +  for (i = 0; i < number_of_symbols; i++)
> +    {
> +      const char *name = bfd_asymbol_name (symbol_table.get ()[i]);
> +
> +      if (strcmp (name, "_vm_stack") == 0
> +	  || strcmp (name, "__p4_stack") == 0)
> +	pikeos_stack_found = 1;
> +
> +      if (strcmp (name, "_vm_stack_size") == 0
> +	  || strcmp (name, "__p4_stack_end") == 0)
> +	pikeos_stack_size_found = 1;
> +    }
> +
> +  if (pikeos_stack_found && pikeos_stack_size_found)
> +    return GDB_OSABI_PIKEOS;
> +  else
> +    return GDB_OSABI_UNKNOWN;
> +}
> +
> +void
> +_initialize_arm_pikeos_tdep (void)
> +{
> +  /* Register the sniffer for the PikeOS targets.  */
> +  gdbarch_register_osabi_sniffer (bfd_arch_arm, bfd_target_elf_flavour,
> +                                  arm_pikeos_osabi_sniffer);
> +  gdbarch_register_osabi (bfd_arch_arm, 0, GDB_OSABI_PIKEOS,
> +                          arm_pikeos_init_abi);
> +}
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 6d1a4df..03c0268 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -180,6 +180,7 @@ arm*-*-symbianelf*)
>  	;;
>  arm*-*-*)
>  	# Target: ARM embedded system
> +	gdb_target_obs="arm-pikeos-tdep.o"
>  	gdb_sim=../sim/arm/libsim.a
>  	;;
>  
> diff --git a/gdb/defs.h b/gdb/defs.h
> index fc42170..28f7a11 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -495,6 +495,7 @@ enum gdb_osabi
>    GDB_OSABI_LYNXOS178,
>    GDB_OSABI_NEWLIB,
>    GDB_OSABI_SDE,
> +  GDB_OSABI_PIKEOS,
>  
>    GDB_OSABI_INVALID		/* keep this last */
>  };
> diff --git a/gdb/osabi.c b/gdb/osabi.c
> index 7d0540b..68f4665 100644
> --- a/gdb/osabi.c
> +++ b/gdb/osabi.c
> @@ -80,6 +80,7 @@ static const struct osabi_names gdb_osabi_names[] =
>    { "LynxOS178", NULL },
>    { "Newlib", NULL },
>    { "SDE", NULL },
> +  { "PikeOS", NULL },
>  
>    { "<invalid>", NULL }
>  };
> -- 
> 1.7.10.4
>
  

On 2018-09-13 20:47, Joel Brobecker wrote:
> Or would people rather I use a different idiom for managing the memory
> where the symbols are stored?

What you have here is fine, I didn't want to focus the discussion on 
this detail :).

Simon
  

> > gdb/ChangeLog:
> > 
> >         * defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
> >         * osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
> >         * arm-pikeos-tdep.c: New file.
> >         * configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
> >         embedded system.
> >         * Makefile.in (ALL_TARGET_OBS): Add arm-pikeos-tdep.o.

I just pushed this patch to master...