Skip to content

Replace CoreCLR PAL wait subsystem - #132927

Draft
jkoritzinsky wants to merge 1 commit into
remove-coreclr-pal-semaphoresfrom
dev/jkoritzinsky/ubiquitous-guacamole
Draft

Replace CoreCLR PAL wait subsystem#132927
jkoritzinsky wants to merge 1 commit into
remove-coreclr-pal-semaphoresfrom
dev/jkoritzinsky/ubiquitous-guacamole

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

CoreCLR's remaining Unix native waits are single-handle event waits, so the legacy PAL synchronization manager and its waitable handle infrastructure are no longer needed.

This change:

  • moves the NativeAOT event implementation into src/coreclr/runtime for shared CoreCLR and NativeAOT use, with direct Windows forwarding and a pthread-based Unix implementation;
  • adds PAL_Sleep, backed by Sleep on Windows and EINTR-safe nanosleep on Unix;
  • migrates Unix-reachable CoreCLR event and sleep callers to the shared PAL;
  • adds a Unix-only manual-reset thread-exit event so CoreCLR waits on that event instead of a PAL thread handle, while Windows continues waiting on the OS thread handle; and
  • removes the old PAL event, sleep, thread-wait, synchronization manager, controller, cache, and associated test infrastructure.

Testing

  • ./build.sh clr+libs+host
  • ./build.sh clr.native+clr.paltests
  • PAL test suite: 256 passed, 0 failed

Note

This pull request description was created by GitHub Copilot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

(void)alertable;

assert(count == 1);
assert(!waitAll);

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.

Can we delete these arguments instead?

if (!allowReentrantWait)
{
return WaitForMultipleObjectsEx(handleCount, pHandles, FALSE, timeout, alertable);
return PAL_WaitForMultipleObjectsEx(handleCount, pHandles, false, timeout, alertable);

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 should not be routed through PAL_WaitForMultipleObjectsEx

//
// CLR wrapper around events. This version directly uses Win32 events (there's no support for host
// interception).
// CLR wrapper around native events. This version does not support host interception.

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.

Suggested change
// CLR wrapper around native events. This version does not support host interception.
// CLR wrapper around native events.

Host interception is not a thing anymore

OBJECT
${SOURCES}
)
target_include_directories(coreclrminipal_objects PRIVATE ${CMAKE_BINARY_DIR}/shared_minipal)

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.

Left-over from earlier iteration?

#include "RuntimeEvent.h"
#include "synch.h"

void CLREventBase::CreateAutoEvent (BOOL bInitialState // If TRUE, initial state is signalled

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.

Can we make CLREventBase (renamed to RuntimeEvent or even minipal_event) to be the abstraction with platform-specific implementations to make this more type-safe and flatter? A lot of code in the runtime is coded against CLREvent already.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants

@jkoritzinsky@jkotas