Fix env::ArgsOs for zkVM

[thaliaarchi]

The zkVM implementation of env::ArgsOs incorrectly reports the full length even after having iterated. Instead, use a range approach which works out to be simpler. Also, implement more iterator methods like the other platforms in #139847.

cc @flaub @jbruestle @SchmErik

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[thaliaarchi]

This PR fixes the iterator, but leaves the arg retrieval and copying the same. There are some interesting alternatives to be considered there.

rust/library/std/src/sys/args/zkvm.rs

Lines 21 to 38 in 2da29db

	fn argv(i: usize) -> OsString {
	let arg_len = unsafe { abi::sys_argv(crate::ptr::null_mut(), 0, i) };
	let arg_len_words = (arg_len + WORD_SIZE - 1) / WORD_SIZE;
	let words = unsafe { abi::sys_alloc_words(arg_len_words) };
	let arg_len2 = unsafe { abi::sys_argv(words, arg_len_words, i) };
	debug_assert_eq!(arg_len, arg_len2);
	// Convert to OsString.
	//
	// FIXME: We can probably get rid of the extra copy here if we
	// reimplement "os_str" instead of just using the generic unix
	// "os_str".
	let arg_bytes: &[u8] =
	unsafe { crate::slice::from_raw_parts(words.cast() as *const u8, arg_len) };
	OsString::from_inner(os_str::Buf { inner: arg_bytes.to_vec() })
	}

It looks like the memory allocated by sys_alloc_words is never freed. If that's intentional and it's 'static, then we could retrieve all the args upon construction of the first Args and store them in a OnceLock<Vec<&'static OsStr>>, then copy them to OsString on demand. However, sys_argv documents that the host controls what is returned. Is caching then inappropriate?

As for the comment about reimplementing OsString, I don't think that's a good idea. It uses one of two abstractions over Vec<u8> for the supported encodings. Although the encoding is platform-dependent, the abstractions are platform-independent. I don't like the idea of adding platform-specific logic there. It seems that the desire is to manage allocations differently with OsString. Can that currently be done with stock Vec<u8>? I'd think such logic should go there, then OsString would work.

[thaliaarchi]

I've now implemented the caching approach. The two commits are complete fixes with different approaches, so they should be considered separately until we decide.

[bors]

☔ The latest upstream changes (presumably #140581) made this pull request unmergeable. Please resolve the merge conflicts.

[flaub]

I think we'd like to test this out somehow before accepting this. It doesn't look like it will cause a problem, but I'm not entirely sure.

[Enselic]

Triage: ping @thaliaarchi , any update on adding tests for this?

[thaliaarchi]

@Enselic Oh, I had interpreted @flaub's comment differently 😅. I thought "we'd like to test this" meant zkVM maintainers would run the existing std test suite for this PR on zkVM. The classic inclusive/exclusive ambiguity in English "we".

Disappointingly, the only tests for Args/ArgsOs are for their Debug impls. I don't have the bandwidth to write a full test suite for these right now and also don't have a setup to run tests for zkVM anyways.

Aside from the general lack of tests, specific to this PR, there are two new tests that would be useful: Testing that the length is reported correctly while iterating (fixed in the first commit). This could be a new test for all platforms and I wouldn't mind adding this. And testing that successive constructions of the iterators will return the same args, since the host could theoretically non-deterministically return different args (fixed in the second commit). I have no idea how to setup this test for zkVM or whether it's a threat model worth considering.

[thaliaarchi]

@Enselic Given the approval by a target maintainer, I think this is ready now

[Enselic]

Ping @ibraheemdev (see comment right above)

[ibraheemdev]

@bors r+ rollup

[bors]

📌 Commit 0009598 has been approved by ibraheemdev

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0009598 with merge 4645a79...

[bors]

☀️ Test successful - checks-actions
Approved by: ibraheemdev
Pushing 4645a79 to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 5d1b897 (parent) -> 4645a79 (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 4645a7988177c286f61609cc667ecae4c571a2e8 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-linux: 8608.3s -> 6470.3s (-24.8%)
2. x86_64-gnu-llvm-20: 2830.4s -> 2407.5s (-14.9%)
3. pr-check-2: 2500.5s -> 2142.7s (-14.3%)
4. i686-gnu-2: 6448.9s -> 5528.6s (-14.3%)
5. dist-aarch64-apple: 7447.9s -> 8500.3s (14.1%)
6. x86_64-gnu-llvm-20-1: 3779.3s -> 3264.6s (-13.6%)
7. x86_64-rust-for-linux: 2992.0s -> 2596.0s (-13.2%)
8. aarch64-msvc-2: 4847.2s -> 5465.3s (12.8%)
9. i686-gnu-1: 8518.7s -> 7440.7s (-12.7%)
10. pr-check-1: 1558.7s -> 1379.4s (-11.5%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (4645a79): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 9.3%, secondary 3.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	9.3%	[9.3%, 9.3%]	1
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	9.3%	[9.3%, 9.3%]	1

Cycles

Results (secondary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.7%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 471.616s -> 471.629s (0.00%)
Artifact size: 387.94 MiB -> 387.93 MiB (-0.00%)