Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert fallback path of GetCommandLineArgs to managed #70608

Merged
merged 10 commits into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ public static extern int ExitCode
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern void FailFast(string? message, Exception? exception, string? errorMessage);

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern string[] GetCommandLineArgsNative();

public static string[] GetCommandLineArgs()
{
// There are multiple entry points to a hosted app. The host could
Expand Down
37 changes: 0 additions & 37 deletions src/coreclr/classlibnative/bcltype/system.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,43 +90,6 @@ FCIMPL0(INT32, SystemNative::GetExitCode)
}
FCIMPLEND

FCIMPL0(Object*, SystemNative::GetCommandLineArgs)
{
FCALL_CONTRACT;

PTRARRAYREF strArray = NULL;

HELPER_METHOD_FRAME_BEGIN_RET_1(strArray);

LPWSTR commandLine;

commandLine = WszGetCommandLine();
if (commandLine==NULL)
COMPlusThrowOM();

DWORD numArgs = 0;
LPWSTR* argv = SegmentCommandLine(commandLine, &numArgs);
Copy link
Member

Choose a reason for hiding this comment

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

The only other use of SegmentCommandLine is in ildasm that can be eliminated by just using the arguments passed into the main method. It would allow us to delete the SegmentCommandLine method.

Copy link
Member

Choose a reason for hiding this comment

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

If ilasm and ildasm could share the same main(), that would also cleanup a lot of code indirection. They are logically doing the same thing (initialize PAL on Unix, and switch over command line args etc.), but ilasm's main implementation is much neater than that of ildasm's.

(in a larger picture, corehost and corerun are also candidate of this consolidations, currently they both have their own PAL)

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jun 11, 2022

Choose a reason for hiding this comment

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

Yes I mentioned this, but I'm not sure if they should be in one PR.
Ideally, many mains can share more code, which is some how out of scope.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, many mains can share more code, which is some how out of scope.

There is infinite amount of cleanup one can do in ilasm/ildasm. I would rather look into C# rewrite than trying to cleanup the existing C/C++ code.

Copy link
Member

Choose a reason for hiding this comment

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

@jkoritzinsky and I have talked about a ilasm/ildasm rewrite for a myriad of reasons. There are many concerns with that but I agree effort to clean-up both code bases is low priority.

if (!argv)
COMPlusThrowOM();

_ASSERTE(numArgs > 0);

strArray = (PTRARRAYREF) AllocateObjectArray(numArgs, g_pStringClass);
// Copy each argument into new Strings.
for(unsigned int i=0; i<numArgs; i++)
{
STRINGREF str = StringObject::NewString(argv[i]);
STRINGREF * destData = ((STRINGREF*)(strArray->GetDataPtr())) + i;
SetObjectReference((OBJECTREF*)destData, (OBJECTREF)str);
}
delete [] argv;

HELPER_METHOD_FRAME_END();

return OBJECTREFToObject(strArray);
}
FCIMPLEND

// Return a method info for the method were the exception was thrown
FCIMPL1(ReflectMethodObject*, SystemNative::GetMethodFromStackTrace, ArrayBase* pStackTraceUNSAFE)
{
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/classlibnative/bcltype/system.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ class SystemNative
static FCDECL1(VOID,SetExitCode,INT32 exitcode);
static FCDECL0(INT32, GetExitCode);

static FCDECL0(Object*, GetCommandLineArgs);
static FCDECL1(VOID, FailFast, StringObject* refMessageUNSAFE);
static FCDECL2(VOID, FailFastWithExitCode, StringObject* refMessageUNSAFE, UINT exitCode);
static FCDECL2(VOID, FailFastWithException, StringObject* refMessageUNSAFE, ExceptionObject* refExceptionUNSAFE);
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/ecalllist.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ FCFuncStart(gEnvironmentFuncs)
FCFuncElement("get_TickCount64", SystemNative::GetTickCount64)
FCFuncElement("set_ExitCode", SystemNative::SetExitCode)
FCFuncElement("get_ExitCode", SystemNative::GetExitCode)
FCFuncElement("GetCommandLineArgsNative", SystemNative::GetCommandLineArgs)

FCFuncElementSig("FailFast", &gsig_SM_Str_RetVoid, SystemNative::FailFast)
FCFuncElementSig("FailFast", &gsig_SM_Str_Exception_RetVoid, SystemNative::FailFastWithException)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;

internal static unsafe partial class Interop
{
internal static partial class Kernel32
{
[LibraryImport(Libraries.Kernel32, EntryPoint = "GetCommandLineW")]
internal static partial char* GetCommandLine();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;

internal static unsafe partial class Interop
{
internal static partial class Shell32
{
[LibraryImport(Libraries.Shell32, EntryPoint = "CommandLineToArgvW")]
internal static partial char** CommandLineToArgv(char* lpCommandLine, int* pNumArgs);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1558,6 +1558,9 @@
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GET_FILEEX_INFO_LEVELS.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GET_FILEEX_INFO_LEVELS.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetCommandLine.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetCommandLine.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetComputerName.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetComputerName.cs</Link>
</Compile>
Expand Down Expand Up @@ -1828,6 +1831,9 @@
<Compile Include="$(CommonPath)Interop\Windows\Secur32\Interop.GetUserNameExW.cs">
<Link>Common\Interop\Windows\Secur32\Interop.GetUserNameExW.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Shell32\Interop.CommandLineToArgv.cs">
<Link>Common\Interop\Windows\Shell32\Interop.CommandLineToArgv.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Shell32\Interop.SHGetKnownFolderPath.cs">
<Link>Common\Interop\Windows\Shell32\Interop.SHGetKnownFolderPath.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,13 @@ public static string MachineName

[MethodImplAttribute(MethodImplOptions.NoInlining)] // Avoid inlining PInvoke frame into the hot path
private static string? GetProcessPath() => Interop.Sys.GetProcessPath();

private static string[] GetCommandLineArgsNative()
{
// This is only used for delegate created from native host

// Consider to use /proc/self/cmdline to get command line
return Array.Empty<string>();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.IO;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -180,5 +181,32 @@ public static unsafe long WorkingSet
return (long)memoryCounters.WorkingSetSize;
}
}

private static unsafe string[] GetCommandLineArgsNative()
{
char* lpCmdLine = Interop.Kernel32.GetCommandLine();
jkotas marked this conversation as resolved.
Show resolved Hide resolved
Debug.Assert(lpCmdLine != null);

int numArgs = 0;
char** argvW = Interop.Shell32.CommandLineToArgv(lpCmdLine, &numArgs);
if (argvW == null)
{
ThrowHelper.ThrowOutOfMemoryException();
}

try
{
string[] result = new string[numArgs];
for (int i = 0; i < result.Length; i++)
{
result[i] = new string(*(argvW + i));
}
return result;
}
finally
{
Interop.Kernel32.LocalFree((IntPtr)argvW);
}
}
}
}
37 changes: 37 additions & 0 deletions src/tests/Interop/CommandLine/CommandLineTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
jkotas marked this conversation as resolved.
Show resolved Hide resolved
using System.IO;
using System.Reflection;
using TestLibrary;
using Xunit;

namespace CommandLineTest
{
class Program
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer this to follow the [Fact] pattern we are moving towards.

Removing the namespace, making the class public, renaming to CommandLineTest and then renaming int Main() as follows should be all that is needed. Also, removing the return as this will all be generated for you.

[Fact]
public void Validate_CommandLineArgsSet()

Copy link
Member

Choose a reason for hiding this comment

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

This test does not really need to be under src/tests. It can be under libraries (and using remote executor). It is a test for Environment.GetCommandLineArgs() library API after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that under xunit we may have much less control of the actual command line of the process. Can xunit be run in-process by Visual Studio?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT What's your thought about the expected value here? I'm afraid the only thing we can test is not empty, which looks not enough.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Jun 18, 2022

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT What's your thought about the expected value here? I'm afraid the only thing we can test is not empty, which looks not enough.

In general, I agree. However, the libraries teams have traditionally made an effort to call every API even if it is only to verify it always throws or returns nothing. It does make for some odd tests, but is helpful in ensuring the API works enough to invoke. More to the point, I think it is already done https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Extensions/tests/System/Environment.GetCommandLineArgs.cs.

Once I update CoreShim to be more useful we can create the more robust suite I think we all want.

{
int Main()
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be marked static, but I'd prefer making it a Fact.

{
// Currently native command line is only implemented on CoreCLR Windows
if (!TestLibrary.Utilities.IsMonoRuntime && !TestLibrary.Utilities.IsNativeAot)
{
// Clear the command line args set for managed entry point
var field = typeof(Environment).GetField("s_commandLineArgs", BindingFlags.Static | BindingFlags.NonPublic);
Assert.NotNull(field);
field.SetValue(null, null);

string[] args = Environment.GetCommandLineArgs();
if (OperatingSystem.IsWindows())
{
// The command line should be "corerun assemblyname.dll" for coreclr test
Assert.Equal(2, args.Length);
Assert.Equal("corerun", Path.GetFileNameWithoutExtension(args[0]));
Assert.Equal(typeof(Program).Assembly.GetName().Name, Path.GetFileNameWithoutExtension(args[1]));
}
}

return 100;
}
}
}
12 changes: 12 additions & 0 deletions src/tests/Interop/CommandLine/CommandLineTest.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="CommandLineTest.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
</ItemGroup>
</Project>