Move OS-dependent parts of std::io to alloc

[a1phyr]

View all comments

ACP: rust-lang/libs-team#755
Tracking issue: #154046

This is the first (and the hardest) part of a plan to move most of std::io to alloc.

This PR has three commits:

• The first enable alloc::ffi with no_rc and no_sync because they where annoying me.
• The second moves io::Error to alloc.
  • Creating Error from raw OS errors is restricted to std via incoherent impls.
  • OS-dependent stuff needed by alloc is set at runtime by std when creating Error from raw OS errors (so no linking required, and not dependent of how Rust code is started). Can be moved to externally implementable one day.
• The third moves IoSlice and IoSliceMut to alloc (required by Read and Write, exact layout depend on the OS).
  • Moves OS-dependent definition to alloc, but do not link to anything either.

I expect the introduction of OS-dependent code to alloc quite controversial, but please note that this PR does not expose anything OS-dependent to alloc users.

Also this PR abuses the fact that tidy does not warn on cfg_select! in alloc like it does for #[cfg()] 😈

The rest of std::io that can move should do so easily (obviously not things like stdio, IsTerminal, pipes, etc), except maybe for io::copy which has OS-dependent specialization. If this get merged, I will make a follup-up PR with all all this. In the meantime, a PR containing only the hard parts will avoid perpetual conflicts and rebases.

Some questions

Why not core?

Well this could be tried in the future. The main issue is that Read directly uses Vec.

Why not new Read/Write traits?

Why about not having them and make the existing one work for everyone?

Previous discussion (far larger than these few links)

Cc #48331
Prior try to move Error to core (not alloc): #116685
Prior discussion on this specific plan: https://internals.rust-lang.org/t/a-plan-to-move-most-of-std-io-to-alloc/23678/9
Many #![no_std] crates reimplementing std::io:

• embedded-io
• core2 and its forks
• ciborium-io.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates
• Random selection from Mark-Simulacrum, jhpratt

[lygstate]

I have a idea, that move iovec into core::ffi first, and it's just different layout on UEFI/Windows/Other, for unknow or other triple, use

struct iovec {
    iov_base: *mut c_void,
    iov_len: usize,
}

for windows/UEFI, use

struct iovec {
    iov_len: c_ulong,
    iov_base: *mut c_void,
}

and for RawOsError, it's can also defined in core:ffi as

os_errno_t

[a1phyr]

It would possible indeed. I think that what we'll do will depend on T-libs team preference.

[a1phyr]

To make rustdoc happy, I had to remove a bunch a links in io documentation. Most of them will be able to be added back when the rest of io arrives. The other will need a closer look to the incoherent impls weirdness.

[jhpratt]

@rustbot reroll

[tgross35]

This is the first (and the hardest) part of a plan to move most of std::io to alloc.

A specific plan should be proposed as an ACP before starting too much work, there are tradeoffs and alternatives that need to be discussed and accepted by the libs-api team.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[a1phyr]

Can this get a perf run please?

Up on this!

Also friendly ping @jhpratt, when do you think that you will have time to review this?

[jhpratt]

I can take a look later, but note that I did reassign reviewers a bit ago :) For perf, I'll start a run if I don't see anything obvious.

[programmerjake]

see also a new attempt to move std::io::Error to core

[a1phyr]

I did reassign reviewers a bit ago :)

Oh sorry, I missed that. Ping @joboet then!

[rust-bors]

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

[Mark-Simulacrum]

@bushrat011899 can you file a new issue or place a comment somewhere (that is kept up to date) on which PRs should be reviewed in what order? I see you have several parallel PRs in flight and it would help to have an overview. E.g., it looks like #155849 pulls out one of the commits from this PR (seems good -- smaller is easier to review, generally), it seems like I should review that first?

[bushrat011899]

@Mark-Simulacrum Yes that is correct, I've opened up:

• Move std::io::Error into core #155625
• Move IoSlice and IoSliceMut to core::io #155849

Which both take up most of the most of the controversial changes of this PR, but they do go further by going straight to core rather than only moving to alloc. #155849 is virtually identical to this PR so I think that can be reviewed and merged before this one uncontroversially. Whereas, #155625 does add some extra code to handle the lack of Box in core, so that might be worth reviewing after the IoSlice PR is merged and this one is rebased.

EDIT: I've made a more detailed summary of the current state of this effort here.

[joboet]

I'll mark this as blocked, since it overlaps with the other PRs. Feel free to flip the labels once this PR is ready.

@rustbot label +S-blocked -S-waiting-on-review

[bushrat011899]

@a1phyr now that #155625 is approved and in the process of being merged, I believe this PR can be closed.