-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Fix env::ArgsOs for zkVM
#139849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix env::ArgsOs for zkVM
#139849
Conversation
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
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
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.
93a672a to
66c407d
Compare
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.
☔ The latest upstream changes (presumably #140581) made this pull request unmergeable. Please resolve the merge conflicts.
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 rust-lang#139847.
Retrieve argc/argv from the host once, on demand when the first `env::ArgsOs` is constructed, and globally cache it. Copy each argument to an `OsString` while iterating.
66c407d to
0009598
Compare
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.
Uh oh!
There was an error while loading. Please reload this page.
The zkVM implementation of
env::ArgsOsincorrectly 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