From ce60da497b664bae7b1476ae4502a073dbb12127 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 29 Sep 2019 10:45:47 +0200 Subject: [PATCH 01/16] Fix `vec![x; n]` with null raw fat pointer zeroing the pointer metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/rust-lang/rust/pull/49496 introduced specialization based on: ``` unsafe impl IsZero for *mut T { fn is_zero(&self) -> bool { (*self).is_null() } } ``` … to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`, which is incorrect for fat pointers since `<*mut T>::is_null` only looks at the data component. That is, a fat pointer can be “null” without being made entirely of zero bits. This commit fixes it by removing the `?Sized` bound on this impl (and the corresponding `*const T` one). This regresses `vec![x; n]` with `x` a null raw slice of length zero, but that seems exceptionally uncommon. (Vtable pointers are never null, so raw trait objects would not take the fast path anyway. An alternative to keep the `?Sized` bound (or even generalize to `impl IsZero for U`) would be to cast to `&[u8]` of length `size_of::()`, but the optimizer seems not to be able to propagate alignment information and sticks with comparing one byte at a time: https://rust.godbolt.org/z/xQFkwL ---- Without the library change, the new test fails as follows: ``` ---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ---- [src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8 [src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000 thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5 ``` --- src/liballoc/tests/vec.rs | 48 +++++++++++++++++++++++++++++++++++++++ src/liballoc/vec.rs | 4 ++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/liballoc/tests/vec.rs b/src/liballoc/tests/vec.rs index 29a22aa0315b0..98d013dfa2b57 100644 --- a/src/liballoc/tests/vec.rs +++ b/src/liballoc/tests/vec.rs @@ -1281,3 +1281,51 @@ fn test_stable_push_pop() { v.pop().unwrap(); assert_eq!(*v0, 13); } + +// https://github.com/rust-lang/rust/pull/49496 introduced specialization based on: +// +// ``` +// unsafe impl IsZero for *mut T { +// fn is_zero(&self) -> bool { +// (*self).is_null() +// } +// } +// ``` +// +// … to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`, +// which is incorrect for fat pointers since `<*mut T>::is_null` only looks at the data component. +// That is, a fat pointer can be “null” without being made entirely of zero bits. +#[test] +fn vec_macro_repeating_null_raw_fat_pointer() { + let raw_dyn = &mut (|| ()) as &mut dyn Fn() as *mut dyn Fn(); + let vtable = dbg!(ptr_metadata(raw_dyn)); + let null_raw_dyn = ptr_from_raw_parts(std::ptr::null_mut(), vtable); + assert!(null_raw_dyn.is_null()); + + let vec = vec![null_raw_dyn; 1]; + dbg!(ptr_metadata(vec[0])); + assert!(vec[0] == null_raw_dyn); + + // Polyfill for https://github.com/rust-lang/rfcs/pull/2580 + + fn ptr_metadata(ptr: *mut dyn Fn()) -> *mut () { + unsafe { + std::mem::transmute::<*mut dyn Fn(), DynRepr>(ptr).vtable + } + } + + fn ptr_from_raw_parts(data: *mut (), vtable: *mut()) -> *mut dyn Fn() { + unsafe { + std::mem::transmute::(DynRepr { + data, + vtable + }) + } + } + + #[repr(C)] + struct DynRepr { + data: *mut (), + vtable: *mut (), + } +} diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index e5672f8542ff6..f6f59ae408272 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -1734,14 +1734,14 @@ impl_is_zero!(char, |x| x == '\0'); impl_is_zero!(f32, |x: f32| x.to_bits() == 0); impl_is_zero!(f64, |x: f64| x.to_bits() == 0); -unsafe impl IsZero for *const T { +unsafe impl IsZero for *const T { #[inline] fn is_zero(&self) -> bool { (*self).is_null() } } -unsafe impl IsZero for *mut T { +unsafe impl IsZero for *mut T { #[inline] fn is_zero(&self) -> bool { (*self).is_null() From 6c01c0e9b5b069ee2a61b6078058c69880a14f19 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 29 Sep 2019 11:14:59 +0200 Subject: [PATCH 02/16] Zero-initialize `vec![None; n]` for `Option<&T>`, `Option<&mut T>` and `Option>` --- src/liballoc/vec.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index e5672f8542ff6..d5089c5e2eba9 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -1748,6 +1748,31 @@ unsafe impl IsZero for *mut T { } } +// `Option<&T>`, `Option<&mut T>` and `Option>` are guaranteed to represent `None` as null. +// For fat pointers, the bytes that would be the pointer metadata in the `Some` variant +// are padding in the `None` variant, so ignoring them and zero-initializing instead is ok. + +unsafe impl IsZero for Option<&T> { + #[inline] + fn is_zero(&self) -> bool { + self.is_none() + } +} + +unsafe impl IsZero for Option<&mut T> { + #[inline] + fn is_zero(&self) -> bool { + self.is_none() + } +} + +unsafe impl IsZero for Option> { + #[inline] + fn is_zero(&self) -> bool { + self.is_none() + } +} + //////////////////////////////////////////////////////////////////////////////// // Common trait implementations for Vec From 218571074887c53b5b62050f8cd08b9ba98a2b03 Mon Sep 17 00:00:00 2001 From: Tyler Mandry Date: Sat, 28 Sep 2019 15:44:20 -0700 Subject: [PATCH 03/16] Use https for curl when building for linux --- src/ci/docker/dist-x86_64-linux/build-curl.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ci/docker/dist-x86_64-linux/build-curl.sh b/src/ci/docker/dist-x86_64-linux/build-curl.sh index fb8b63d7920b1..8200bbe2fdce5 100755 --- a/src/ci/docker/dist-x86_64-linux/build-curl.sh +++ b/src/ci/docker/dist-x86_64-linux/build-curl.sh @@ -3,9 +3,11 @@ set -ex source shared.sh -VERSION=7.51.0 +VERSION=7.66.0 -curl http://cool.haxx.se/download/curl-$VERSION.tar.bz2 | tar xjf - +curl https://rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com/rustc/curl-$VERSION.tar.xz \ + | xz --decompress \ + | tar xf - mkdir curl-build cd curl-build From 6c6d27d685c05671e156b506570b07902a6b76b7 Mon Sep 17 00:00:00 2001 From: hman523 Date: Mon, 30 Sep 2019 00:26:42 -0500 Subject: [PATCH 04/16] Fixed a misleading documentation issue #64844 --- src/libcore/option.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcore/option.rs b/src/libcore/option.rs index 5569d99f8d81d..301e432c98dfc 100644 --- a/src/libcore/option.rs +++ b/src/libcore/option.rs @@ -46,7 +46,7 @@ //! # Options and pointers ("nullable" pointers) //! //! Rust's pointer types must always point to a valid location; there are -//! no "null" pointers. Instead, Rust has *optional* pointers, like +//! no "null" references. Instead, Rust has *optional* pointers, like //! the optional owned box, [`Option`]`<`[`Box`]`>`. //! //! The following example uses [`Option`] to create an optional box of From cc9db523bec3bf3828c0427a53801f5ffbea4514 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 11 Sep 2019 13:51:03 +0200 Subject: [PATCH 05/16] Add long error explanation for E0493 --- src/librustc_mir/error_codes.rs | 46 ++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/error_codes.rs b/src/librustc_mir/error_codes.rs index ba299e9463b8d..6208bb294d205 100644 --- a/src/librustc_mir/error_codes.rs +++ b/src/librustc_mir/error_codes.rs @@ -1128,6 +1128,51 @@ Remember this solution is unsafe! You will have to ensure that accesses to the cell are synchronized. "##, +E0493: r##" +A type with a `Drop` implementation was destructured when trying to initialize +a static item. + +Erroneous code example: + +```compile_fail,E0493 +enum DropType { + A, +} + +impl Drop for DropType { + fn drop(&mut self) {} +} + +struct Foo { + field1: DropType, +} + +static FOO: Foo = Foo { ..Foo { field1: DropType::A } }; // error! +``` + +The problem here is that if the given type or one of its fields implements the +`Drop` trait, this `Drop` implementation cannot be called during the static +type initialization which might cause a memory leak. To prevent this issue, +you need to instantiate all the static type's fields by hand. + +``` +enum DropType { + A, +} + +impl Drop for DropType { + fn drop(&mut self) {} +} + +struct Foo { + field1: DropType, +} + +static FOO: Foo = Foo { field1: DropType::A }; // We initialize all fields + // by hand. +``` +"##, + E0499: r##" A variable was borrowed as mutable more than once. Erroneous code example: @@ -2391,7 +2436,6 @@ There are some known bugs that trigger this message. // E0299, // mismatched types between arms // E0471, // constant evaluation error (in pattern) // E0385, // {} in an aliasable location - E0493, // destructors cannot be evaluated at compile-time E0521, // borrowed data escapes outside of closure E0524, // two closures require unique access to `..` at the same time E0526, // shuffle indices are not constant From 9f978b7f17c3e40b1fb5eb42bc1cfd8cf8338124 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 12 Sep 2019 15:52:51 +0200 Subject: [PATCH 06/16] update tests --- src/test/ui/check-static-values-constraints.stderr | 2 +- src/test/ui/consts/const-eval/const_let.stderr | 1 + src/test/ui/consts/min_const_fn/min_const_fn.stderr | 4 ++-- .../feature-gate-unleash_the_miri_inside_of_you.stderr | 1 + src/test/ui/span/E0493.stderr | 1 + src/test/ui/static/static-drop-scope.stderr | 3 ++- 6 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test/ui/check-static-values-constraints.stderr b/src/test/ui/check-static-values-constraints.stderr index a13c217483d5d..7d7ecbd1a26a5 100644 --- a/src/test/ui/check-static-values-constraints.stderr +++ b/src/test/ui/check-static-values-constraints.stderr @@ -108,5 +108,5 @@ LL | let y = { static x: Box = box 3; x }; error: aborting due to 17 previous errors -Some errors have detailed explanations: E0010, E0015, E0019, E0507. +Some errors have detailed explanations: E0010, E0015, E0019, E0493, E0507. For more information about an error, try `rustc --explain E0010`. diff --git a/src/test/ui/consts/const-eval/const_let.stderr b/src/test/ui/consts/const-eval/const_let.stderr index 0a6a222ae2963..4753222a7c07d 100644 --- a/src/test/ui/consts/const-eval/const_let.stderr +++ b/src/test/ui/consts/const-eval/const_let.stderr @@ -24,3 +24,4 @@ LL | const Z2: () = { let mut x; x = None; x = Some(FakeNeedsDrop); }; error: aborting due to 4 previous errors +For more information about this error, try `rustc --explain E0493`. diff --git a/src/test/ui/consts/min_const_fn/min_const_fn.stderr b/src/test/ui/consts/min_const_fn/min_const_fn.stderr index 211902b687b1b..db70a775b779c 100644 --- a/src/test/ui/consts/min_const_fn/min_const_fn.stderr +++ b/src/test/ui/consts/min_const_fn/min_const_fn.stderr @@ -328,5 +328,5 @@ LL | const fn no_fn_ptrs2() -> fn() { fn foo() {} foo } error: aborting due to 36 previous errors -Some errors have detailed explanations: E0515, E0723. -For more information about an error, try `rustc --explain E0515`. +Some errors have detailed explanations: E0493, E0515, E0723. +For more information about an error, try `rustc --explain E0493`. diff --git a/src/test/ui/consts/miri_unleashed/feature-gate-unleash_the_miri_inside_of_you.stderr b/src/test/ui/consts/miri_unleashed/feature-gate-unleash_the_miri_inside_of_you.stderr index c56ebf60df481..7ede44c65b83a 100644 --- a/src/test/ui/consts/miri_unleashed/feature-gate-unleash_the_miri_inside_of_you.stderr +++ b/src/test/ui/consts/miri_unleashed/feature-gate-unleash_the_miri_inside_of_you.stderr @@ -14,3 +14,4 @@ LL | const X: Vec = Vec::new(); error: aborting due to 2 previous errors +For more information about this error, try `rustc --explain E0493`. diff --git a/src/test/ui/span/E0493.stderr b/src/test/ui/span/E0493.stderr index 7e164ba9681c6..d05e89e257f45 100644 --- a/src/test/ui/span/E0493.stderr +++ b/src/test/ui/span/E0493.stderr @@ -6,3 +6,4 @@ LL | const F : Foo = (Foo { a : 0 }, Foo { a : 1 }).1; error: aborting due to previous error +For more information about this error, try `rustc --explain E0493`. diff --git a/src/test/ui/static/static-drop-scope.stderr b/src/test/ui/static/static-drop-scope.stderr index 8a23dad1ba3ea..bc08f33f82093 100644 --- a/src/test/ui/static/static-drop-scope.stderr +++ b/src/test/ui/static/static-drop-scope.stderr @@ -68,4 +68,5 @@ LL | const EARLY_DROP_C_OPTION_CONSTANT: i32 = (HELPER, 0).1; error: aborting due to 10 previous errors -For more information about this error, try `rustc --explain E0716`. +Some errors have detailed explanations: E0493, E0716. +For more information about an error, try `rustc --explain E0493`. From 67eabe110be3f958df7063d961716002fd2c3030 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 27 Sep 2019 13:36:50 +0200 Subject: [PATCH 07/16] Add long error explanation for E0550 --- src/libsyntax/error_codes.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/libsyntax/error_codes.rs b/src/libsyntax/error_codes.rs index 9925dd8ada0d5..8a78daee6e49b 100644 --- a/src/libsyntax/error_codes.rs +++ b/src/libsyntax/error_codes.rs @@ -144,6 +144,25 @@ fn deprecated_function() {} ``` "##, +E0550: r##" +More than one `deprecated` attribute has been put on an item. + +Erroneous code example: + +```compile_fail,E0550 +#[deprecated(note = "because why not?")] +#[deprecated(note = "right?")] // error! +fn the_banished() {} +``` + +The `deprecated` attribute can only be present **once** on an item. + +``` +#[deprecated(note = "because why not, right?")] +fn the_banished() {} // ok! +``` +"##, + E0552: r##" A unrecognized representation attribute was used. @@ -435,7 +454,6 @@ features in the `-Z allow_features` flag. // rustc_deprecated attribute must be paired with either stable or unstable // attribute E0549, - E0550, // multiple deprecated attributes E0551, // incorrect meta item E0553, // multiple rustc_const_unstable attributes // E0555, // replaced with a generic attribute input check From e67ae0e023edcf78ecf64e60da67ff19261afc09 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 27 Sep 2019 14:08:47 +0200 Subject: [PATCH 08/16] update ui tests --- src/test/ui/deprecation/deprecation-sanity.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/deprecation/deprecation-sanity.stderr b/src/test/ui/deprecation/deprecation-sanity.stderr index 7ff68a1038b1c..15afa78b140d5 100644 --- a/src/test/ui/deprecation/deprecation-sanity.stderr +++ b/src/test/ui/deprecation/deprecation-sanity.stderr @@ -54,5 +54,5 @@ LL | #[deprecated(since = "a", since = "b", note = "c")] error: aborting due to 9 previous errors -Some errors have detailed explanations: E0538, E0541, E0565. +Some errors have detailed explanations: E0538, E0541, E0550, E0565. For more information about an error, try `rustc --explain E0538`. From 5bf4397abca2f22fd76af5c463d292216d1a56d3 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Tue, 1 Oct 2019 00:38:08 +0900 Subject: [PATCH 09/16] Add test for issue-64662 --- src/test/ui/consts/issue-64662.rs | 10 ++++++++++ src/test/ui/consts/issue-64662.stderr | 15 +++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 src/test/ui/consts/issue-64662.rs create mode 100644 src/test/ui/consts/issue-64662.stderr diff --git a/src/test/ui/consts/issue-64662.rs b/src/test/ui/consts/issue-64662.rs new file mode 100644 index 0000000000000..e3a8c85830f73 --- /dev/null +++ b/src/test/ui/consts/issue-64662.rs @@ -0,0 +1,10 @@ +enum Foo { + A = foo(), //~ ERROR: type annotations needed + B = foo(), //~ ERROR: type annotations needed +} + +const fn foo() -> isize { + 0 +} + +fn main() {} diff --git a/src/test/ui/consts/issue-64662.stderr b/src/test/ui/consts/issue-64662.stderr new file mode 100644 index 0000000000000..b81daae330bfa --- /dev/null +++ b/src/test/ui/consts/issue-64662.stderr @@ -0,0 +1,15 @@ +error[E0282]: type annotations needed + --> $DIR/issue-64662.rs:2:9 + | +LL | A = foo(), + | ^^^ cannot infer type for `T` + +error[E0282]: type annotations needed + --> $DIR/issue-64662.rs:3:9 + | +LL | B = foo(), + | ^^^ cannot infer type for `T` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0282`. From cdf1852a61db9d21fc36dfc806a960a28aa059c0 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Mon, 30 Sep 2019 16:12:01 +0000 Subject: [PATCH 10/16] Add missing links for mem::needs_drop --- src/libcore/mem/mod.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 8767625d4ed37..95ad4272cedd0 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -368,15 +368,17 @@ pub fn align_of_val(val: &T) -> usize { /// make a difference in release builds (where a loop that has no side-effects /// is easily detected and eliminated), but is often a big win for debug builds. /// -/// Note that `ptr::drop_in_place` already performs this check, so if your workload -/// can be reduced to some small number of drop_in_place calls, using this is -/// unnecessary. In particular note that you can drop_in_place a slice, and that +/// Note that [`drop_in_place`] already performs this check, so if your workload +/// can be reduced to some small number of [`drop_in_place`] calls, using this is +/// unnecessary. In particular note that you can [`drop_in_place`] a slice, and that /// will do a single needs_drop check for all the values. /// /// Types like Vec therefore just `drop_in_place(&mut self[..])` without using -/// needs_drop explicitly. Types like `HashMap`, on the other hand, have to drop +/// `needs_drop` explicitly. Types like [`HashMap`], on the other hand, have to drop /// values one at a time and should use this API. /// +/// [`drop_in_place`]: ../ptr/fn.drop_in_place.html +/// [`HashMap`]: ../../std/collections/struct.HashMap.html /// /// # Examples /// From f33d94d88f800ef00a13747a3ee9dda77f6f42d1 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 26 Sep 2019 16:24:02 -0700 Subject: [PATCH 11/16] Fix typo in docs --- src/librustc_mir/dataflow/generic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/dataflow/generic.rs b/src/librustc_mir/dataflow/generic.rs index 6f598469e9dae..18dd8a0ecbd05 100644 --- a/src/librustc_mir/dataflow/generic.rs +++ b/src/librustc_mir/dataflow/generic.rs @@ -77,7 +77,7 @@ pub trait Analysis<'tcx>: BottomValue { location: Location, ); - /// Updates the current dataflow state with the effect of evaluating a statement. + /// Updates the current dataflow state with the effect of evaluating a terminator. /// /// Note that the effect of a successful return from a `Call` terminator should **not** be /// acounted for in this function. That should go in `apply_call_return_effect`. For example, From 3e88aa20b49fdc7693e253dafc3d782973f05c84 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 26 Sep 2019 16:25:27 -0700 Subject: [PATCH 12/16] Allow `ResultsCursor` to borrow the underlying `Results` --- src/librustc_mir/dataflow/generic.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/dataflow/generic.rs b/src/librustc_mir/dataflow/generic.rs index 18dd8a0ecbd05..400f612a0fc9f 100644 --- a/src/librustc_mir/dataflow/generic.rs +++ b/src/librustc_mir/dataflow/generic.rs @@ -180,17 +180,20 @@ impl CursorPosition { } } +type ResultsRefCursor<'a, 'mir, 'tcx, A> = + ResultsCursor<'mir, 'tcx, A, &'a Results<'tcx, A>>; + /// Inspect the results of dataflow analysis. /// /// This cursor has linear performance when visiting statements in a block in order. Visiting /// statements within a block in reverse order is `O(n^2)`, where `n` is the number of statements /// in that block. -pub struct ResultsCursor<'mir, 'tcx, A> +pub struct ResultsCursor<'mir, 'tcx, A, R = Results<'tcx, A>> where A: Analysis<'tcx>, { body: &'mir mir::Body<'tcx>, - results: Results<'tcx, A>, + results: R, state: BitSet, pos: CursorPosition, @@ -202,24 +205,29 @@ where is_call_return_effect_applied: bool, } -impl<'mir, 'tcx, A> ResultsCursor<'mir, 'tcx, A> +impl<'mir, 'tcx, A, R> ResultsCursor<'mir, 'tcx, A, R> where A: Analysis<'tcx>, + R: Borrow>, { /// Returns a new cursor for `results` that points to the start of the `START_BLOCK`. - pub fn new(body: &'mir mir::Body<'tcx>, results: Results<'tcx, A>) -> Self { + pub fn new(body: &'mir mir::Body<'tcx>, results: R) -> Self { ResultsCursor { body, pos: CursorPosition::AtBlockStart(mir::START_BLOCK), is_call_return_effect_applied: false, - state: results.entry_sets[mir::START_BLOCK].clone(), + state: results.borrow().entry_sets[mir::START_BLOCK].clone(), results, } } + pub fn analysis(&self) -> &A { + &self.results.borrow().analysis + } + /// Resets the cursor to the start of the given `block`. pub fn seek_to_block_start(&mut self, block: BasicBlock) { - self.state.overwrite(&self.results.entry_sets[block]); + self.state.overwrite(&self.results.borrow().entry_sets[block]); self.pos = CursorPosition::AtBlockStart(block); self.is_call_return_effect_applied = false; } @@ -275,7 +283,7 @@ where } = &term.kind { if !self.is_call_return_effect_applied { self.is_call_return_effect_applied = true; - self.results.analysis.apply_call_return_effect( + self.results.borrow().analysis.apply_call_return_effect( &mut self.state, target.block, func, @@ -316,7 +324,7 @@ where }; let block_data = &self.body.basic_blocks()[target_block]; - self.results.analysis.apply_partial_block_effect( + self.results.borrow().analysis.apply_partial_block_effect( &mut self.state, target_block, block_data, From d37c3186779e6fcafe216e291ce30d1775ce4cbb Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 26 Sep 2019 16:26:11 -0700 Subject: [PATCH 13/16] Add graphviz debug output for generic dataflow --- src/librustc_mir/dataflow/generic.rs | 103 ++++- src/librustc_mir/dataflow/generic/graphviz.rs | 412 ++++++++++++++++++ 2 files changed, 510 insertions(+), 5 deletions(-) create mode 100644 src/librustc_mir/dataflow/generic/graphviz.rs diff --git a/src/librustc_mir/dataflow/generic.rs b/src/librustc_mir/dataflow/generic.rs index 400f612a0fc9f..dd6238b80d174 100644 --- a/src/librustc_mir/dataflow/generic.rs +++ b/src/librustc_mir/dataflow/generic.rs @@ -16,16 +16,24 @@ //! [gk]: https://en.wikipedia.org/wiki/Data-flow_analysis#Bit_vector_problems //! [#64566]: https://github.com/rust-lang/rust/pull/64566 +use std::borrow::Borrow; use std::cmp::Ordering; -use std::ops; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::{fs, io, ops}; +use rustc::hir::def_id::DefId; use rustc::mir::{self, traversal, BasicBlock, Location}; +use rustc::ty::{self, TyCtxt}; +use rustc_data_structures::work_queue::WorkQueue; use rustc_index::bit_set::BitSet; use rustc_index::vec::{Idx, IndexVec}; -use rustc_data_structures::work_queue::WorkQueue; +use syntax::symbol::sym; use crate::dataflow::BottomValue; +mod graphviz; + /// A specific kind of dataflow analysis. /// /// To run a dataflow analysis, one must set the initial state of the `START_BLOCK` via @@ -62,6 +70,13 @@ pub trait Analysis<'tcx>: BottomValue { /// and try to keep it short. const NAME: &'static str; + /// How each element of your dataflow state will be displayed during debugging. + /// + /// By default, this is the `fmt::Debug` representation of `Self::Idx`. + fn pretty_print_idx(&self, w: &mut impl io::Write, idx: Self::Idx) -> io::Result<()> { + write!(w, "{:?}", idx) + } + /// The size of each bitvector allocated for each block. fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize; @@ -357,7 +372,9 @@ where { analysis: A, bits_per_block: usize, + tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, + def_id: DefId, dead_unwinds: &'a BitSet, entry_sets: IndexVec>, } @@ -367,7 +384,9 @@ where A: Analysis<'tcx>, { pub fn new( + tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, + def_id: DefId, dead_unwinds: &'a BitSet, analysis: A, ) -> Self { @@ -385,7 +404,9 @@ where Engine { analysis, bits_per_block, + tcx, body, + def_id, dead_unwinds, entry_sets, } @@ -421,10 +442,26 @@ where ); } - Results { - analysis: self.analysis, - entry_sets: self.entry_sets, + let Engine { + tcx, + body, + def_id, + analysis, + entry_sets, + .. + } = self; + + let results = Results { analysis, entry_sets }; + + let attrs = tcx.get_attrs(def_id); + if let Some(path) = get_dataflow_graphviz_output_path(tcx, attrs, A::NAME) { + let result = write_dataflow_graphviz_results(body, def_id, &path, &results); + if let Err(e) = result { + warn!("Failed to write dataflow results to {}: {}", path.display(), e); + } } + + results } fn propagate_bits_into_graph_successors_of( @@ -518,3 +555,59 @@ where } } } + +/// Looks for attributes like `#[rustc_mir(borrowck_graphviz_postflow="./path/to/suffix.dot")]` and +/// extracts the path with the given analysis name prepended to the suffix. +/// +/// Returns `None` if no such attribute exists. +fn get_dataflow_graphviz_output_path( + tcx: TyCtxt<'tcx>, + attrs: ty::Attributes<'tcx>, + analysis: &str, +) -> Option { + let mut rustc_mir_attrs = attrs + .into_iter() + .filter(|attr| attr.check_name(sym::rustc_mir)) + .flat_map(|attr| attr.meta_item_list().into_iter().flat_map(|v| v.into_iter())); + + let borrowck_graphviz_postflow = rustc_mir_attrs + .find(|attr| attr.check_name(sym::borrowck_graphviz_postflow))?; + + let path_and_suffix = match borrowck_graphviz_postflow.value_str() { + Some(p) => p, + None => { + tcx.sess.span_err( + borrowck_graphviz_postflow.span(), + "borrowck_graphviz_postflow requires a path", + ); + + return None; + } + }; + + // Change "path/suffix.dot" to "path/analysis_name_suffix.dot" + let mut ret = PathBuf::from(path_and_suffix.to_string()); + let suffix = ret.file_name().unwrap(); + + let mut file_name: OsString = analysis.into(); + file_name.push("_"); + file_name.push(suffix); + ret.set_file_name(file_name); + + Some(ret) +} + +fn write_dataflow_graphviz_results>( + body: &mir::Body<'tcx>, + def_id: DefId, + path: &Path, + results: &Results<'tcx, A> +) -> io::Result<()> { + debug!("printing dataflow results for {:?} to {}", def_id, path.display()); + + let mut buf = Vec::new(); + let graphviz = graphviz::Formatter::new(body, def_id, results); + + dot::render(&graphviz, &mut buf)?; + fs::write(path, buf) +} diff --git a/src/librustc_mir/dataflow/generic/graphviz.rs b/src/librustc_mir/dataflow/generic/graphviz.rs new file mode 100644 index 0000000000000..2a08feff9e77a --- /dev/null +++ b/src/librustc_mir/dataflow/generic/graphviz.rs @@ -0,0 +1,412 @@ +use std::cell::RefCell; +use std::io::{self, Write}; +use std::{ops, str}; + +use rustc::hir::def_id::DefId; +use rustc::mir::{self, BasicBlock, Body, Location}; +use rustc_index::bit_set::{BitSet, HybridBitSet}; +use rustc_index::vec::Idx; + +use crate::util::graphviz_safe_def_name; +use super::{Analysis, Results, ResultsRefCursor}; + +pub struct Formatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + body: &'a Body<'tcx>, + def_id: DefId, + + // This must be behind a `RefCell` because `dot::Labeller` takes `&self`. + block_formatter: RefCell>, +} + +impl Formatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + pub fn new( + body: &'a Body<'tcx>, + def_id: DefId, + results: &'a Results<'tcx, A>, + ) -> Self { + let block_formatter = BlockFormatter { + bg: Background::Light, + prev_state: BitSet::new_empty(results.analysis.bits_per_block(body)), + results: ResultsRefCursor::new(body, results), + }; + + Formatter { + body, + def_id, + block_formatter: RefCell::new(block_formatter), + } + } +} + +/// A pair of a basic block and an index into that basic blocks `successors`. +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +pub struct CfgEdge { + source: BasicBlock, + index: usize, +} + +fn outgoing_edges(body: &Body<'_>, bb: BasicBlock) -> Vec { + body[bb] + .terminator() + .successors() + .enumerate() + .map(|(index, _)| CfgEdge { source: bb, index }) + .collect() +} + +impl dot::Labeller<'_> for Formatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + type Node = BasicBlock; + type Edge = CfgEdge; + + fn graph_id(&self) -> dot::Id<'_> { + let name = graphviz_safe_def_name(self.def_id); + dot::Id::new(format!("graph_for_def_id_{}", name)).unwrap() + } + + fn node_id(&self, n: &Self::Node) -> dot::Id<'_> { + dot::Id::new(format!("bb_{}", n.index())).unwrap() + } + + fn node_label(&self, block: &Self::Node) -> dot::LabelText<'_> { + let mut label = Vec::new(); + self.block_formatter + .borrow_mut() + .write_node_label(&mut label, self.body, *block) + .unwrap(); + dot::LabelText::html(String::from_utf8(label).unwrap()) + } + + fn node_shape(&self, _n: &Self::Node) -> Option> { + Some(dot::LabelText::label("none")) + } + + fn edge_label(&self, e: &Self::Edge) -> dot::LabelText<'_> { + let label = &self.body + [e.source] + .terminator() + .kind + .fmt_successor_labels() + [e.index]; + dot::LabelText::label(label.clone()) + } +} + +impl dot::GraphWalk<'a> for Formatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + type Node = BasicBlock; + type Edge = CfgEdge; + + fn nodes(&self) -> dot::Nodes<'_, Self::Node> { + self.body + .basic_blocks() + .indices() + .collect::>() + .into() + } + + fn edges(&self) -> dot::Edges<'_, Self::Edge> { + self.body + .basic_blocks() + .indices() + .flat_map(|bb| outgoing_edges(self.body, bb)) + .collect::>() + .into() + } + + fn source(&self, edge: &Self::Edge) -> Self::Node { + edge.source + } + + fn target(&self, edge: &Self::Edge) -> Self::Node { + self.body + [edge.source] + .terminator() + .successors() + .nth(edge.index) + .copied() + .unwrap() + } +} + +struct BlockFormatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + prev_state: BitSet, + results: ResultsRefCursor<'a, 'a, 'tcx, A>, + bg: Background, +} + +impl BlockFormatter<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + fn toggle_background(&mut self) -> Background { + let bg = self.bg; + self.bg = !bg; + bg + } + + fn write_node_label( + &mut self, + w: &mut impl io::Write, + body: &'a Body<'tcx>, + block: BasicBlock, + ) -> io::Result<()> { + // Sample output: + // +-+--------------------------------------------------+ + // A | bb4 | + // +-+----------------------------------+---------------+ + // B | MIR | STATE | + // +-+----------------------------------+---------------+ + // C | | (on entry) | {_0,_2,_3} | + // +-+----------------------------------+---------------+ + // D |0| 0: StorageLive(_7) | | + // +-+----------------------------------+---------------+ + // |1| 1: StorageLive(_8) | | + // +-+----------------------------------+---------------+ + // |2| 2: _8 = &mut _1 | +_8 | + // +-+----------------------------------+---------------+ + // E |T| _7 = const Foo::twiddle(move _8) | -_8 | + // +-+----------------------------------+---------------+ + // F | | (on unwind) | {_0,_2,_3,_7} | + // +-+----------------------------------+---------------+ + // | | (on successful return) | +_7 | + // +-+----------------------------------+---------------+ + + write!( + w, + r#""#, + )?; + + // A: Block info + write!( + w, + r#" + + "#, + num_headers = 3, + block_id = block.index(), + )?; + + // B: Column headings + write!( + w, + r#" + + + "#, + fmt = r##"bgcolor="#a0a0a0" sides="tl""##, + )?; + + // C: Entry state + self.results.seek_to_block_start(block); + self.write_row_with_curr_state(w, "", "(on entry)")?; + self.prev_state.overwrite(self.results.get()); + + // D: Statement transfer functions + for (i, statement) in body[block].statements.iter().enumerate() { + let location = Location { block, statement_index: i }; + + let mir_col = format!("{:?}", statement); + let i_col = i.to_string(); + + self.results.seek_after(location); + self.write_row_with_curr_diff(w, &i_col, &mir_col)?; + self.prev_state.overwrite(self.results.get()); + } + + // E: Terminator transfer function + let terminator = body[block].terminator(); + let location = body.terminator_loc(block); + + let mut mir_col = String::new(); + terminator.kind.fmt_head(&mut mir_col).unwrap(); + + self.results.seek_after(location); + self.write_row_with_curr_diff(w, "T", &mir_col)?; + self.prev_state.overwrite(self.results.get()); + + // F: Exit state + if let mir::TerminatorKind::Call { destination: Some(_), .. } = &terminator.kind { + self.write_row_with_curr_state(w, "", "(on unwind)")?; + + self.results.seek_after_assume_call_returns(location); + self.write_row_with_curr_diff(w, "", "(on successful return)")?; + } else { + self.write_row_with_curr_state(w, "", "(on exit)")?; + } + + write!(w, "
bb{block_id}
MIRSTATE
") + } + + fn write_row_with_curr_state( + &mut self, + w: &mut impl io::Write, + i: &str, + mir: &str, + ) -> io::Result<()> { + let bg = self.toggle_background(); + + let mut out = Vec::new(); + write!(&mut out, "{{")?; + pretty_print_state_elems(&mut out, self.results.analysis(), self.results.get().iter())?; + write!(&mut out, "}}")?; + + write!( + w, + r#" + {i} + {mir} + {state} + "#, + fmt = &["sides=\"tl\"", bg.attr()].join(" "), + i = i, + mir = dot::escape_html(mir), + state = dot::escape_html(str::from_utf8(&out).unwrap()), + ) + } + + fn write_row_with_curr_diff( + &mut self, + w: &mut impl io::Write, + i: &str, + mir: &str, + ) -> io::Result<()> { + let bg = self.toggle_background(); + let analysis = self.results.analysis(); + + let diff = BitSetDiff::compute(&self.prev_state, self.results.get()); + + let mut set = Vec::new(); + pretty_print_state_elems(&mut set, analysis, diff.set.iter())?; + + let mut clear = Vec::new(); + pretty_print_state_elems(&mut clear, analysis, diff.clear.iter())?; + + write!( + w, + r#" + {i} + {mir} + "#, + i = i, + fmt = &["sides=\"tl\"", bg.attr()].join(" "), + mir = dot::escape_html(mir), + )?; + + if !set.is_empty() { + write!( + w, + r#"+{}"#, + dot::escape_html(str::from_utf8(&set).unwrap()), + )?; + } + + if !set.is_empty() && !clear.is_empty() { + write!(w, " ")?; + } + + if !clear.is_empty() { + write!( + w, + r#"-{}"#, + dot::escape_html(str::from_utf8(&clear).unwrap()), + )?; + } + + write!(w, "") + } +} + +/// The operations required to transform one `BitSet` into another. +struct BitSetDiff { + set: HybridBitSet, + clear: HybridBitSet, +} + +impl BitSetDiff { + fn compute(from: &BitSet, to: &BitSet) -> Self { + assert_eq!(from.domain_size(), to.domain_size()); + let len = from.domain_size(); + + let mut set = HybridBitSet::new_empty(len); + let mut clear = HybridBitSet::new_empty(len); + + // FIXME: This could be made faster if `BitSet::xor` were implemented. + for i in (0..len).map(|i| T::new(i)) { + match (from.contains(i), to.contains(i)) { + (false, true) => set.insert(i), + (true, false) => clear.insert(i), + _ => continue, + }; + } + + BitSetDiff { + set, + clear, + } + } +} + +/// Formats each `elem` using the pretty printer provided by `analysis` into a comma-separated +/// list. +fn pretty_print_state_elems( + w: &mut impl io::Write, + analysis: &A, + elems: impl Iterator, +) -> io::Result<()> +where + A: Analysis<'tcx>, +{ + let mut first = true; + for idx in elems { + if first { + first = false; + } else { + write!(w, ",")?; + } + + analysis.pretty_print_idx(w, idx)?; + } + + Ok(()) +} + +/// The background color used for zebra-striping the table. +#[derive(Clone, Copy)] +enum Background { + Light, + Dark, +} + +impl Background { + fn attr(self) -> &'static str { + match self { + Self::Dark => "bgcolor=\"#f0f0f0\"", + Self::Light => "", + } + } +} + +impl ops::Not for Background { + type Output = Self; + + fn not(self) -> Self { + match self { + Self::Light => Self::Dark, + Self::Dark => Self::Light, + } + } +} From cd24cd4eecf39c545b0373746631a8bddf2b46d3 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 29 Sep 2019 21:33:51 -0700 Subject: [PATCH 14/16] Update consumers of `generic::Engine` API --- src/librustc_mir/transform/check_consts/resolver.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index f95b240195b1e..54cb4fbad6d5d 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -208,7 +208,8 @@ where _qualif: PhantomData, }; let results = - dataflow::Engine::new(item.body, dead_unwinds, analysis).iterate_to_fixpoint(); + dataflow::Engine::new(item.tcx, item.body, item.def_id, dead_unwinds, analysis) + .iterate_to_fixpoint(); let cursor = dataflow::ResultsCursor::new(item.body, results); let mut qualifs_in_any_value_of_ty = BitSet::new_empty(item.body.local_decls.len()); From cf5f5c55be027a055867bef7773d27be1799ac06 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 29 Sep 2019 21:43:09 -0700 Subject: [PATCH 15/16] Use separate files for debugging `Qualif` dataflow results --- src/librustc_mir/transform/check_consts/qualifs.rs | 5 +++++ src/librustc_mir/transform/check_consts/resolver.rs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index 0c6468309dc4b..40007eb3c4a3e 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -27,6 +27,9 @@ impl QualifSet { pub trait Qualif { const IDX: usize; + /// The name of the file used to debug the dataflow analysis that computes this qualif. + const ANALYSIS_NAME: &'static str; + /// Whether this `Qualif` is cleared when a local is moved from. const IS_CLEARED_ON_MOVE: bool = false; @@ -207,6 +210,7 @@ pub struct HasMutInterior; impl Qualif for HasMutInterior { const IDX: usize = 0; + const ANALYSIS_NAME: &'static str = "flow_has_mut_interior"; fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool { !ty.is_freeze(cx.tcx, cx.param_env, DUMMY_SP) @@ -264,6 +268,7 @@ pub struct NeedsDrop; impl Qualif for NeedsDrop { const IDX: usize = 1; + const ANALYSIS_NAME: &'static str = "flow_needs_drop"; const IS_CLEARED_ON_MOVE: bool = true; fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool { diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index 54cb4fbad6d5d..4fa4eba4c23b6 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -309,7 +309,7 @@ where { type Idx = Local; - const NAME: &'static str = "flow_sensitive_qualif"; + const NAME: &'static str = Q::ANALYSIS_NAME; fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize { body.local_decls.len() From 2b8e023b9d28f2f912ad21427b5266b62c007f11 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 29 Sep 2019 21:45:10 -0700 Subject: [PATCH 16/16] Stop printing `Qualif` results in debug logs Now we can use the dataflow graphviz debugging. --- src/librustc_mir/transform/check_consts/validation.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index e74b22b43525c..3045239d7a770 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -467,8 +467,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { self.qualifs.needs_drop.visit_statement(statement, location); self.qualifs.has_mut_interior.visit_statement(statement, location); - debug!("needs_drop: {:?}", self.qualifs.needs_drop.get()); - debug!("has_mut_interior: {:?}", self.qualifs.has_mut_interior.get()); match statement.kind { StatementKind::Assign(..) => { @@ -494,8 +492,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { self.qualifs.needs_drop.visit_terminator(terminator, location); self.qualifs.has_mut_interior.visit_terminator(terminator, location); - debug!("needs_drop: {:?}", self.qualifs.needs_drop.get()); - debug!("has_mut_interior: {:?}", self.qualifs.has_mut_interior.get()); self.super_terminator(terminator, location); }