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

[Luis Machado via Gdb <gdb@sourceware.org>]

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);
>  }