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

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

[Shahab Vahedi]

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.

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)
+{
+  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;
+
+  /* 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;
+        }
+
+      /* 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;
+        }
+    }
+
+  if (is_pattern_valid)
+    /* Get next instruction after scond(d).  There is no limm.  */
+    next_pcs.push_back (insn.address + insn.length);
+
+  return next_pcs;
+}
+
 /* Implement the "software_single_step" gdbarch method.  */
 
 static std::vector<CORE_ADDR>
@@ -349,6 +410,9 @@ arc_linux_software_single_step (struct regcache *regcache)
 
   std::vector<CORE_ADDR> next_pcs;
 
+  if (curr_insn.insn_class == LLOCK)
+    return handle_atomic_sequence (curr_insn, di);
+
   /* For instructions with delay slots, the fall thru is not the
      instruction immediately after the current instruction, but the one
      after that.  */
-- 
2.30.0

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

[Simon Marchi]

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

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

[Shahab Vahedi]

Hi Simon,

Thank you for your interest in this patch.  Indeed, you put some
interesting questions in place.  I try my best to address them:

On Mon, Jan 25, 2021 at 10:34:15PM -0500, Simon Marchi wrote:
> 
> 
> 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,
(Or Line by line: "step", "stepi", "next", "nexti")
> you'll never manage to successfully get past the critical section, as
The code here is not the critical section per se, but the entry for
it.  The critical section starts at "68".
> you will always restart it.  So it's not a matter that the critical
Yes, it will be restarted because:
1. GDB (implicitly) inserts breakpoints
2. Breakpoint raises an exception (the program flow is interrupted)
3. As a result LF flag is cleared
4. Things have to be restarted again
> 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.

Actually it is explained in that page, but more in the lines of:

"And not all architectures provide a single instruction for atomically
updating data. For instance, ARM uses link-load/store-conditional [1]
instructions to read the old data, modify it, and write the new data.
If two threads try to write the same data simultaneously only one will
succeed and the other will fail and restart the read-modify-write
sequence."

That is an interesting article nonetheless.

[1]
https://en.wikipedia.org/wiki/Load-link/store-conditional

> > +
> > +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.

Actually "arc_insn_decode()" [1] is writing to that address.

[1]
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=opcodes/arc-dis.c;h=6a6404a16;hb=HEAD#l1461

> > +{
> > +  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.

I want this function to handle any (24-bytes) instruction sequences.
If the caller happens to check for the "LLOCK" class, it's just about
deciding what to do next.  Nevertheless, I don't feel strong about it.
If you still think it's a good practice to do it as you suggested, I
can change it.

> > +
> > +  /* 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 pattern [1] of the macro is being exactly looked for.  Therefore,
"no branch" or "more than one branch" or "having different sizes" (e.g.
"llock" vs "scondD", etc) is a no-go.

[1]
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46

> > +
> > +      /* This is almost a happy ending.  */
> > +      if (insn.insn_class == SCOND)
> > +        {
> > +	  /* SCOND should match the LLOKCK's data size.  */

I noticed a typo here ("LLOKCK" -> "LLOCK") and will fix it.

> > +	  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?

That is a very profound question.  Because I have some reservations on
my answer, I asked the architect and will relay their answer here.


Cheers,
Shahab

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

[Simon Marchi]

On 2021-01-26 5:11 a.m., Shahab Vahedi wrote:
> Hi Simon,
> 
> Thank you for your interest in this patch.  Indeed, you put some
> interesting questions in place.  I try my best to address them:
> 
> On Mon, Jan 25, 2021 at 10:34:15PM -0500, Simon Marchi wrote:
>>
>>
>> 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,
> (Or Line by line: "step", "stepi", "next", "nexti")
>> you'll never manage to successfully get past the critical section, as
> The code here is not the critical section per se, but the entry for
> it.  The critical section starts at "68".
>> you will always restart it.  So it's not a matter that the critical
> Yes, it will be restarted because:
> 1. GDB (implicitly) inserts breakpoints
> 2. Breakpoint raises an exception (the program flow is interrupted)
> 3. As a result LF flag is cleared
> 4. Things have to be restarted again
>> 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.
> 
> Actually it is explained in that page, but more in the lines of:
> 
> "And not all architectures provide a single instruction for atomically
> updating data. For instance, ARM uses link-load/store-conditional [1]
> instructions to read the old data, modify it, and write the new data.
> If two threads try to write the same data simultaneously only one will
> succeed and the other will fail and restart the read-modify-write
> sequence."
> 
> That is an interesting article nonetheless.
> 
> [1]
> https://en.wikipedia.org/wiki/Load-link/store-conditional
> 
>>> +
>>> +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.
> 
> Actually "arc_insn_decode()" [1] is writing to that address.

That's what I'm saying.  The caller of handle_atomic_sequence passes in an
instruction, but probably wouldn't expect that instruction change (why would
it?), unless it was documented as such.  So using arc_insn_decode on the
caller's instruction changes the instruction under the feet of the caller.

I'd suggest having:

  static std::vector<CORE_ADDR>
  handle_atomic_sequence (const arc_instruction &insn, disassemble_info &di)
  {
    ...
  }

And use a local instance of arc_instruction inside handle_atomic_sequence to
be modified.

> 
> [1]
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=opcodes/arc-dis.c;h=6a6404a16;hb=HEAD#l1461
> 
>>> +{
>>> +  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.
> 
> I want this function to handle any (24-bytes) instruction sequences.
> If the caller happens to check for the "LLOCK" class, it's just about
> deciding what to do next.  Nevertheless, I don't feel strong about it.
> If you still think it's a good practice to do it as you suggested, I
> can change it.

Is is because this function is going to be in other contexts where this
behavior will be useful?  For that code path to be useful, there would
need to be some code that uses the function like:

  std::vector<CORE_ADDR> next_pcs = handle_atomic_sequence (...);
  if (next_pcs.empty ())
    // do something
  else
    // do something else

Because at the moment, I find it a bit pointless to have the exact same
check twice in a row.

>>> +
>>> +  /* 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 pattern [1] of the macro is being exactly looked for.  Therefore,
> "no branch" or "more than one branch" or "having different sizes" (e.g.
> "llock" vs "scondD", etc) is a no-go.
> 
> [1]
> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46

Ok, then it should perhaps be documented as such, that you are looking for
this particular macro (including a link to it), and not just any
LLOCK/SCOND section.

> 
>>> +
>>> +      /* This is almost a happy ending.  */
>>> +      if (insn.insn_class == SCOND)
>>> +        {
>>> +	  /* SCOND should match the LLOKCK's data size.  */
> 
> I noticed a typo here ("LLOKCK" -> "LLOCK") and will fix it.
> 
>>> +	  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?
> 
> That is a very profound question.  Because I have some reservations on
> my answer, I asked the architect and will relay their answer here.

Ok, not a big deal.  The answer is likely "undefined behavior" or something
like that, in which case it would be fine for GDB to not identify it as a
section to be skipped.

Simon

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

[Shahab Vahedi]

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.

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:

https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46

v2 (after Simon's remarks[2]):
- handle_atomic_sequence() gets a copy of an instruction instead of
  a reference.
- handle_atomic_sequence() asserts if the given instruction is an llock.

[2]
https://sourceware.org/pipermail/gdb-patches/2021-January/175487.html

gdb/ChangeLog:

	* arc-linux-tdep.c (handle_atomic_sequence): New.
	(arc_linux_software_single_step): Call handle_atomic_sequence().
---
 gdb/arc-linux-tdep.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index c9fbd7ddc28..45bce67638b 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -332,6 +332,76 @@ 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.
+
+   These patterns are hand coded in libc's (glibc and uclibc). Take
+   a look at [1] for instance:
+
+   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.
+
+   This function expects the INSN to be a "llock(d)" instruction.
+
+   [1]
+   https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/ \
+     sysdeps/linux/arc/bits/atomic.h#n46
+   */
+
+static std::vector<CORE_ADDR>
+handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+{
+  const int atomic_seq_len = 24;    /* Instruction sequence length.  */
+  std::vector<CORE_ADDR> next_pcs = {};
+
+  /* Sanity check.  */
+  gdb_assert (insn.insn_class == LLOCK);
+
+  /* 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;
+        }
+
+      /* This is almost a happy ending.  */
+      if (insn.insn_class == SCOND)
+        {
+	  /* SCOND should match the LLOCK's data size.  */
+	  if (insn.data_size_mode == llock_data_size_mode)
+	    is_pattern_valid = true;
+	  break;
+        }
+    }
+
+  if (is_pattern_valid)
+    /* Get next instruction after scond(d).  There is no limm.  */
+    next_pcs.push_back (insn.address + insn.length);
+
+  return next_pcs;
+}
+
 /* Implement the "software_single_step" gdbarch method.  */
 
 static std::vector<CORE_ADDR>
@@ -349,6 +419,9 @@ arc_linux_software_single_step (struct regcache *regcache)
 
   std::vector<CORE_ADDR> next_pcs;
 
+  if (curr_insn.insn_class == LLOCK)
+    return handle_atomic_sequence (curr_insn, di);
+
   /* For instructions with delay slots, the fall thru is not the
      instruction immediately after the current instruction, but the one
      after that.  */
-- 
2.30.0

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

[Shahab Vahedi]

On Tue, Jan 26, 2021 at 11:02:31AM -0500, Simon Marchi wrote:
> On 2021-01-26 5:11 a.m., Shahab Vahedi wrote:
> > On Mon, Jan 25, 2021 at 10:34:15PM -0500, Simon Marchi wrote:
> >> On 2021-01-25 6:02 p.m., Shahab Vahedi via Gdb-patches wrote:
> >>> +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.
> > 
> > Actually "arc_insn_decode()" [1] is writing to that address.
> 
> That's what I'm saying.  The caller of handle_atomic_sequence passes in an
> instruction, but probably wouldn't expect that instruction change (why would
> it?), unless it was documented as such.  So using arc_insn_decode on the
> caller's instruction changes the instruction under the feet of the caller.
> 
> I'd suggest having:
> 
>   static std::vector<CORE_ADDR>
>   handle_atomic_sequence (const arc_instruction &insn, disassemble_info &di)
>   {
>     ...
>   }
> 
> And use a local instance of arc_instruction inside handle_atomic_sequence to
> be modified.

Then I will use a "pass-by-value" arguemt instead of a reference.

> >>> +{
> >>> +  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.
> > 
> > I want this function to handle any (24-bytes) instruction sequences.
> > If the caller happens to check for the "LLOCK" class, it's just about
> > deciding what to do next.  Nevertheless, I don't feel strong about it.
> > If you still think it's a good practice to do it as you suggested, I
> > can change it.
> 
> Is is because this function is going to be in other contexts where this
> behavior will be useful?  For that code path to be useful, there would
> need to be some code that uses the function like:
> 
>   std::vector<CORE_ADDR> next_pcs = handle_atomic_sequence (...);
>   if (next_pcs.empty ())
>     // do something
>   else
>     // do something else
> 
> Because at the moment, I find it a bit pointless to have the exact same
> check twice in a row.

Done.

> >>> +
> >>> +  /* 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 pattern [1] of the macro is being exactly looked for.  Therefore,
> > "no branch" or "more than one branch" or "having different sizes" (e.g.
> > "llock" vs "scondD", etc) is a no-go.
> > 
> > [1]
> > https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46
> 
> Ok, then it should perhaps be documented as such, that you are looking for
> this particular macro (including a link to it), and not just any
> LLOCK/SCOND section.

Added.

> >>> +
> >>> +      /* This is almost a happy ending.  */
> >>> +      if (insn.insn_class == SCOND)
> >>> +        {
> >>> +	  /* SCOND should match the LLOKCK's data size.  */
> > 
> > I noticed a typo here ("LLOKCK" -> "LLOCK") and will fix it.
> > 
> >>> +	  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?
> > 
> > That is a very profound question.  Because I have some reservations on
> > my answer, I asked the architect and will relay their answer here.
> 
> Ok, not a big deal.  The answer is likely "undefined behavior" or something
> like that, in which case it would be fine for GDB to not identify it as a
> section to be skipped.

I've got the response and to put it shortly:
Having llock and scond of different sizes (32bit vs 64bit) is an anti-pattern
(pun intended) and must be avoided.


Cheers.
Shahab

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

[Tom Tromey]

>>>>> "Shahab" == Shahab Vahedi via Gdb-patches <gdb-patches@sourceware.org> writes:

Shahab> From: Shahab Vahedi <shahab@synopsys.com>
Shahab> An atomic sequence for ARC looks something like below (For a better
Shahab> understanding of "llock", "scond", and "LF", please read [1]):

Hi.  Thanks for the patch.
I had a couple of nits but nothing serious.

Shahab> +  std::vector<CORE_ADDR> next_pcs = {};

You don't need " = {}" here.

Shahab> +          /* If more than one conditial branch is found, this is not

Typo, "conditional".

Shahab> +  if (is_pattern_valid)
Shahab> +    /* Get next instruction after scond(d).  There is no limm.  */
Shahab> +    next_pcs.push_back (insn.address + insn.length);

In the gdb style, the body of this "if" should be braced.
Normally a single statement isn't braced, but a comment requires the
braces once again.

Shahab>    std::vector<CORE_ADDR> next_pcs;
 
Shahab> +  if (curr_insn.insn_class == LLOCK)
Shahab> +    return handle_atomic_sequence (curr_insn, di);

I'd push this condition before the declaration of next_pcs, to avoid
invoking that constructor on this branch.

I think this is ok with these minor tweaks.  Thanks.

Tom

[PUSHED master] gdb: Do not interrupt atomic sequences for ARC

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

When stepping over thread-lock related codes (in uClibc), the inferior process
gets stuck and never manages to enter the critical section:

------8<-------
 1 size_t fwrite(const void * __restrict ptr, size_t size,
 2               size_t nmemb, register FILE * __restrict stream)
 3 {
 4     size_t retval;
 5     __STDIO_AUTO_THREADLOCK_VAR;
 6
 7 >   __STDIO_AUTO_THREADLOCK(stream);
 8
 9     retval = fwrite_unlocked(ptr, size, nmemb, stream);
10
11     __STDIO_AUTO_THREADUNLOCK(stream);
12
13     return retval;
14 }
------>8-------

Here, we are at line 7.  Using the "next" command leads no where.
However, setting a breakpoint on line 9 and issuing "continue" works.

Looking at the assembly instructions reveals that we're dealing with the
critical section entry code [1] that should never be interrupted, in this
case by the debugger's implicit breakpoints:

------8<-------
  ...
1 add_s   r0,r13,0x38
2 mov_s   r3,1
3 llock   r2,[r0]        <-.
4 brne.nt r2,0,14     --.  |
5 scond   r3,[r0]       |  |
6 bne     -10         --|--'
7 brne_s  r2,0,84     <-'
  ...
------>8-------

Lines 3 until 5 (inclusive) are supposed to be executed atomically.
Therefore, GDB should never (implicitly) insert a breakpoint on lines
4 and 5, else the program will try to acquire the lock again by jumping
back to line 3 and gets stuck in an infinite loop.

The solution is to make GDB aware of these patterns so it inserts
breakpoints after the sequence -- line 6 in this example.

[1]
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46
------8<-------
  ({									\
	__typeof(oldval) prev;						\
									\
	__asm__ __volatile__(						\
	"1:	llock   %0, [%1]	\n"				\
	"	brne    %0, %2, 2f	\n"				\
	"	scond   %3, [%1]	\n"				\
	"	bnz     1b		\n"				\
	"2:				\n"				\
	: "=&r"(prev)							\
	: "r"(mem), "ir"(oldval),					\
	  "r"(newval) /* can't be "ir". scond can't take limm for "b" */\
	: "cc", "memory");						\
									\
	prev;								\
  })
------>8-------
"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:
-----------------------

v3 (after Tom's remarks[2]):
 handle_atomic_sequence()
  - no need to initialize the std::vector with "{}"
  - fix typo in comments: "conditial" -> "conditional"
  - add braces to the body of "if" condition because of the comment line
 arc_linux_software_single_step()
  - make the performance slightly more efficient by moving a few
    variables after the likely "return" point.

v2 (after Simon's remarks[3]):
- handle_atomic_sequence() gets a copy of an instruction instead of
  a reference.
- handle_atomic_sequence() asserts if the given instruction is an llock.

[2]
https://sourceware.org/pipermail/gdb-patches/2021-February/175805.html

[3]
https://sourceware.org/pipermail/gdb-patches/2021-January/175487.html

gdb/ChangeLog:

	PR tdep/27369
	* arc-linux-tdep.c (handle_atomic_sequence): New.
	(arc_linux_software_single_step): Call handle_atomic_sequence().
---
 gdb/ChangeLog        |  6 ++++
 gdb/arc-linux-tdep.c | 77 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 912ca6b9444..e67668d315c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-02-08  Shahab Vahedi  <shahab@synopsys.com>
+
+	PR tdep/27369
+	* arc-linux-tdep.c (handle_atomic_sequence): New.
+	(arc_linux_software_single_step): Call handle_atomic_sequence().
+
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index c9fbd7ddc28..cf18b8d6b03 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -332,6 +332,78 @@ 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.
+
+   These patterns are hand coded in libc's (glibc and uclibc). Take
+   a look at [1] for instance:
+
+   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.
+
+   This function expects the INSN to be a "llock(d)" instruction.
+
+   [1]
+   https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/ \
+     sysdeps/linux/arc/bits/atomic.h#n46
+   */
+
+static std::vector<CORE_ADDR>
+handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+{
+  const int atomic_seq_len = 24;    /* Instruction sequence length.  */
+  std::vector<CORE_ADDR> next_pcs;
+
+  /* Sanity check.  */
+  gdb_assert (insn.insn_class == LLOCK);
+
+  /* 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 conditional branch is found, this is not
+             the pattern we are interested in.  */
+          if (found_bc)
+	    break;
+	  found_bc = true;
+	  continue;
+        }
+
+      /* This is almost a happy ending.  */
+      if (insn.insn_class == SCOND)
+        {
+	  /* SCOND should match the LLOCK's data size.  */
+	  if (insn.data_size_mode == llock_data_size_mode)
+	    is_pattern_valid = true;
+	  break;
+        }
+    }
+
+  if (is_pattern_valid)
+    {
+      /* Get next instruction after scond(d).  There is no limm.  */
+      next_pcs.push_back (insn.address + insn.length);
+    }
+
+  return next_pcs;
+}
+
 /* Implement the "software_single_step" gdbarch method.  */
 
 static std::vector<CORE_ADDR>
@@ -345,8 +417,11 @@ arc_linux_software_single_step (struct regcache *regcache)
   struct arc_instruction curr_insn;
   arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
 		   &curr_insn);
-  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
 
+  if (curr_insn.insn_class == LLOCK)
+    return handle_atomic_sequence (curr_insn, di);
+
+  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
   std::vector<CORE_ADDR> next_pcs;
 
   /* For instructions with delay slots, the fall thru is not the
-- 
2.30.0

[PUSHED gdb-10-branch] gdb: Do not interrupt atomic sequences for ARC

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

When stepping over thread-lock related codes (in uClibc), the inferior process
gets stuck and never manages to enter the critical section:

------8<-------
 1 size_t fwrite(const void * __restrict ptr, size_t size,
 2               size_t nmemb, register FILE * __restrict stream)
 3 {
 4     size_t retval;
 5     __STDIO_AUTO_THREADLOCK_VAR;
 6
 7 >   __STDIO_AUTO_THREADLOCK(stream);
 8
 9     retval = fwrite_unlocked(ptr, size, nmemb, stream);
10
11     __STDIO_AUTO_THREADUNLOCK(stream);
12
13     return retval;
14 }
------>8-------

Here, we are at line 7.  Using the "next" command leads no where.
However, setting a breakpoint on line 9 and issuing "continue" works.

Looking at the assembly instructions reveals that we're dealing with the
critical section entry code [1] that should never be interrupted, in this
case by the debugger's implicit breakpoints:

------8<-------
  ...
1 add_s   r0,r13,0x38
2 mov_s   r3,1
3 llock   r2,[r0]        <-.
4 brne.nt r2,0,14     --.  |
5 scond   r3,[r0]       |  |
6 bne     -10         --|--'
7 brne_s  r2,0,84     <-'
  ...
------>8-------

Lines 3 until 5 (inclusive) are supposed to be executed atomically.
Therefore, GDB should never (implicitly) insert a breakpoint on lines
4 and 5, else the program will try to acquire the lock again by jumping
back to line 3 and gets stuck in an infinite loop.

The solution is to make GDB aware of these patterns so it inserts
breakpoints after the sequence -- line 6 in this example.

[1]
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46
------8<-------
  ({									\
	__typeof(oldval) prev;						\
									\
	__asm__ __volatile__(						\
	"1:	llock   %0, [%1]	\n"				\
	"	brne    %0, %2, 2f	\n"				\
	"	scond   %3, [%1]	\n"				\
	"	bnz     1b		\n"				\
	"2:				\n"				\
	: "=&r"(prev)							\
	: "r"(mem), "ir"(oldval),					\
	  "r"(newval) /* can't be "ir". scond can't take limm for "b" */\
	: "cc", "memory");						\
									\
	prev;								\
  })
------>8-------
"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:
-----------------------

v3 (after Tom's remarks[2]):
 handle_atomic_sequence()
  - no need to initialize the std::vector with "{}"
  - fix typo in comments: "conditial" -> "conditional"
  - add braces to the body of "if" condition because of the comment line
 arc_linux_software_single_step()
  - make the performance slightly more efficient by moving a few
    variables after the likely "return" point.

v2 (after Simon's remarks[3]):
- handle_atomic_sequence() gets a copy of an instruction instead of
  a reference.
- handle_atomic_sequence() asserts if the given instruction is an llock.

[2]
https://sourceware.org/pipermail/gdb-patches/2021-February/175805.html

[3]
https://sourceware.org/pipermail/gdb-patches/2021-January/175487.html

gdb/ChangeLog:

	PR tdep/27369
	* arc-linux-tdep.c (handle_atomic_sequence): New.
	(arc_linux_software_single_step): Call handle_atomic_sequence().
---
 gdb/ChangeLog        |  6 ++++
 gdb/arc-linux-tdep.c | 77 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fde6f9c3830..9d12c466952 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-02-08  Shahab Vahedi  <shahab@synopsys.com>
+
+	PR tdep/27369
+	* arc-linux-tdep.c (handle_atomic_sequence): New.
+	(arc_linux_software_single_step): Call handle_atomic_sequence().
+
 2021-02-05  Tom de Vries  <tdevries@suse.de>
 
 	PR breakpoints/27330
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 1fef62a6128..a7bace12623 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -155,6 +155,78 @@ 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.
+
+   These patterns are hand coded in libc's (glibc and uclibc). Take
+   a look at [1] for instance:
+
+   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.
+
+   This function expects the INSN to be a "llock(d)" instruction.
+
+   [1]
+   https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/ \
+     sysdeps/linux/arc/bits/atomic.h#n46
+   */
+
+static std::vector<CORE_ADDR>
+handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+{
+  const int atomic_seq_len = 24;    /* Instruction sequence length.  */
+  std::vector<CORE_ADDR> next_pcs;
+
+  /* Sanity check.  */
+  gdb_assert (insn.insn_class == LLOCK);
+
+  /* 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 conditional branch is found, this is not
+             the pattern we are interested in.  */
+          if (found_bc)
+	    break;
+	  found_bc = true;
+	  continue;
+        }
+
+      /* This is almost a happy ending.  */
+      if (insn.insn_class == SCOND)
+        {
+	  /* SCOND should match the LLOCK's data size.  */
+	  if (insn.data_size_mode == llock_data_size_mode)
+	    is_pattern_valid = true;
+	  break;
+        }
+    }
+
+  if (is_pattern_valid)
+    {
+      /* Get next instruction after scond(d).  There is no limm.  */
+      next_pcs.push_back (insn.address + insn.length);
+    }
+
+  return next_pcs;
+}
+
 /* Implement the "software_single_step" gdbarch method.  */
 
 static std::vector<CORE_ADDR>
@@ -168,8 +240,11 @@ arc_linux_software_single_step (struct regcache *regcache)
   struct arc_instruction curr_insn;
   arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
 		   &curr_insn);
-  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
 
+  if (curr_insn.insn_class == LLOCK)
+    return handle_atomic_sequence (curr_insn, di);
+
+  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
   std::vector<CORE_ADDR> next_pcs;
 
   /* For instructions with delay slots, the fall thru is not the
-- 
2.30.0