[RFA] arm-pikeos: software single step
Commit Message
From: Jerome Guitton <guitton@adacore.com>
Hello,
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 (Jerome Guitton <guitton@adacore.com>):
* arm-pikeos-tdep.c: New file.
* configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
embedded system.
* defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
* osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
Tested on arm-pikeos and arm-elf using AdaCore's testsuite.
We also evaluated it on armhf-linux as a cross platform.
OK to apply?
Thanks!
Comments
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> The challenge is that, up to now, the PikeOS target was in all respects
Joel> identical to a baremetal target as far as GDB was concerned, meaning
Joel> we were using the baremetal osabi for this target too. This is no longer
Joel> possible, and we need to introduce a new OSABI variant.
I don't really know anything about this area, but...
Is there any chance of introducing some flag into the object files so
that the heuristic could be removed at some future point?
Joel> +
Joel> +static void
Joel> +arm_pikeos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
The usual thing about functions needing an intro comment.
Joel> + { "Pikeos", NULL },
The email spelled it "PikeOS", so maybe it should read that way here as
well.
Tom
> Joel> The challenge is that, up to now, the PikeOS target was in all respects
> Joel> identical to a baremetal target as far as GDB was concerned, meaning
> Joel> we were using the baremetal osabi for this target too. This is no longer
> Joel> possible, and we need to introduce a new OSABI variant.
>
> I don't really know anything about this area, but...
>
> Is there any chance of introducing some flag into the object files so
> that the heuristic could be removed at some future point?
I don't really see this, because we don't really control the platform.
> Joel> +
> Joel> +static void
> Joel> +arm_pikeos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>
> The usual thing about functions needing an intro comment.
OK, I will do that. I saw it, and grepped around, saw that it wasn't
really done for the few spot checks I did, so thought it was just
so obvious that it was deliberately omitted.
> Joel> + { "Pikeos", NULL },
>
> The email spelled it "PikeOS", so maybe it should read that way here as
> well.
Indeed, that's a good point.
Thanks for the review, Tom. New version of the patch coming up.
On 09/10/2018 04:13 PM, Joel Brobecker wrote:
> From: Jerome Guitton <guitton@adacore.com>
>
> Hello,
>
> 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).
It's unfortunate to have to introduce an OSABI for something that is a
property of the target that doesn't effect anything when e.g., you're
not connected yet.
It seems to me we can take a better approach here, that eliminates
this particular problem not just for PikeOS, but for all other cases
of random RTOS's.
That is, you should be able to make your stub tell GDB that it
can or can't hardware single step, and then GDB adjusts itself.
We already have all the info we need, we're just not using it.
See target_can_do_single_step and the remote.c implementation:
int
remote_target::can_do_single_step ()
{
/* We can only tell whether target supports single step or not by
supported s and S vCont actions if the stub supports vContSupported
feature. If the stub doesn't support vContSupported feature,
we have conservatively to think target doesn't supports single
step. */
if (packet_support (PACKET_vContSupported) == PACKET_ENABLE)
From the manual:
@item vContSupported
This feature indicates whether @value{GDBN} wants to know the
supported actions in the reply to @samp{vCont?} packet.
@end table
@item vCont?
@cindex @samp{vCont?} packet
Request a list of actions supported by the @samp{vCont} packet.
So, if you make your stub indicate support for the "vContSupported"
feature, and make sure the the reply to "vCont?" does not include
the s/S features, then GDB knows the target does not support
hardware single step.
The only think missing then I think is moving the only call to
target_can_do_single_step out of the arm-linux-tdep.c file:
static std::vector<CORE_ADDR>
arm_linux_software_single_step (struct regcache *regcache)
{
struct gdbarch *gdbarch = regcache->arch ();
struct arm_get_next_pcs next_pcs_ctx;
/* If the target does have hardware single step, GDB doesn't have
to bother software single step. */
if (target_can_do_single_step () == 1)
return {};
into somewhere more generic, around infrun.c:maybe_software_singlestep.
Can you try that?
Thanks,
Pedro Alves
>
> gdb/ChangeLog (Jerome Guitton <guitton@adacore.com>):
>
> * arm-pikeos-tdep.c: New file.
> * configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
> embedded system.
> * defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
> * osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
>
> Tested on arm-pikeos and arm-elf using AdaCore's testsuite.
> We also evaluated it on armhf-linux as a cross platform.
>
> OK to apply?
>
> Thanks!
>
Hi Pedro,
> It seems to me we can take a better approach here, that eliminates
> this particular problem not just for PikeOS, but for all other cases
> of random RTOS's.
>
> That is, you should be able to make your stub tell GDB that it
> can or can't hardware single step, and then GDB adjusts itself.
Unfortunately, the problem is that we do not control the stub (muxa),
it is a tool that the vendor provides.
On 09/11/2018 12:04 PM, Joel Brobecker wrote:
> Hi Pedro,
>
>> It seems to me we can take a better approach here, that eliminates
>> this particular problem not just for PikeOS, but for all other cases
>> of random RTOS's.
>>
>> That is, you should be able to make your stub tell GDB that it
>> can or can't hardware single step, and then GDB adjusts itself.
>
> Unfortunately, the problem is that we do not control the stub (muxa),
> it is a tool that the vendor provides.
Did you check whether it is already reporting the RSP packets as I
had suggested? We wouldn't be adding new packets, but instead using
some that are already defined.
If the stub really needs modification, I'm not opposed to your patch
as stop gap. Would there be any chance to forward the information to
the sysgo folks, see if they're willing to tweak the stub? It should
be a trivial change.
IWBN to fix the infrastructure, so that the next time a similar
patch comes in we could just "no, fix your stub". :-)
Thanks,
Pedro Alves
Hi again Pedro,
> > Unfortunately, the problem is that we do not control the stub (muxa),
> > it is a tool that the vendor provides.
>
> Did you check whether it is already reporting the RSP packets as I
> had suggested? We wouldn't be adding new packets, but instead using
> some that are already defined.
>
> If the stub really needs modification, I'm not opposed to your patch
> as stop gap. Would there be any chance to forward the information to
> the sysgo folks, see if they're willing to tweak the stub? It should
> be a trivial change.
So, here is what I could gather (full log at the end of this email [1]).
GDB sends the $qSupported packet, equiring about support...
| Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+#df...Ack
... and the list returned is fairly small, as I suspected:
| Packet received: qXfer:features:read+
After that, I don't see very much happening at the vCont level,
so fast-forward to the first "cont" command (after having inserted
a breakpoint), and we see some interesting stuff. In particular,
GDB asks the target what vCont support it provides, and here is
what we receive:
| Sending packet: $vCont?#49...Ack
| Packet received: vCont;c;s;C;S
| Packet vCont (verbose-resume) is supported
Hah! So, on the one end, the stub says stepping is supported
('s' and 'S'), but on the other hand we were told that this
feature is not supported on ARM.
So, if I understand correctly the current situation, we do have
one small infrastructure adjustment to do in GDB, but then once
done, if the stub on advertised support for "vCont;c;C", then
GDB would automatically be switching to software single step.
Do I understand correctly?
If that's correct, I'll make sure AdaCore forwards the suggestion
to Sysgo.
Thanks!
On 09/11/2018 09:57 PM, Joel Brobecker wrote:
> Hi again Pedro,
>
>>> Unfortunately, the problem is that we do not control the stub (muxa),
>>> it is a tool that the vendor provides.
>>
>> Did you check whether it is already reporting the RSP packets as I
>> had suggested? We wouldn't be adding new packets, but instead using
>> some that are already defined.
>>
>> If the stub really needs modification, I'm not opposed to your patch
>> as stop gap. Would there be any chance to forward the information to
>> the sysgo folks, see if they're willing to tweak the stub? It should
>> be a trivial change.
>
> So, here is what I could gather (full log at the end of this email [1]).
> GDB sends the $qSupported packet, equiring about support...
>
> | Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+#df...Ack
>
> ... and the list returned is fairly small, as I suspected:
>
> | Packet received: qXfer:features:read+
>
> After that, I don't see very much happening at the vCont level,
> so fast-forward to the first "cont" command (after having inserted
> a breakpoint), and we see some interesting stuff. In particular,
> GDB asks the target what vCont support it provides, and here is
> what we receive:
>
> | Sending packet: $vCont?#49...Ack
> | Packet received: vCont;c;s;C;S
> | Packet vCont (verbose-resume) is supported
>
> Hah! So, on the one end, the stub says stepping is supported
> ('s' and 'S'), but on the other hand we were told that this
> feature is not supported on ARM.
>
> So, if I understand correctly the current situation, we do have
> one small infrastructure adjustment to do in GDB, but then once
> done, if the stub on advertised support for "vCont;c;C", then
> GDB would automatically be switching to software single step.
> Do I understand correctly?
Almost. GDB can't trust "vCont;c;C" alone, because for a long
while GDBserver would send "vCont;c;s;C;S" even if the target
did not support hardware stepping. So what a stub needs to do
is:
Return "vCont;c;C" to "vCont?" _AND_ include "vContSupported"
in the reported "qSupported" features. The latter tells GDB
to trust that the actions included in "vCont?" are really the
supported ones. (I wish we had implemented this a little bit
differently, but that ship has sailed, and although a bit
cumbersome, it works.)
>
> If that's correct, I'll make sure AdaCore forwards the suggestion
> to Sysgo.
>
> Thanks!
Thanks,
Pedro Alves
> Almost. GDB can't trust "vCont;c;C" alone, because for a long
> while GDBserver would send "vCont;c;s;C;S" even if the target
> did not support hardware stepping. So what a stub needs to do
> is:
>
> Return "vCont;c;C" to "vCont?" _AND_ include "vContSupported"
> in the reported "qSupported" features. The latter tells GDB
> to trust that the actions included in "vCont?" are really the
> supported ones. (I wish we had implemented this a little bit
> differently, but that ship has sailed, and although a bit
> cumbersome, it works.)
Excellent. I've passed that information in what is hopefully
a concise and clear message that JeromeG can pass to Sysgo.
Hopefully something will come out of it.
Thanks Pedro!
new file mode 100644
@@ -0,0 +1,95 @@
+/* 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"
+
+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);
+}
+
+static enum gdb_osabi
+arm_pikeos_osabi_sniffer (bfd *abfd)
+{
+ long storage_needed;
+ asymbol **symbol_table;
+ long number_of_symbols;
+ long i;
+ int pikeos_stack_found = 0;
+ int pikeos_stack_size_found = 0;
+ struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
+
+ /* 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. */
+
+ storage_needed = bfd_get_symtab_upper_bound (abfd);
+
+ if (storage_needed <= 0)
+ return GDB_OSABI_UNKNOWN;
+
+ symbol_table = (asymbol **) xmalloc (storage_needed);
+ make_cleanup (xfree, symbol_table);
+
+ number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
+
+ if (number_of_symbols < 0)
+ {
+ do_cleanups (old_chain);
+ return GDB_OSABI_UNKNOWN;
+ }
+
+ 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;
+ }
+
+ do_cleanups (old_chain);
+
+ 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);
+}
@@ -180,6 +180,7 @@ arm*-*-symbianelf*)
;;
arm*-*-*)
# Target: ARM embedded system
+ gdb_target_obs="arm-pikeos-tdep.o"
gdb_sim=../sim/arm/libsim.a
;;
@@ -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 */
};
@@ -80,6 +80,7 @@ static const struct osabi_names gdb_osabi_names[] =
{ "LynxOS178", NULL },
{ "Newlib", NULL },
{ "SDE", NULL },
+ { "Pikeos", NULL },
{ "<invalid>", NULL }
};