Skip to content

Simplify adapter-vs-controller stop plumbing after worker-queue refactor #1089

@xusheng6

Description

@xusheng6

Follow-up to #1088 / #1087.

What is the adapter-vs-controller stop distinction?

The controller currently distinguishes two kinds of stop:

  • Adapter stop — the underlying engine (DbgEng, LLDB, gdbstub, …) reports that the target has stopped. Posted as AdapterStoppedEventType events.
  • Controller stop — the moment the debugger hands control back to the user. Posted as TargetStoppedEventType events via NotifyStopped.

The two can diverge because:

  1. Conditional breakpoints — adapter says "stopped at a breakpoint," controller evaluates the condition, and if it's false, silently resumes (debuggercontroller.cpp:2218-2256). The user never sees the adapter stop.
  2. IL stepping — a single user "Step Into HLIL" causes N underlying single-step adapter stops. Only the final one, where the IP lands on the right IL boundary, should be user-visible.
  3. External adapter commands — when the user types e.g. si directly into the LLDB REPL, no controller-driven op is in flight to consume the adapter stop. The dispatcher synthesizes a TargetStoppedEvent for the UI (debuggercontroller.cpp:2276-2297).

The semantic need is genuine — these features can't go away.

What's awkward about the current implementation

The mechanism that implements this distinction is more complicated than the semantics require:

  • AdapterStoppedEventType is a public event type going through the same PostDebuggerEvent queue as user-visible events.
  • m_lastAdapterStopEventConsumed is a flag the dispatcher and ExecuteAdapterAndWait pass back and forth to decide whether a stop should be "silenced" or surfaced.
  • The conditional-breakpoint check runs on the dispatcher thread, and from there calls m_adapter->Go() directly. The code apologizes for this with a comment: // Using m_adapter->Go() directly instead of Go() to avoid mutex deadlock. The m_suppressResumeEvent flag exists to paper over the fact that the dispatcher is now driving the adapter.
  • ExecuteAdapterAndWait registers a temporary event callback, blocks on a semaphore, waits for the dispatcher to fire it, then unregisters (debuggercontroller.cpp:2860-2883).

All of that exists because in the original design, the adapter's event thread, the controller's dispatcher thread, and a per-op spawned thread are three separate threads that need a common rendezvous — and PostDebuggerEvent is what they have.

What the worker-queue refactor enables

With #1087 in place, the worker is the single thread that drives the adapter. So the cleaner architecture is:

Adapter thread          posts adapter stops to an INTERNAL channel
(EngineLoop, etc.)      (condvar + reason field on the controller)

Worker thread           consumes adapter stops from that channel.
                        Two waits, never both active:

  Outer loop            - new task in m_workQueue?       → run it
  (idle)                - spontaneous adapter stop?      → NotifyStopped
                        - shutdown?                      → exit

  Inner WaitForAdapterStop (during ExecuteAdapterAndWait):
                        - adapter stop with conditional-bp false condition?
                            → m_adapter->Go(), loop
                        - adapter stop while IL step not at boundary?
                            → step again, loop
                        - else                            → return reason

Dispatcher thread       only sees user-visible events
                        (TargetStopped, Launch, Resume, ...)
                        no longer handles AdapterStoppedEventType

Once the decision moves to the worker (the single owner of adapter operations), the three-thread rendezvous collapses into the worker's local control flow.

Spontaneous adapter stops — the case to be careful about

When the user types e.g. si directly into the LLDB REPL while no controller op is in flight, the adapter stops with no inner WaitForAdapterStop to consume the event. The current dispatcher synthesizes a TargetStoppedEvent for that case. The proposed design must preserve this — naïvely moving consumption into WaitForAdapterStop alone would drop spontaneous stops on the floor.

The fix is the outer loop above: the worker waits on m_workQueue or the adapter-stop channel or shutdown. When the channel signals and no queued op is in flight, the outer loop picks it up and calls NotifyStopped(reason) directly — same effect as today's dispatcher synthesis, just on the worker.

Because only one op can run in the engine at a time, an adapter stop is unambiguously either "the response to whatever the worker last asked for" (handled by the inner wait) or "the user did this themselves while we were idle" (handled by the outer loop). A m_inAdapterWait state bit or a single shared condvar that both waits monitor is enough to route the signal correctly.

What can go away

  • AdapterStoppedEventType as a public event type — becomes a controller-internal signal. The dispatcher no longer routes it; external callbacks no longer receive it.
  • m_lastAdapterStopEventConsumed — irrelevant when the worker is the single decider.
  • The temporary callback-registration pattern in ExecuteAdapterAndWait — replaced by a direct m_adapterStopCv.wait(lock) returning a reason.
  • The conditional-breakpoint Go-from-dispatcher hack and the m_suppressResumeEvent workaround — the worker is allowed to drive the adapter; no special-case re-entry needed.
  • The "external command" synthesis at debuggercontroller.cpp:2279 — moves to the worker's outer loop, as described above.

Proposed steps

  1. Add an internal adapter-stop channel (mutex + condvar + pending-reason field + m_inAdapterWait flag) on DebuggerController. PostDebuggerEvent intercepts AdapterStoppedEventType and routes the reason to this channel instead of (or in addition to) the public event queue.
  2. Replace ExecuteAdapterAndWait's callback+semaphore with a private WaitForAdapterStop() that reads from the internal channel.
  3. Move the conditional-breakpoint check from the dispatcher into WaitForAdapterStop() (or into the worker's op body, just before WaitForAdapterStop returns to the op).
  4. Extend the worker's outer loop to wait on both m_workQueue and the adapter-stop channel; when a spontaneous stop arrives, call NotifyStopped(reason) directly.
  5. Remove AdapterStoppedEventType from the public event enum, or mark it deprecated/internal.
  6. Delete m_lastAdapterStopEventConsumed and m_suppressResumeEvent.

Estimated size: ~200 added / ~300 removed across debuggercontroller.cpp and debuggercontroller.h. Blocked on #1087 landing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions