diff mbox

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

Message ID 20180911085612.GA3379@adacore.com
State New
Headers show

Commit Message

Joel Brobecker Sept. 11, 2018, 8:56 a.m. UTC
> (not sure why the From address is y@eu.adacore.com, I'll replace it with
> Joel's).

It was my mistake answering "y" rather than just typing answer
to use my email address as the default when "git send-email"
asked me about it. Sorry, and thanks for fixing!

> > +  symbol_table = (asymbol **) xmalloc (storage_needed);
> > +  make_cleanup (xfree, symbol_table);
> 
> It would make Tom really happy if you could avoid introducing a cleanup :).
> I would suggest using an std::vector<asymbol *>.  If you'd rather not do it
> it's not a big deal, we can change it after.

Attached is the patch I am testing. I am wondering if this is the best
way, though. What do you think?

One question I asked myself is whether we needed the std::vector at
all, as the building of the vector is a bit clunky in this situation.
As I understand it, this is mostly to automate the destruction of
the array. I was wondering whether we could do without the std::vector
entirely, and just handle the array without a cleanup, since the code
is simple enough that we can make sure it doesn't throw (I was hoping
that eg marking the function noexcept would help guaranty that). But
at the end of the day, although it's manageable in this case, I felt
it was better to go with the safer approach.

Same remark with resizing the array: In practice, we don't need to do
it since we know the bounds and iterate over the elements without
accessing the them from the vector; but it's clearer and safer this way.

> > diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> > index 6d1a4df..a1f703f 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
> >  	;;
> 
> The object should be added to ALL_TARGETS_OBS in the Makefile, so it's
> included in an enable-targets=all build.

Argh, yes, true. Added.

> Just to understand better why the object file needs to be added here, what
> is the target triplet? arm-pikeos-something, arm-none-something?

It is arm-*-pikeos*, which config.sub then translates to arm-*-eabi.

> Just wondering: when you connect to the target, you talk to a gdb stub
> that is built right into the application/rtos?

Something like that. We have to use a tool provided by the vendor
called muxa; the tool connects to the system running PikeOS, and
we connect to that tool using the remote protocol.

Thanks for the review!

Comments

Simon Marchi Sept. 11, 2018, 9:58 a.m. UTC | #1
On 2018-09-11 09:56, Joel Brobecker wrote:
>> > +  symbol_table = (asymbol **) xmalloc (storage_needed);
>> > +  make_cleanup (xfree, symbol_table);
>> 
>> It would make Tom really happy if you could avoid introducing a 
>> cleanup :).
>> I would suggest using an std::vector<asymbol *>.  If you'd rather not 
>> do it
>> it's not a big deal, we can change it after.
> 
> Attached is the patch I am testing. I am wondering if this is the best
> way, though. What do you think?

It looks good to me.  You could use a range-for, because it's cool:

   for (asymbol *sym : symbol_table)

but what you have is also fine.

> One question I asked myself is whether we needed the std::vector at
> all, as the building of the vector is a bit clunky in this situation.
> As I understand it, this is mostly to automate the destruction of
> the array. I was wondering whether we could do without the std::vector
> entirely, and just handle the array without a cleanup, since the code
> is simple enough that we can make sure it doesn't throw (I was hoping
> that eg marking the function noexcept would help guaranty that). But
> at the end of the day, although it's manageable in this case, I felt
> it was better to go with the safer approach.

Well, it would be fine to not use a vector, but in any case I would 
recommend the use of an object that ensures the memory is de-allocated 
in any case, whether it is an std::vector, an std::unique_ptr or a 
gdb::unique_xmalloc_ptr.  I don't see any advantage of doing a manual 
free over using one of those.

An alternative would be to use a VLA:

   asymbol *symbol_table[max_number_of_symbols];

which I think ends up being like an alloca.  But in this case there 
could be a huge number of symbols I suppose, so I would avoid that.

> Same remark with resizing the array: In practice, we don't need to do
> it since we know the bounds and iterate over the elements without
> accessing the them from the vector; but it's clearer and safer this 
> way.

Right, with the range-for you would need the resize though.

Simon
Joel Brobecker Sept. 11, 2018, 1:51 p.m. UTC | #2
> > Attached is the patch I am testing. I am wondering if this is the best
> > way, though. What do you think?
> 
> It looks good to me.  You could use a range-for, because it's cool:
> 
>   for (asymbol *sym : symbol_table)
> 
> but what you have is also fine.

Unfortunately, it looks like the change is causing regressions, and
it is a bit obscure at the moment, which is not entirely surprising
when one deals with memory.

> > One question I asked myself is whether we needed the std::vector at
> > all, as the building of the vector is a bit clunky in this situation.
> > As I understand it, this is mostly to automate the destruction of
> > the array. I was wondering whether we could do without the std::vector
> > entirely, and just handle the array without a cleanup, since the code
> > is simple enough that we can make sure it doesn't throw (I was hoping
> > that eg marking the function noexcept would help guaranty that). But
> > at the end of the day, although it's manageable in this case, I felt
> > it was better to go with the safer approach.
> 
> Well, it would be fine to not use a vector, but in any case I would
> recommend the use of an object that ensures the memory is de-allocated in
> any case, whether it is an std::vector, an std::unique_ptr or a
> gdb::unique_xmalloc_ptr.  I don't see any advantage of doing a manual free
> over using one of those.
> 
> An alternative would be to use a VLA:
> 
>   asymbol *symbol_table[max_number_of_symbols];
> 
> which I think ends up being like an alloca.  But in this case there could be
> a huge number of symbols I suppose, so I would avoid that.

Yes, the table might be too large and blow the stack. All things
considered, I think the better model here seems to be to use a
gdb::unique_xmalloc_ptr. I tried that, and the code looks simpler,
but I get the same kind of regressions as above.

And unfortunately at this point, I have to hold off for a while,
because I am out of license, and then I will be traveling home.
But thanks a lot for all the feedback received so far; when I pick
things up again, I will not be starting from scratch.
Tom Tromey Sept. 11, 2018, 3:40 p.m. UTC | #3
>>>>> "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.

Joel> +  for (i = 0; i < number_of_symbols; i++)

If you have an explicit bound on the loop then you don't need to resize
the vector to be smaller.

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.

Tom
diff mbox

Patch

From 5e98b8cb6fefc1419e3357675e3c5c6d375cc461 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 |   93 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/configure.tgt     |    1 +
 gdb/defs.h            |    1 +
 gdb/osabi.c           |    1 +
 5 files changed, 97 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..786f67a
--- /dev/null
+++ b/gdb/arm-pikeos-tdep.c
@@ -0,0 +1,93 @@ 
+/* 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 max_number_of_symbols
+    = bfd_get_symtab_upper_bound (abfd) / sizeof (asymbol *);
+  if (max_number_of_symbols <= 0)
+    return GDB_OSABI_UNKNOWN;
+
+  std::vector<asymbol *> symbol_table (max_number_of_symbols);
+  number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table.data ());
+  if (number_of_symbols <= 0)
+    return GDB_OSABI_UNKNOWN;
+  gdb_assert (number_of_symbols <= max_number_of_symbols);
+  symbol_table.resize (number_of_symbols);
+
+  for (i = 0; i < number_of_symbols; i++)
+    {
+      const char *name = bfd_asymbol_name (symbol_table[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