Conversation
dotnet-policy-service
bot
added
the
community-contribution
| // exePath contains the full path to the executable. | ||
| MAKE_WIDEPTR_FROMUTF8(exePath, minipal_getexepath()); | ||
| SIZE_T commandLineLen = (u16_strlen(exePath) + 1); |
There was a problem hiding this comment.
This is a potential behavioral change. Now it will always use the executable path of current process, instead of exePath from coreclr_initialize. This is only used through ep_rt_diagnostics_command_line_get.
| #ifdef TARGET_WINDOWS | ||
| // Use the result from GetCommandLineW() instead | ||
| pCmdLine = GetCommandLineW(); | ||
| #endif // HOST_WINDOWS |
There was a problem hiding this comment.
This could affect ep for hosted dll. In nativeaot /proc/self/cmdline is used.
What's the desired semantic here?
There was a problem hiding this comment.
A couple scenarios that his this:
-
Access violation - coreclr!EventPipeEventSource::SendProcessInfo+0x135 #12656 - This was the issue the fallback was added to fix. Presumably unless we find some alternate solution then this PR would re-introduce that issue which we wouldn't want. I don't know that there is a well documented semantic but the low risk approach is that we would preserve the pre-existing behavior exactly. If we don't think we can do that, or we don't want to do that then it would be good to understand what are we gaining that would justify the compat risk.
-
There are GetProcessInfo IPC commands (multiple variants) which also call into this code:
server_thread
-> server_loop_tick
-> ds_process_protocol_helper_handle_ipc_message
-> process_protocol_helper_get_process_info // command 0x00
process_protocol_helper_get_process_info_2 // command 0x04
process_protocol_helper_get_process_info_3 // command 0x08
-> ep_rt_diagnostics_command_line_get
-> GetCommandLineForDiagnostics
Those commands have documented behavior here. The expectation is that those commands are legal for tools to call as soon as the IPC server starts up (dotnet-monitor is one use-case). I believe that occurs before GetManagedCommandLine() is populated so those commands probably come down this fallback path as well and returning NULL won't work.
There was a problem hiding this comment.
OK. Since the documentation specifies argv[0], I assume the OS-native executable path should be desirable.
|
@dotnet/dotnet-diag-contrib for the EventPipe impact |
|
What is the motivation behind doing the change? |
|
The motivation is to cleanup PAL layer and transit to minipal gradually. |
src/coreclr/vm/ceeload.cpp
Outdated
| // Use the result from GetCommandLineW() instead | ||
| pCmdLine = GetCommandLineW(); | ||
| #else | ||
| pCmdLine = W(""); |
There was a problem hiding this comment.
I don't think we can just return a sentinel. We need to return similar results to what GetCommandLineW() would have returned before the change. I'm not sure if that means adjusting the timeline so that GetManagedCommandLine() gets populated earlier or maybe it means something similar to GetCommandLineW() needs to stay in the PAL.
4 participants
No description provided.