Trusty: Implement write_vectored for stdio by thaliaarchi · Pull Request #138876 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Currently, write for stdout and stderr on Trusty is implemented with the semantics of write_all. Instead, call the underlying syscall only once in write and use the default implementation of write_all like other platforms. Also, implement write_vectored by adding support for IoSlice.
Refactor stdin to reuse the unsupported type like #136769.
It requires #138875 to fix the build for Trusty, though they do not conflict and can merge in either order.
r? @Noratrieb
rustbot has assigned @Noratrieb.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
@randomPoison I have some questions on Trusty's current state of stdio and std:
Why use libc::writev for implementing io::Write::write instead of libc::write, which has been in libc since Trusty was added? Confusingly, the Trusty API reference only lists non-scatter read and write for file descriptors. It seems like the reference is incomplete or I'm looking in the wrong place?
Is there a reason why io::Write::write was implemented here with io::Write::write_all semantics, instead of a single call to the underlying syscall?
Do you plan on implementing a std::sys::pal::trusty::fd module? Something like this? Much of the current logic in std::sys::stdio::trusty could be moved there.
Why use libc::writev for implementing io::Write::write instead of libc::write, which has been in libc since Trusty was added?
write in Trusty is a thin wrapper over writev anyway, so I thought we might as well use the actual syscall directly.
Confusingly, the Trusty API reference only lists non-scatter read and write for file descriptors. It seems like the reference is incomplete or I'm looking in the wrong place?
Right, good catch. We need to update the reference to list writev.
| @@ -58,20 +76,43 @@ pub fn panic_output() -> Option { |
|---|
| Some(Stderr) |
| } |
| fn _write(fd: i32, message: &[u8]) -> io::Result { |
| let mut iov = libc::iovec { iov_base: message.as_ptr() as *mut _, iov_len: message.len() }; |
| // FIXME: This should be in libc with a proper value. Several platforms use |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik Trusty does not have a limit for this (yet).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps not a constant that's exposed to users, but is there a limit in the implementation? If so, I want to make sure to avoid issues like found in #138879.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know of, but 1024 should be safe to use. The kernel code implements an infinite iterator over the user iovec, and avoids copying the whole array.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I'll change it to libc::c_int::MAX, since that's what the API limits it to. We don't need to limit it to something arbitrarily lower. Windows does it like that, for example.
Is there a reason why
io::Write::writewas implemented here withio::Write::write_allsemantics, instead of a single call to the underlying syscall?
That one seems to come from https://r.android.com/2124958 (@randomPoison can correct me). Maybe it needed to be different there?
This change looks fine to me, but we should test it. We'll need to rebuild the toolchain with this change, and see if Trusty prints stuff to stdout correctly. I think this should be doable on AOSP.
Why use
libc::writevfor implementingio::Write::writeinstead of libc::write, which has been in libc since Trusty was added?
Is there a reason why
io::Write::writewas implemented here withio::Write::write_allsemantics, instead of a single call to the underlying syscall?
I didn't write the original implementation of the _write fn used here, that actually predates our effort to add libstd support for Trusty. When I took the implementation from the Trusty internals and added them to libstd it didn't occur to me that the implementation has unexpected semantics here, so that's an oversight on my part. My suspicion is that this is also why it has write_all semantics: It was likely written like that originally for convenience so that we didn't have to worry about partial writes, and then I made the mistake of preserving those semantics when moving the write logic into libstd.
Do you plan on implementing a
std::sys::pal::trusty::fdmodule? Something like this? Much of the current logic instd::sys::stdio::trustycould be moved there.
I hadn't planned on it (because I wasn't aware of that approach), but if this would simplify the implementation or cut down on duplication between other platforms then it's probably worth doing. Let me look at it a bit more and see how much effort it would be to put up a PR.
This change looks fine to me, but we should test it. We'll need to rebuild the toolchain with this change, and see if Trusty prints stuff to
stdoutcorrectly. I think this should be doable on AOSP.
We discussed this elsewhere and unfortunately we can't actually test this change in the way we want due to the details of how we pull Rust into Android. We'll have to land the PR here and then wait until we later import it into Android, at which point we can do testing. So testing shouldn't block this PR, and we'll come back and fix any issues if we later find them.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than the one outstanding question about IOV_MAX.
Since #138875 does not conflict with this, I have dropped its changes from this PR. Keep in mind, though, that you'll need it for now to fix the build for Trusty if you want to build this PR.
I have also addressed the writev upper bound.
Currently, write for stdout and stderr on Trusty is implemented with
the semantics of write_all. Instead, call the underlying syscall only
once in write and use the default implementation of write_all like
other platforms. Also, implement write_vectored by adding support for
IoSlice.
Refactor stdin to reuse the unsupported type like rust-lang#136769.
I decided to drop the write_all implementation and use the default one, because it doesn't handle an early EOF or is_interrupted. Although, Trusty currently doesn't have is_interrupted, this will make it more seamless when it does. With inlining, it should be pretty much the same, as the slice panic should be easy for LLVM to prove can't happen. Anyways, no other platforms override write_all, so I don't see a strong reason to do so here.
Trusty: Implement Trusty: Implement write_vectored and write_all for stdiowrite_vectored for stdio
📌 Commit 41b0465 has been approved by Noratrieb
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#138876 - thaliaarchi:trusty-stdio, r=Noratrieb
Trusty: Implement write_vectored for stdio
Currently, write for stdout and stderr on Trusty is implemented with the semantics of write_all. Instead, call the underlying syscall only once in write and use the default implementation of write_all like other platforms. Also, implement write_vectored by adding support for IoSlice.
Refactor stdin to reuse the unsupported type like rust-lang#136769.
It requires rust-lang#138875 to fix the build for Trusty, though they do not conflict and can merge in either order.
cc @randomPoison
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request
Trusty: Implement write_vectored for stdio
Currently, write for stdout and stderr on Trusty is implemented with the semantics of write_all. Instead, call the underlying syscall only once in write and use the default implementation of write_all like other platforms. Also, implement write_vectored by adding support for IoSlice.
Refactor stdin to reuse the unsupported type like rust-lang#136769.
It requires rust-lang#138875 to fix the build for Trusty, though they do not conflict and can merge in either order.
cc @randomPoison
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.