Page MenuHomePhabricator

[LLDB][MIPS] Single step atomic sequences
Needs RevisionPublic

Authored by bhushan on Feb 22 2016, 9:44 PM.

Details

Summary

This patch handles atomic sequences during single step.

LLDB should treat all instructions in the atomic sequence as if they are a single instruction i.e as a "single instruction block".
So while single stepping, LLDB should detect and skip such sequence by placing breakpoints only outside of such sequence i.e at the end of the sequence.

In MIPS, atomic sequence starts with LL (linked load) instruction and end with SC (store conditional) instruction.

Example of atomic sequence in MIPS:

0x400c08 <+372>: ll     $4, 0($2) ----> Start of sequence
0x400c0c <+376>: bne    $4, $5, 28    ---> Atomic sequence can contain branches that jump outside sequence range.
0x400c10 <+380>: addiu  $3, $zero, 0
0x400c14 <+384>: move   $1, $6
0x400c18 <+388>: sc     $1, 0($2) -----> End of sequence
0x400c1c <+392>: beqz   $1, -20   
0x400c20 <+396>: addiu  $3, $zero, 1
0x400c24 <+400>: sync
0x400c28 <+404>: bnez   $3, 16    ---> Target of branch from sequence

Considering above example, while single stepping from LL instruction, LLDB should stop at instruction
after the end of sequence (instead of stopping at instruction next to LL).

There can be multiple exit/end points to atomic sequence.

  1. SC instruction
  2. Branch instructions in atomic sequence that jump outside sequence.

So to handle this, LLDB should place breakpoints at multiple locations:

  1. Breakpoint at instruction after SC instruction (i.e at 0x400c1c in above ex)
  2. Breakpoints at target addresses of branch instructions (if the branch target address is outside the sequence) i.e at 0x400c28, target of "bne $4, $5, 28" instruction.

These breakpoint addresses are determined by EmulateInstruction.

This patch makes few assumptions:

  1. Assumes that no atomic sequence for mips is longer than 16 instructions. i.e scan upto maximum 16 instructions from LL to find end of sequence.
  2. Assumes that the atomic sequence ends with a sc/scd instruction. So if we dont find "sc/scd" instruction then do not put any breakpoint. i.e fallback to the standard single-step code.

Testcase:

This patch also adds a testcase "TestStepInAtomicSequence.py" to test this change (currently enabled for MIPS only).
The test finds starting instruction of atomic sequence, runs till that instruction then does a single step and
finally verifies that we have stopped after end of sequence.

Diff Detail

Repository
rL LLVM

Event Timeline

bhushan updated this revision to Diff 48778.Feb 22 2016, 9:44 PM
bhushan retitled this revision from to [LLDB][MIPS] Single step atomic sequences.
bhushan updated this object.
bhushan added reviewers: clayborg, tberghammer.
bhushan set the repository for this revision to rL LLVM.
bhushan added subscribers: lldb-commits, jaydeep, nitesh.jain and 2 others.
labath added a subscriber: labath.Feb 23 2016, 4:23 AM

I'll leave the final review to Tamas, just a couple of comments here.

packages/Python/lldbsuite/test/functionalities/single_step_atomic_sequence/TestStepInAtomicSequence.py
70

If you don't find the instructions you are looking for (perhaps due to a compiler change), the test will end up being a no-op. You might want to assert that you actually find the instructions you are looking for.

94

These kinds of debug statements are usually made conditional on self.TraceOn() (-t flag to dotest), to minimize the noise it the general case when you're not debugging this test.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1689

I've been seeing this pattern a lot lately. Isn't it time to make a isMipsArchitecture utility function somewhere?

clayborg resigned from this revision.Feb 23 2016, 9:40 AM
clayborg edited reviewers, added: jingham; removed: clayborg.

Looks good to me, but I think Jim Ingham should have a look in case he has anything to add since Jim is the master or stepping.

tberghammer edited edge metadata.Feb 23 2016, 10:06 AM

I am not sure I agree with the following starting point of the CL (I know about the similar functionality in GDB) and even if we agree in it I think you are handling a very small fraction of the problems.

LLDB should treat all instructions in the atomic sequence as if they are a single instruction i.e as a "single instruction block".

I think there are 3 scenario around atomic sequences:

  1. You are setting a breakpoint what fails inside of the atomic sequence
  2. You are line level stepping over an atomic sequence
  3. You are instruction level single stepping over an atomic sequence

For me the expected behavior in scenario 1 and 2 is to set the breakpoint at the address specified and handle the atomic sequence when you try to continue from the breakpoint (I hope that the compilers are generally not putting line breaks / symbols in the middle of an atomic sequence). I think the 3. scenario will happen when you are doing assembly level debugging of an atomic sequence where I would expect to step only 1 instruction as I am debugging assembly code.

If I understand your CL correctly then you only handle the scenario when we are single stepping from an LL instruction. This definitely not solving the issue when an we setting a breakpoint in the middle of an atomic sequence and then try to continue from there while it is preventing the user from doing an instruction level debugging of the atomic sequences.

I would be more happy with the following functionalities (not necessarily implementing all edge case for the first iteration):

  • Step instruction behaves as it did before (1 instruction, ignores the atomic sequence)
  • If we continue from a breakpoint placed inside an atomic sequence then we put back the breakpoint only after we left the atomic sequence

Please let me know what do you think.

P.S.: Have you hit any issue regarding atomic sequences during regular debugging? I would be interested about a usecase when they are affecting the debug experience so we can target the problematic point.

jingham requested changes to this revision.Feb 23 2016, 11:23 AM
jingham edited edge metadata.

IIUC, what you are doing here is when lldb-server gets a "step instruction" request that would step into one of these atomic regions, it sets a bunch of breakpoints on it's end (not telling the LLDB stepping machinery what is going on), and continues till it hits one of these. Then it will end up not where the thread plans expect it to at all, but presumably they will just have to cope?

That doesn't seem the right way to go about this, since it means that you can't do any reasoning based on what the stepping machinery was trying to achieve.
It would be better to wire this into the ThreadPlanStepRange's AddNextRangeBreakpoint. That's where we are doing "I want to make the next step within this range". Usually we just run to the next branch, setting breakpoints to do that. Or we single step if we are on a branch. But there's no reason that's the only thing we can do here.

This would be much better because we could decide whether to do this (because we are source line stepping) or not (because we are instruction stepping) etc. We could also do things like warn if the compiler accidentally puts a line break in the middle of an atomic region.

This would make the kind of considerations Tamas mentioned possible, whereas if you do things behind the stepping machinery's back, we're can't do this, and will probably end up confused in other ways as well.

This revision now requires changes to proceed.Feb 23 2016, 11:23 AM

In MIPS, we can not put a breakpoint in middle of an atomic sequence.
If we do so (and that breakpoint is hit) then continuing from breakpoint address will cause "SC" to fail due to a breakpoint exception.
SC fails when there’s been any exception serviced since the LL. This will then become a "never ending" sequence.

Similarly when doing assembly level debugging of an atomic sequence if we step only 1 instruction (as we are debugging assembly code)
we will end up putting breakpoint on next instruction (within atomic sequence) and will cause "SC" to fail because of reason mentioned above.

That means an atomic sequence, starting with LL and ending with SC needs to be treated as a "single instruction block".

In MIPS, we can not put a breakpoint in middle of an atomic sequence.
If we do so (and that breakpoint is hit) then continuing from breakpoint address will cause "SC" to fail due to a breakpoint exception.
SC fails when there’s been any exception serviced since the LL. This will then become a "never ending" sequence.

I agree that we shouldn't put a breakpoint in the middle of an atomic sequence (same is true on arm/aarch64) but your patch isn't preventing LLDB from doing it. I think what Jim and I are asking for is a solution where the atomic sequences are handled on host side so we either ensure that we don't set a breakpoint inside them or gracefully handle the continuing from them (with removing the breakpoint and only putting it back after the "SC" succeeded in the next iteration).

Similarly when doing assembly level debugging of an atomic sequence if we step only 1 instruction (as we are debugging assembly code)
we will end up putting breakpoint on next instruction (within atomic sequence) and will cause "SC" to fail because of reason mentioned above.

I expect that if somebody debug some atomic instruction sequences at assemply level then they will know about the behavior of LL and SC and they will work around the limitation with putting a breakpoint after SC to exit the loop around the atomic sequence. For me as a user it would be worrying if an instruction single step steps more then 1 actual instruction. Additionally we can add a new step command what steps out from an atomic sequence if there is user need for it.

That means an atomic sequence, starting with LL and ending with SC needs to be treated as a "single instruction block".

Agree, but I think we should do it on the host side.

Yes, exactly.

Jim