Skip to content

Replace debugger PAL waits with cross-platform wait types - #132529

Merged
jkoritzinsky merged 12 commits into
mainfrom
dev/jkoritzinsky/prototype-wait-abstraction
Aug 21, 2026
Merged

Replace debugger PAL waits with cross-platform wait types#132529
jkoritzinsky merged 12 commits into
mainfrom
dev/jkoritzinsky/prototype-wait-abstraction

Conversation

@jkoritzinsky

@jkoritzinskyjkoritzinsky commented Aug 19, 2026

Copy link
Copy Markdown
Member

The debugger's remaining multi-object PAL waits couple debugger-runtime communication to legacy PAL wait handles on non-Windows platforms. This replaces those uses with debugger-owned, cross-platform wait types and keeps native handles confined to Windows.

Design

  • Add WaitHandle, WaitEvent, WaitLatch, and Windows-only NativeHandle to the debug PAL.
  • Implement Unix events and latches with nonblocking anonymous pipes and poll; latches remain signaled after their first Set.
  • Migrate debugger event and helper-thread waits to the typed API.
  • Tie transport-session opening directly to its state predicate with a shared minipal condition variable instead of using a manual reset event (condition var is a better primitive for the abstraction and Windows has one since Vista).
  • Add shared minipal recursive/non-recursive mutex and condition-variable support, and use it for System.Native LowLevelMonitor.

The Unix process-exit path intentionally retains the established polling model. This keeps process lifetime ownership in DbgTransportTarget while making the published exit notification sticky for every debugger waiter.

I looked at adding support for pidfd_open or kqueue based process waits, but the volume of code to support these platform-specific pathways for a single caller was not worth the cost compared to the wait thread + latch model.

Validation

  • Windows x64 Debug CoreCLR build
  • Linux x64 Debug CoreCLR build on directly hosted Ubuntu WSL
  • Private diagnostic Attach.AttachedTest and Attach.BreakTest on Windows and Ubuntu
  • Unix WaitEvent/WaitLatch semantics harness, 20/20 runs
  • System.Native LowLevelMonitor harness, 100 iterations

Fixes#132149

Note

This PR description was generated by GitHub Copilot.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

jkoritzinskyand others added 10 commits August 19, 2026 11:49
Introduce typed RAII event and process wait handles, migrate debugger waits off PAL multi-wait APIs, and add cross-platform wait tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Separate event, process descriptor, and process pipe state into tagged union arms. Use nanosleep for watcher polling and remove the obsolete poll error state.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Delete the standalone debugger condition test and minipal wait test targets. Remove the associated test-only process watcher hooks and compile-time forcing.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use a sticky minipal latch for Unix process exit and restore the debugger-owned poller thread. Remove pidfd and kqueue waits, and restrict the renamed minipal_native_handle wrapper to Windows process and thread handles.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Relocate the cross-platform wait implementation and header under the debugger PAL, rename the types to match debugger conventions, and transfer their build ownership from coreclr minipal.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes debugger/runtime waiting on non-Windows platforms by replacing remaining multi-object PAL wait usage with debugger-owned, cross-platform wait primitives, and by adding shared minipal mutex/condition-variable support that is then used by System.Native’s LowLevelMonitor.

Changes:

  • Introduces debugger PAL WaitHandle/WaitEvent/WaitLatch (and Windows-only NativeHandle) with platform-specific wait implementations (Win32 waits; Unix pipes + poll).
  • Adds minipal condition variable support and a distinct non-recursive mutex type, and migrates System.Native LowLevelMonitor to use them.
  • Migrates multiple debugger and transport wait paths away from legacy PAL multi-waits, including transport session open state synchronization via a condition variable.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.

Show a summary per file
FileDescription
src/native/minipal/mutex.hAdds non-recursive mutex type declarations.
src/native/minipal/mutex.cImplements non-recursive mutex init/enter/leave/destroy.
src/native/minipal/minipalconfig.h.inAdds HAVE_PTHREAD_CONDATTR_SETCLOCK feature define.
src/native/minipal/configure.cmakeDetects pthread library location and pthread_condattr_setclock.
src/native/minipal/conditionvariable.hAdds minipal condition variable API surface.
src/native/minipal/conditionvariable.cImplements minipal condition variables (Win32 + pthread).
src/native/minipal/CMakeLists.txtAdds condition variable source to minipal build.
src/native/libs/System.Native/pal_threading.cSwitches LowLevelMonitor to minipal mutex/condvar.
src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.Unix.csUpdates comments to reflect new native minipal monitor backing.
src/coreclr/inc/cordebug.idlClarifies ICorDebugProcess::GetHandle behavior on Unix vs Windows.
src/coreclr/debug/shared/dbgtransportsession.cppReplaces event-based session-open signaling with condition variable; migrates wait usage.
src/coreclr/debug/inc/debugwait.hIntroduces debugger-owned wait handle abstractions.
src/coreclr/debug/inc/dbgtransportsession.hUpdates transport interfaces and state to use new wait types and condition variable.
src/coreclr/debug/ee/rcthread.cppMigrates helper-thread waits to WaitHandle::Wait and wait-type API.
src/coreclr/debug/ee/debugger.hUpdates favor events and RC thread fields to WaitEvent*.
src/coreclr/debug/ee/debugger.cppMigrates favor event initialization to new wait types.
src/coreclr/debug/di/windowspipeline.cppReturns process wait handle via debugger wait abstraction.
src/coreclr/debug/di/shimremotedatatarget.cppUpdates transport acquisition to return a WaitHandle* and manages ownership.
src/coreclr/debug/di/rspriv.hSwitches stored process “handle” to WaitHandle* and introduces wait-handle accessor.
src/coreclr/debug/di/remoteeventchannel.cppAdjusts event-ack handle types to WaitEvent* and updates transport acquisition ownership.
src/coreclr/debug/di/process.cppMigrates multiple wait sites to WaitHandle::Wait and replaces LSEA with WaitEvent*.
src/coreclr/debug/di/nativepipeline.hUpdates native pipeline process handle contract to WaitHandle*.
src/coreclr/debug/di/localeventchannel.cppWraps event-ack handle in WaitEvent for waiting via the new API.
src/coreclr/debug/di/eventchannel.hUpdates event channel API to return WaitEvent* ack handles.
src/coreclr/debug/di/dbgtransportpipeline.cppMigrates transport pipeline multi-waits to WaitHandle::Wait; tracks synthetic exit event delivery.
src/coreclr/debug/di/dbgtransportmanager.hUpdates transport manager to return WaitHandle** for process-exit waiting.
src/coreclr/debug/di/dbgtransportmanager.cppImplements Unix latch-based process exit signaling and Windows handle wrapping.
src/coreclr/debug/debug-pal/win/wait.cppImplements wait primitives atop Win32 handles and WaitForMultipleObjectsEx.
src/coreclr/debug/debug-pal/unix/wait.cppImplements wait primitives with nonblocking pipes + poll and refcounted waitables.
src/coreclr/debug/debug-pal/CMakeLists.txtBuilds/links new wait sources and configures Unix HAVE_PIPE2 compile defs.
Suppressed comments (1)

src/coreclr/debug/di/process.cpp:3019

  • WaitEvent detachSetThreadContextNeededEvent(m_detachSetThreadContextNeededEvent); has the same Unix hazard as other WaitEvent(existingHandle) sites: if m_detachSetThreadContextNeededEvent is an opaque pointer HANDLE on Unix, this will select WaitEvent(bool) and create a fresh event instead of duplicating the existing one.

After adding a Unix importing ctor/factory for WaitEvent, update this site to use it so the wait set observes the real detach notification.

#ifdef OUT_OF_PROCESS_SETTHREADCONTEXT
WaitEvent detachSetThreadContextNeededEvent(m_detachSetThreadContextNeededEvent);
const WaitHandle *waitSet[] = {
&detachSetThreadContextNeededEvent, // Signaled on every debug event after the first SendCanDetach request
UnsafeGetProcessWaitHandle() // Signaled when the process exits

Comment threadsrc/coreclr/debug/inc/debugwait.h
Comment threadsrc/coreclr/debug/ee/rcthread.cpp
Comment threadsrc/coreclr/debug/di/process.cpp
Comment threadsrc/coreclr/debug/inc/dbgtransportsession.h
CopilotAI review requested due to automatic review settings August 19, 2026 20:13
@jkoritzinsky
jkoritzinskyforce-pushed the dev/jkoritzinsky/prototype-wait-abstraction branch from 85f5f70 to 729a833CompareAugust 19, 2026 20:13

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/coreclr/debug/inc/dbgtransportsession.h:654

  • m_fInitSessionStateCondition is not default-initialized (DbgTransportSession uses a defaulted ctor), but it is read in the destructor and only set on the successful init path. If Init() returns early (e.g., condition-variable init failure), the destructor can call minipal_condition_variable_destroy on uninitialized storage.
 minipal_condition_variable m_sessionStateCondition;
bool m_fInitSessionStateCondition;

src/coreclr/debug/inc/dbgtransportsession.h:693

  • m_rghEventReadyEvent is deleted in the destructor, but the array elements are not default-initialized (DbgTransportSession has a defaulted ctor). If Init() fails before setting these pointers, the destructor can end up deleting indeterminate pointer values.
 WaitEvent *m_rghEventReadyEvent[IPCET_Max]; // The event signalled when a new event arrives

Comment threadsrc/coreclr/debug/inc/dbgtransportsession.h Outdated
Reject accidental Unix PAL handle conversions to WaitEvent and explicitly initialize transport-session teardown guards and owned pointers.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings August 19, 2026 20:37
Copilot stopped reviewing on behalf of jkoritzinsky due to an error August 19, 2026 20:58
@steveisok
steveisok requested a review from a teamAugust 19, 2026 21:07

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me. If there were subtle changes in the synchronization its unlikely I'd catch it but if debugger tests are passing I'm fine to take that risk and deal with it if anything pops up.

Comment threadsrc/coreclr/debug/di/nativepipeline.h Outdated
Comment threadsrc/coreclr/debug/di/rspriv.h Outdated
Comment threadsrc/coreclr/debug/di/rspriv.h Outdated
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
CopilotAI review requested due to automatic review settings August 20, 2026 18:18

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/coreclr/debug/di/dbgtransportmanager.cpp:137

  • The initial Unix liveness probe kill(pid, 0) does not handle EINTR. A signal delivery can cause a spurious failure to attach, even though the process exists. The poller thread already handles EINTR; the probe should match that behavior.
#ifdef HOST_UNIX
if (kill(dwPID, 0) != 0)
{
transport->Shutdown();
return (errno == ESRCH) ? E_INVALIDARG : E_FAIL;

src/coreclr/debug/shared/dbgtransportsession.cpp:748

  • In the failure path for duplicating the reply waitable, this returns a generic E_FAIL. On Windows this can lose the underlying Win32 error from DuplicateHandle, making it hard to diagnose (and inconsistent with the earlier behavior that surfaced the OS error). Capture GetLastError() before deleting the original event and return HRESULT_FROM_WIN32(error) on Windows.
 WaitEvent hReplyEvent(*pMessage->m_hReplyEvent);
if (!hReplyEvent.IsValid())
{
delete pMessage->m_hReplyEvent;
pMessage->m_hReplyEvent = nullptr;

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jkoritzinsky

Copy link
Copy Markdown
MemberAuthor

/ba-g WASM failure unrelated

@jkoritzinsky
jkoritzinsky enabled auto-merge (squash) August 21, 2026 22:15
@jkoritzinsky
jkoritzinsky merged commit cb6d74a into mainAug 21, 2026
151 of 154 checks passed
@jkoritzinsky
jkoritzinsky deleted the dev/jkoritzinsky/prototype-wait-abstraction branch August 21, 2026 22:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Replace usage of multiple events in WaitForMultipleObjectsEx on non-Windows with multi-platform abstraction

3 participants

@jkoritzinsky@noahfalk