[RFC PATCH] gdb: powerpc: Provide an option to disable single step atomic heuristic

[RFC PATCH] gdb: powerpc: Provide an option to disable single step atomic heuristic

[Nicholas Piggin]

gdb tries to step over atomic (larx/stcx.) sequences because stepping
through them tends to clear the reservation and prevent forward
progress.

More specialised targets like an emulator or hardware debug interface
can support single stepping without clearing reservation. It would be
nice to permit atomic sequences to be stepped into as an option. QEMU
can do this, for example.

Other targets not just powerpc could have the same issue, so not sure
whether it would make sense to be a generic option, or if it's such a
niche case that it's not worthwhile.

Also, would there be a way to describe this capability to a gdb client
in the protocol?

Thanks,
Nick
---
 gdb/rs6000-tdep.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index c8450345be2..e911abc7604 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -153,6 +153,9 @@ static const char *const powerpc_vector_strings[] =
 static enum powerpc_vector_abi powerpc_vector_abi_global = POWERPC_VEC_AUTO;
 static const char *powerpc_vector_abi_string = "auto";
 
+/* Single-step tries to step over entire larx/stcx. atomic sequence */
+static bool powerpc_step_over_atomic = true;
+
 /* PowerPC-related per-inferior data.  */
 
 static const registry<inferior>::key<ppc_inferior_data> ppc_inferior_data_key;
@@ -1132,7 +1135,7 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
 
 /* Checks for an atomic sequence of instructions beginning with a
    Load And Reserve instruction and ending with a Store Conditional
-   instruction.  If such a sequence is found, attempt to step through it.
+   instruction.  If such a sequence is found, attempt to step over it.
    A breakpoint is placed at the end of the sequence.  */
 std::vector<CORE_ADDR>
 ppc_deal_with_atomic_sequence (struct regcache *regcache)
@@ -1143,13 +1146,19 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
   CORE_ADDR breaks[2] = {CORE_ADDR_MAX, CORE_ADDR_MAX};
   CORE_ADDR loc = pc;
   CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
-  int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+  int insn;
   int insn_count;
   int index;
   int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
   int bc_insn_count = 0; /* Conditional branch instruction count.  */
 
+  /* global enable option for atomic sequence single step heuristic */
+  if (!powerpc_step_over_atomic)
+    return {};
+
+  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
+
   /* Assume all atomic sequences start with a Load And Reserve instruction.  */
   if (!IS_LOAD_AND_RESERVE_INSN (insn))
     return {};
@@ -8644,6 +8653,14 @@ show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
   gdb_printf (file, _("Use of exact watchpoints is %s.\n"), value);
 }
 
+static void
+show_powerpc_step_over_atomic (struct ui_file *file, int from_tty,
+			       struct cmd_list_element *c,
+			       const char *value)
+{
+  gdb_printf (file, _("Single-step over atomic (larx/stcx.) sequences %s.\n"), value);
+}
+
 /* Read a PPC instruction from memory.  */
 
 static unsigned int
@@ -8787,4 +8804,19 @@ scalar type, thus assuming that the variable is accessed through the address\n\
 of its first byte."),
 			   NULL, show_powerpc_exact_watchpoints,
 			   &setpowerpccmdlist, &showpowerpccmdlist);
+
+  add_setshow_boolean_cmd ("step-over-atomic", class_support,
+			   &powerpc_step_over_atomic,
+			   _("\
+Set whether single-step tries to step over atomics."),
+			   _("\
+Show whether single-step tries to step over atomics."),
+			   _("\
+If true, when GDB single-steps a larx instruction, it will try to find the\n\
+the matching stcx. instruction and step over the entire atomic sequence.\n\
+This can be required for forward-progress because single-stepping may clear\n\
+the reservation. Special environments like emulators and low level hardware\n\
+debug interfaces may not have this restriction, so this could be disabled."),
+			   NULL, show_powerpc_step_over_atomic,
+			   &setpowerpccmdlist, &showpowerpccmdlist);
 }
-- 
2.42.0

Re: [RFC PATCH] gdb: powerpc: Provide an option to disable single step atomic heuristic

[Luis Machado]

Hi,

On 2/22/24 05:52, Nicholas Piggin via Gdb wrote:
> gdb tries to step over atomic (larx/stcx.) sequences because stepping
> through them tends to clear the reservation and prevent forward
> progress.
> 
> More specialised targets like an emulator or hardware debug interface
> can support single stepping without clearing reservation. It would be
> nice to permit atomic sequences to be stepped into as an option. QEMU
> can do this, for example.
> 
> Other targets not just powerpc could have the same issue, so not sure
> whether it would make sense to be a generic option, or if it's such a
> niche case that it's not worthwhile.
> 
> Also, would there be a way to describe this capability to a gdb client
> in the protocol?
> 

I think you'd need some way to tell gdb that it can skip registering the atomic-stepping hook.

While that is easy for a remote target to do, it may be a bit more complex to convey that information if, say,
gdb is running within a system QEMU instance and you want QEMU to handle the atomic sequence and tell gdb it
doesn't need to bother with additional atomic-stepping logic.

So it needs to be something that works both for remote targets and for native targets, like a flag in /proc/<pid>/*
, some register value and so on.

I don't have a strong opinion on whether this should be a generic setting or not. If QEMU can do it for multiple architectures,
then it may make sense to make it generic. Otherwise a ppc-specific option would work OK.

> Thanks,
> Nick
> ---
>  gdb/rs6000-tdep.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index c8450345be2..e911abc7604 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -153,6 +153,9 @@ static const char *const powerpc_vector_strings[] =
>  static enum powerpc_vector_abi powerpc_vector_abi_global = POWERPC_VEC_AUTO;
>  static const char *powerpc_vector_abi_string = "auto";
>  
> +/* Single-step tries to step over entire larx/stcx. atomic sequence */
> +static bool powerpc_step_over_atomic = true;
> +
>  /* PowerPC-related per-inferior data.  */
>  
>  static const registry<inferior>::key<ppc_inferior_data> ppc_inferior_data_key;
> @@ -1132,7 +1135,7 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>  
>  /* Checks for an atomic sequence of instructions beginning with a
>     Load And Reserve instruction and ending with a Store Conditional
> -   instruction.  If such a sequence is found, attempt to step through it.
> +   instruction.  If such a sequence is found, attempt to step over it.
>     A breakpoint is placed at the end of the sequence.  */
>  std::vector<CORE_ADDR>
>  ppc_deal_with_atomic_sequence (struct regcache *regcache)
> @@ -1143,13 +1146,19 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
>    CORE_ADDR breaks[2] = {CORE_ADDR_MAX, CORE_ADDR_MAX};
>    CORE_ADDR loc = pc;
>    CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
> -  int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +  int insn;
>    int insn_count;
>    int index;
>    int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>    int bc_insn_count = 0; /* Conditional branch instruction count.  */
>  
> +  /* global enable option for atomic sequence single step heuristic */
> +  if (!powerpc_step_over_atomic)
> +    return {};
> +
> +  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +
>    /* Assume all atomic sequences start with a Load And Reserve instruction.  */
>    if (!IS_LOAD_AND_RESERVE_INSN (insn))
>      return {};
> @@ -8644,6 +8653,14 @@ show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
>    gdb_printf (file, _("Use of exact watchpoints is %s.\n"), value);
>  }
>  
> +static void
> +show_powerpc_step_over_atomic (struct ui_file *file, int from_tty,
> +			       struct cmd_list_element *c,
> +			       const char *value)
> +{
> +  gdb_printf (file, _("Single-step over atomic (larx/stcx.) sequences %s.\n"), value);
> +}
> +
>  /* Read a PPC instruction from memory.  */
>  
>  static unsigned int
> @@ -8787,4 +8804,19 @@ scalar type, thus assuming that the variable is accessed through the address\n\
>  of its first byte."),
>  			   NULL, show_powerpc_exact_watchpoints,
>  			   &setpowerpccmdlist, &showpowerpccmdlist);
> +
> +  add_setshow_boolean_cmd ("step-over-atomic", class_support,
> +			   &powerpc_step_over_atomic,
> +			   _("\
> +Set whether single-step tries to step over atomics."),
> +			   _("\
> +Show whether single-step tries to step over atomics."),
> +			   _("\
> +If true, when GDB single-steps a larx instruction, it will try to find the\n\
> +the matching stcx. instruction and step over the entire atomic sequence.\n\
> +This can be required for forward-progress because single-stepping may clear\n\
> +the reservation. Special environments like emulators and low level hardware\n\
> +debug interfaces may not have this restriction, so this could be disabled."),
> +			   NULL, show_powerpc_step_over_atomic,
> +			   &setpowerpccmdlist, &showpowerpccmdlist);
>  }

Re: [RFC PATCH] gdb: powerpc: Provide an option to disable single step atomic heuristic

[Nicholas Piggin]

On Tue Mar 5, 2024 at 1:41 AM AEST, Luis Machado wrote:
> Hi,
>
> On 2/22/24 05:52, Nicholas Piggin via Gdb wrote:
> > gdb tries to step over atomic (larx/stcx.) sequences because stepping
> > through them tends to clear the reservation and prevent forward
> > progress.
> > 
> > More specialised targets like an emulator or hardware debug interface
> > can support single stepping without clearing reservation. It would be
> > nice to permit atomic sequences to be stepped into as an option. QEMU
> > can do this, for example.
> > 
> > Other targets not just powerpc could have the same issue, so not sure
> > whether it would make sense to be a generic option, or if it's such a
> > niche case that it's not worthwhile.
> > 
> > Also, would there be a way to describe this capability to a gdb client
> > in the protocol?
> > 
>
> I think you'd need some way to tell gdb that it can skip registering the atomic-stepping hook.
>
> While that is easy for a remote target to do,

I don't know how though :) I assume add some xml description for it?

> it may be a bit more complex to convey that information if, say,
> gdb is running within a system QEMU instance and you want QEMU to handle the atomic sequence and tell gdb it
> doesn't need to bother with additional atomic-stepping logic.

Yeah it was more for remote targets -- attaching to the QEMU machine
from the outside. I think gdb running inside QEMU would have problems
with single stepping, because the single step interrupts may clear the
reservation explicitly (in QEMU and/or the OS).

It is an interesting idea, possibly something we could look into
further, but might not be feasible and likely requires some OS and
QEMU changes. Remote only for now would be enough I think, and the
option could always be changed manually.

>
> So it needs to be something that works both for remote targets and for native targets, like a flag in /proc/<pid>/*
> , some register value and so on.
>
> I don't have a strong opinion on whether this should be a generic setting or not. If QEMU can do it for multiple architectures,
> then it may make sense to make it generic. Otherwise a ppc-specific option would work OK.

Okay. That can be decided after the mechanism is agreed upon.

Thanks,
Nick

>
> > Thanks,
> > Nick
> > ---
> >  gdb/rs6000-tdep.c | 36 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> > index c8450345be2..e911abc7604 100644
> > --- a/gdb/rs6000-tdep.c
> > +++ b/gdb/rs6000-tdep.c
> > @@ -153,6 +153,9 @@ static const char *const powerpc_vector_strings[] =
> >  static enum powerpc_vector_abi powerpc_vector_abi_global = POWERPC_VEC_AUTO;
> >  static const char *powerpc_vector_abi_string = "auto";
> >  
> > +/* Single-step tries to step over entire larx/stcx. atomic sequence */
> > +static bool powerpc_step_over_atomic = true;
> > +
> >  /* PowerPC-related per-inferior data.  */
> >  
> >  static const registry<inferior>::key<ppc_inferior_data> ppc_inferior_data_key;
> > @@ -1132,7 +1135,7 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
> >  
> >  /* Checks for an atomic sequence of instructions beginning with a
> >     Load And Reserve instruction and ending with a Store Conditional
> > -   instruction.  If such a sequence is found, attempt to step through it.
> > +   instruction.  If such a sequence is found, attempt to step over it.
> >     A breakpoint is placed at the end of the sequence.  */
> >  std::vector<CORE_ADDR>
> >  ppc_deal_with_atomic_sequence (struct regcache *regcache)
> > @@ -1143,13 +1146,19 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
> >    CORE_ADDR breaks[2] = {CORE_ADDR_MAX, CORE_ADDR_MAX};
> >    CORE_ADDR loc = pc;
> >    CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
> > -  int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> > +  int insn;
> >    int insn_count;
> >    int index;
> >    int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
> >    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
> >    int bc_insn_count = 0; /* Conditional branch instruction count.  */
> >  
> > +  /* global enable option for atomic sequence single step heuristic */
> > +  if (!powerpc_step_over_atomic)
> > +    return {};
> > +
> > +  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> > +
> >    /* Assume all atomic sequences start with a Load And Reserve instruction.  */
> >    if (!IS_LOAD_AND_RESERVE_INSN (insn))
> >      return {};
> > @@ -8644,6 +8653,14 @@ show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
> >    gdb_printf (file, _("Use of exact watchpoints is %s.\n"), value);
> >  }
> >  
> > +static void
> > +show_powerpc_step_over_atomic (struct ui_file *file, int from_tty,
> > +			       struct cmd_list_element *c,
> > +			       const char *value)
> > +{
> > +  gdb_printf (file, _("Single-step over atomic (larx/stcx.) sequences %s.\n"), value);
> > +}
> > +
> >  /* Read a PPC instruction from memory.  */
> >  
> >  static unsigned int
> > @@ -8787,4 +8804,19 @@ scalar type, thus assuming that the variable is accessed through the address\n\
> >  of its first byte."),
> >  			   NULL, show_powerpc_exact_watchpoints,
> >  			   &setpowerpccmdlist, &showpowerpccmdlist);
> > +
> > +  add_setshow_boolean_cmd ("step-over-atomic", class_support,
> > +			   &powerpc_step_over_atomic,
> > +			   _("\
> > +Set whether single-step tries to step over atomics."),
> > +			   _("\
> > +Show whether single-step tries to step over atomics."),
> > +			   _("\
> > +If true, when GDB single-steps a larx instruction, it will try to find the\n\
> > +the matching stcx. instruction and step over the entire atomic sequence.\n\
> > +This can be required for forward-progress because single-stepping may clear\n\
> > +the reservation. Special environments like emulators and low level hardware\n\
> > +debug interfaces may not have this restriction, so this could be disabled."),
> > +			   NULL, show_powerpc_step_over_atomic,
> > +			   &setpowerpccmdlist, &showpowerpccmdlist);
> >  }