Uh oh!
There was an error while loading. Please reload this page.
Implement GetCommandLineArgs fallback on Unix - #131431
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @dotnet/area-system-runtime |
aa575d4 to
f7afdefCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
d375f27 to
fd686e7Comparefd686e7 to
675263dCompareUh oh!
There was an error while loading. Please reload this page.
| return resultArgv; | ||
| #elif defined(__linux__) || defined(__sun) | ||
| int fd = open("/proc/self/cmdline", O_RDONLY); |
There was a problem hiding this comment.
I don't see a good reason why this, or really much of anything else in the file, needs to be written in C. I think we could use standard platform pinvokes (or even just opening the proc fs in this case) from C#.
There was a problem hiding this comment.
There are number of varied syscalls for each platform; it'd require exposing them all to managed side just to move the implementation there as opposed to having it here next to getexepath.c (which also uses approximately same syscalls) with a single P/Invoke.
There was a problem hiding this comment.
We have most of the required syscalls exposed in managed code already. I do not think it would be too bad to expose the rest.
However, I agree with your point that moving the whole implementation to C# would just be more complicated. We had faced similar tradeoff in the native shims. https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/interop-guidelines.md touches on it: "At first, it seemed that we'd want to use 1:1 names throughout, but it turns out there are many cases where being strictly 1:1 isn't practical."
having it here next to getexepath.c (which also uses approximately same syscalls)
It is similar code, but it is not a good general-purpose functionality that the minipal should be about. I do not think we would expose it at all if we were starting from scratch. The reason for fixing the linked issue is combination of:
- it works on Windows for historic reasons
- parity between OSes is good
- introducing breaking change on Windows to achieve the parity is not worth it
I think it would be better for this to live in the System.Native shim directly as I have mentioned in the other comment.
There was a problem hiding this comment.
Pull request overview
Implements a Unix/WASI/browser fallback path for Environment.GetCommandLineArgs() when Environment.s_commandLineArgs isn’t initialized (hosted-library scenario), by adding a System.Native entrypoint that retrieves argv via a new minipal_getcmdline helper and wiring it into CoreLib’s GetCommandLineArgsNative().
Changes:
- Add
minipal_getcmdlineand expose it viaSystemNative_GetCommandLine/SystemNative_FreeCommandLine. - Update CoreLib Unix/Browser implementation to call the new System.Native interop and remove the prior stubbed-out implementation.
- Broaden the fallback test coverage beyond “Windows CoreCLR only” (still excluding Mono).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/minipal/getcmdline.h | New minipal helper that attempts to obtain argv across multiple Unix-like targets (plus WASI/browser). |
| src/native/libs/System.Native/pal_process.h | Declares new SystemNative_GetCommandLine / SystemNative_FreeCommandLine exports. |
| src/native/libs/System.Native/pal_process.c | Implements the new exports by delegating to minipal_getcmdline. |
| src/native/libs/System.Native/pal_process_wasi.c | Implements the new exports for WASI builds. |
| src/native/libs/System.Native/entrypoints.c | Registers the new exports for System.Native DllImport dispatch. |
| src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetCommandLine.cs | Adds LibraryImport declarations for SystemNative_GetCommandLine / SystemNative_FreeCommandLine. |
| src/libraries/System.Private.CoreLib/src/System/Environment.UnixOrBrowser.cs | Adds a Unix/Browser GetCommandLineArgsNative() implementation that calls the new interop. |
| src/libraries/System.Private.CoreLib/src/System/Environment.Unix.cs | Removes the old stub GetCommandLineArgsNative() implementation. |
| src/libraries/System.Private.CoreLib/src/System/Environment.Browser.cs | Removes the old Browser stub for GetCommandLineArgsNative(). |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Includes the new interop source file in CoreLib build items. |
| src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.GetCommandLineArgs.cs | Updates conditions so fallback tests run on non-Mono (and parser tests run on Windows non-Mono). |
Suppressed comments (1)
src/native/minipal/getcmdline.h:83
- The /proc/self/cmdline reader doesn't check whether the initial malloc succeeded, and it doesn't handle read() returning -1 (including EINTR). As written, a read error after partial data can silently return a truncated/invalid argv buffer, and a NULL buf would be dereferenced.
size_t bufSize = 1024;
char* buf = (char*)malloc(bufSize);
size_t totalBytes = 0;
ssize_t bytesRead;
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
#128545