Trusty: Implement write_vectored for stdio

[thaliaarchi]

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.

cc @randomPoison

[rustbot]

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

[thaliaarchi]

@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.

[ahomescu]

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.

[ahomescu]

Afaik Trusty does not have a limit for this (yet).

[thaliaarchi]

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.

[ahomescu]

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.

[thaliaarchi]

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.

[ahomescu]

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?

That one seems to come from https://r.android.com/2124958 (@randomPoison can correct me). Maybe it needed to be different there?

[ahomescu]

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.

[randomPoison]

Why use libc::writev for implementing io::Write::write instead of libc::write, which has been in libc since Trusty was added?

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?

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::fd module? Something like this? Much of the current logic in std::sys::stdio::trusty could 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.

[randomPoison]

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.

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.

[randomPoison]

Looks good other than the one outstanding question about IOV_MAX.

[thaliaarchi]

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.

[thaliaarchi]

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.

[Noratrieb]

@bors r+

[bors]

📌 Commit 41b0465 has been approved by Noratrieb

It is now in the queue for this repository.

[thaliaarchi]

Thanks, Nora!