[Lldb-commits] [lldb] [lldb] Give thread specific async interrupt requests a lifetime (PR #224272)

[via lldb-commits]

jimingham wrote:

This problem comes from awkward sequencing of the following events.

A) When the timeout thread wakes up and goes to SendAsyncInterrupt.
B) When the Process StopID is bumped
C) When the PrivateStateThread receives the broadcast stop event
D) When the PrivateStateThread receives the broadcast Interrupt event.

C is always after B, because we fully process the event that caused us to bump the StopID before we fetch another event.
D is always after A for obvious reasons.

We're only considering the case where the stop event is not caused by the Halt sent to the stub.

A happens on a free-running thread
B, C, & D are sequenced by fetching events on the Private state thread.
The async interrupt and the stop event come from sources on different threads, so there's no ordering there.

That leads to the following orderings of events.  Call the StopID we recorded in step A as: "A".  And I'm calling your guarded struct "the dingus".  The "Interrupt Stop ID" is the one we calculate when the PST handles the Interrupt event (dingusStopID != interruptStopID is the condition to say this is a stale interrupt.

======================
1: { A, B, C, D}:
dingus stop ID: A
Interrupt stop ID: A+1
State at Interrupt: running initiated by plan?

This one is handled correctly by this patch, we suppress the interrupt and either return the stop if the plan that was using the timeout was done, or let the running plans continue uninterrupted.
----------------------
2: {A, D, B, C}
dingus stop ID: A
interrupt stop ID: A
state at Interrupt: running since we haven't processed the stop yet

This one is okay because even though there's an interrupt pending, when you tell the ProcessGDBRemote end to effect the interrupt it will see that it is already stopped and do nothing.  Then you'll handle the stop event correctly.
-----------------------
3: {B, C, A, D}
dingus stop ID: A+1
interrupt stop ID: A+1
State at Interrupt: running initiated by plan?

In this case, when we get the interrupt event, the stop ID's will be the same, and so we won't call the interrupt stale, and will send out an errant interrupt.
-----------------------
4: {B, A, C, D}
dingus stop ID: A+1
interrupt stop ID: A+1 - (handling the stop may have resumed the target, but it won't have stopped again)
State at Interrupt: running initiated by plan.

In this case you will also do the wrong thing.
==================

So one problem comes about because you record the dingus stop ID when you do the async interrupt.  The StopID you want to record for the SingleThreadTimeout plan is the one at which the single thread timeout plan resumes, and use that when you make the dingus.  The stopID you actually care about is the one at which the SingleStep plan first resumed, not whatever it happens to be when the timeout fires (case 4).  That removes the ordering problem between stopping and firing the timeout.

You should check the stop state (private state) in the SingleThreadTimeout plan's timeout function, and if you find it already stopped, don't send the interrupt.  That fixes case 3.

The other thing that I noted while playing around with this is that when I don't add your patch, but just do:

```
void ThreadPlanSingleThreadTimeout::HandleTimeout() {
  Log *log = GetLog(LLDBLog::Step);
  // Private state thread will only send async interrupt
  // in running state so no need to check state here.
  if (m_process.GetPrivateState() == eStateRunning) {
    LLDB_LOGF(
        log,
        "ThreadPlanSingleThreadTimeout::HandleTimeout() send async interrupt.");
      m_process.SendAsyncInterrupt(&GetThread());

    m_state = State::AsyncInterrupt;
  } else {
    LLDB_LOGF(log, "ThreadPlanSingleThreadTimeout::HandleTimeout() process "
                   "already stopped - didn't send interrupt");
  }

}

```

(plus friend this class to Process) then even with your little delay which did make the test fail reliably w/o the patch, I can run this test for 10 min or so on an idle machine then as I load my machine with one and then two llvm builds, and I never saw a failure.  

That's a little curious because if I'm right above then your patch shouldn't have fixed that case, yet it seems to be the one I'm always hitting?





https://github.com/llvm/llvm-project/pull/224272