diff options
| author | bors <bors@rust-lang.org> | 2022-02-07 20:32:56 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2022-02-07 20:32:56 +0000 |
| commit | 734368a200904ef9c21db86c595dc04263c87be0 (patch) | |
| tree | 9b73116410fcc1adf4cb2b454377586fa195905b /library/std/src/sys/hermit | |
| parent | f52c31840df7ec9c9350baff51a8964b20b5e1ba (diff) | |
| parent | 9cbe99488bbeb8bdd742a4d15fe484ee665f9363 (diff) | |
| download | rust-734368a200904ef9c21db86c595dc04263c87be0.tar.gz rust-734368a200904ef9c21db86c595dc04263c87be0.zip | |
Auto merge of #87869 - thomcc:skinny-io-error, r=yaahc
Make io::Error use 64 bits on targets with 64 bit pointers.
I've wanted this for a long time, but didn't see a good way to do it without having extra allocation. When looking at it yesterday, it was more clear what to do for some reason.
This approach avoids any additional allocations, and reduces the size by half (8 bytes, down from 16). AFAICT it doesn't come additional runtime cost, and the compiler seems to do a better job with code using it.
Additionally, this `io::Error` has a niche (still), so `io::Result<()>` is *also* 64 bits (8 bytes, down from 16), and `io::Result<usize>` (used for lots of io trait functions) is 2x64 bits (16 bytes, down from 24 — this means on x86_64 it can use the nice rax/rdx 2-reg struct return). More generally, it shaves a whole 64 bit integer register off of the size of basically any `io::Result<()>`.
(For clarity: Improving `io::Result` (rather than io::Error) was most of the motivation for this)
On 32 bit (or other non-64bit) targets we still use something equivalent the old repr — I don't think think there's improving it, since one of the fields it stores is a `i32`, so we can't get below that, and it's already about as close as we can get to it.
---
### Isn't Pointer Tagging Dodgy?
The details of the layout, and why its implemented the way it is, are explained in the header comment of library/std/src/io/error/repr_bitpacked.rs. There's probably more details than there need to be, but I didn't trim it down that much, since there's a lot of stuff I did deliberately, that might have not seemed that way.
There's actually only one variant holding a pointer which gets tagged. This one is the (holder for the) user-provided error.
I believe the scheme used to tag it is not UB, and that it preserves pointer provenance (even though often pointer tagging does not) because the tagging operation is just `core::ptr::add`, and untagging is `core::ptr::sub`. The result of both operations lands inside the original allocation, so it would follow the safety contract of `core::ptr::{add,sub}`.
The other pointer this had to encode is not tagged — or rather, the tagged repr is equivalent to untagged (it's tagged with 0b00, and has >=4b alignment, so we can reuse the bottom bits). And the other variants we encode are just integers, which (which can be untagged using bitwise operations without worry — they're integers).
CC `@RalfJung` for the stuff in repr_bitpacked.rs, as my comments are informed by a lot of the UCG work, but it's possible I missed something or got it wrong (even if the implementation is okay, there are parts of the header comment that says things like "We can't do $x" which could be false).
---
### Why So Many Changes?
The repr change was mostly internal, but changed one widely used API: I had to switch how `io::Error::new_const` works.
This required switching `io::Error::new_const` to take the full message data (including the kind) as a `&'static`, rather than just the string. This would have been really tedious, but I made a macro that made it much simpler, but it was a wide change since `io::Error::new_const` is used everywhere.
This included changing files for a lot of targets I don't have easy access to (SGX? Haiku? Windows? Who has heard of these things), so I expect there to be spottiness in CI initially, unless luck is on my side.
Anyway this large only tangentially-related change is all in the first commit (although that commit also pulls the previous repr out into its own file), whereas the packing stuff is all in commit 2.
---
P.S. I haven't looked at all of this since writing it, and will do a pass over it again later, sorry for any obvious typos or w/e. I also definitely repeat myself in comments and such.
(It probably could use more tests too. I did some basic testing, and made it so we `debug_assert!` in cases the decode isn't what we encoded, but I don't know the degree which I can assume libstd's testing of IO would exercise this. That is: it wouldn't be surprising to me if libstds IO testing were minimal, especially around error cases, although I have no idea).
Diffstat (limited to 'library/std/src/sys/hermit')
| -rw-r--r-- | library/std/src/sys/hermit/fs.rs | 10 | ||||
| -rw-r--r-- | library/std/src/sys/hermit/mod.rs | 4 | ||||
| -rw-r--r-- | library/std/src/sys/hermit/net.rs | 49 | ||||
| -rw-r--r-- | library/std/src/sys/hermit/stdio.rs | 8 | ||||
| -rw-r--r-- | library/std/src/sys/hermit/thread.rs | 2 |
5 files changed, 36 insertions, 37 deletions
diff --git a/library/std/src/sys/hermit/fs.rs b/library/std/src/sys/hermit/fs.rs index 974c44eb8dd..fa9a7fb19e4 100644 --- a/library/std/src/sys/hermit/fs.rs +++ b/library/std/src/sys/hermit/fs.rs @@ -226,7 +226,7 @@ impl OpenOptions { (false, _, true) => Ok(O_WRONLY | O_APPEND), (true, _, true) => Ok(O_RDWR | O_APPEND), (false, false, false) => { - Err(io::Error::new_const(ErrorKind::InvalidInput, &"invalid access mode")) + Err(io::const_io_error!(ErrorKind::InvalidInput, "invalid access mode")) } } } @@ -236,17 +236,17 @@ impl OpenOptions { (true, false) => {} (false, false) => { if self.truncate || self.create || self.create_new { - return Err(io::Error::new_const( + return Err(io::const_io_error!( ErrorKind::InvalidInput, - &"invalid creation mode", + "invalid creation mode", )); } } (_, true) => { if self.truncate && !self.create_new { - return Err(io::Error::new_const( + return Err(io::const_io_error!( ErrorKind::InvalidInput, - &"invalid creation mode", + "invalid creation mode", )); } } diff --git a/library/std/src/sys/hermit/mod.rs b/library/std/src/sys/hermit/mod.rs index 185b68c0a78..b798c97448b 100644 --- a/library/std/src/sys/hermit/mod.rs +++ b/library/std/src/sys/hermit/mod.rs @@ -58,9 +58,9 @@ pub fn unsupported<T>() -> crate::io::Result<T> { } pub fn unsupported_err() -> crate::io::Error { - crate::io::Error::new_const( + crate::io::const_io_error!( crate::io::ErrorKind::Unsupported, - &"operation not supported on HermitCore yet", + "operation not supported on HermitCore yet", ) } diff --git a/library/std/src/sys/hermit/net.rs b/library/std/src/sys/hermit/net.rs index 1a6b3bc63e6..f65fd8e53bd 100644 --- a/library/std/src/sys/hermit/net.rs +++ b/library/std/src/sys/hermit/net.rs @@ -14,9 +14,9 @@ use crate::time::Duration; /// if not, starts it. pub fn init() -> io::Result<()> { if abi::network_init() < 0 { - return Err(io::Error::new_const( + return Err(io::const_io_error!( ErrorKind::Uncategorized, - &"Unable to initialize network interface", + "Unable to initialize network interface", )); } @@ -50,9 +50,9 @@ impl TcpStream { match abi::tcpstream::connect(addr.ip().to_string().as_bytes(), addr.port(), None) { Ok(handle) => Ok(TcpStream(Arc::new(Socket(handle)))), - _ => Err(io::Error::new_const( + _ => Err(io::const_io_error!( ErrorKind::Uncategorized, - &"Unable to initiate a connection on a socket", + "Unable to initiate a connection on a socket", )), } } @@ -64,9 +64,9 @@ impl TcpStream { Some(duration.as_millis() as u64), ) { Ok(handle) => Ok(TcpStream(Arc::new(Socket(handle)))), - _ => Err(io::Error::new_const( + _ => Err(io::const_io_error!( ErrorKind::Uncategorized, - &"Unable to initiate a connection on a socket", + "Unable to initiate a connection on a socket", )), } } @@ -74,7 +74,7 @@ impl TcpStream { pub fn set_read_timeout(&self, duration: Option<Duration>) -> io::Result<()> { abi::tcpstream::set_read_timeout(*self.0.as_inner(), duration.map(|d| d.as_millis() as u64)) .map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"Unable to set timeout value") + io::const_io_error!(ErrorKind::Uncategorized, "Unable to set timeout value") }) } @@ -83,12 +83,12 @@ impl TcpStream { *self.0.as_inner(), duration.map(|d| d.as_millis() as u64), ) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"Unable to set timeout value")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "Unable to set timeout value")) } pub fn read_timeout(&self) -> io::Result<Option<Duration>> { let duration = abi::tcpstream::get_read_timeout(*self.0.as_inner()).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"Unable to determine timeout value") + io::const_io_error!(ErrorKind::Uncategorized, "Unable to determine timeout value") })?; Ok(duration.map(|d| Duration::from_millis(d))) @@ -96,7 +96,7 @@ impl TcpStream { pub fn write_timeout(&self) -> io::Result<Option<Duration>> { let duration = abi::tcpstream::get_write_timeout(*self.0.as_inner()).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"Unable to determine timeout value") + io::const_io_error!(ErrorKind::Uncategorized, "Unable to determine timeout value") })?; Ok(duration.map(|d| Duration::from_millis(d))) @@ -104,7 +104,7 @@ impl TcpStream { pub fn peek(&self, buf: &mut [u8]) -> io::Result<usize> { abi::tcpstream::peek(*self.0.as_inner(), buf) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"peek failed")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "peek failed")) } pub fn read(&self, buffer: &mut [u8]) -> io::Result<usize> { @@ -116,7 +116,7 @@ impl TcpStream { for i in ioslice.iter_mut() { let ret = abi::tcpstream::read(*self.0.as_inner(), &mut i[0..]).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"Unable to read on socket") + io::const_io_error!(ErrorKind::Uncategorized, "Unable to read on socket") })?; if ret != 0 { @@ -141,7 +141,7 @@ impl TcpStream { for i in ioslice.iter() { size += abi::tcpstream::write(*self.0.as_inner(), i).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"Unable to write on socket") + io::const_io_error!(ErrorKind::Uncategorized, "Unable to write on socket") })?; } @@ -155,13 +155,13 @@ impl TcpStream { pub fn peer_addr(&self) -> io::Result<SocketAddr> { let (ipaddr, port) = abi::tcpstream::peer_addr(*self.0.as_inner()) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"peer_addr failed"))?; + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "peer_addr failed"))?; let saddr = match ipaddr { Ipv4(ref addr) => SocketAddr::new(IpAddr::V4(Ipv4Addr::from(addr.0)), port), Ipv6(ref addr) => SocketAddr::new(IpAddr::V6(Ipv6Addr::from(addr.0)), port), _ => { - return Err(io::Error::new_const(ErrorKind::Uncategorized, &"peer_addr failed")); + return Err(io::const_io_error!(ErrorKind::Uncategorized, "peer_addr failed")); } }; @@ -173,9 +173,8 @@ impl TcpStream { } pub fn shutdown(&self, how: Shutdown) -> io::Result<()> { - abi::tcpstream::shutdown(*self.0.as_inner(), how as i32).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"unable to shutdown socket") - }) + abi::tcpstream::shutdown(*self.0.as_inner(), how as i32) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "unable to shutdown socket")) } pub fn duplicate(&self) -> io::Result<TcpStream> { @@ -192,22 +191,22 @@ impl TcpStream { pub fn set_nodelay(&self, mode: bool) -> io::Result<()> { abi::tcpstream::set_nodelay(*self.0.as_inner(), mode) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"set_nodelay failed")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "set_nodelay failed")) } pub fn nodelay(&self) -> io::Result<bool> { abi::tcpstream::nodelay(*self.0.as_inner()) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"nodelay failed")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "nodelay failed")) } pub fn set_ttl(&self, tll: u32) -> io::Result<()> { abi::tcpstream::set_tll(*self.0.as_inner(), tll) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"unable to set TTL")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "unable to set TTL")) } pub fn ttl(&self) -> io::Result<u32> { abi::tcpstream::get_tll(*self.0.as_inner()) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"unable to get TTL")) + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "unable to get TTL")) } pub fn take_error(&self) -> io::Result<Option<io::Error>> { @@ -216,7 +215,7 @@ impl TcpStream { pub fn set_nonblocking(&self, mode: bool) -> io::Result<()> { abi::tcpstream::set_nonblocking(*self.0.as_inner(), mode).map_err(|_| { - io::Error::new_const(ErrorKind::Uncategorized, &"unable to set blocking mode") + io::const_io_error!(ErrorKind::Uncategorized, "unable to set blocking mode") }) } } @@ -243,12 +242,12 @@ impl TcpListener { pub fn accept(&self) -> io::Result<(TcpStream, SocketAddr)> { let (handle, ipaddr, port) = abi::tcplistener::accept(self.0.port()) - .map_err(|_| io::Error::new_const(ErrorKind::Uncategorized, &"accept failed"))?; + .map_err(|_| io::const_io_error!(ErrorKind::Uncategorized, "accept failed"))?; let saddr = match ipaddr { Ipv4(ref addr) => SocketAddr::new(IpAddr::V4(Ipv4Addr::from(addr.0)), port), Ipv6(ref addr) => SocketAddr::new(IpAddr::V6(Ipv6Addr::from(addr.0)), port), _ => { - return Err(io::Error::new_const(ErrorKind::Uncategorized, &"accept failed")); + return Err(io::const_io_error!(ErrorKind::Uncategorized, "accept failed")); } }; diff --git a/library/std/src/sys/hermit/stdio.rs b/library/std/src/sys/hermit/stdio.rs index 33b8390431f..514de1df6f9 100644 --- a/library/std/src/sys/hermit/stdio.rs +++ b/library/std/src/sys/hermit/stdio.rs @@ -40,7 +40,7 @@ impl io::Write for Stdout { unsafe { len = abi::write(1, data.as_ptr() as *const u8, data.len()) } if len < 0 { - Err(io::Error::new_const(io::ErrorKind::Uncategorized, &"Stdout is not able to print")) + Err(io::const_io_error!(io::ErrorKind::Uncategorized, "Stdout is not able to print")) } else { Ok(len as usize) } @@ -52,7 +52,7 @@ impl io::Write for Stdout { unsafe { len = abi::write(1, data.as_ptr() as *const u8, data.len()) } if len < 0 { - Err(io::Error::new_const(io::ErrorKind::Uncategorized, &"Stdout is not able to print")) + Err(io::const_io_error!(io::ErrorKind::Uncategorized, "Stdout is not able to print")) } else { Ok(len as usize) } @@ -81,7 +81,7 @@ impl io::Write for Stderr { unsafe { len = abi::write(2, data.as_ptr() as *const u8, data.len()) } if len < 0 { - Err(io::Error::new_const(io::ErrorKind::Uncategorized, &"Stderr is not able to print")) + Err(io::const_io_error!(io::ErrorKind::Uncategorized, "Stderr is not able to print")) } else { Ok(len as usize) } @@ -93,7 +93,7 @@ impl io::Write for Stderr { unsafe { len = abi::write(2, data.as_ptr() as *const u8, data.len()) } if len < 0 { - Err(io::Error::new_const(io::ErrorKind::Uncategorized, &"Stderr is not able to print")) + Err(io::const_io_error!(io::ErrorKind::Uncategorized, "Stderr is not able to print")) } else { Ok(len as usize) } diff --git a/library/std/src/sys/hermit/thread.rs b/library/std/src/sys/hermit/thread.rs index 81b21fbbb16..e53a1fea6a0 100644 --- a/library/std/src/sys/hermit/thread.rs +++ b/library/std/src/sys/hermit/thread.rs @@ -39,7 +39,7 @@ impl Thread { // The thread failed to start and as a result p was not consumed. Therefore, it is // safe to reconstruct the box so that it gets deallocated. drop(Box::from_raw(p)); - Err(io::Error::new_const(io::ErrorKind::Uncategorized, &"Unable to create thread!")) + Err(io::const_io_error!(io::ErrorKind::Uncategorized, "Unable to create thread!")) } else { Ok(Thread { tid: tid }) }; |
