Move std::io::Error into core#155625
Conversation
This comment has been minimized.
This comment has been minimized.
b86f41f to
c538e5d
Compare
This comment has been minimized.
This comment has been minimized.
6c37f53 to
65d4011
Compare
This comment has been minimized.
This comment has been minimized.
65d4011 to
56a8e8c
Compare
93b1fa3 to
d3835aa
Compare
This comment has been minimized.
This comment has been minimized.
|
example of how to link from |
d3835aa to
8bfceb6
Compare
That's a clever trick I never knew about! Looks like it's also required for linking to incoherent implementations even within the file that creates them too. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8bfceb6 to
96864eb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
96864eb to
7cad458
Compare
This comment has been minimized.
This comment has been minimized.
maybe just leave it until you get the PR to work, that way there's less comment spam. |
5fff01b to
d1ecdab
Compare
This comment has been minimized.
This comment has been minimized.
d1ecdab to
55b632f
Compare
This comment has been minimized.
This comment has been minimized.
55b632f to
7c41c8e
Compare
Move `IoSlice` and `IoSliceMut` to `core::io` ACP: rust-lang/libs-team#755 Tracking issue: #154046 Related: #152918 Related: #155625 ## Description Moves `std::io::IoSlice` and `std::io::IoSliceMut` into `core::io`. This is required for the `Read` and `Write` traits to be moved into `alloc` and/or `core`, as they contain stable methods which work with IO slices. Similar to #155574, this PR inlines the `std::sys` types required to create an ABI compatible type for the platforms where such compatibility is guaranteed. Additionally, I've moved the relevant tests out of `std::io::tests` into `coretests::io::io_slice`. --- ## Notes * This PR overlaps with #152918, but goes further than moving the IO slice types to `alloc` and instead moves them straight to `core`. Since these types have no interaction with allocation, and doing so will allow `Write` to move to `core::io`, I consider this a better home for these types. * Some discussion around the decision to not use a `core::sys` module can be found [here](#155574 (comment)). * I've renamed `unsupported` to `generic` to better reflect that `IoSlice(Mut)` _is_ supported by all platforms, it just doesn't have a special ABI-compatible type. I don't want to imply that parts of `core` "don't work" depending on the target; `IoSlice(Mut)` works exactly as expected on all targets. * I've made `pub` items within each platform-specific representation `pub(super)` to highlight that everything within `core::io::io_slice` is an internal implementation detail not meant for any other part of the crate to be aware of. * No AI tooling of any kind was used during the creation of this PR.
Move `IoSlice` and `IoSliceMut` to `core::io` ACP: rust-lang/libs-team#755 Tracking issue: #154046 Related: #152918 Related: #155625 ## Description Moves `std::io::IoSlice` and `std::io::IoSliceMut` into `core::io`. This is required for the `Read` and `Write` traits to be moved into `alloc` and/or `core`, as they contain stable methods which work with IO slices. Similar to #155574, this PR inlines the `std::sys` types required to create an ABI compatible type for the platforms where such compatibility is guaranteed. Additionally, I've moved the relevant tests out of `std::io::tests` into `coretests::io::io_slice`. --- ## Notes * This PR overlaps with #152918, but goes further than moving the IO slice types to `alloc` and instead moves them straight to `core`. Since these types have no interaction with allocation, and doing so will allow `Write` to move to `core::io`, I consider this a better home for these types. * Some discussion around the decision to not use a `core::sys` module can be found [here](#155574 (comment)). * I've renamed `unsupported` to `generic` to better reflect that `IoSlice(Mut)` _is_ supported by all platforms, it just doesn't have a special ABI-compatible type. I don't want to imply that parts of `core` "don't work" depending on the target; `IoSlice(Mut)` works exactly as expected on all targets. * I've made `pub` items within each platform-specific representation `pub(super)` to highlight that everything within `core::io::io_slice` is an internal implementation detail not meant for any other part of the crate to be aware of. * No AI tooling of any kind was used during the creation of this PR.
Move `IoSlice` and `IoSliceMut` to `core::io` ACP: rust-lang/libs-team#755 Tracking issue: #154046 Related: #152918 Related: #155625 ## Description Moves `std::io::IoSlice` and `std::io::IoSliceMut` into `core::io`. This is required for the `Read` and `Write` traits to be moved into `alloc` and/or `core`, as they contain stable methods which work with IO slices. Similar to #155574, this PR inlines the `std::sys` types required to create an ABI compatible type for the platforms where such compatibility is guaranteed. Additionally, I've moved the relevant tests out of `std::io::tests` into `coretests::io::io_slice`. --- ## Notes * This PR overlaps with #152918, but goes further than moving the IO slice types to `alloc` and instead moves them straight to `core`. Since these types have no interaction with allocation, and doing so will allow `Write` to move to `core::io`, I consider this a better home for these types. * Some discussion around the decision to not use a `core::sys` module can be found [here](#155574 (comment)). * I've renamed `unsupported` to `generic` to better reflect that `IoSlice(Mut)` _is_ supported by all platforms, it just doesn't have a special ABI-compatible type. I don't want to imply that parts of `core` "don't work" depending on the target; `IoSlice(Mut)` works exactly as expected on all targets. * I've made `pub` items within each platform-specific representation `pub(super)` to highlight that everything within `core::io::io_slice` is an internal implementation detail not meant for any other part of the crate to be aware of. * No AI tooling of any kind was used during the creation of this PR.
Move `IoSlice` and `IoSliceMut` to `core::io` ACP: rust-lang/libs-team#755 Tracking issue: #154046 Related: #152918 Related: #155625 ## Description Moves `std::io::IoSlice` and `std::io::IoSliceMut` into `core::io`. This is required for the `Read` and `Write` traits to be moved into `alloc` and/or `core`, as they contain stable methods which work with IO slices. Similar to #155574, this PR inlines the `std::sys` types required to create an ABI compatible type for the platforms where such compatibility is guaranteed. Additionally, I've moved the relevant tests out of `std::io::tests` into `coretests::io::io_slice`. --- ## Notes * This PR overlaps with #152918, but goes further than moving the IO slice types to `alloc` and instead moves them straight to `core`. Since these types have no interaction with allocation, and doing so will allow `Write` to move to `core::io`, I consider this a better home for these types. * Some discussion around the decision to not use a `core::sys` module can be found [here](#155574 (comment)). * I've renamed `unsupported` to `generic` to better reflect that `IoSlice(Mut)` _is_ supported by all platforms, it just doesn't have a special ABI-compatible type. I don't want to imply that parts of `core` "don't work" depending on the target; `IoSlice(Mut)` works exactly as expected on all targets. * I've made `pub` items within each platform-specific representation `pub(super)` to highlight that everything within `core::io::io_slice` is an internal implementation detail not meant for any other part of the crate to be aware of. * No AI tooling of any kind was used during the creation of this PR.
This comment has been minimized.
This comment has been minimized.
Viewing internals wont be possible from `std` once moved into `core`.
Adjust `Error` documentation `core` is more restrictive with documentation quality and linking to other items. Methods that will be implemented through incoherence must also be explicitly linked.
Personal preference that will be used for another module in a later commit. Purely stylistic.
They'll be moved into `core` but must be accessible from `std`.
Incoherence is required to define inherit methods on `Error` from `alloc` and `std` once it is moved into `core`. This is required because: 1. `Box` is part of `Error`'s public API and that is only accessible from `alloc`. 2. `RawOsError` methods must ensure the `OsFunctions` atomic pointer is appropriately set, which can only be done from `std`.
Required to allow `std` access from `core`
7c41c8e to
0defc68
Compare
|
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. |
|
@Mark-Simulacrum A polite poke for a review on this PR? I've updated my tracking comment with the current status of this effort, and this PR is now the blocker on further progress. |
Move `IoSlice` and `IoSliceMut` to `core::io` ACP: rust-lang/libs-team#755 Tracking issue: rust-lang/rust#154046 Related: rust-lang/rust#152918 Related: rust-lang/rust#155625 ## Description Moves `std::io::IoSlice` and `std::io::IoSliceMut` into `core::io`. This is required for the `Read` and `Write` traits to be moved into `alloc` and/or `core`, as they contain stable methods which work with IO slices. Similar to rust-lang/rust#155574, this PR inlines the `std::sys` types required to create an ABI compatible type for the platforms where such compatibility is guaranteed. Additionally, I've moved the relevant tests out of `std::io::tests` into `coretests::io::io_slice`. --- ## Notes * This PR overlaps with rust-lang/rust#152918, but goes further than moving the IO slice types to `alloc` and instead moves them straight to `core`. Since these types have no interaction with allocation, and doing so will allow `Write` to move to `core::io`, I consider this a better home for these types. * Some discussion around the decision to not use a `core::sys` module can be found [here](rust-lang/rust#155574 (comment)). * I've renamed `unsupported` to `generic` to better reflect that `IoSlice(Mut)` _is_ supported by all platforms, it just doesn't have a special ABI-compatible type. I don't want to imply that parts of `core` "don't work" depending on the target; `IoSlice(Mut)` works exactly as expected on all targets. * I've made `pub` items within each platform-specific representation `pub(super)` to highlight that everything within `core::io::io_slice` is an internal implementation detail not meant for any other part of the crate to be aware of. * No AI tooling of any kind was used during the creation of this PR.
| use crate::sync::atomic; | ||
|
|
||
| /// These default functions are not reachable, but have them just to be safe. | ||
| static OS_FUNCTIONS: atomic::AtomicPtr<OsFunctions> = |
There was a problem hiding this comment.
Is this actually used anywhere? It seems like we're not yet supporting dynamic overrides in the current state of this PR?
I believe there's open concerns from Linux and maybe others on having global mutable statics in libcore, so I think it would be best if we can remove this from the initial implementation. I'll nominate for T-libs to check if that is a blocking concern or if we're OK doing this.
Would it increase the size of io::Error if we stored the vtable pointer inline instead of into global state? Or do we have enough spare bits in the enum variant containing RawOsError that it would not be a size increase? (And does the API work out that such a path is workable?)
There was a problem hiding this comment.
There has been a general preference for keeping the size of Error as small as possible, which is why we have all sorts of macros generating static references to make stuff work. I would also assume that atomic statics are fine, but someone else would have to verify that.
Unfortunately, we can't just use extern functions here, since it would require LTO to make everything work, and we don't want to require that.
There was a problem hiding this comment.
There was an explicit mention from someone (I'm not sure who) at the all hands that they don't want atomic statics in libcore in Linux.
Taking another look, io::Error is 8 bytes on 64-bit Linux today, so we can't pack a full pointer into that in addition to other data (i32 + tag bits). So I think that largely rules out trying to do that to land this.
There was a problem hiding this comment.
I guess we are free of atomic statics outside of libstd currently, so, that makes sense. Not really sure how we could do this otherwise without one of the externally-implementable shenanigans features that are currently proposed.
There was a problem hiding this comment.
there are atomic statics in alloc:
rust/library/alloc/src/sync.rs
Lines 3782 to 3788 in 61d7280
There was a problem hiding this comment.
what could work well is to have a cfg that is enabled only on targets with std and use that cfg on the static atomic.
There was a problem hiding this comment.
It's actually quite interesting that we've decided to create a static Arc for that instead of allocating a new one. I guess that the idea is that creating empty CStr is a compelling enough use case that we don't want to allocate?
This is also another case where with LTO, this thing gets stripped out anyway if it's not used, but we probably don't want to rely on LTO for anything.
There was a problem hiding this comment.
Is this actually used anywhere? It seems like we're not yet supporting dynamic overrides in the current state of this PR?
Yes, it's written from std when creating an io::Error from a RawOsError. And it's read from core for the error message, error kind, and determining if it is an interruption.
I believe there's open concerns from Linux and maybe others on having global mutable statics in libcore, so I think it would be best if we can remove this from the initial implementation. I'll nominate for T-libs to check if that is a blocking concern or if we're OK doing this.
Agreed, but I'm not sure what a good alternative would be. What I need is a way to declare a default function in core, and potentially replace it when std is linked. Since any given target may or may not choose to link std, it can't be set using conditional compilation.
Similar to how alloc has a feature for disabling global error handling for R4L, maybe core could also get a feature to disable the atomic and instead always use the default implementation?
Would it increase the size of io::Error if we stored the vtable pointer inline instead of into global state? Or do we have enough spare bits in the enum variant containing RawOsError that it would not be a size increase? (And does the API work out that such a path is workable?)
Yes, as others have mentioned that is sadly not viable. The RawOsError path needs to fit an i32 and 2 tag bits into 8 bytes (for 64 bit targets excluding UEFI). On all others it's permitted up to 16 bytes.
This allows `Error` to be moved into `core` while still retaining the ability to store custom error data. This may have performance implications!
Stored in a `static` `AtomicPtr` to allow definition in `core` and setting in `std`. Should be replaced with Externally Implemented Items (EII) or similar once stable.
Now that `Error` lives in `core::io`, doc links to it from `core` can be simplified.
0defc68 to
4302863
Compare
View all comments
ACP: rust-lang/libs-team#755
Tracking issue: #154046
Related: #155574
Related: #152918
Description
Moves
std::io::Errorintocore, deferringBox-adjacent methods to incoherent implementations inalloc, andRawOsErrormethods tostd. This requires some substantial changes to the internals ofError, but none of them are breaking changes or externally visible.Notably, I've replaced usage of
Boxwith a wrapper around a pointer and an appropriate drop function. This requires the addition of quite a few lines of unsafe, but is required to work aroundBoxonly being accessible fromalloc. Additionally, an atomic pointer to a VTable is used for working withRawOsErrorincore, since we cannot know the required implementations withoutstd.Notes
std::iotoalloc#152918std::io::RawOsErrortocore::io#155574