The const propagator cannot trace references. #73613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oli-obk
added
the
beta-nominated
rust-highfive
added
the
S-waiting-on-review
RalfJung
reviewed
Jun 22, 2020
|
Given how big and fundamental this flaw is, I wonder how it was possible that this even landed? I am starting to be increasingly worried about the correctness of our optimizations.^^ Any bug here is critical, and the code is easy to get wrong. |
RalfJung
reviewed
Jun 22, 2020
RalfJung
reviewed
Jun 22, 2020
An oversight on my side. I put most of the time in that review into the other |
|
I guess more constructively said: is there something we can do -- with types or tests or whatever -- to reduce the likelihood of such oversights? In Miri we have a couple of safety nets in place because the code is so tricky; this here is arguably even trickier so it is not very surprising that mistakes happen. |
lcnr
reviewed
Jun 22, 2020
oli-obk
force-pushed
the
const_prop_miscompile
branch
from
70ec8d9 to
bd0c12d
Compare
Idk, there's some research work on proving the correctness of optimizations, but that's likely either impossible for us here or too slow in practice since we'd have to write the optimizations in some scheme that allows running proofs on it. The typical const prop bug (propping something it should not) will likely be eliminated for good once we move to dataflow based const propping, because then we're using a single scheme that we can verify which isn't entangled with the actual individual const propping logic, which is much easier to check for correctness. |
Wanted: Alive2 for MIR. ;) (see this blog post) |
oli-obk
commented
Jun 22, 2020
| @@ -46,22 +46,8 @@ | |||
| // mir::Constant | |||
| // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:23: 7:24 | |||
| // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000003)) } | |||
| - _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | |||
There was a problem hiding this comment.
this is "fallout" of checking for Rvalue::AddressOf. I think we should exhaustively match every enum in const prop to prevent such things from falling through.
oli-obk
commented
Jun 22, 2020
| // value the local has right now. | ||
| // Thus, all locals that have their reference taken | ||
| // must not take part in propagation. | ||
| Self::remove_const(&mut self.ecx, place.local); |
There was a problem hiding this comment.
I tried simply asserting here, but that won't work for
- zsts or
- locals that got a reference assigned from a constant instead of an
Rvalue::Refand are now hitting thisRvalue::Refbecause the reference is getting reborrowed. This is basicallyProjection::Derefbiting us again - arguments, because they don't get marked as
NoPropagation
oli-obk
commented
Jun 22, 2020
| other => { | ||
| ConstPropMode::NoPropagation => {} | ||
| ConstPropMode::OnlyPropagateInto => {} | ||
| other @ ConstPropMode::FullConstProp => { |
There was a problem hiding this comment.
Part of exhaustively matching on things, but also part of the bugfix, because we don't go from NoPropagation to OnlyPropagateInto anymore.
oli-obk
commented
Jun 22, 2020
| @@ -813,7 +826,7 @@ impl<'tcx> Visitor<'tcx> for CanConstProp { | |||
| | MutatingUse(MutatingUseContext::Borrow) | |||
| | MutatingUse(MutatingUseContext::AddressOf) => { | |||
| trace!("local {:?} can't be propagaged because it's used: {:?}", local, context); | |||
| self.can_const_prop[local] = ConstPropMode::OnlyPropagateInto; | |||
| self.can_const_prop[local] = ConstPropMode::NoPropagation; | |||
There was a problem hiding this comment.
Main part of the bugfix, while we can do things for borrows, this is super hard to reason about and I don't think we should do it without dataflow
oli-obk
commented
Jun 22, 2020
| trace!("can't propagate into {:?}", place); | ||
| if place.local != RETURN_PLACE { | ||
| Self::remove_const(&mut self.ecx, place.local); | ||
| match can_const_prop { |
There was a problem hiding this comment.
more exhaustively matching.
oli-obk
commented
Jun 22, 2020
| @@ -890,6 +906,12 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> { | |||
| ); | |||
| Self::remove_const(&mut self.ecx, place.local); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
This arm didn't exist before, it's the else for the layout_of check. If we can't compute the layout, erase anything that's in that local, even if I don't think that it can actually happen that there's something in the local.
|
Looks like ui tests just need to be blessed. |
|
blessed ui tests |
oli-obk
force-pushed
the
const_prop_miscompile
branch
from
bd0c12d to
6b9ec71
Compare
|
Now the 32-bit MIR tests need to be blessed. r=me with that. |
wesleywiser
approved these changes
Jun 22, 2020
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
removed
the
beta-nominated
Thus we avoid propagation of a local the moment we encounter references to it.
oli-obk
force-pushed
the
const_prop_miscompile
branch
from
6b9ec71 to
5fa8b08
Compare
|
@bors r=wesleywiser |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
bumping priority |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 23, 2020
…arth Rollup of 11 pull requests Successful merges: - rust-lang#72780 (Enforce doc alias check) - rust-lang#72876 (Mention that BTreeMap::new() doesn't allocate) - rust-lang#73244 (Check for assignments between non-conflicting generator saved locals) - rust-lang#73488 (code coverage foundation for hash and num_counters) - rust-lang#73523 (Fix -Z unpretty=everybody_loops) - rust-lang#73587 (Move remaining `NodeId` APIs from `Definitions` to `Resolver`) - rust-lang#73601 (Point at the call span when overflow occurs during monomorphization) - rust-lang#73613 (The const propagator cannot trace references.) - rust-lang#73614 (fix `intrinsics::needs_drop` docs) - rust-lang#73630 (Provide context on E0308 involving fn items) - rust-lang#73665 (rustc: Modernize wasm checks for atomics) Failed merges: r? @ghost
RalfJung
reviewed
Jun 24, 2020
| @@ -575,8 +575,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |||
| } | |||
|
|
|||
| // Do not try creating references (#67862) | |||
| Rvalue::Ref(_, _, place_ref) => { | |||
| trace!("skipping Ref({:?})", place_ref); | |||
| Rvalue::AddressOf(_, place) | Rvalue::Ref(_, _, place) => { | |||
There was a problem hiding this comment.
Maybe this match should also be made exhaustive? It currently has a fallback case, basically assuming "every rvalue I don't know will definitely be harmless". That seems like a rather strong assumption.
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 4, 2020
…atch, r=oli-obk Use exhaustive match in const_prop.rs Addresses a comment left by @RalfJung on rust-lang#73613 r? @RalfJung
jonas-schievink
added
stable-nominated
|
Looks like this missed the beta cutoff, so the miscompilation is now on stable #74739 |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 26, 2020
…Mark-Simulacrum Stable backport of rust-lang#73613 This is the backport of rust-lang#73613 to stable. r? @ghost cc @Mark-Simulacrum In addition the tests added in the original PR passing, I've also confirmed that the test case in rust-lang#74739 works correctly.
|
discussed at T-compiler meeting; stable-accepted |
pnkfelix
added
the
stable-accepted
|
This made it to stable in 1.45.1 via #74746. |
cuviper
removed
the
stable-nominated
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Aug 6, 2020
While here clean up all pkglint warnings. Changes since 1.44.1: Version 1.45.2 (2020-08-03) ========================== * [Fix bindings in tuple struct patterns][74954] * [Fix track_caller integration with trait objects][74784] [74954]: rust-lang/rust#74954 [74784]: rust-lang/rust#74784 Version 1.45.1 (2020-07-30) ========================== * [Fix const propagation with references.][73613] * [rustfmt accepts rustfmt_skip in cfg_attr again.][73078] * [Avoid spurious implicit region bound.][74509] * [Install clippy on x.py install][74457] [73613]: rust-lang/rust#73613 [73078]: rust-lang/rust#73078 [74509]: rust-lang/rust#74509 [74457]: rust-lang/rust#74457 Version 1.45.0 (2020-07-16) ========================== Language -------- - [Out of range float to int conversions using `as` has been defined as a saturating conversion.][71269] This was previously undefined behaviour, but you can use the `{f64, f32}::to_int_unchecked` methods to continue using the current behaviour, which may be desirable in rare performance sensitive situations. - [`mem::Discriminant<T>` now uses `T`'s discriminant type instead of always using `u64`.][70705] - [Function like procedural macros can now be used in expression, pattern, and statement positions.][68717] This means you can now use a function-like procedural macro anywhere you can use a declarative (`macro_rules!`) macro. Compiler -------- - [You can now override individual target features through the `target-feature` flag.][72094] E.g. `-C target-feature=+avx2 -C target-feature=+fma` is now equivalent to `-C target-feature=+avx2,+fma`. - [Added the `force-unwind-tables` flag.][69984] This option allows rustc to always generate unwind tables regardless of panic strategy. - [Added the `embed-bitcode` flag.][71716] This codegen flag allows rustc to include LLVM bitcode into generated `rlib`s (this is on by default). - [Added the `tiny` value to the `code-model` codegen flag.][72397] - [Added tier 3 support\* for the `mipsel-sony-psp` target.][72062] - [Added tier 3 support for the `thumbv7a-uwp-windows-msvc` target.][72133] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`net::{SocketAddr, SocketAddrV4, SocketAddrV6}` now implements `PartialOrd` and `Ord`.][72239] - [`proc_macro::TokenStream` now implements `Default`.][72234] - [You can now use `char` with `ops::{Range, RangeFrom, RangeFull, RangeInclusive, RangeTo}` to iterate over a range of codepoints.][72413] E.g. you can now write the following; ```rust for ch in 'a'..='z' { print!("{}", ch); } println!(); // Prints "abcdefghijklmnopqrstuvwxyz" ``` - [`OsString` now implements `FromStr`.][71662] - [The `saturating_neg` method as been added to all signed integer primitive types, and the `saturating_abs` method has been added for all integer primitive types.][71886] - [`Arc<T>`, `Rc<T>` now implement `From<Cow<'_, T>>`, and `Box` now implements `From<Cow>` when `T` is `[T: Copy]`, `str`, `CStr`, `OsStr`, or `Path`.][71447] - [`Box<[T]>` now implements `From<[T; N]>`.][71095] - [`BitOr` and `BitOrAssign` are implemented for all `NonZero` integer types.][69813] - [The `fetch_min`, and `fetch_max` methods have been added to all atomic integer types.][72324] - [The `fetch_update` method has been added to all atomic integer types.][71843] Stabilized APIs --------------- - [`Arc::as_ptr`] - [`BTreeMap::remove_entry`] - [`Rc::as_ptr`] - [`rc::Weak::as_ptr`] - [`rc::Weak::from_raw`] - [`rc::Weak::into_raw`] - [`str::strip_prefix`] - [`str::strip_suffix`] - [`sync::Weak::as_ptr`] - [`sync::Weak::from_raw`] - [`sync::Weak::into_raw`] - [`char::UNICODE_VERSION`] - [`Span::resolved_at`] - [`Span::located_at`] - [`Span::mixed_site`] - [`unix::process::CommandExt::arg0`] Cargo ----- Misc ---- - [Rustdoc now supports strikethrough text in Markdown.][71928] E.g. `~~outdated information~~` becomes "~~outdated information~~". - [Added an emoji to Rustdoc's deprecated API message.][72014] Compatibility Notes ------------------- - [Trying to self initialize a static value (that is creating a value using itself) is unsound and now causes a compile error.][71140] - [`{f32, f64}::powi` now returns a slightly different value on Windows.][73420] This is due to changes in LLVM's intrinsics which `{f32, f64}::powi` uses. - [Rustdoc's CLI's extra error exit codes have been removed.][71900] These were previously undocumented and not intended for public use. Rustdoc still provides a non-zero exit code on errors. Internals Only -------------- - [Make clippy a git subtree instead of a git submodule][70655] - [Unify the undo log of all snapshot types][69464] [73420]: rust-lang/rust#73420 [72324]: rust-lang/rust#72324 [71843]: rust-lang/rust#71843 [71886]: rust-lang/rust#71886 [72234]: rust-lang/rust#72234 [72239]: rust-lang/rust#72239 [72397]: rust-lang/rust#72397 [72413]: rust-lang/rust#72413 [72014]: rust-lang/rust#72014 [72062]: rust-lang/rust#72062 [72094]: rust-lang/rust#72094 [72133]: rust-lang/rust#72133 [71900]: rust-lang/rust#71900 [71928]: rust-lang/rust#71928 [71662]: rust-lang/rust#71662 [71716]: rust-lang/rust#71716 [71447]: rust-lang/rust#71447 [71269]: rust-lang/rust#71269 [71095]: rust-lang/rust#71095 [71140]: rust-lang/rust#71140 [70655]: rust-lang/rust#70655 [70705]: rust-lang/rust#70705 [69984]: rust-lang/rust#69984 [69813]: rust-lang/rust#69813 [69464]: rust-lang/rust#69464 [68717]: rust-lang/rust#68717 [`Arc::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.as_ptr [`BTreeMap::remove_entry`]: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.remove_entry [`Rc::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr [`rc::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.as_ptr [`rc::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.from_raw [`rc::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.into_raw [`sync::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.as_ptr [`sync::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.from_raw [`sync::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.into_raw [`str::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_prefix [`str::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_suffix [`char::UNICODE_VERSION`]: https://doc.rust-lang.org/stable/std/char/constant.UNICODE_VERSION.html [`Span::resolved_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.resolved_at [`Span::located_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.located_at [`Span::mixed_site`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.mixed_site [`unix::process::CommandExt::arg0`]: https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.arg0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thus we avoid propagation of a local the moment we encounter references to it.
fixes #73609
cc @RalfJung
r? @wesleywiser