[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