SunOS process and thread support #105403

gwr · 2024-07-24T14:38:56Z

Read binary psinfo for System.Diagnostic.Process on SunOS (Solaris or illumos).
No failures in System.Diagnostic.Process.Tests (but lots of skip)

BTW, I tried rebasing on main from Mon. this week and ran into problems downloading stuff.
Not sure why, but it didn't seem to have anything to do with my changes.

am11 · 2024-07-24T15:37:42Z

Lets drop the changes which are already submitted in other PRs.

src/coreclr/pal/src/exception/signal.cpp

src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFsStat.cs

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessThread.SunOS.cs

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs

am11 · 2024-07-24T15:59:34Z

Good start! I've left initial feedback, which I anticipate the maintainers will point out. :)

gwr · 2024-07-24T18:48:41Z

Good start! I've left initial feedback, which I anticipate the maintainers will point out. :)

Thanks. I had not yet seen #105207 when I opened this. I think @AustinWise and I should figure out how to get the prerequisite fixes shown there (and here) all integrated, and then I'll rebase the last parts of this onto that.

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.SunOS.cs

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.SunOS.cs

gwr · 2024-07-24T21:41:26Z

Lets drop the changes which are already submitted in other PRs.

I'd love to, but I do not see a way to do "stacked" PRs, where one PR targets the branch of another PR.
If I drop the prerequisite changes then things break. Suggestions welcome.

am11 · 2024-07-24T22:18:57Z

Create a local branch feature/illumos-port with all the miscellaneous changes. Then keep this branch only for System.Diagnostics.Port changes. You can git cherry-pick <spaces-separated commit hashes> across the branches.

src/coreclr/runtime/amd64/AllocFast.S

gwr · 2025-06-25T11:48:38Z

Similar feedback as #112323 (comment) - the changes will be reviewed and merged faster if there is one PR per area I/O, process,

I'm happy to convert all the prerequisite commits to pull requests, though that will mean this one will fail to build again until those are merged. Is that what we want?

am11 · 2025-06-25T12:01:26Z

I'm happy to convert all the prerequisite commits to pull requests, though that will mean this one will fail to build again until those are merged. Is that what we want?

You can keep this PR and open another PR with eng/ src/coreclr src/installer src/native/minipal changes: "Misc. illumous build fixes". This PR can be titled something like Port System.Diagnostics.Process to illumos with related changes in src/libraries and src/native/libs.

jkotas · 2025-06-25T12:21:32Z

src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs

+        [StructLayout(LayoutKind.Sequential)]
+        internal unsafe struct @lwpsinfo
+        {
+            private     int     pr_flag;        /* lwp flags (DEPRECATED; do not use) */


What's the license on the file that this was copied from?

https://illumos.org/license/CDDL

https://en.wikipedia.org/wiki/Common_Development_and_Distribution_License#Terms suggests source (not binary) should maintain a copy of the license. We can add it to https://github.com/dotnet/runtime/blob/main/THIRD-PARTY-NOTICES.TXT.

https://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/procfs.h
It's a published interface.

Yea it's fine. Struct definitions and constants usually don't trigger license restrictions. We are doing the same in linux interop code and CDDL is less restrictive than GPL.

We prefer to err on the side of doing more attribution.

This is more than 100 lines copied nearly verbatim, including docs. I think it is above the threshold.

OK, should I just copy the top matter from the illumos procfs.h header?
Or what should the updated top matter look like? Thanks.

BTW, the ideas (and maybe some code) were borrowed from the following files under src/libraries/Common/src/Interop/
OSX/Interop.libproc.GetProcessInfoById.cs
Linux/procfs/Interop.ProcFsStat.TryReadStatusFile.cs

I've tried to follow what other platform files looked like.
I don't see an example of an attribution to copy.
The OSX code is the closest, if that helps.

Would something like this suffice?

diff --git a/src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs b/src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs index 7e7a98bda7c..ec65abb5ee9 100644 --- a/src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs +++ b/src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs @@ -6,7 +6,8 @@ // C# equivalents for <sys/procfs.h> structures. See: struct lwpsinfo, struct psinfo. // We read directly onto these from procfs, so the layouts and sizes of these structures -// must _exactly_ match those in <sys/procfs.h> +// must _exactly_ match those in the illumos <sys/procfs.h> header. The original is: +// https://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/procfs.h // analyzer incorrectly flags fixed buffer length const // (https://github.com/dotnet/roslyn/issues/37593)

The copied (really derived) parts are the two structs:

internal unsafe struct @lwpsinfo { ... 23 members... } internal unsafe struct @psinfo { ... 36 members... }

We could also consider going back to using (a pair of) SystemNative_... functions, one each to read the lwpsinfo and psinfo, which has the advantage that those can be simply C code that uses the actual system header for these (and converts to C# form).
There are pros and cons both ways. I kinda like what's here now, but I'd be open to trying it the other way if reviewers like that better.

I took a stab at the approach using C functions in src/native/libs/System.Native/pal_io.c to read the data.
See: #117098
That avoids the C# code having to "know" details of the procfs structs.
Better?

BTW, I'm having trouble getting the System.Diagnostics.Process.Tests installed.
The xunit.console.dll is missing ... (found. needed to ./build.sh libs.test ...)
Thanks again.

gwr · 2025-06-25T14:16:51Z

Similar feedback as #112323 (comment) - the changes will be reviewed and merged faster if there is one PR per area I/O, process,

OK, now have one prerequsites PR:

#117023 Misc. illumous build fixes (prereq. for #105403)

That commit is here a well because without that the code in this PR will not build.
Once #117023 integrates I can rebase this and remove that commit.

I'm not so familiar with the normal practices here, so please let me know what I can do to organize things for your convenience. Thanks.

src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs

am11 · 2025-06-27T11:14:34Z

That commit is here a well because without that the code in this PR will not build. Once #117023 integrates I can rebase this and remove that commit.

You can have a separate local branch with mixed changes for testing. As far as pull request is concerned, anything unrelated to Port System.Diagnostics.Process to illumos should go into a separate PR. That’s not a “prerequisite”, it’s ongoing maintenance, which is expected in an active repo without CI for illumos. You don’t need to wait on issues that cropped up later or happened in parallel on main. When you opened this PR a year ago, main branch was building fine on illumos. Just keep this focused and moving forward, otherwise it's a never ending chore.

src/libraries/System.Private.CoreLib/src/System/Environment.SunOS.cs

src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFs.Definitions.cs

gwr · 2025-06-28T13:13:33Z

I updated this to use src/native/libs/System.Native C functions, which means we don't need to replicate the two procfs.h structures in the C# platform code. It should also be a bit more efficient. This works as well for me as the previous version I had in this PR.

I also got rid of the changes in other PR(s), but note that this doesn't build for illumos without the changes in #117023

src/native/libs/System.Native/pal_io.c

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessThread.cs

Read /proc (binary) psinfo for System.Diagnostic.Process using src/native/libs/System.Native C functions. Add native/libs/System.Native/pal_io.c etc. Add src/libraries/Common/src/Interop/SunOS/procfs Add src/libraries/System.Diagnostics.Process Co-authored-by: Austin Wise <AustinWise@gmail.com> Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

gwr · 2025-07-02T01:37:51Z

Needs approvers. @am11 ?

ghost added the area-System.Diagnostics.Process label Jul 24, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2024

gwr mentioned this pull request Jul 24, 2024

Tracking issue for illumos and Solaris x86-64 port work #34944

Open

19 tasks