Inline CreateProcessW impl at callsites and cleanup

[am11]

#126888 (comment)

[AaronRobinsonMSFT]

/cc @janvorli

[jkotas]

I see the following callers of CreateProcess:

src\coreclr\debug\di\dbgtransportmanager.cpp(175):BOOL result = WszCreateProcess(lpApplicationName,

• This one is the only one that looks reachable on Unix that we need to be concerned about. @AaronRobinsonMSFT Do you happen to know whether it is actually needed on Unix? There is similar thing in dbgshim. Do we need both?

I would push all the implementation to here if needed. Also, this is the only place that uses OpenProcess that should be deleted and moved to here as well.

I do not think that the technique to use shell to launch the process is going to work here. The debugger is not going to be happy about the intermediate process.

src\coreclr\debug\di\windowspipeline.cpp(145):BOOL ret = ::WszCreateProcess(

• Windows specific.

src\coreclr\debug\ee\debugger.cpp(6661):fCreateSucceeded = WszCreateProcess(NULL, const_cast<WCHAR*> (wszDbgCommand),

• Windows specific.

src\coreclr\vm\excep.cpp(3412):if (WszCreateProcess(NULL, lpCommandLine, NULL, NULL, TRUE, 0, NULL, NULL, &StartupInfo, &processInformation))

• Windows specific

src\coreclr\vm\autotrace.cpp:

• Dead code

[huoyaoyuan]

• This one is the only one that looks reachable on Unix that we need to be concerned about. @AaronRobinsonMSFT Do you happen to know whether it is actually needed on Unix? There is similar thing in dbgshim. Do we need both?

I looked this earlier and it was touched in dotnet/coreclr#21068.

[am11]

src\coreclr\vm\autotrace.cpp:

* Dead code

Once #127604 (comment) is confirmed, we can delete it wholesale.

[am11]

@janvorli, any concerns about using sh -c? Alternatively, I can bring over the splitter code from the first commit (a cleaned-up version from coreclr/pal). I have it staged (+145 lines) but wanted to check if we should avoid shelling out. CI looks fine either way.

[AaronRobinsonMSFT]

This one is the only one that looks reachable on Unix that we need to be concerned about. @AaronRobinsonMSFT Do you happen to know whether it is actually needed on Unix? There is similar thing in dbgshim. Do we need both?

Sadly this is a huge rat's nest - "we hates it!" 😱 The dbgshim entry point (CreateProcessForLaunch) can/should be used for most things, but this logic is also exposed in the RS (DBI) and triggered based on the W32ETA_CREATE_PROCESS message. The DbgTransportTarget in the RS is currently only functional, let alone buildable, for non-Windows. The CreateProcessForLaunch in dbgshim is used for what I would call the "simple" sequence of launching suspended and then attaching in the dance to start-up and debug a process.

The above DbgTransportTarget::CreateProcess is only called via DbgTransportPipeline::CreateProcessUnderDebugger, which is only called in CordbWin32EventThread::CreateProcess, which is only called in CordbWin32EventThread::Win32EventLoop() under the W32ETA_CREATE_PROCESS message, which can be called from the RS (DBI) (that is, ICorDebug::CreateProcess).

It isn't clear why one would use the former or the latter because the entire contract of dbgshim and ICorDebug is simply unintelligible. Both are "functional" but why one would be taken versus the other is something I defer to the diagnostics team. My experiments relied on the dbgshim entry point and ICorDebug::DebugActiveProcess and works without issue on all platforms. Creating a process via ICorDebug might be useful somewhere, but I'm not sure where or why.

[AaronRobinsonMSFT]

I do not think that the technique to use shell to launch the process is going to work here. The debugger is not going to be happy about the intermediate process.

+1

but wanted to check if we should avoid shelling out. CI looks fine either way.

This CI doesn't run the code in question. Sorry. This is something we'd need to validate in another repo.

/cc @dotnet/dotnet-diag @steveisok

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[jkotas]

@am11 I think the best way to make progress is to add return E_NOTIMPL to DbgTransportTarget::CreateProcess and do some ad-hoc testing of Visual Studio debugger on Linux to see whether this path is used there. If not, chances are this can be all deleted.

[noahfalk]

@am11 I think the best way to make progress is to add return E_NOTIMPL to DbgTransportTarget::CreateProcess and do some ad-hoc testing of Visual Studio debugger on Linux to see whether this path is used there. If not, chances are this can be all deleted.

I believe DbgTransportTarget::CreateProcess is dead code, as is the ICorDebug::CreateProcess API that leads to it. @jkotas' suggested test will be good confirmation. When we moved to .NET Core circa 2015 dbgshim's CreateProcessForLaunch API became the supported way to launch a .NET Core app for debugging. Mscordbi doesn't load until after the debuggee is already running. The only reason DbgTransportTarget::CreateProcess appears in the cross platform code is because it was part of Mac Silverlight work that was also cross-platform but predates .NET Core.