-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
closure field capturing: don't depend on alignment of packed fields #115315
closure field capturing: don't depend on alignment of packed fields #115315
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
cbbb044
to
e17af73
Compare
@bors try |
⌛ Trying commit e17af739d0ffdef2d40b093d9bedfa8f44205707 with merge 6c5b2f071bf9fe3a4b05942f31d42d076171aa61... |
☀️ Try build successful - checks-actions |
@craterbot run mode=build-and-test |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
e17af73
to
a671127
Compare
🎉 Experiment
|
@craterbot run mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-115315/retry-regressed-list.txt |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
None of these look like they would be caused by this PR... 🤷 @craterbot run mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-115315-1/retry-regressed-list.txt |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r=oli-obk |
I added the relnotes label because this is a breaking change that maybe should be announced in the release notes -- not sure of our exact policy here. |
…ent, r=oli-obk closure field capturing: don't depend on alignment of packed fields This fixes the closure field capture part of rust-lang#115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want. Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program: ```rust #![allow(dead_code)] mod m { // before patch #[derive(Default)] pub struct S1(u8); // after patch #[derive(Default)] pub struct S2(u64); } struct NoisyDrop; impl Drop for NoisyDrop { fn drop(&mut self) { eprintln!("dropped!"); } } #[repr(packed)] struct MyType { field: m::S1, // output changes when this becomes S2 other_field: NoisyDrop, third_field: Vec<()>, } fn test(r: MyType) { let c = || { let _val = std::ptr::addr_of!(r.field); let _val = r.third_field; }; drop(c); eprintln!("before dropping"); } fn main() { test(MyType { field: Default::default(), other_field: NoisyDrop, third_field: Vec::new(), }); } ``` Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much. Also see the [nomination comment](rust-lang#115315 (comment)). Cc `@rust-lang/wg-rfc-2229` `@ehuss`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry The runner has received a shutdown signal (aarch64-gnu) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (790309b): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.885s -> 632.342s (-0.09%) |
…i-obk closure field capturing: don't depend on alignment of packed fields This fixes the closure field capture part of rust-lang/rust#115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want. Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program: ```rust #![allow(dead_code)] mod m { // before patch #[derive(Default)] pub struct S1(u8); // after patch #[derive(Default)] pub struct S2(u64); } struct NoisyDrop; impl Drop for NoisyDrop { fn drop(&mut self) { eprintln!("dropped!"); } } #[repr(packed)] struct MyType { field: m::S1, // output changes when this becomes S2 other_field: NoisyDrop, third_field: Vec<()>, } fn test(r: MyType) { let c = || { let _val = std::ptr::addr_of!(r.field); let _val = r.third_field; }; drop(c); eprintln!("before dropping"); } fn main() { test(MyType { field: Default::default(), other_field: NoisyDrop, third_field: Vec::new(), }); } ``` Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much. Also see the [nomination comment](rust-lang/rust#115315 (comment)). Cc `@rust-lang/wg-rfc-2229` `@ehuss`
Pkgsrc changes: * Remove NetBSD-8 support (embedded LLVm requires newer C++ than what is in -8; it's conceivable that this could still build with an external LLVM) * undo powerpc 9.0 file naming tweak, since we no longer support -8. * Remove patch to LLVM for powerpc now included by upstream. * Minor adjustments, checksum changes etc. Upstream changes: Version 1.74.1 (2023-12-07) =========================== - [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM] (rust-lang/rust#118464) - [Clarify guarantees for std::mem::discriminant] (rust-lang/rust#118006) - [Fix some subtyping-related regressions] (rust-lang/rust#116415) Version 1.74.0 (2023-11-16) ========================== Language -------- - [Codify that `std::mem::Discriminant<T>` does not depend on any lifetimes in T] (rust-lang/rust#104299) - [Replace `private_in_public` lint with `private_interfaces` and `private_bounds` per RFC 2145] (rust-lang/rust#113126) Read more in [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html). - [Allow explicit `#[repr(Rust)]`] (rust-lang/rust#114201) - [closure field capturing: don't depend on alignment of packed fields] (rust-lang/rust#115315) - [Enable MIR-based drop-tracking for `async` blocks] (rust-lang/rust#107421) Compiler -------- - [stabilize combining +bundle and +whole-archive link modifiers] (rust-lang/rust#113301) - [Stabilize `PATH` option for `--print KIND=PATH`] (rust-lang/rust#114183) - [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`] (rust-lang/rust#115644) - [Promote loongarch64-unknown-none* to Tier 2] (rust-lang/rust#115368) - [Add `i686-pc-windows-gnullvm` as a tier 3 target] (rust-lang/rust#115687) Libraries --------- - [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err] (rust-lang/rust#98704) - [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`] (rust-lang/rust#111278) - [impl Step for IP addresses] (rust-lang/rust#113748) - [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`] (rust-lang/rust#114041) - [`impl TryFrom<char> for u16`] (rust-lang/rust#114065) - [Stabilize `io_error_other` feature] (rust-lang/rust#115453) - [Stabilize the `Saturating` type] (rust-lang/rust#115477) - [Stabilize const_transmute_copy] (rust-lang/rust#115520) Stabilized APIs --------------- - [`core::num::Saturating`] (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html) - [`impl From<io::Stdout> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio) - [`impl From<io::Stderr> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`std::ffi::OsString::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsString::into_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes) - [`std::ffi::OsStr::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsStr::as_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes) - [`std::io::Error::other`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other) - [`impl TryFrom<char> for u16`] (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16) - [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E) These APIs are now stable in const contexts: - [`core::mem::transmute_copy`] (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html) - [`str::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii) - [`[u8]::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii) Cargo ----- - [fix: Set MSRV for internal packages] (rust-lang/cargo#12381) - [config: merge lists in precedence order] (rust-lang/cargo#12515) - [fix(update): Clarify meaning of --aggressive as --recursive] (rust-lang/cargo#12544) - [fix(update): Make `-p` more convenient by being positional] (rust-lang/cargo#12545) - [feat(help): Add styling to help output ] (rust-lang/cargo#12578) - [feat(pkgid): Allow incomplete versions when unambigious] (rust-lang/cargo#12614) - [feat: stabilize credential-process and registry-auth] (rust-lang/cargo#12649) - [feat(cli): Add '-n' to dry-run] (rust-lang/cargo#12660) - [Add support for `target.'cfg(..)'.linker`] (rust-lang/cargo#12535) - [Stabilize `--keep-going`] (rust-lang/cargo#12568) - [feat: Stabilize lints] (rust-lang/cargo#12648) Rustdoc ------- - [Add warning block support in rustdoc] (rust-lang/rust#106561) - [Accept additional user-defined syntax classes in fenced code blocks] (rust-lang/rust#110800) - [rustdoc-search: add support for type parameters] (rust-lang/rust#112725) - [rustdoc: show inner enum and struct in type definition for concrete type] (rust-lang/rust#114855) Compatibility Notes ------------------- - [Raise minimum supported Apple OS versions] (rust-lang/rust#104385) - [make Cell::swap panic if the Cells partially overlap] (rust-lang/rust#114795) - [Reject invalid crate names in `--extern`] (rust-lang/rust#116001) - [Don't resolve generic impls that may be shadowed by dyn built-in impls] (rust-lang/rust#114941) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. None this cycle.
…i-obk closure field capturing: don't depend on alignment of packed fields This fixes the closure field capture part of rust-lang/rust#115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want. Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program: ```rust #![allow(dead_code)] mod m { // before patch #[derive(Default)] pub struct S1(u8); // after patch #[derive(Default)] pub struct S2(u64); } struct NoisyDrop; impl Drop for NoisyDrop { fn drop(&mut self) { eprintln!("dropped!"); } } #[repr(packed)] struct MyType { field: m::S1, // output changes when this becomes S2 other_field: NoisyDrop, third_field: Vec<()>, } fn test(r: MyType) { let c = || { let _val = std::ptr::addr_of!(r.field); let _val = r.third_field; }; drop(c); eprintln!("before dropping"); } fn main() { test(MyType { field: Default::default(), other_field: NoisyDrop, third_field: Vec::new(), }); } ``` Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much. Also see the [nomination comment](rust-lang/rust#115315 (comment)). Cc `@rust-lang/wg-rfc-2229` `@ehuss`
…i-obk closure field capturing: don't depend on alignment of packed fields This fixes the closure field capture part of rust-lang/rust#115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want. Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program: ```rust #![allow(dead_code)] mod m { // before patch #[derive(Default)] pub struct S1(u8); // after patch #[derive(Default)] pub struct S2(u64); } struct NoisyDrop; impl Drop for NoisyDrop { fn drop(&mut self) { eprintln!("dropped!"); } } #[repr(packed)] struct MyType { field: m::S1, // output changes when this becomes S2 other_field: NoisyDrop, third_field: Vec<()>, } fn test(r: MyType) { let c = || { let _val = std::ptr::addr_of!(r.field); let _val = r.third_field; }; drop(c); eprintln!("before dropping"); } fn main() { test(MyType { field: Default::default(), other_field: NoisyDrop, third_field: Vec::new(), }); } ``` Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much. Also see the [nomination comment](rust-lang/rust#115315 (comment)). Cc `@rust-lang/wg-rfc-2229` `@ehuss`
This fixes the closure field capture part of #115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from
u8
tou64
can never change how closures capture fields, which is probably what we want.Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program:
Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much.
Also see the nomination comment.
Cc @rust-lang/wg-rfc-2229 @ehuss