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
f7afdef
Compare
d375f27 to
fd686e7
Compare
fd686e7 to
675263d
Compare
| 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;
#128545