Add proper single-step support for arm-none-eabi
Commit Message
This commit adds proper support for single-stepping on arm-none-eabi.
arm_software_single_step is now always registered in arm_gdbarch_init (arm-tdep.c) before calling gdbarch_osabi_init. Additionally, arm_software_single_step now checks whether or not the target supports single-stepping itself (which is the case for most simulators, emulators, etc.).
Following this change, we need to assume, without the knowledge the OS (if there is one), that SVC instructions return to the next user-mode instructions by default (arm_get_next_pcs_syscall_next_pc currently returns 0 which is virtually guaranteed to fail).
gdb/Changelog:
* arm-tdep.c (arm_software_single_step): Check whether the target actually has declared single-step support. Declare this function static.
(arm_get_next_pcs_syscall_next_pc): Assume that SVC instructions return to the next user-mode instruction by default.
(arm_gdbarch_init): register arm_software_single_step with set_gdbarch_software_single_step before calling gdbarch_osabi_init.
* arm-tdep.h: remove the prototype of function arm_software_single_step accordingly to the changes above.
* arm-nbsd-tdep.c: (arm_netbsd_init_eabi): Remove redundant call to set_gdbarch_software_single_step.
* arm-obsd-tdep.c: (armobsd_init_eabi): Likewise.
* arm-wince-tdep.c: (arm_wince_init_abi): Likewise.
---
gdb/arm-nbsd-tdep.c | 3 ---
gdb/arm-obsd-tdep.c | 3 ---
gdb/arm-tdep.c | 32 +++++++++++++++++++++++++++++---
gdb/arm-tdep.h | 1 -
gdb/arm-wince-tdep.c | 3 ---
5 files changed, 29 insertions(+), 13 deletions(-)
Comments
Elouan Appere <elouan.appere@club-internet.fr> writes:
> This commit adds proper support for single-stepping on arm-none-eabi.
>
> arm_software_single_step is now always registered in arm_gdbarch_init (arm-tdep.c) before calling gdbarch_osabi_init. Additionally, arm_software_single_step now checks whether or not the target supports single-stepping itself (which is the case for most simulators, emulators, etc.).
This changes the arm-none-eabi gdb software single step behavior.
Currently, arm-none-eabi gdb doesn't do software single step, and
debugging stubs support hardware single step "silently".
With your patch applied, arm-none-eabi gdb thinks existing debugging
stubs don't support hardware single step, it changes to software single
step, because they don't tell gdb they do support hardware single step.
Why do you need software single step in bare-metal debugging?
On 05/04/2017 13:30, Yao Qi wrote:
> Why do you need software single step in bare-metal debugging?
I'm currently developing a stub on real hardware, and ARMv6 does not
support single-step at all (and there's no OS feature to do the work,
either), therefore I need GDB to do software single-step (like it did
before commit 750ce8d).
This patch indeed changes the single-step behavior on arm-none-eabi, but
this will match the documentation of "vCont?" (whereas the current
behavior does not); if stubs rely on the behavior you mentioned for
efficient single-step behavior, they do not respect the documentation
(and the respective patches to fix this are trivial).
Conversely, existing compliant stubs (on ARMv6 and older platforms, for
example) may have been broken by 750ce8d ("Support single step by arch
or target"), since they were rightfully expecting gdb to perform
software single-step itself. My patch restores compatibility for these.
Another non-breaking possibilty would be to check whether or not the
stub replies an error to "vCont;s" (or the deprecated "s" packet), but
this would then make the "s;S" vCont? kind of reply useless.
Elouan Appéré
Elouan Appere <elouan.appere@club-internet.fr> writes:
> I'm currently developing a stub on real hardware, and ARMv6 does not
> support single-step at all (and there's no OS feature to do the work,
> either), therefore I need GDB to do software single-step (like it did
> before commit 750ce8d).
Commit 750ce8d only affects arm-linux target. It doesn't affect
arm-none-eabi at all. Nowadays, GDB arm-linux target still doesn't
support "target side" single step (it can be done in hardware or in
software in stub) unless the target tells it does. The logic here is
that GDB keeps the default behavior unless the target tells it something
different.
>
> This patch indeed changes the single-step behavior on arm-none-eabi,
> but this will match the documentation of "vCont?" (whereas the current
> behavior does not); if stubs rely on the behavior you mentioned for
> efficient single-step behavior, they do not respect the documentation
> (and the respective patches to fix this are trivial).
The packet "vCont?" is about query the supported actions in stub, and
GDB can decide its operations according to the supported actions. If
the stub tells GDB that it supports single-step, GDB can still don't use
vCont;s. The document is about the protocol, not GDB behavior.
Secondly, some stubs may not support vCont at all, instead, they use s
and S packet. GDB has no idea does the stub support single step or not,
so GDB has to keep its default behavior (in arm-none-eabi, GDB thinks
target supports single-step, in arm-linux, GDB thinks target doesn't
support single-step).
>
> Conversely, existing compliant stubs (on ARMv6 and older platforms,
> for example) may have been broken by 750ce8d ("Support single step by
> arch or target"), since they were rightfully expecting gdb to perform
> software single-step itself. My patch restores compatibility for
> these.
As I mentioned above, commit 750ce8d still keeps the GDB behavior
unchanged, unless the stub tells GDB that it can do something
different. I can't figure out a case that 750ce8d breaks existing stubs.
>
> Another non-breaking possibilty would be to check whether or not the
> stub replies an error to "vCont;s" (or the deprecated "s" packet), but
> this would then make the "s;S" vCont? kind of reply useless.
We can extend .to_can_do_single_step interface a little to get more
information. Current .to_can_do_single_step only returns true if we
definitely know the target can do single-step, otherwise return false.
The change .to_can_do_single_step returning tribool (unknown, true and
false). true/false means we definitely know the target can/can't do
single-step, unknown means we don't know that. In arm-none-eabi target,
we only use software single step if .to_can_do_single_step returns
false. In arm-linux target, we only use hardware single step if
.to_can_do_single_step returns true.
On 04/05/2017 06:30 AM, Yao Qi wrote:
> Elouan Appere <elouan.appere@club-internet.fr> writes:
>
>> This commit adds proper support for single-stepping on arm-none-eabi.
>>
>> arm_software_single_step is now always registered in arm_gdbarch_init (arm-tdep.c) before calling gdbarch_osabi_init. Additionally, arm_software_single_step now checks whether or not the target supports single-stepping itself (which is the case for most simulators, emulators, etc.).
>
> This changes the arm-none-eabi gdb software single step behavior.
> Currently, arm-none-eabi gdb doesn't do software single step, and
> debugging stubs support hardware single step "silently".
> With your patch applied, arm-none-eabi gdb thinks existing debugging
> stubs don't support hardware single step, it changes to software single
> step, because they don't tell gdb they do support hardware single step.
>
> Why do you need software single step in bare-metal debugging?
>
We have bare-metal debugging for arm-eabi and it uses hardware/stub
single-stepping, so i'd rather not have this change of defaults as Yao
pointed out.
But i agree that a more reasonable mechanism to enable/disable
hardware/stub single-stepping in an automatically way would be great.
On 10/04/2017 12:35, Yao Qi wrote:
> Commit 750ce8d only affects arm-linux target. It doesn't affect
> arm-none-eabi at all.
You're right. Sorry for that wrong claim.
>> Another non-breaking possibilty would be to check whether or not the
>> stub replies an error to "vCont;s" (or the deprecated "s" packet), but
>> this would then make the "s;S" vCont? kind of reply useless.
> We can extend .to_can_do_single_step interface a little to get more
> information. Current .to_can_do_single_step only returns true if we
> definitely know the target can do single-step, otherwise return false.
> The change .to_can_do_single_step returning tribool (unknown, true and
> false). true/false means we definitely know the target can/can't do
> single-step, unknown means we don't know that. In arm-none-eabi target,
> we only use software single step if .to_can_do_single_step returns
> false. In arm-linux target, we only use hardware single step if
> .to_can_do_single_step returns true.
>
This would, I think, be the best solution
@@ -65,9 +65,6 @@ arm_netbsd_init_abi_common (struct gdbarch_info info,
tdep->jb_pc = ARM_NBSD_JB_PC;
tdep->jb_elt_size = ARM_NBSD_JB_ELEMENT_SIZE;
-
- /* Single stepping. */
- set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
}
static void
@@ -97,9 +97,6 @@ armobsd_init_abi (struct gdbarch_info info,
/* OpenBSD/arm uses -fpcc-struct-return by default. */
tdep->struct_return = pcc_struct_return;
- /* Single stepping. */
- set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
-
/* Breakpoints. */
switch (info.byte_order)
{
@@ -6279,7 +6279,24 @@ arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
static CORE_ADDR
arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
{
- return 0;
+ CORE_ADDR next_pc = 0;
+ CORE_ADDR pc = regcache_read_pc (self->regcache);
+ int is_thumb = arm_is_thumb (self->regcache);
+
+ /* Without the knowledge of the OS (if any) we need to assume that the next
+ user-mode instruction is executed */
+
+ if (is_thumb)
+ {
+ next_pc = pc + 2;
+ }
+
+ else
+ {
+ next_pc = pc + 4;
+ }
+
+ return next_pc;
}
/* Wrapper over arm_is_thumb for use in arm_get_next_pcs. */
@@ -6295,7 +6312,7 @@ arm_get_next_pcs_is_thumb (struct arm_get_next_pcs *self)
single-step support. We find the target of the coming instructions
and breakpoint them. */
-VEC (CORE_ADDR) *
+static VEC (CORE_ADDR) *
arm_software_single_step (struct regcache *regcache)
{
struct gdbarch *gdbarch = get_regcache_arch (regcache);
@@ -6303,7 +6320,14 @@ arm_software_single_step (struct regcache *regcache)
CORE_ADDR pc;
int i;
VEC (CORE_ADDR) *next_pcs = NULL;
- struct cleanup *old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), &next_pcs);
+ struct cleanup *old_chain;
+
+ /* 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 NULL;
+
+ old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), &next_pcs);
arm_get_next_pcs_ctor (&next_pcs_ctx,
&arm_get_next_pcs_ops,
@@ -9452,6 +9476,8 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_sw_breakpoint_from_kind (gdbarch, arm_sw_breakpoint_from_kind);
set_gdbarch_breakpoint_kind_from_current_state (gdbarch,
arm_breakpoint_kind_from_current_state);
+ set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
+
/* Information about registers, etc. */
set_gdbarch_sp_regnum (gdbarch, ARM_SP_REGNUM);
@@ -259,7 +259,6 @@ CORE_ADDR arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
int arm_get_next_pcs_is_thumb (struct arm_get_next_pcs *self);
-VEC (CORE_ADDR) *arm_software_single_step (struct regcache *);
int arm_is_thumb (struct regcache *regcache);
int arm_frame_is_thumb (struct frame_info *frame);
@@ -136,9 +136,6 @@ arm_wince_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
/* Shared library handling. */
set_gdbarch_skip_trampoline_code (gdbarch, arm_pe_skip_trampoline_code);
- /* Single stepping. */
- set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
-
/* Skip call to __gccmain that gcc places in main. */
set_gdbarch_skip_main_prologue (gdbarch, arm_wince_skip_main_prologue);
}