Skip to content

Inline remaining usages of OpenProcess on Unix#127795

Open
am11 wants to merge 7 commits intodotnet:mainfrom
am11:chore/cleanup-pal-openproc
Open

Inline remaining usages of OpenProcess on Unix#127795
am11 wants to merge 7 commits intodotnet:mainfrom
am11:chore/cleanup-pal-openproc

Conversation

@am11
Copy link
Copy Markdown
Member

@am11 am11 commented May 5, 2026

Two usages of OpenProcess and one of OpenProcessMemory.

Follow-up: #127604 (comment).

@github-actions github-actions Bot added the area-PAL-coreclr only for closed issues label May 5, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 5, 2026
@am11 am11 requested review from janvorli and jkotas May 5, 2026 12:48
Comment thread src/coreclr/pal/inc/pal.h Outdated
Comment thread src/coreclr/pal/inc/pal.h Outdated
Comment thread src/coreclr/pal/src/synchmgr/synchmanager.cpp Outdated
@jkotas jkotas added area-Diagnostics-coreclr and removed area-PAL-coreclr only for closed issues labels May 5, 2026
@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.

Comment thread src/coreclr/debug/di/dbgtransportmanager.cpp Outdated
@am11 am11 marked this pull request as ready for review May 5, 2026 18:09
Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Comment thread src/coreclr/debug/ee/debugger.cpp Outdated

#ifdef TARGET_UNIX
// JIT-attach via CreateProcess + waitable process handle is Windows-only. Returning a
// failing HRESULT lets the calling code fall back to the standard hosting failure path.
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.

What is the "standard hosting failure path" that this is talking about?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated for clarification. It uses "no debugger attached" / unwind path on Unix.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented May 6, 2026

@am11 Have you done any ad-hoc testing on this to validate that the debugging is not broken?

@janvorli @tommcdon Do you think it is worth it to run some of the internal diagnostics tests on this PR before merging?

Copy link
Copy Markdown
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Looks great otherwise

@am11
Copy link
Copy Markdown
Member Author

am11 commented May 6, 2026

@am11 Have you done any ad-hoc testing on this to validate that the debugging is not broken?

I have done vscode testing on macOS, killing the process when debugger is running does give the same behavior as before. Though I'm not sure if that's conclusive / sufficient amount of testing? 👀

@janvorli
Copy link
Copy Markdown
Member

janvorli commented May 6, 2026

@janvorli @tommcdon Do you think it is worth it to run some of the internal diagnostics tests on this PR before merging?

I'll run the private diagnostic tests with it.

@am11
Copy link
Copy Markdown
Member Author

am11 commented May 6, 2026

@janvorli, thanks for looking into it. I'm working on part B of this refactoring which will be in the next PR. It involves inlining wait subsystem usage at the callsite while deduplicating stuff, with massive cleanups in PAL; code would end up using the same familiar pthread APIs with minimum synchronization work. The sophisticated part of this Win32 emulation has been largely unused ever since the managed wait subsystem landed in .NET 11, so technically the remaining few native usages don't need to go through the PAL layer at all.

Heads up that round 2 will likely need your and Tom's help running the internal diagnostics test suites against it. 🙂

@janvorli
Copy link
Copy Markdown
Member

janvorli commented May 6, 2026

@am11 that is awesome!

@janvorli
Copy link
Copy Markdown
Member

janvorli commented May 6, 2026

@am11 the diagnostic tests have the same pass rate with and without your change.

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

Labels

area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants