Re: [PATCH] gdb: Do not interrupt atomic sequences for ARC

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-01-25 6:02 p.m., Shahab Vahedi via Gdb-patches wrote:
> From: Shahab Vahedi <shahab@synopsys.com>
> 
> An atomic sequence for ARC looks something like below (For a better
> understanding of "llock", "scond", and "LF", please read [1]):
> 
>  50: llock   r2,[r0]         ; initiate an atomic write by setting LF=1
>  54: brne.nt r2,0,66         ; somebody else wrote? break if so.
>  58: scond   r3,[r0]         ; only write if LF is still 1
>  62: bne     fwrite+50       ; did we succeed? try again if not.
>  66: brne_s  r2,0,148        ; another thread won the game, go waiting
>  68: st      r18,[r13,64]    ; welcome to the realm
> 
> Normally GDB sets breakpoint on every instructions.  This means that
> there are exceptions raised between "llock" and "scond".  When that
> happens the "LF" flag is cleared and we lose the atomicity.

What do you mean exactly by "we lose the atomicity"?  If I understand
correctly, that means that if you step instruction by instruction,
you'll never manage to successfully get past the critical section, as
you will always restart it.  So it's not a matter that the critical
section is suddenly not atomic, but rather than you simply can't get
past it.  Is that right?

If so, that reminds me of restartable sequences, a new concept in the
Linux kernel:

  https://www.efficios.com/blog/2019/02/08/linux-restartable-sequences/

That's kind of the same thing, but implemented by the kernel.

> This patch teaches GDB the pattern of "llock .. scond" and will set
> the next breakpoint _after_ scond.  No matter if you use "n", "ni",
> "s", "si"; they all will skip over the atomic sequence now.  If you
> really want to break in between you must explicitly set a breakpoint.
> 
> [1]
> "llock" (Load Locked) loads the 32-bit word pointed by the source
> operand.  If the load is completed without any interruption or
> exception, the physical address is remembered, in Lock Physical Address
> (LPA), and the Lock Flag (LF) is set to 1.  LF is a non-architecturally
> visible flag and is cleared whenever an interrupt or exception takes
> place.  LF is also cleared (atomically) whenever another process writes
> to the LPA.
> 
> "scond" (Store Conditional) will write to the destination address if
> and only if the LF is set to 1.  When finished, with or without a write,
> it atomically copies the LF value to ZF (Zero Flag).
> 
> These two instructions together provide the mechanism for entering a
> critical section.  The code snippet above comes from uClibc's fwrite()
> function.

> 
> gdb/ChangeLog:
> 
> 	* arc-linux-tdep.c (handle_atomic_sequence): New.
> 	(arc_linux_software_single_step): Call handle_atomic_sequence().
> ---
>  gdb/arc-linux-tdep.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
> index c9fbd7ddc28..0e485d6b967 100644
> --- a/gdb/arc-linux-tdep.c
> +++ b/gdb/arc-linux-tdep.c
> @@ -332,6 +332,67 @@ arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch,
>  	  : arc_linux_trap_s_le);
>  }
>  
> +/* Check for an atomic sequence of instructions beginning with an
> +   LLOCK instruction and ending with a SCOND instruction:
> +
> +   main+14: llock   r2,[r0]
> +   main+18: brne.nt r2,0,main+30
> +   main+22: scond   r3,[r0]
> +   main+26: bne     main+14
> +   main+30: mov_s   r0,0
> +
> +   If such a sequence is found, attempt to step over it.
> +   A breakpoint is placed at the end of the sequence.  */
> +
> +static std::vector<CORE_ADDR>
> +handle_atomic_sequence (arc_instruction &insn, disassemble_info &di)

Hmm, I would not use a non-const reference here for the first param
here, as the intention is not to modify insn in the caller.  I'd suggest
passing in a const reference and using a local variable as the
function's scratch insn.

> +{
> +  const int atomic_seq_len = 24;    /* Instruction sequence length.  */
> +  std::vector<CORE_ADDR> next_pcs = {};
> +
> +  /* Sanity check.  */
> +  if (insn.insn_class != LLOCK)
> +    return next_pcs;

You already check that the insn_class is LLOCK in the caller, so this is
basically dead code.  Make the contract of the function clear: document
that this function can only be called with an instruction of class
LLOCK, and change this to a gdb_assert.

> +
> +  /* Data size we are dealing with: LLOCK vs. LLOCKD  */
> +  arc_ldst_data_size llock_data_size_mode = insn.data_size_mode;
> +  /* Indicator if any conditional branch is found in the sequence.  */
> +  bool found_bc = false;
> +  /* Becomes true if "LLOCK(D) .. SCOND(D)" sequence is found.  */
> +  bool is_pattern_valid = false;
> +
> +  for (int insn_count = 0; insn_count < atomic_seq_len; ++insn_count)
> +    {
> +      arc_insn_decode (arc_insn_get_linear_next_pc (insn),
> +		       &di, arc_delayed_print_insn, &insn);
> +
> +      if (insn.insn_class == BRCC)
> +        {
> +          /* If more than one conditial branch is found, this is not
> +             the pattern we are interested in.  */
> +          if (found_bc)
> +	    break;
> +	  found_bc = true;
> +	  continue;
> +        }

I'm wondering why this check.  I understand that you are probably
looking for a specific common pattern.  But if there are two branches in
the section, does that make it not an LLOCK/SCOND section that we would
like to skip over?  What if there are no branches?

> +
> +      /* This is almost a happy ending.  */
> +      if (insn.insn_class == SCOND)
> +        {
> +	  /* SCOND should match the LLOKCK's data size.  */
> +	  if (insn.data_size_mode == llock_data_size_mode)
> +	    is_pattern_valid = true;
> +	  break;
> +        }
> +    }

Just wondering, is it ill-formed code if an LLOCK is paired with an
SCOND of a different size?  How does the processor react?

Simon