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
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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,38 @@ public static int CheckCommandLineArgs(string[] args)

return RemoteExecutor.SuccessExitCode;
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void GetCommandLineArgs_Fallback_Returns()
{
if (PlatformDetection.IsNotMonoRuntime
&& PlatformDetection.IsNotNativeAot
&& PlatformDetection.IsWindows)
{
// Currently fallback command line is only implemented on Windows coreclr
RemoteExecutor.Invoke(CheckCommandLineArgsFallback).Dispose();
}
}

public static int CheckCommandLineArgsFallback()
{
string[] oldArgs = Environment.GetCommandLineArgs();

// 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();
Assert.NotEmpty(args);
Copy link
Member

Choose a reason for hiding this comment

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

Can we compare the return value with the original command line? The new value should be a super-set of the original command line.


// The native command line should be superset of managed command line
foreach (string arg in oldArgs)
{
Assert.Contains(arg, args);
}

return RemoteExecutor.SuccessExitCode;
}
}
}