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.

dotnet-policy-service · 2024-07-24T14:39:28Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

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

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

am11 · 2024-07-24T15:41:41Z

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

As there are different teams owning these areas, CoreCLR (runtime) changes should be in separate PR than libraries. Also, please run the PAL tests against this change when you open a separate PR.

OK, I figured we might need to do that. Do all 8 commits need to be separate PRs? Or can some be combined?

I think everything which is not related to "System.Diagnostics.Process SunOS port" should be in separate PR. i.e. keep src/libraries and src/native/libs changes and move the rest.

am11 · 2024-07-24T15:46:45Z

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

+
+internal static partial class Interop
+{
+    internal static partial class @procfs


Is it missing the functionality or here to-be-filled in the future? In case it's the latter, lets delete it for now.

It looks like the original idea was to split up that code. That may still make sense later -- I'm not sure.
I find this file name: Interop.ProcFsStat.TryReadProcessStatusInfo.cs to be annoyingly long.
How about I delete that and put this all in Interop.ProcFsStat.cs instead?

Based on https://github.com/dotnet/runtime/blob/dff1d8467845fd93c517f89dc81598e5fd17c270/docs/coding-guidelines/interop-guidelines.md, we can move:

struct definitions in Interop.ProcFs.Definitions.cs

then for each internal method like TryReadProcessStatusInfo, TryReadProcessThreadInfo etc. create a separate Interop.ProcFs.XX.cs file.

Linux procfs is not using this approach because it wasn't upgraded when guidelines were written, but for new platform port, we can follow the guidelines to keep things tidy.

If something (struct, const or a method method) is not used outside the @procfs class, change internal to private.

Based on https://github.com/dotnet/runtime/blob/dff1d8467845fd93c517f89dc81598e5fd17c270/docs/coding-guidelines/interop-guidelines.md, we can move:

struct definitions in Interop.ProcFs.Definitions.cs

then for each internal method like TryReadProcessStatusInfo, TryReadProcessThreadInfo etc. create a separate Interop.ProcFs.XX.cs file.

I tried moving some of the structs and stuff like RootPath but then the C# compiler did not "know about" the structures in the other file. Any tips on how to do that? Thanks!

Did you add lines for new files in src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj?

Yes, and it seems like that *.csproj file is being ignored. New files I add are apparently ignored. Files I've removed are still being looked for, and I don't see why. Eg.

CSC : error CS2001: Source file '/home/gwr/dotnet/runtime/src/libraries/Common/src/Interop/SunOS/procfs/Interop.ProcFsStat.TryReadProcessStatusInfo.cs' could not be found. [/home/gwr/dotnet/runtime/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj]

What am I missing?

Oh, found it:

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

am11 · 2024-07-24T15:48:39Z

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

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-maccatalyst;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-maccatalyst;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)</TargetFrameworks>


Lets keep then in sync for now:

Suggested change

<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-maccatalyst;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)</TargetFrameworks>

<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-maccatalyst;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)-solaris;$(NetCoreAppCurrent)</TargetFrameworks>

am11 · 2024-07-24T15:48:48Z

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

@@ -363,6 +363,19 @@
             Link="Common\Interop\FreeBSD\Interop.Process.GetProcInfo.cs" />
  </ItemGroup>

+  <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'illumos' or '$(TargetPlatformIdentifier)' == 'sunos'">


Suggested change

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'illumos' or '$(TargetPlatformIdentifier)' == 'sunos'">

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'illumos' or '$(TargetPlatformIdentifier)' == 'solaris'">

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

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

am11 · 2024-07-24T15:56:02Z

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

@@ -629,7 +629,7 @@ public void TestMinWorkingSet()
                Assert.InRange((long)p.MinWorkingSet, 0, long.MaxValue);
            }

-            if (OperatingSystem.IsMacOS() || OperatingSystem.IsFreeBSD()) {
+            if (PlatformDetection.IsBsdLike || PlatformDetection.IsSunOS) {


IsBsdLike includes more platforms than this code was tested against. I'd keep the existing part of the condition untouched.

Are your sure? I looked at the implementation of IsBsdLike() and it appeared to me that all the platforms that includes use the same get/set Min/Max WorkingSet code that only supports acting on the current process.

OperatingSystem.IsMacOS() || OperatingSystem.IsFreeBSD()
does not include iOS, macCatalyst, NetBSD etc. which IsBsdLike is pulling.

Besides, anything which is not part of the port work is best left for other PRs. This is to make the review process easier for all participants.

am11 · 2024-07-24T15:56:09Z

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

@@ -574,7 +574,7 @@ public void TestMaxWorkingSet()
                Assert.InRange((long)p.MinWorkingSet, 0, long.MaxValue);
            }

-            if (OperatingSystem.IsMacOS() || OperatingSystem.IsFreeBSD()) {
+            if (PlatformDetection.IsBsdLike || PlatformDetection.IsSunOS) {


am11 · 2024-07-24T15:58:10Z

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

@@ -57,6 +57,8 @@ private static IntPtr ProcessorAffinityCore
        /// </summary>
        private void GetWorkingSetLimits(out IntPtr minWorkingSet, out IntPtr maxWorkingSet)
        {
+            EnsureState(State.HaveNonExitedId);


This looks like a good change if you have tested it on FreeBSD, but it would be best to submit it as a separate improvement and not part of other platform port.

Right, though it's pretty simple. The test is complaining that we throw the wrong exception code.
This makes it throw the right code for a process object that has not yet started.

I'm not in a position test on FreeBSD at the moment. Are there FreeBSD people around who could help me?

I'm not in a position test on FreeBSD at the moment. Are there FreeBSD people around who could help me?

Which is why it is best to put it in a separate PR, so someone can test before approving it.

Opened an issue and a PR
#105422
#105424

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.

Read binary psinfo for System.Diagnostic.Process on SunOS Thanks for initial prototype help from: Austin Wise <AustinWise@gmail.com>

dotnet-issue-labeler bot 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