From 837c1aff0546edca18b43fbfc65b8dacf0b0b699 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Mon, 16 Jan 2023 01:31:58 +0300 Subject: [PATCH 01/17] rework min_choice algorithm of member constraints See the inline comments for the description of the new algorithm. --- .../rustc_borrowck/src/region_infer/mod.rs | 35 ++++++--- ...mber-constraints-min-choice-issue-63033.rs | 10 +++ .../min-choice-reject-ambiguous.rs | 43 +++++++++++ .../min-choice-reject-ambiguous.stderr | 40 ++++++++++ tests/ui/nll/member-constraints/min-choice.rs | 34 +++++++++ .../nested-impl-trait-fail.rs | 33 ++++++++ .../nested-impl-trait-fail.stderr | 75 +++++++++++++++++++ .../nested-impl-trait-pass.rs | 29 +++++++ 8 files changed, 288 insertions(+), 11 deletions(-) create mode 100644 tests/ui/async-await/multiple-lifetimes/member-constraints-min-choice-issue-63033.rs create mode 100644 tests/ui/nll/member-constraints/min-choice-reject-ambiguous.rs create mode 100644 tests/ui/nll/member-constraints/min-choice-reject-ambiguous.stderr create mode 100644 tests/ui/nll/member-constraints/min-choice.rs create mode 100644 tests/ui/nll/member-constraints/nested-impl-trait-fail.rs create mode 100644 tests/ui/nll/member-constraints/nested-impl-trait-fail.stderr create mode 100644 tests/ui/nll/member-constraints/nested-impl-trait-pass.rs diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index 308f6e19a73e8..9dd5a6c760716 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -739,20 +739,33 @@ impl<'tcx> RegionInferenceContext<'tcx> { } debug!(?choice_regions, "after ub"); - // If we ruled everything out, we're done. - if choice_regions.is_empty() { - return false; - } - - // Otherwise, we need to find the minimum remaining choice, if - // any, and take that. - debug!("choice_regions remaining are {:#?}", choice_regions); - let Some(&min_choice) = choice_regions.iter().find(|&r1| { + // At this point we can pick any member of `choice_regions`, but to avoid potential + // non-determinism we will pick the *unique minimum* choice. + // + // Because universal regions are only partially ordered (i.e, not every two regions are + // comparable), we will ignore any region that doesn't compare to all others when picking + // the minimum choice. + // For example, consider `choice_regions = ['static, 'a, 'b, 'c, 'd, 'e]`, where + // `'static: 'a, 'static: 'b, 'a: 'c, 'b: 'c, 'c: 'd, 'c: 'e`. + // `['d, 'e]` are ignored because they do not compare - the same goes for `['a, 'b]`. + let totally_ordered_subset = choice_regions.iter().copied().filter(|&r1| { choice_regions.iter().all(|&r2| { - self.universal_region_relations.outlives(r2, *r1) + self.universal_region_relations.outlives(r1, r2) + || self.universal_region_relations.outlives(r2, r1) }) + }); + // Now we're left with `['static, 'c]`. Pick `'c` as the minimum! + let Some(min_choice) = totally_ordered_subset.reduce(|r1, r2| { + let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2); + let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1); + match (r1_outlives_r2, r2_outlives_r1) { + (true, true) => r1.min(r2), + (true, false) => r2, + (false, true) => r1, + (false, false) => bug!("incomparable regions in total order"), + } }) else { - debug!("no choice region outlived by all others"); + debug!("no unique minimum choice"); return false; }; diff --git a/tests/ui/async-await/multiple-lifetimes/member-constraints-min-choice-issue-63033.rs b/tests/ui/async-await/multiple-lifetimes/member-constraints-min-choice-issue-63033.rs new file mode 100644 index 0000000000000..614f189729126 --- /dev/null +++ b/tests/ui/async-await/multiple-lifetimes/member-constraints-min-choice-issue-63033.rs @@ -0,0 +1,10 @@ +// Regression test for #63033. + +// check-pass +// edition: 2018 + +async fn test1(_: &'static u8, _: &'_ u8, _: &'_ u8) {} + +async fn test2<'s>(_: &'s u8, _: &'_ &'s u8, _: &'_ &'s u8) {} + +fn main() {} diff --git a/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.rs b/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.rs new file mode 100644 index 0000000000000..52ea0f28d69f3 --- /dev/null +++ b/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.rs @@ -0,0 +1,43 @@ +// ... continued from ./min-choice.rs + +// check-fail + +trait Cap<'a> {} +impl Cap<'_> for T {} + +fn type_test<'a, T: 'a>() -> &'a u8 { &0 } + +// Make sure we don't pick `'b`. +fn test_b<'a, 'b, 'c, T>() -> impl Cap<'a> + Cap<'b> + Cap<'c> +where + 'a: 'b, + 'a: 'c, + T: 'b, +{ + type_test::<'_, T>() // This should pass if we pick 'b. + //~^ ERROR the parameter type `T` may not live long enough +} + +// Make sure we don't pick `'c`. +fn test_c<'a, 'b, 'c, T>() -> impl Cap<'a> + Cap<'b> + Cap<'c> +where + 'a: 'b, + 'a: 'c, + T: 'c, +{ + type_test::<'_, T>() // This should pass if we pick 'c. + //~^ ERROR the parameter type `T` may not live long enough +} + +// We need to pick min_choice from `['b, 'c]`, but it's ambiguous which one to pick because +// they're incomparable. +fn test_ambiguous<'a, 'b, 'c>(s: &'a u8) -> impl Cap<'b> + Cap<'c> +where + 'a: 'b, + 'a: 'c, +{ + s + //~^ ERROR captures lifetime that does not appear in bounds +} + +fn main() {} diff --git a/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.stderr b/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.stderr new file mode 100644 index 0000000000000..1e6ef614dee24 --- /dev/null +++ b/tests/ui/nll/member-constraints/min-choice-reject-ambiguous.stderr @@ -0,0 +1,40 @@ +error[E0309]: the parameter type `T` may not live long enough + --> $DIR/min-choice-reject-ambiguous.rs:17:5 + | +LL | type_test::<'_, T>() // This should pass if we pick 'b. + | ^^^^^^^^^^^^^^^^^^ ...so that the type `T` will meet its required lifetime bounds + | +help: consider adding an explicit lifetime bound... + | +LL | T: 'b + 'a, + | ++++ + +error[E0309]: the parameter type `T` may not live long enough + --> $DIR/min-choice-reject-ambiguous.rs:28:5 + | +LL | type_test::<'_, T>() // This should pass if we pick 'c. + | ^^^^^^^^^^^^^^^^^^ ...so that the type `T` will meet its required lifetime bounds + | +help: consider adding an explicit lifetime bound... + | +LL | T: 'c + 'a, + | ++++ + +error[E0700]: hidden type for `impl Cap<'b> + Cap<'c>` captures lifetime that does not appear in bounds + --> $DIR/min-choice-reject-ambiguous.rs:39:5 + | +LL | fn test_ambiguous<'a, 'b, 'c>(s: &'a u8) -> impl Cap<'b> + Cap<'c> + | -- hidden type `&'a u8` captures the lifetime `'a` as defined here +... +LL | s + | ^ + | +help: to declare that `impl Cap<'b> + Cap<'c>` captures `'a`, you can add an explicit `'a` lifetime bound + | +LL | fn test_ambiguous<'a, 'b, 'c>(s: &'a u8) -> impl Cap<'b> + Cap<'c> + 'a + | ++++ + +error: aborting due to 3 previous errors + +Some errors have detailed explanations: E0309, E0700. +For more information about an error, try `rustc --explain E0309`. diff --git a/tests/ui/nll/member-constraints/min-choice.rs b/tests/ui/nll/member-constraints/min-choice.rs new file mode 100644 index 0000000000000..14b4dae7abfde --- /dev/null +++ b/tests/ui/nll/member-constraints/min-choice.rs @@ -0,0 +1,34 @@ +// Assuming that the hidden type in these tests is `&'_#15r u8`, +// we have a member constraint: `'_#15r member ['static, 'a, 'b, 'c]`. +// +// Make sure we pick up the minimum non-ambiguous region among them. +// We will have to exclude `['b, 'c]` because they're incomparable, +// and then we should pick `'a` because we know `'static: 'a`. + +// check-pass + +trait Cap<'a> {} +impl Cap<'_> for T {} + +fn type_test<'a, T: 'a>() -> &'a u8 { &0 } + +// Basic test: make sure we don't bail out because 'b and 'c are incomparable. +fn basic<'a, 'b, 'c>() -> impl Cap<'a> + Cap<'b> + Cap<'c> +where + 'a: 'b, + 'a: 'c, +{ + &0 +} + +// Make sure we don't pick `'static`. +fn test_static<'a, 'b, 'c, T>() -> impl Cap<'a> + Cap<'b> + Cap<'c> +where + 'a: 'b, + 'a: 'c, + T: 'a, +{ + type_test::<'_, T>() // This will fail if we pick 'static +} + +fn main() {} diff --git a/tests/ui/nll/member-constraints/nested-impl-trait-fail.rs b/tests/ui/nll/member-constraints/nested-impl-trait-fail.rs new file mode 100644 index 0000000000000..66ff828a84f7c --- /dev/null +++ b/tests/ui/nll/member-constraints/nested-impl-trait-fail.rs @@ -0,0 +1,33 @@ +// Nested impl-traits can impose different member constraints on the same region variable. + +// check-fail + +trait Cap<'a> {} +impl Cap<'_> for T {} + +// Assuming the hidden type is `[&'_#15r u8; 1]`, we have two distinct member constraints: +// - '_#15r member ['static, 'a, 'b] // from outer impl-trait +// - '_#15r member ['static, 'a, 'b] // from inner impl-trait +// To satisfy both we can choose 'a or 'b, so it's a failure due to ambiguity. +fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b>> +where + 's: 'a, + 's: 'b, +{ + [a] + //~^ E0700 + //~| E0700 +} + +// Same as the above but with late-bound regions. +fn fail_late_bound<'s, 'a, 'b>( + a: &'s u8, + _: &'a &'s u8, + _: &'b &'s u8, +) -> impl IntoIterator + Cap<'b>> { + [a] + //~^ E0700 + //~| E0700 +} + +fn main() {} diff --git a/tests/ui/nll/member-constraints/nested-impl-trait-fail.stderr b/tests/ui/nll/member-constraints/nested-impl-trait-fail.stderr new file mode 100644 index 0000000000000..6824e27ead028 --- /dev/null +++ b/tests/ui/nll/member-constraints/nested-impl-trait-fail.stderr @@ -0,0 +1,75 @@ +error[E0700]: hidden type for `impl IntoIterator + Cap<'b>>` captures lifetime that does not appear in bounds + --> $DIR/nested-impl-trait-fail.rs:17:5 + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b>> + | -- hidden type `[&'s u8; 1]` captures the lifetime `'s` as defined here +... +LL | [a] + | ^^^ + | +help: to declare that `impl IntoIterator + Cap<'b>>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b>> + 's + | ++++ +help: to declare that `impl Cap<'a> + Cap<'b>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b> + 's> + | ++++ + +error[E0700]: hidden type for `impl Cap<'a> + Cap<'b>` captures lifetime that does not appear in bounds + --> $DIR/nested-impl-trait-fail.rs:17:5 + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b>> + | -- hidden type `&'s u8` captures the lifetime `'s` as defined here +... +LL | [a] + | ^^^ + | +help: to declare that `impl IntoIterator + Cap<'b>>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b>> + 's + | ++++ +help: to declare that `impl Cap<'a> + Cap<'b>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | fn fail_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator + Cap<'b> + 's> + | ++++ + +error[E0700]: hidden type for `impl IntoIterator + Cap<'b>>` captures lifetime that does not appear in bounds + --> $DIR/nested-impl-trait-fail.rs:28:5 + | +LL | fn fail_late_bound<'s, 'a, 'b>( + | -- hidden type `[&'s u8; 1]` captures the lifetime `'s` as defined here +... +LL | [a] + | ^^^ + | +help: to declare that `impl IntoIterator + Cap<'b>>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | ) -> impl IntoIterator + Cap<'b>> + 's { + | ++++ +help: to declare that `impl Cap<'a> + Cap<'b>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | ) -> impl IntoIterator + Cap<'b> + 's> { + | ++++ + +error[E0700]: hidden type for `impl Cap<'a> + Cap<'b>` captures lifetime that does not appear in bounds + --> $DIR/nested-impl-trait-fail.rs:28:5 + | +LL | fn fail_late_bound<'s, 'a, 'b>( + | -- hidden type `&'s u8` captures the lifetime `'s` as defined here +... +LL | [a] + | ^^^ + | +help: to declare that `impl IntoIterator + Cap<'b>>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | ) -> impl IntoIterator + Cap<'b>> + 's { + | ++++ +help: to declare that `impl Cap<'a> + Cap<'b>` captures `'s`, you can add an explicit `'s` lifetime bound + | +LL | ) -> impl IntoIterator + Cap<'b> + 's> { + | ++++ + +error: aborting due to 4 previous errors + +For more information about this error, try `rustc --explain E0700`. diff --git a/tests/ui/nll/member-constraints/nested-impl-trait-pass.rs b/tests/ui/nll/member-constraints/nested-impl-trait-pass.rs new file mode 100644 index 0000000000000..15540cb460e7a --- /dev/null +++ b/tests/ui/nll/member-constraints/nested-impl-trait-pass.rs @@ -0,0 +1,29 @@ +// Nested impl-traits can impose different member constraints on the same region variable. + +// check-pass + +trait Cap<'a> {} +impl Cap<'_> for T {} + +// Assuming the hidden type is `[&'_#15r u8; 1]`, we have two distinct member constraints: +// - '_#15r member ['static, 'a, 'b] // from outer impl-trait +// - '_#15r member ['static, 'a] // from inner impl-trait +// To satisfy both we can only choose 'a. +fn pass_early_bound<'s, 'a, 'b>(a: &'s u8) -> impl IntoIterator> + Cap<'b> +where + 's: 'a, + 's: 'b, +{ + [a] +} + +// Same as the above but with late-bound regions. +fn pass_late_bound<'s, 'a, 'b>( + a: &'s u8, + _: &'a &'s u8, + _: &'b &'s u8, +) -> impl IntoIterator> + Cap<'b> { + [a] +} + +fn main() {} From 8df27d07aed4e12ce6e767f8675599aa0a8e46b9 Mon Sep 17 00:00:00 2001 From: Michael Benfield Date: Fri, 20 Jan 2023 20:56:16 -0800 Subject: [PATCH 02/17] Remove some superfluous type parameters from layout.rs. Specifically remove V, which can always be VariantIdx, and F, which can always be Layout. --- compiler/rustc_abi/src/layout.rs | 146 +++++++++++------------ compiler/rustc_abi/src/lib.rs | 71 +++++++++-- compiler/rustc_middle/src/arena.rs | 2 +- compiler/rustc_middle/src/ty/context.rs | 4 +- compiler/rustc_target/src/abi/mod.rs | 48 +------- compiler/rustc_ty_utils/src/layout.rs | 22 ++-- src/librustdoc/html/render/print_item.rs | 4 +- 7 files changed, 148 insertions(+), 149 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index 9c2cf58efed4a..54858b52008f9 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -1,11 +1,5 @@ use super::*; -use std::{ - borrow::Borrow, - cmp, - fmt::Debug, - iter, - ops::{Bound, Deref}, -}; +use std::{borrow::Borrow, cmp, iter, ops::Bound}; #[cfg(feature = "randomize")] use rand::{seq::SliceRandom, SeedableRng}; @@ -33,7 +27,7 @@ pub trait LayoutCalculator { fn delay_bug(&self, txt: &str); fn current_data_layout(&self) -> Self::TargetDataLayoutRef; - fn scalar_pair(&self, a: Scalar, b: Scalar) -> LayoutS { + fn scalar_pair(&self, a: Scalar, b: Scalar) -> LayoutS { let dl = self.current_data_layout(); let dl = dl.borrow(); let b_align = b.align(dl); @@ -49,7 +43,7 @@ pub trait LayoutCalculator { .max_by_key(|niche| niche.available(dl)); LayoutS { - variants: Variants::Single { index: V::new(0) }, + variants: Variants::Single { index: VariantIdx::new(0) }, fields: FieldsShape::Arbitrary { offsets: vec![Size::ZERO, b_offset], memory_index: vec![0, 1], @@ -61,13 +55,13 @@ pub trait LayoutCalculator { } } - fn univariant<'a, V: Idx, F: Deref> + Debug>( + fn univariant( &self, dl: &TargetDataLayout, - fields: &[F], + fields: &[Layout<'_>], repr: &ReprOptions, kind: StructKind, - ) -> Option> { + ) -> Option { let pack = repr.pack; let mut align = if pack.is_some() { dl.i8_align } else { dl.aggregate_align }; let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); @@ -76,17 +70,17 @@ pub trait LayoutCalculator { let end = if let StructKind::MaybeUnsized = kind { fields.len() - 1 } else { fields.len() }; let optimizing = &mut inverse_memory_index[..end]; - let effective_field_align = |f: &F| { + let effective_field_align = |layout: Layout<'_>| { if let Some(pack) = pack { // return the packed alignment in bytes - f.align.abi.min(pack).bytes() + layout.align().abi.min(pack).bytes() } else { // returns log2(effective-align). // This is ok since `pack` applies to all fields equally. // The calculation assumes that size is an integer multiple of align, except for ZSTs. // // group [u8; 4] with align-4 or [u8; 6] with align-2 fields - f.align.abi.bytes().max(f.size.bytes()).trailing_zeros() as u64 + layout.align().abi.bytes().max(layout.size().bytes()).trailing_zeros() as u64 } }; @@ -111,9 +105,9 @@ pub trait LayoutCalculator { // Place ZSTs first to avoid "interesting offsets", // especially with only one or two non-ZST fields. // Then place largest alignments first, largest niches within an alignment group last - let f = &fields[x as usize]; - let niche_size = f.largest_niche.map_or(0, |n| n.available(dl)); - (!f.is_zst(), cmp::Reverse(effective_field_align(f)), niche_size) + let f = fields[x as usize]; + let niche_size = f.largest_niche().map_or(0, |n| n.available(dl)); + (!f.0.is_zst(), cmp::Reverse(effective_field_align(f)), niche_size) }); } @@ -123,8 +117,8 @@ pub trait LayoutCalculator { // And put the largest niche in an alignment group at the end // so it can be used as discriminant in jagged enums optimizing.sort_by_key(|&x| { - let f = &fields[x as usize]; - let niche_size = f.largest_niche.map_or(0, |n| n.available(dl)); + let f = fields[x as usize]; + let niche_size = f.largest_niche().map_or(0, |n| n.available(dl)); (effective_field_align(f), niche_size) }); } @@ -160,15 +154,15 @@ pub trait LayoutCalculator { )); } - if field.is_unsized() { + if field.0.is_unsized() { sized = false; } // Invariant: offset < dl.obj_size_bound() <= 1<<61 let field_align = if let Some(pack) = pack { - field.align.min(AbiAndPrefAlign::new(pack)) + field.align().min(AbiAndPrefAlign::new(pack)) } else { - field.align + field.align() }; offset = offset.align_to(field_align.abi); align = align.max(field_align); @@ -176,7 +170,7 @@ pub trait LayoutCalculator { debug!("univariant offset: {:?} field: {:#?}", offset, field); offsets[i as usize] = offset; - if let Some(mut niche) = field.largest_niche { + if let Some(mut niche) = field.largest_niche() { let available = niche.available(dl); if available > largest_niche_available { largest_niche_available = available; @@ -185,7 +179,7 @@ pub trait LayoutCalculator { } } - offset = offset.checked_add(field.size, dl)?; + offset = offset.checked_add(field.size(), dl)?; } if let Some(repr_align) = repr.align { align = align.max(AbiAndPrefAlign::new(repr_align)); @@ -205,24 +199,26 @@ pub trait LayoutCalculator { // Unpack newtype ABIs and find scalar pairs. if sized && size.bytes() > 0 { // All other fields must be ZSTs. - let mut non_zst_fields = fields.iter().enumerate().filter(|&(_, f)| !f.is_zst()); + let mut non_zst_fields = fields.iter().enumerate().filter(|&(_, f)| !f.0.is_zst()); match (non_zst_fields.next(), non_zst_fields.next(), non_zst_fields.next()) { // We have exactly one non-ZST field. (Some((i, field)), None, None) => { // Field fills the struct and it has a scalar or scalar pair ABI. - if offsets[i].bytes() == 0 && align.abi == field.align.abi && size == field.size + if offsets[i].bytes() == 0 + && align.abi == field.align().abi + && size == field.size() { - match field.abi { + match field.abi() { // For plain scalars, or vectors of them, we can't unpack // newtypes for `#[repr(C)]`, as that affects C ABIs. Abi::Scalar(_) | Abi::Vector { .. } if optimize => { - abi = field.abi; + abi = field.abi(); } // But scalar pairs are Rust-specific and get // treated as aggregates by C ABIs anyway. Abi::ScalarPair(..) => { - abi = field.abi; + abi = field.abi(); } _ => {} } @@ -231,7 +227,7 @@ pub trait LayoutCalculator { // Two non-ZST fields, and they're both scalars. (Some((i, a)), Some((j, b)), None) => { - match (a.abi, b.abi) { + match (a.abi(), b.abi()) { (Abi::Scalar(a), Abi::Scalar(b)) => { // Order by the memory placement, not source order. let ((i, a), (j, b)) = if offsets[i] < offsets[j] { @@ -239,7 +235,7 @@ pub trait LayoutCalculator { } else { ((j, b), (i, a)) }; - let pair = self.scalar_pair::(a, b); + let pair = self.scalar_pair(a, b); let pair_offsets = match pair.fields { FieldsShape::Arbitrary { ref offsets, ref memory_index } => { assert_eq!(memory_index, &[0, 1]); @@ -264,11 +260,11 @@ pub trait LayoutCalculator { _ => {} } } - if fields.iter().any(|f| f.abi.is_uninhabited()) { + if fields.iter().any(|f| f.abi().is_uninhabited()) { abi = Abi::Uninhabited; } Some(LayoutS { - variants: Variants::Single { index: V::new(0) }, + variants: Variants::Single { index: VariantIdx::new(0) }, fields: FieldsShape::Arbitrary { offsets, memory_index }, abi, largest_niche, @@ -277,11 +273,11 @@ pub trait LayoutCalculator { }) } - fn layout_of_never_type(&self) -> LayoutS { + fn layout_of_never_type(&self) -> LayoutS { let dl = self.current_data_layout(); let dl = dl.borrow(); LayoutS { - variants: Variants::Single { index: V::new(0) }, + variants: Variants::Single { index: VariantIdx::new(0) }, fields: FieldsShape::Primitive, abi: Abi::Uninhabited, largest_niche: None, @@ -290,18 +286,18 @@ pub trait LayoutCalculator { } } - fn layout_of_struct_or_enum<'a, V: Idx, F: Deref> + Debug>( + fn layout_of_struct_or_enum( &self, repr: &ReprOptions, - variants: &IndexVec>, + variants: &IndexVec>>, is_enum: bool, is_unsafe_cell: bool, scalar_valid_range: (Bound, Bound), discr_range_of_repr: impl Fn(i128, i128) -> (Integer, bool), - discriminants: impl Iterator, + discriminants: impl Iterator, niche_optimize_enum: bool, always_sized: bool, - ) -> Option> { + ) -> Option { let dl = self.current_data_layout(); let dl = dl.borrow(); @@ -316,9 +312,9 @@ pub trait LayoutCalculator { // but *not* an encoding of the discriminant (e.g., a tag value). // See issue #49298 for more details on the need to leave space // for non-ZST uninhabited data (mostly partial initialization). - let absent = |fields: &[F]| { - let uninhabited = fields.iter().any(|f| f.abi.is_uninhabited()); - let is_zst = fields.iter().all(|f| f.is_zst()); + let absent = |fields: &[Layout<'_>]| { + let uninhabited = fields.iter().any(|f| f.abi().is_uninhabited()); + let is_zst = fields.iter().all(|f| f.0.is_zst()); uninhabited && is_zst }; let (present_first, present_second) = { @@ -335,7 +331,7 @@ pub trait LayoutCalculator { } // If it's a struct, still compute a layout so that we can still compute the // field offsets. - None => V::new(0), + None => VariantIdx::new(0), }; let is_struct = !is_enum || @@ -439,12 +435,12 @@ pub trait LayoutCalculator { // variant layouts, so we can't store them in the // overall LayoutS. Store the overall LayoutS // and the variant LayoutSs here until then. - struct TmpLayout { - layout: LayoutS, - variants: IndexVec>, + struct TmpLayout { + layout: LayoutS, + variants: IndexVec, } - let calculate_niche_filling_layout = || -> Option> { + let calculate_niche_filling_layout = || -> Option { if niche_optimize_enum { return None; } @@ -464,15 +460,16 @@ pub trait LayoutCalculator { Some(st) }) - .collect::>>()?; + .collect::>>()?; let largest_variant_index = variant_layouts .iter_enumerated() .max_by_key(|(_i, layout)| layout.size.bytes()) .map(|(i, _layout)| i)?; - let all_indices = (0..=variants.len() - 1).map(V::new); - let needs_disc = |index: V| index != largest_variant_index && !absent(&variants[index]); + let all_indices = (0..=variants.len() - 1).map(VariantIdx::new); + let needs_disc = + |index: VariantIdx| index != largest_variant_index && !absent(&variants[index]); let niche_variants = all_indices.clone().find(|v| needs_disc(*v)).unwrap().index() ..=all_indices.rev().find(|v| needs_disc(*v)).unwrap().index(); @@ -482,7 +479,7 @@ pub trait LayoutCalculator { let (field_index, niche, (niche_start, niche_scalar)) = variants[largest_variant_index] .iter() .enumerate() - .filter_map(|(j, field)| Some((j, field.largest_niche?))) + .filter_map(|(j, field)| Some((j, field.largest_niche()?))) .max_by_key(|(_, niche)| niche.available(dl)) .and_then(|(j, niche)| Some((j, niche, niche.reserve(dl, count)?)))?; let niche_offset = @@ -514,7 +511,7 @@ pub trait LayoutCalculator { match layout.fields { FieldsShape::Arbitrary { ref mut offsets, .. } => { for (j, offset) in offsets.iter_mut().enumerate() { - if !variants[i][j].is_zst() { + if !variants[i][j].0.is_zst() { *offset += this_offset; } } @@ -572,8 +569,8 @@ pub trait LayoutCalculator { tag: niche_scalar, tag_encoding: TagEncoding::Niche { untagged_variant: largest_variant_index, - niche_variants: (V::new(*niche_variants.start()) - ..=V::new(*niche_variants.end())), + niche_variants: (VariantIdx::new(*niche_variants.start()) + ..=VariantIdx::new(*niche_variants.end())), niche_start, }, tag_field: 0, @@ -598,7 +595,7 @@ pub trait LayoutCalculator { let discr_type = repr.discr_type(); let bits = Integer::from_attr(dl, discr_type).size().bits(); for (i, mut val) in discriminants { - if variants[i].iter().any(|f| f.abi.is_uninhabited()) { + if variants[i].iter().any(|f| f.abi().is_uninhabited()) { continue; } if discr_type.is_signed() { @@ -636,7 +633,7 @@ pub trait LayoutCalculator { if repr.c() { for fields in variants { for field in fields { - prefix_align = prefix_align.max(field.align.abi); + prefix_align = prefix_align.max(field.align().abi); } } } @@ -655,8 +652,8 @@ pub trait LayoutCalculator { // Find the first field we can't move later // to make room for a larger discriminant. for field in st.fields.index_by_increasing_offset().map(|j| &field_layouts[j]) { - if !field.is_zst() || field.align.abi.bytes() != 1 { - start_align = start_align.min(field.align.abi); + if !field.0.is_zst() || field.align().abi.bytes() != 1 { + start_align = start_align.min(field.align().abi); break; } } @@ -664,7 +661,7 @@ pub trait LayoutCalculator { align = align.max(st.align); Some(st) }) - .collect::>>()?; + .collect::>>()?; // Align the maximum variant size to the largest alignment. size = size.align_to(align.abi); @@ -759,7 +756,7 @@ pub trait LayoutCalculator { let FieldsShape::Arbitrary { ref offsets, .. } = layout_variant.fields else { panic!(); }; - let mut fields = iter::zip(field_layouts, offsets).filter(|p| !p.0.is_zst()); + let mut fields = iter::zip(field_layouts, offsets).filter(|p| !p.0.0.is_zst()); let (field, offset) = match (fields.next(), fields.next()) { (None, None) => { common_prim_initialized_in_all_variants = false; @@ -771,7 +768,7 @@ pub trait LayoutCalculator { break; } }; - let prim = match field.abi { + let prim = match field.abi() { Abi::Scalar(scalar) => { common_prim_initialized_in_all_variants &= matches!(scalar, Scalar::Initialized { .. }); @@ -802,7 +799,7 @@ pub trait LayoutCalculator { // Common prim might be uninit. Scalar::Union { value: prim } }; - let pair = self.scalar_pair::(tag, prim_scalar); + let pair = self.scalar_pair(tag, prim_scalar); let pair_offsets = match pair.fields { FieldsShape::Arbitrary { ref offsets, ref memory_index } => { assert_eq!(memory_index, &[0, 1]); @@ -862,9 +859,8 @@ pub trait LayoutCalculator { // pick the layout with the larger niche; otherwise, // pick tagged as it has simpler codegen. use cmp::Ordering::*; - let niche_size = |tmp_l: &TmpLayout| { - tmp_l.layout.largest_niche.map_or(0, |n| n.available(dl)) - }; + let niche_size = + |tmp_l: &TmpLayout| tmp_l.layout.largest_niche.map_or(0, |n| n.available(dl)); match (tl.layout.size.cmp(&nl.layout.size), niche_size(&tl).cmp(&niche_size(&nl))) { (Greater, _) => nl, (Equal, Less) => nl, @@ -884,11 +880,11 @@ pub trait LayoutCalculator { Some(best_layout.layout) } - fn layout_of_union<'a, V: Idx, F: Deref> + Debug>( + fn layout_of_union( &self, repr: &ReprOptions, - variants: &IndexVec>, - ) -> Option> { + variants: &IndexVec>>, + ) -> Option { let dl = self.current_data_layout(); let dl = dl.borrow(); let mut align = if repr.pack.is_some() { dl.i8_align } else { dl.aggregate_align }; @@ -900,15 +896,15 @@ pub trait LayoutCalculator { let optimize = !repr.inhibit_union_abi_opt(); let mut size = Size::ZERO; let mut abi = Abi::Aggregate { sized: true }; - let index = V::new(0); + let index = VariantIdx::new(0); for field in &variants[index] { - assert!(field.is_sized()); - align = align.max(field.align); + assert!(field.0.is_sized()); + align = align.max(field.align()); // If all non-ZST fields have the same ABI, forward this ABI - if optimize && !field.is_zst() { + if optimize && !field.0.is_zst() { // Discard valid range information and allow undef - let field_abi = match field.abi { + let field_abi = match field.abi() { Abi::Scalar(x) => Abi::Scalar(x.to_union()), Abi::ScalarPair(x, y) => Abi::ScalarPair(x.to_union(), y.to_union()), Abi::Vector { element: x, count } => { @@ -926,7 +922,7 @@ pub trait LayoutCalculator { } } - size = cmp::max(size, field.size); + size = cmp::max(size, field.size()); } if let Some(pack) = repr.pack { diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index f4cb459f32fdd..5cd0aff2d5b71 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -8,6 +8,7 @@ use std::ops::{Add, AddAssign, Mul, RangeInclusive, Sub}; use std::str::FromStr; use bitflags::bitflags; +use rustc_data_structures::intern::Interned; #[cfg(feature = "nightly")] use rustc_data_structures::stable_hasher::StableOrd; use rustc_index::vec::{Idx, IndexVec}; @@ -1257,9 +1258,9 @@ impl Abi { #[derive(PartialEq, Eq, Hash, Clone, Debug)] #[cfg_attr(feature = "nightly", derive(HashStable_Generic))] -pub enum Variants { +pub enum Variants { /// Single enum variants, structs/tuples, unions, and all non-ADTs. - Single { index: V }, + Single { index: VariantIdx }, /// Enum-likes with more than one inhabited variant: each variant comes with /// a *discriminant* (usually the same as the variant index but the user can @@ -1269,15 +1270,15 @@ pub enum Variants { /// For enums, the tag is the sole field of the layout. Multiple { tag: Scalar, - tag_encoding: TagEncoding, + tag_encoding: TagEncoding, tag_field: usize, - variants: IndexVec>, + variants: IndexVec, }, } #[derive(PartialEq, Eq, Hash, Clone, Debug)] #[cfg_attr(feature = "nightly", derive(HashStable_Generic))] -pub enum TagEncoding { +pub enum TagEncoding { /// The tag directly stores the discriminant, but possibly with a smaller layout /// (so converting the tag to the discriminant can require sign extension). Direct, @@ -1292,7 +1293,11 @@ pub enum TagEncoding { /// For example, `Option<(usize, &T)>` is represented such that /// `None` has a null pointer for the second tuple field, and /// `Some` is the identity function (with a non-null reference). - Niche { untagged_variant: V, niche_variants: RangeInclusive, niche_start: u128 }, + Niche { + untagged_variant: VariantIdx, + niche_variants: RangeInclusive, + niche_start: u128, + }, } #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] @@ -1379,9 +1384,14 @@ impl Niche { } } +rustc_index::newtype_index! { + #[derive(HashStable_Generic)] + pub struct VariantIdx {} +} + #[derive(PartialEq, Eq, Hash, Clone)] #[cfg_attr(feature = "nightly", derive(HashStable_Generic))] -pub struct LayoutS { +pub struct LayoutS { /// Says where the fields are located within the layout. pub fields: FieldsShape, @@ -1392,7 +1402,7 @@ pub struct LayoutS { /// /// To access all fields of this layout, both `fields` and the fields of the active variant /// must be taken into account. - pub variants: Variants, + pub variants: Variants, /// The `abi` defines how this data is passed between functions, and it defines /// value restrictions via `valid_range`. @@ -1411,13 +1421,13 @@ pub struct LayoutS { pub size: Size, } -impl LayoutS { +impl LayoutS { pub fn scalar(cx: &C, scalar: Scalar) -> Self { let largest_niche = Niche::from_scalar(cx, Size::ZERO, scalar); let size = scalar.size(cx); let align = scalar.align(cx); LayoutS { - variants: Variants::Single { index: V::new(0) }, + variants: Variants::Single { index: VariantIdx::new(0) }, fields: FieldsShape::Primitive, abi: Abi::Scalar(scalar), largest_niche, @@ -1427,7 +1437,7 @@ impl LayoutS { } } -impl fmt::Debug for LayoutS { +impl fmt::Debug for LayoutS { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { // This is how `Layout` used to print before it become // `Interned`. We print it like this to avoid having to update @@ -1444,6 +1454,43 @@ impl fmt::Debug for LayoutS { } } +#[derive(Copy, Clone, PartialEq, Eq, Hash, HashStable_Generic)] +#[rustc_pass_by_value] +pub struct Layout<'a>(pub Interned<'a, LayoutS>); + +impl<'a> fmt::Debug for Layout<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // See comment on `::fmt` above. + self.0.0.fmt(f) + } +} + +impl<'a> Layout<'a> { + pub fn fields(self) -> &'a FieldsShape { + &self.0.0.fields + } + + pub fn variants(self) -> &'a Variants { + &self.0.0.variants + } + + pub fn abi(self) -> Abi { + self.0.0.abi + } + + pub fn largest_niche(self) -> Option { + self.0.0.largest_niche + } + + pub fn align(self) -> AbiAndPrefAlign { + self.0.0.align + } + + pub fn size(self) -> Size { + self.0.0.size + } +} + #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum PointerKind { /// Most general case, we know no restrictions to tell LLVM. @@ -1479,7 +1526,7 @@ pub enum InitKind { UninitMitigated0x01Fill, } -impl LayoutS { +impl LayoutS { /// Returns `true` if the layout corresponds to an unsized type. pub fn is_unsized(&self) -> bool { self.abi.is_unsized() diff --git a/compiler/rustc_middle/src/arena.rs b/compiler/rustc_middle/src/arena.rs index f816d614500a0..f0a63c243a5e1 100644 --- a/compiler/rustc_middle/src/arena.rs +++ b/compiler/rustc_middle/src/arena.rs @@ -8,7 +8,7 @@ macro_rules! arena_types { ($macro:path) => ( $macro!([ - [] layout: rustc_target::abi::LayoutS, + [] layout: rustc_target::abi::LayoutS, [] fn_abi: rustc_target::abi::call::FnAbi<'tcx, rustc_middle::ty::Ty<'tcx>>, // AdtDef are interned and compared by address [decode] adt_def: rustc_middle::ty::AdtDefData, diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index ce04d8d21f4cd..c207d639f4a1f 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -142,7 +142,7 @@ pub struct CtxtInterners<'tcx> { const_: InternedSet<'tcx, ConstData<'tcx>>, const_allocation: InternedSet<'tcx, Allocation>, bound_variable_kinds: InternedSet<'tcx, List>, - layout: InternedSet<'tcx, LayoutS>, + layout: InternedSet<'tcx, LayoutS>, adt_def: InternedSet<'tcx, AdtDefData>, } @@ -1586,7 +1586,7 @@ direct_interners! { region: mk_region(RegionKind<'tcx>): Region -> Region<'tcx>, const_: mk_const_internal(ConstData<'tcx>): Const -> Const<'tcx>, const_allocation: intern_const_alloc(Allocation): ConstAllocation -> ConstAllocation<'tcx>, - layout: intern_layout(LayoutS): Layout -> Layout<'tcx>, + layout: intern_layout(LayoutS): Layout -> Layout<'tcx>, adt_def: intern_adt_def(AdtDefData): AdtDef -> AdtDef<'tcx>, } diff --git a/compiler/rustc_target/src/abi/mod.rs b/compiler/rustc_target/src/abi/mod.rs index 88a0a1f8ecfde..653f99fb11a10 100644 --- a/compiler/rustc_target/src/abi/mod.rs +++ b/compiler/rustc_target/src/abi/mod.rs @@ -3,10 +3,8 @@ pub use Primitive::*; use crate::json::{Json, ToJson}; -use std::fmt; use std::ops::Deref; -use rustc_data_structures::intern::Interned; use rustc_macros::HashStable_Generic; pub mod call; @@ -19,48 +17,6 @@ impl ToJson for Endian { } } -rustc_index::newtype_index! { - #[derive(HashStable_Generic)] - pub struct VariantIdx {} -} - -#[derive(Copy, Clone, PartialEq, Eq, Hash, HashStable_Generic)] -#[rustc_pass_by_value] -pub struct Layout<'a>(pub Interned<'a, LayoutS>); - -impl<'a> fmt::Debug for Layout<'a> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // See comment on `::fmt` above. - self.0.0.fmt(f) - } -} - -impl<'a> Layout<'a> { - pub fn fields(self) -> &'a FieldsShape { - &self.0.0.fields - } - - pub fn variants(self) -> &'a Variants { - &self.0.0.variants - } - - pub fn abi(self) -> Abi { - self.0.0.abi - } - - pub fn largest_niche(self) -> Option { - self.0.0.largest_niche - } - - pub fn align(self) -> AbiAndPrefAlign { - self.0.0.align - } - - pub fn size(self) -> Size { - self.0.0.size - } -} - /// The layout of a type, alongside the type itself. /// Provides various type traversal APIs (e.g., recursing into fields). /// @@ -75,8 +31,8 @@ pub struct TyAndLayout<'a, Ty> { } impl<'a, Ty> Deref for TyAndLayout<'a, Ty> { - type Target = &'a LayoutS; - fn deref(&self) -> &&'a LayoutS { + type Target = &'a LayoutS; + fn deref(&self) -> &&'a LayoutS { &self.layout.0.0 } } diff --git a/compiler/rustc_ty_utils/src/layout.rs b/compiler/rustc_ty_utils/src/layout.rs index 6aa016133ca59..68312f8b11c63 100644 --- a/compiler/rustc_ty_utils/src/layout.rs +++ b/compiler/rustc_ty_utils/src/layout.rs @@ -78,10 +78,10 @@ fn invert_mapping(map: &[u32]) -> Vec { fn univariant_uninterned<'tcx>( cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, ty: Ty<'tcx>, - fields: &[TyAndLayout<'_>], + fields: &[Layout<'_>], repr: &ReprOptions, kind: StructKind, -) -> Result, LayoutError<'tcx>> { +) -> Result> { let dl = cx.data_layout(); let pack = repr.pack; if pack.is_some() && repr.align.is_some() { @@ -106,7 +106,7 @@ fn layout_of_uncached<'tcx>( }; let scalar = |value: Primitive| tcx.intern_layout(LayoutS::scalar(cx, scalar_unit(value))); - let univariant = |fields: &[TyAndLayout<'_>], repr: &ReprOptions, kind| { + let univariant = |fields: &[Layout<'_>], repr: &ReprOptions, kind| { Ok(tcx.intern_layout(univariant_uninterned(cx, ty, fields, repr, kind)?)) }; debug_assert!(!ty.has_non_region_infer()); @@ -272,7 +272,7 @@ fn layout_of_uncached<'tcx>( ty::Closure(_, ref substs) => { let tys = substs.as_closure().upvar_tys(); univariant( - &tys.map(|ty| cx.layout_of(ty)).collect::, _>>()?, + &tys.map(|ty| Ok(cx.layout_of(ty)?.layout)).collect::, _>>()?, &ReprOptions::default(), StructKind::AlwaysSized, )? @@ -283,7 +283,7 @@ fn layout_of_uncached<'tcx>( if tys.len() == 0 { StructKind::AlwaysSized } else { StructKind::MaybeUnsized }; univariant( - &tys.iter().map(|k| cx.layout_of(k)).collect::, _>>()?, + &tys.iter().map(|k| Ok(cx.layout_of(k)?.layout)).collect::, _>>()?, &ReprOptions::default(), kind, )? @@ -412,7 +412,7 @@ fn layout_of_uncached<'tcx>( .map(|v| { v.fields .iter() - .map(|field| cx.layout_of(field.ty(tcx, substs))) + .map(|field| Ok(cx.layout_of(field.ty(tcx, substs))?.layout)) .collect::, _>>() }) .collect::, _>>()?; @@ -630,23 +630,21 @@ fn generator_layout<'tcx>( // `info.variant_fields` already accounts for the reserved variants, so no need to add them. let max_discr = (info.variant_fields.len() - 1) as u128; let discr_int = Integer::fit_unsigned(max_discr); - let discr_int_ty = discr_int.to_ty(tcx, false); let tag = Scalar::Initialized { value: Primitive::Int(discr_int, false), valid_range: WrappingRange { start: 0, end: max_discr }, }; let tag_layout = cx.tcx.intern_layout(LayoutS::scalar(cx, tag)); - let tag_layout = TyAndLayout { ty: discr_int_ty, layout: tag_layout }; let promoted_layouts = ineligible_locals .iter() .map(|local| subst_field(info.field_tys[local])) .map(|ty| tcx.mk_maybe_uninit(ty)) - .map(|ty| cx.layout_of(ty)); + .map(|ty| Ok(cx.layout_of(ty)?.layout)); let prefix_layouts = substs .as_generator() .prefix_tys() - .map(|ty| cx.layout_of(ty)) + .map(|ty| Ok(cx.layout_of(ty)?.layout)) .chain(iter::once(Ok(tag_layout))) .chain(promoted_layouts) .collect::, _>>()?; @@ -715,7 +713,9 @@ fn generator_layout<'tcx>( let mut variant = univariant_uninterned( cx, ty, - &variant_only_tys.map(|ty| cx.layout_of(ty)).collect::, _>>()?, + &variant_only_tys + .map(|ty| Ok(cx.layout_of(ty)?.layout)) + .collect::, _>>()?, &ReprOptions::default(), StructKind::Prefixed(prefix_size, prefix_align.abi), )?; diff --git a/src/librustdoc/html/render/print_item.rs b/src/librustdoc/html/render/print_item.rs index f824c9e3ad2bd..e57f087642702 100644 --- a/src/librustdoc/html/render/print_item.rs +++ b/src/librustdoc/html/render/print_item.rs @@ -10,7 +10,7 @@ use rustc_middle::ty::layout::LayoutError; use rustc_middle::ty::{self, Adt, TyCtxt}; use rustc_span::hygiene::MacroKind; use rustc_span::symbol::{kw, sym, Symbol}; -use rustc_target::abi::{LayoutS, Primitive, TagEncoding, VariantIdx, Variants}; +use rustc_target::abi::{LayoutS, Primitive, TagEncoding, Variants}; use std::cmp::Ordering; use std::fmt; use std::rc::Rc; @@ -1887,7 +1887,7 @@ fn document_non_exhaustive(w: &mut Buffer, item: &clean::Item) { } fn document_type_layout(w: &mut Buffer, cx: &Context<'_>, ty_def_id: DefId) { - fn write_size_of_layout(w: &mut Buffer, layout: &LayoutS, tag_size: u64) { + fn write_size_of_layout(w: &mut Buffer, layout: &LayoutS, tag_size: u64) { if layout.abi.is_unsized() { write!(w, "(unsized)"); } else { From 0d59b8c997e31095732c9f9864e10d76daaeb42e Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 21 Jan 2023 21:52:40 +0000 Subject: [PATCH 03/17] Remove redundant test. --- compiler/rustc_mir_dataflow/src/value_analysis.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 8bf6493be4b01..45afd16c313e4 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -622,8 +622,7 @@ impl Map { ty: Ty<'tcx>, filter: &mut impl FnMut(Ty<'tcx>) -> bool, ) { - // Note: The framework supports only scalars for now. - if filter(ty) && ty.is_scalar() { + if filter(ty) { // We know that the projection only contains trackable elements. let place = self.make_place(local, projection).unwrap(); From cd3649b2a595da17dcff983b6d5f74a28a98dd00 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 21 Jan 2023 21:53:26 +0000 Subject: [PATCH 04/17] Only exclude locals if the place is not indirect. --- .../src/impls/borrowed_locals.rs | 4 +++- .../rustc_mir_dataflow/src/value_analysis.rs | 20 ++++++++++--------- compiler/rustc_mir_transform/src/sroa.rs | 18 +++++++++-------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs b/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs index 0f8e86d1d6679..6f4e7fd4682c1 100644 --- a/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs +++ b/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs @@ -121,7 +121,9 @@ where // for now. See discussion on [#61069]. // // [#61069]: https://github.com/rust-lang/rust/pull/61069 - self.trans.gen(dropped_place.local); + if !dropped_place.is_indirect() { + self.trans.gen(dropped_place.local); + } } TerminatorKind::Abort diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 45afd16c313e4..8003da6bbd269 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -35,6 +35,7 @@ use std::fmt::{Debug, Formatter}; use rustc_data_structures::fx::FxHashMap; +use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; @@ -589,7 +590,7 @@ impl Map { ) -> Self { let mut map = Self::new(); let exclude = excluded_locals(body); - map.register_with_filter(tcx, body, filter, &exclude); + map.register_with_filter(tcx, body, filter, exclude); debug!("registered {} places ({} nodes in total)", map.value_count, map.places.len()); map } @@ -600,12 +601,12 @@ impl Map { tcx: TyCtxt<'tcx>, body: &Body<'tcx>, mut filter: impl FnMut(Ty<'tcx>) -> bool, - exclude: &IndexVec, + exclude: BitSet, ) { // We use this vector as stack, pushing and popping projections. let mut projection = Vec::new(); for (local, decl) in body.local_decls.iter_enumerated() { - if !exclude[local] { + if !exclude.contains(local) { self.register_with_filter_rec(tcx, local, &mut projection, decl.ty, &mut filter); } } @@ -823,26 +824,27 @@ pub fn iter_fields<'tcx>( } /// Returns all locals with projections that have their reference or address taken. -pub fn excluded_locals(body: &Body<'_>) -> IndexVec { +pub fn excluded_locals(body: &Body<'_>) -> BitSet { struct Collector { - result: IndexVec, + result: BitSet, } impl<'tcx> Visitor<'tcx> for Collector { fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { - if context.is_borrow() + if (context.is_borrow() || context.is_address_of() || context.is_drop() - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) + || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput)) + && !place.is_indirect() { // A pointer to a place could be used to access other places with the same local, // hence we have to exclude the local completely. - self.result[place.local] = true; + self.result.insert(place.local); } } } - let mut collector = Collector { result: IndexVec::from_elem(false, &body.local_decls) }; + let mut collector = Collector { result: BitSet::new_empty(body.local_decls.len()) }; collector.visit_body(body); collector.result } diff --git a/compiler/rustc_mir_transform/src/sroa.rs b/compiler/rustc_mir_transform/src/sroa.rs index 26acd406ed8a9..3fb7836ed6867 100644 --- a/compiler/rustc_mir_transform/src/sroa.rs +++ b/compiler/rustc_mir_transform/src/sroa.rs @@ -1,5 +1,5 @@ use crate::MirPass; -use rustc_index::bit_set::BitSet; +use rustc_index::bit_set::{BitSet, GrowableBitSet}; use rustc_index::vec::IndexVec; use rustc_middle::mir::patch::MirPatch; use rustc_middle::mir::visit::*; @@ -26,10 +26,12 @@ impl<'tcx> MirPass<'tcx> for ScalarReplacementOfAggregates { debug!(?replacements); let all_dead_locals = replace_flattened_locals(tcx, body, replacements); if !all_dead_locals.is_empty() { - for local in excluded.indices() { - excluded[local] |= all_dead_locals.contains(local); - } - excluded.raw.resize(body.local_decls.len(), false); + excluded.union(&all_dead_locals); + excluded = { + let mut growable = GrowableBitSet::from(excluded); + growable.ensure(body.local_decls.len()); + growable.into() + }; } else { break; } @@ -44,11 +46,11 @@ impl<'tcx> MirPass<'tcx> for ScalarReplacementOfAggregates { /// - the locals is a union or an enum; /// - the local's address is taken, and thus the relative addresses of the fields are observable to /// client code. -fn escaping_locals(excluded: &IndexVec, body: &Body<'_>) -> BitSet { +fn escaping_locals(excluded: &BitSet, body: &Body<'_>) -> BitSet { let mut set = BitSet::new_empty(body.local_decls.len()); set.insert_range(RETURN_PLACE..=Local::from_usize(body.arg_count)); for (local, decl) in body.local_decls().iter_enumerated() { - if decl.ty.is_union() || decl.ty.is_enum() || excluded[local] { + if decl.ty.is_union() || decl.ty.is_enum() || excluded.contains(local) { set.insert(local); } } @@ -172,7 +174,7 @@ fn replace_flattened_locals<'tcx>( body: &mut Body<'tcx>, replacements: ReplacementMap<'tcx>, ) -> BitSet { - let mut all_dead_locals = BitSet::new_empty(body.local_decls.len()); + let mut all_dead_locals = BitSet::new_empty(replacements.fragments.len()); for (local, replacements) in replacements.fragments.iter_enumerated() { if replacements.is_some() { all_dead_locals.insert(local); From 9a6c04f5d0e6ec47bf150187cffcb7f737799db4 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 21 Jan 2023 22:28:54 +0000 Subject: [PATCH 05/17] Handle discriminants in dataflow-const-prop. --- compiler/rustc_mir_dataflow/src/lib.rs | 1 + .../rustc_mir_dataflow/src/value_analysis.rs | 173 ++++++++++++++---- .../src/dataflow_const_prop.rs | 103 +++++++++-- ...mutate_discriminant.DataflowConstProp.diff | 26 +++ tests/mir-opt/dataflow-const-prop/enum.rs | 45 ++++- ...iff => enum.simple.DataflowConstProp.diff} | 16 +- 6 files changed, 305 insertions(+), 59 deletions(-) create mode 100644 tests/mir-opt/dataflow-const-prop/enum.mutate_discriminant.DataflowConstProp.diff rename tests/mir-opt/dataflow-const-prop/{enum.main.DataflowConstProp.diff => enum.simple.DataflowConstProp.diff} (84%) diff --git a/compiler/rustc_mir_dataflow/src/lib.rs b/compiler/rustc_mir_dataflow/src/lib.rs index 7f40cfca32fff..3e382f500afbe 100644 --- a/compiler/rustc_mir_dataflow/src/lib.rs +++ b/compiler/rustc_mir_dataflow/src/lib.rs @@ -1,6 +1,7 @@ #![feature(associated_type_defaults)] #![feature(box_patterns)] #![feature(exact_size_is_empty)] +#![feature(let_chains)] #![feature(min_specialization)] #![feature(once_cell)] #![feature(stmt_expr_attributes)] diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 8003da6bbd269..03b6c182062db 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -65,10 +65,8 @@ pub trait ValueAnalysis<'tcx> { StatementKind::Assign(box (place, rvalue)) => { self.handle_assign(*place, rvalue, state); } - StatementKind::SetDiscriminant { .. } => { - // Could treat this as writing a constant to a pseudo-place. - // But discriminants are currently not tracked, so we do nothing. - // Related: https://github.com/rust-lang/unsafe-code-guidelines/issues/84 + StatementKind::SetDiscriminant { box ref place, .. } => { + state.flood_discr(place.as_ref(), self.map()); } StatementKind::Intrinsic(box intrinsic) => { self.handle_intrinsic(intrinsic, state); @@ -447,26 +445,29 @@ impl State { } pub fn flood_with(&mut self, place: PlaceRef<'_>, map: &Map, value: V) { - if let Some(root) = map.find(place) { - self.flood_idx_with(root, map, value); - } + let StateData::Reachable(values) = &mut self.0 else { return }; + map.for_each_aliasing_place(place, None, &mut |place| { + if let Some(vi) = map.places[place].value_index { + values[vi] = value.clone(); + } + }); } pub fn flood(&mut self, place: PlaceRef<'_>, map: &Map) { self.flood_with(place, map, V::top()) } - pub fn flood_idx_with(&mut self, place: PlaceIndex, map: &Map, value: V) { + pub fn flood_discr_with(&mut self, place: PlaceRef<'_>, map: &Map, value: V) { let StateData::Reachable(values) = &mut self.0 else { return }; - map.preorder_invoke(place, &mut |place| { + map.for_each_aliasing_place(place, Some(TrackElem::Discriminant), &mut |place| { if let Some(vi) = map.places[place].value_index { values[vi] = value.clone(); } }); } - pub fn flood_idx(&mut self, place: PlaceIndex, map: &Map) { - self.flood_idx_with(place, map, V::top()) + pub fn flood_discr(&mut self, place: PlaceRef<'_>, map: &Map) { + self.flood_discr_with(place, map, V::top()) } /// Copies `source` to `target`, including all tracked places beneath. @@ -474,7 +475,9 @@ impl State { /// If `target` contains a place that is not contained in `source`, it will be overwritten with /// Top. Also, because this will copy all entries one after another, it may only be used for /// places that are non-overlapping or identical. - pub fn assign_place_idx(&mut self, target: PlaceIndex, source: PlaceIndex, map: &Map) { + /// + /// The target place must have been flooded before calling this method. + fn assign_place_idx(&mut self, target: PlaceIndex, source: PlaceIndex, map: &Map) { let StateData::Reachable(values) = &mut self.0 else { return }; // If both places are tracked, we copy the value to the target. If the target is tracked, @@ -492,26 +495,28 @@ impl State { let projection = map.places[target_child].proj_elem.unwrap(); if let Some(source_child) = map.projections.get(&(source, projection)) { self.assign_place_idx(target_child, *source_child, map); - } else { - self.flood_idx(target_child, map); } } } pub fn assign(&mut self, target: PlaceRef<'_>, result: ValueOrPlace, map: &Map) { + self.flood(target, map); if let Some(target) = map.find(target) { self.assign_idx(target, result, map); - } else { - // We don't track this place nor any projections, assignment can be ignored. } } + pub fn assign_discr(&mut self, target: PlaceRef<'_>, result: ValueOrPlace, map: &Map) { + self.flood_discr(target, map); + if let Some(target) = map.find_discr(target) { + self.assign_idx(target, result, map); + } + } + + /// The target place must have been flooded before calling this method. pub fn assign_idx(&mut self, target: PlaceIndex, result: ValueOrPlace, map: &Map) { match result { ValueOrPlace::Value(value) => { - // First flood the target place in case we also track any projections (although - // this scenario is currently not well-supported by the API). - self.flood_idx(target, map); let StateData::Reachable(values) = &mut self.0 else { return }; if let Some(value_index) = map.places[target].value_index { values[value_index] = value; @@ -526,6 +531,14 @@ impl State { map.find(place).map(|place| self.get_idx(place, map)).unwrap_or(V::top()) } + /// Retrieve the value stored for a place, or ⊤ if it is not tracked. + pub fn get_discr(&self, place: PlaceRef<'_>, map: &Map) -> V { + match map.find_discr(place) { + Some(place) => self.get_idx(place, map), + None => V::top(), + } + } + /// Retrieve the value stored for a place index, or ⊤ if it is not tracked. pub fn get_idx(&self, place: PlaceIndex, map: &Map) -> V { match &self.0 { @@ -582,7 +595,6 @@ impl Map { /// This is currently the only way to create a [`Map`]. The way in which the tracked places are /// chosen is an implementation detail and may not be relied upon (other than that their type /// passes the filter). - #[instrument(skip_all, level = "debug")] pub fn from_filter<'tcx>( tcx: TyCtxt<'tcx>, body: &Body<'tcx>, @@ -614,7 +626,7 @@ impl Map { /// Potentially register the (local, projection) place and its fields, recursively. /// - /// Invariant: The projection must only contain fields. + /// Invariant: The projection must only contain trackable elements. fn register_with_filter_rec<'tcx>( &mut self, tcx: TyCtxt<'tcx>, @@ -623,21 +635,46 @@ impl Map { ty: Ty<'tcx>, filter: &mut impl FnMut(Ty<'tcx>) -> bool, ) { - if filter(ty) { - // We know that the projection only contains trackable elements. - let place = self.make_place(local, projection).unwrap(); + // We know that the projection only contains trackable elements. + let place = self.make_place(local, projection).unwrap(); - // Allocate a value slot if it doesn't have one. - if self.places[place].value_index.is_none() { - self.places[place].value_index = Some(self.value_count.into()); - self.value_count += 1; + // Allocate a value slot if it doesn't have one, and the user requested one. + if self.places[place].value_index.is_none() && filter(ty) { + self.places[place].value_index = Some(self.value_count.into()); + self.value_count += 1; + } + + if ty.is_enum() { + let discr_ty = ty.discriminant_ty(tcx); + if filter(discr_ty) { + let discr = *self + .projections + .entry((place, TrackElem::Discriminant)) + .or_insert_with(|| { + // Prepend new child to the linked list. + let next = self.places.push(PlaceInfo::new(Some(TrackElem::Discriminant))); + self.places[next].next_sibling = self.places[place].first_child; + self.places[place].first_child = Some(next); + next + }); + + // Allocate a value slot if it doesn't have one. + if self.places[discr].value_index.is_none() { + self.places[discr].value_index = Some(self.value_count.into()); + self.value_count += 1; + } } } // Recurse with all fields of this place. iter_fields(ty, tcx, |variant, field, ty| { - if variant.is_some() { - // Downcasts are currently not supported. + if let Some(variant) = variant { + projection.push(PlaceElem::Downcast(None, variant)); + let _ = self.make_place(local, projection); + projection.push(PlaceElem::Field(field, ty)); + self.register_with_filter_rec(tcx, local, projection, ty, filter); + projection.pop(); + projection.pop(); return; } projection.push(PlaceElem::Field(field, ty)); @@ -694,13 +731,77 @@ impl Map { Some(index) } + /// Locates the given place, if it exists in the tree. + pub fn find_discr(&self, place: PlaceRef<'_>) -> Option { + let index = self.find(place)?; + self.apply(index, TrackElem::Discriminant) + } + /// Iterate over all direct children. pub fn children(&self, parent: PlaceIndex) -> impl Iterator + '_ { Children::new(self, parent) } + /// Invoke a function on the given place and all places that may alias it. + /// + /// In particular, when the given place has a variant downcast, we invoke the function on all + /// the other variants. + /// + /// `tail_elem` allows to support discriminants that are not a place in MIR, but that we track + /// as such. + fn for_each_aliasing_place( + &self, + place: PlaceRef<'_>, + tail_elem: Option, + f: &mut impl FnMut(PlaceIndex), + ) { + let Some(&Some(mut index)) = self.locals.get(place.local) else { + // The local is not tracked at all, nothing to invalidate. + return; + }; + let elems = place + .projection + .iter() + .map(|&elem| elem.try_into()) + .chain(tail_elem.map(Ok).into_iter()); + for elem in elems { + let Ok(elem) = elem else { return }; + let sub = self.apply(index, elem); + if let TrackElem::Variant(..) | TrackElem::Discriminant = elem { + // Writing to an enum variant field invalidates the other variants and the discriminant. + self.for_each_variant_sibling(index, sub, f); + } + if let Some(sub) = sub { + index = sub + } else { + return; + } + } + self.preorder_invoke(index, f); + } + + /// Invoke the given function on all the descendants of the given place, except one branch. + pub fn for_each_variant_sibling( + &self, + parent: PlaceIndex, + preserved_child: Option, + f: &mut impl FnMut(PlaceIndex), + ) { + for sibling in self.children(parent) { + let elem = self.places[sibling].proj_elem; + // Only invalidate variants and discriminant. Fields (for generators) are not + // invalidated by assignment to a variant. + if let Some(TrackElem::Variant(..) | TrackElem::Discriminant) = elem + // Only invalidate the other variants, the current one is fine. + && Some(sibling) != preserved_child + { + self.preorder_invoke(sibling, f); + } + } + } + /// Invoke a function on the given place and all descendants. - pub fn preorder_invoke(&self, root: PlaceIndex, f: &mut impl FnMut(PlaceIndex)) { + fn preorder_invoke(&self, root: PlaceIndex, f: &mut impl FnMut(PlaceIndex)) { f(root); for child in self.children(root) { self.preorder_invoke(child, f); @@ -759,6 +860,7 @@ impl<'a> Iterator for Children<'a> { } /// Used as the result of an operand or r-value. +#[derive(Debug)] pub enum ValueOrPlace { Value(V), Place(PlaceIndex), @@ -776,6 +878,8 @@ impl ValueOrPlace { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum TrackElem { Field(Field), + Variant(VariantIdx), + Discriminant, } impl TryFrom> for TrackElem { @@ -784,6 +888,7 @@ impl TryFrom> for TrackElem { fn try_from(value: ProjectionElem) -> Result { match value { ProjectionElem::Field(field, _) => Ok(TrackElem::Field(field)), + ProjectionElem::Downcast(_, idx) => Ok(TrackElem::Variant(idx)), _ => Err(()), } } @@ -900,6 +1005,12 @@ fn debug_with_context_rec( for child in map.children(place) { let info_elem = map.places[child].proj_elem.unwrap(); let child_place_str = match info_elem { + TrackElem::Discriminant => { + format!("discriminant({})", place_str) + } + TrackElem::Variant(idx) => { + format!("({} as {:?})", place_str, idx) + } TrackElem::Field(field) => { if place_str.starts_with('*') { format!("({}).{}", place_str, field.index()) diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index 949a59a97bfb6..f10f208f5de3c 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -13,6 +13,7 @@ use rustc_mir_dataflow::value_analysis::{Map, State, TrackElem, ValueAnalysis, V use rustc_mir_dataflow::{lattice::FlatSet, Analysis, ResultsVisitor, SwitchIntEdgeEffects}; use rustc_span::DUMMY_SP; use rustc_target::abi::Align; +use rustc_target::abi::VariantIdx; use crate::MirPass; @@ -30,6 +31,7 @@ impl<'tcx> MirPass<'tcx> for DataflowConstProp { #[instrument(skip_all level = "debug")] fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + debug!(def_id = ?body.source.def_id()); if tcx.sess.mir_opt_level() < 4 && body.basic_blocks.len() > BLOCK_LIMIT { debug!("aborted dataflow const prop due too many basic blocks"); return; @@ -63,14 +65,31 @@ impl<'tcx> MirPass<'tcx> for DataflowConstProp { } } -struct ConstAnalysis<'tcx> { +struct ConstAnalysis<'a, 'tcx> { map: Map, tcx: TyCtxt<'tcx>, + local_decls: &'a LocalDecls<'tcx>, ecx: InterpCx<'tcx, 'tcx, DummyMachine>, param_env: ty::ParamEnv<'tcx>, } -impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { +impl<'tcx> ConstAnalysis<'_, 'tcx> { + fn eval_disciminant( + &self, + enum_ty: Ty<'tcx>, + variant_index: VariantIdx, + ) -> Option> { + if !enum_ty.is_enum() { + return None; + } + let discr = enum_ty.discriminant_for_variant(self.tcx, variant_index)?; + let discr_layout = self.tcx.layout_of(self.param_env.and(discr.ty)).ok()?; + let discr_value = Scalar::try_from_uint(discr.val, discr_layout.size)?; + Some(ScalarTy(discr_value, discr.ty)) + } +} + +impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { type Value = FlatSet>; const NAME: &'static str = "ConstAnalysis"; @@ -79,6 +98,25 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { &self.map } + fn handle_statement(&self, statement: &Statement<'tcx>, state: &mut State) { + match statement.kind { + StatementKind::SetDiscriminant { box ref place, variant_index } => { + state.flood_discr(place.as_ref(), &self.map); + if self.map.find_discr(place.as_ref()).is_some() { + let enum_ty = place.ty(self.local_decls, self.tcx).ty; + if let Some(discr) = self.eval_disciminant(enum_ty, variant_index) { + state.assign_discr( + place.as_ref(), + ValueOrPlace::Value(FlatSet::Elem(discr)), + &self.map, + ); + } + } + } + _ => self.super_statement(statement, state), + } + } + fn handle_assign( &self, target: Place<'tcx>, @@ -87,17 +125,22 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { ) { match rvalue { Rvalue::Aggregate(kind, operands) => { - let target = self.map().find(target.as_ref()); - if let Some(target) = target { - state.flood_idx_with(target, self.map(), FlatSet::Bottom); - let field_based = match **kind { - AggregateKind::Tuple | AggregateKind::Closure(..) => true, - AggregateKind::Adt(def_id, ..) => { - matches!(self.tcx.def_kind(def_id), DefKind::Struct) + state.flood_with(target.as_ref(), self.map(), FlatSet::Bottom); + if let Some(target_idx) = self.map().find(target.as_ref()) { + let (variant_target, variant_index) = match **kind { + AggregateKind::Tuple | AggregateKind::Closure(..) => { + (Some(target_idx), None) + } + AggregateKind::Adt(def_id, variant_index, ..) => { + match self.tcx.def_kind(def_id) { + DefKind::Struct => (Some(target_idx), None), + DefKind::Enum => (Some(target_idx), Some(variant_index)), + _ => (None, None), + } } - _ => false, + _ => (None, None), }; - if field_based { + if let Some(target) = variant_target { for (field_index, operand) in operands.iter().enumerate() { if let Some(field) = self .map() @@ -108,15 +151,20 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { } } } + if let Some(variant_index) = variant_index + && let Some(discr_idx) = self.map().apply(target_idx, TrackElem::Discriminant) + { + let enum_ty = target.ty(self.local_decls, self.tcx).ty; + if let Some(discr_val) = self.eval_disciminant(enum_ty, variant_index) { + state.assign_idx(discr_idx, ValueOrPlace::Value(FlatSet::Elem(discr_val)), &self.map); + } + } } } Rvalue::CheckedBinaryOp(op, box (left, right)) => { + state.flood(target.as_ref(), self.map()); + let target = self.map().find(target.as_ref()); - if let Some(target) = target { - // We should not track any projections other than - // what is overwritten below, but just in case... - state.flood_idx(target, self.map()); - } let value_target = target .and_then(|target| self.map().apply(target, TrackElem::Field(0_u32.into()))); @@ -195,6 +243,9 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { FlatSet::Bottom => ValueOrPlace::Value(FlatSet::Bottom), FlatSet::Top => ValueOrPlace::Value(FlatSet::Top), }, + Rvalue::Discriminant(place) => { + ValueOrPlace::Value(state.get_discr(place.as_ref(), self.map())) + } _ => self.super_rvalue(rvalue, state), } } @@ -268,12 +319,13 @@ impl<'tcx> std::fmt::Debug for ScalarTy<'tcx> { } } -impl<'tcx> ConstAnalysis<'tcx> { - pub fn new(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, map: Map) -> Self { +impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { + pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, map: Map) -> Self { let param_env = tcx.param_env(body.source.def_id()); Self { map, tcx, + local_decls: &body.local_decls, ecx: InterpCx::new(tcx, DUMMY_SP, param_env, DummyMachine), param_env: param_env, } @@ -466,6 +518,21 @@ impl<'tcx, 'map, 'a> Visitor<'tcx> for OperandCollector<'tcx, 'map, 'a> { _ => (), } } + + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + match rvalue { + Rvalue::Discriminant(place) => { + match self.state.get_discr(place.as_ref(), self.visitor.map) { + FlatSet::Top => (), + FlatSet::Elem(value) => { + self.visitor.before_effect.insert((location, *place), value); + } + FlatSet::Bottom => (), + } + } + _ => self.super_rvalue(rvalue, location), + } + } } struct DummyMachine; diff --git a/tests/mir-opt/dataflow-const-prop/enum.mutate_discriminant.DataflowConstProp.diff b/tests/mir-opt/dataflow-const-prop/enum.mutate_discriminant.DataflowConstProp.diff new file mode 100644 index 0000000000000..038e6c6bd9005 --- /dev/null +++ b/tests/mir-opt/dataflow-const-prop/enum.mutate_discriminant.DataflowConstProp.diff @@ -0,0 +1,26 @@ +- // MIR for `mutate_discriminant` before DataflowConstProp ++ // MIR for `mutate_discriminant` after DataflowConstProp + + fn mutate_discriminant() -> u8 { + let mut _0: u8; // return place in scope 0 at $DIR/enum.rs:+0:29: +0:31 + let mut _1: std::option::Option; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + let mut _2: isize; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + + bb0: { + discriminant(_1) = 1; // scope 0 at $DIR/enum.rs:+4:13: +4:34 + (((_1 as variant#1).0: NonZeroUsize).0: usize) = const 0_usize; // scope 0 at $DIR/enum.rs:+6:13: +6:64 + _2 = discriminant(_1); // scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + switchInt(_2) -> [0: bb1, otherwise: bb2]; // scope 0 at $DIR/enum.rs:+9:13: +12:14 + } + + bb1: { + _0 = const 1_u8; // scope 0 at $DIR/enum.rs:+15:13: +15:20 + return; // scope 0 at $DIR/enum.rs:+16:13: +16:21 + } + + bb2: { + _0 = const 2_u8; // scope 0 at $DIR/enum.rs:+19:13: +19:20 + unreachable; // scope 0 at $DIR/enum.rs:+20:13: +20:26 + } + } + diff --git a/tests/mir-opt/dataflow-const-prop/enum.rs b/tests/mir-opt/dataflow-const-prop/enum.rs index 13288577dea3f..7ea405bd9c408 100644 --- a/tests/mir-opt/dataflow-const-prop/enum.rs +++ b/tests/mir-opt/dataflow-const-prop/enum.rs @@ -1,13 +1,52 @@ // unit-test: DataflowConstProp -// Not trackable, because variants could be aliased. +#![feature(custom_mir, core_intrinsics, rustc_attrs)] + +use std::intrinsics::mir::*; + enum E { V1(i32), V2(i32) } -// EMIT_MIR enum.main.DataflowConstProp.diff -fn main() { +// EMIT_MIR enum.simple.DataflowConstProp.diff +fn simple() { let e = E::V1(0); let x = match e { E::V1(x) => x, E::V2(x) => x }; } + +#[rustc_layout_scalar_valid_range_start(1)] +#[rustc_nonnull_optimization_guaranteed] +struct NonZeroUsize(usize); + +// EMIT_MIR enum.mutate_discriminant.DataflowConstProp.diff +#[custom_mir(dialect = "runtime", phase = "post-cleanup")] +fn mutate_discriminant() -> u8 { + mir!( + let x: Option; + { + SetDiscriminant(x, 1); + // This assignment overwrites the niche in which the discriminant is stored. + place!(Field(Field(Variant(x, 1), 0), 0)) = 0_usize; + // So we cannot know the value of this discriminant. + let a = Discriminant(x); + match a { + 0 => bb1, + _ => bad, + } + } + bb1 = { + RET = 1; + Return() + } + bad = { + RET = 2; + Unreachable() + } + ) +} + +fn main() { + simple(); + mutate_discriminant(); +} diff --git a/tests/mir-opt/dataflow-const-prop/enum.main.DataflowConstProp.diff b/tests/mir-opt/dataflow-const-prop/enum.simple.DataflowConstProp.diff similarity index 84% rename from tests/mir-opt/dataflow-const-prop/enum.main.DataflowConstProp.diff rename to tests/mir-opt/dataflow-const-prop/enum.simple.DataflowConstProp.diff index d049c79d78def..1fb65e6584525 100644 --- a/tests/mir-opt/dataflow-const-prop/enum.main.DataflowConstProp.diff +++ b/tests/mir-opt/dataflow-const-prop/enum.simple.DataflowConstProp.diff @@ -1,8 +1,8 @@ -- // MIR for `main` before DataflowConstProp -+ // MIR for `main` after DataflowConstProp +- // MIR for `simple` before DataflowConstProp ++ // MIR for `simple` after DataflowConstProp - fn main() -> () { - let mut _0: (); // return place in scope 0 at $DIR/enum.rs:+0:11: +0:11 + fn simple() -> () { + let mut _0: (); // return place in scope 0 at $DIR/enum.rs:+0:13: +0:13 let _1: E; // in scope 0 at $DIR/enum.rs:+1:9: +1:10 let mut _3: isize; // in scope 0 at $DIR/enum.rs:+2:23: +2:31 scope 1 { @@ -25,8 +25,10 @@ StorageLive(_1); // scope 0 at $DIR/enum.rs:+1:9: +1:10 _1 = E::V1(const 0_i32); // scope 0 at $DIR/enum.rs:+1:13: +1:21 StorageLive(_2); // scope 1 at $DIR/enum.rs:+2:9: +2:10 - _3 = discriminant(_1); // scope 1 at $DIR/enum.rs:+2:19: +2:20 - switchInt(move _3) -> [0: bb3, 1: bb1, otherwise: bb2]; // scope 1 at $DIR/enum.rs:+2:13: +2:20 +- _3 = discriminant(_1); // scope 1 at $DIR/enum.rs:+2:19: +2:20 +- switchInt(move _3) -> [0: bb3, 1: bb1, otherwise: bb2]; // scope 1 at $DIR/enum.rs:+2:13: +2:20 ++ _3 = const 0_isize; // scope 1 at $DIR/enum.rs:+2:19: +2:20 ++ switchInt(const 0_isize) -> [0: bb3, 1: bb1, otherwise: bb2]; // scope 1 at $DIR/enum.rs:+2:13: +2:20 } bb1: { @@ -50,7 +52,7 @@ } bb4: { - _0 = const (); // scope 0 at $DIR/enum.rs:+0:11: +3:2 + _0 = const (); // scope 0 at $DIR/enum.rs:+0:13: +3:2 StorageDead(_2); // scope 1 at $DIR/enum.rs:+3:1: +3:2 StorageDead(_1); // scope 0 at $DIR/enum.rs:+3:1: +3:2 return; // scope 0 at $DIR/enum.rs:+3:2: +3:2 From c48756cdbfb1725251cbfa6fe760b2cb4e47b2d9 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 28 Jan 2023 11:23:18 +0000 Subject: [PATCH 06/17] Limit creation of tracked place directly. --- .../rustc_mir_dataflow/src/value_analysis.rs | 22 +++++++++++++++---- .../src/dataflow_const_prop.rs | 11 ++++------ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 03b6c182062db..f587f17f12a1b 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -599,10 +599,11 @@ impl Map { tcx: TyCtxt<'tcx>, body: &Body<'tcx>, filter: impl FnMut(Ty<'tcx>) -> bool, + place_limit: Option, ) -> Self { let mut map = Self::new(); let exclude = excluded_locals(body); - map.register_with_filter(tcx, body, filter, exclude); + map.register_with_filter(tcx, body, filter, exclude, place_limit); debug!("registered {} places ({} nodes in total)", map.value_count, map.places.len()); map } @@ -614,12 +615,20 @@ impl Map { body: &Body<'tcx>, mut filter: impl FnMut(Ty<'tcx>) -> bool, exclude: BitSet, + place_limit: Option, ) { // We use this vector as stack, pushing and popping projections. let mut projection = Vec::new(); for (local, decl) in body.local_decls.iter_enumerated() { if !exclude.contains(local) { - self.register_with_filter_rec(tcx, local, &mut projection, decl.ty, &mut filter); + self.register_with_filter_rec( + tcx, + local, + &mut projection, + decl.ty, + &mut filter, + place_limit, + ); } } } @@ -634,7 +643,12 @@ impl Map { projection: &mut Vec>, ty: Ty<'tcx>, filter: &mut impl FnMut(Ty<'tcx>) -> bool, + place_limit: Option, ) { + if let Some(place_limit) = place_limit && self.value_count >= place_limit { + return + } + // We know that the projection only contains trackable elements. let place = self.make_place(local, projection).unwrap(); @@ -672,13 +686,13 @@ impl Map { projection.push(PlaceElem::Downcast(None, variant)); let _ = self.make_place(local, projection); projection.push(PlaceElem::Field(field, ty)); - self.register_with_filter_rec(tcx, local, projection, ty, filter); + self.register_with_filter_rec(tcx, local, projection, ty, filter, place_limit); projection.pop(); projection.pop(); return; } projection.push(PlaceElem::Field(field, ty)); - self.register_with_filter_rec(tcx, local, projection, ty, filter); + self.register_with_filter_rec(tcx, local, projection, ty, filter, place_limit); projection.pop(); }); } diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index f10f208f5de3c..bfb1eb8b5fb79 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -37,9 +37,6 @@ impl<'tcx> MirPass<'tcx> for DataflowConstProp { return; } - // Decide which places to track during the analysis. - let map = Map::from_filter(tcx, body, Ty::is_scalar); - // We want to have a somewhat linear runtime w.r.t. the number of statements/terminators. // Let's call this number `n`. Dataflow analysis has `O(h*n)` transfer function // applications, where `h` is the height of the lattice. Because the height of our lattice @@ -48,10 +45,10 @@ impl<'tcx> MirPass<'tcx> for DataflowConstProp { // `O(num_nodes * tracked_places * n)` in terms of time complexity. Since the number of // map nodes is strongly correlated to the number of tracked places, this becomes more or // less `O(n)` if we place a constant limit on the number of tracked places. - if tcx.sess.mir_opt_level() < 4 && map.tracked_places() > PLACE_LIMIT { - debug!("aborted dataflow const prop due to too many tracked places"); - return; - } + let place_limit = if tcx.sess.mir_opt_level() < 4 { Some(PLACE_LIMIT) } else { None }; + + // Decide which places to track during the analysis. + let map = Map::from_filter(tcx, body, Ty::is_scalar, place_limit); // Perform the actual dataflow analysis. let analysis = ConstAnalysis::new(tcx, body, map); From 9af191f86f2c81ec5613ae35ab1a3b2ac3edbdee Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 29 Jan 2023 14:20:45 +0000 Subject: [PATCH 07/17] Improve value_analysis API. --- .../rustc_mir_dataflow/src/value_analysis.rs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index f587f17f12a1b..353b8d801d57b 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -735,20 +735,31 @@ impl Map { } /// Locates the given place, if it exists in the tree. - pub fn find(&self, place: PlaceRef<'_>) -> Option { + pub fn find_extra( + &self, + place: PlaceRef<'_>, + extra: impl IntoIterator, + ) -> Option { let mut index = *self.locals.get(place.local)?.as_ref()?; for &elem in place.projection { index = self.apply(index, elem.try_into().ok()?)?; } + for elem in extra { + index = self.apply(index, elem)?; + } Some(index) } /// Locates the given place, if it exists in the tree. + pub fn find(&self, place: PlaceRef<'_>) -> Option { + self.find_extra(place, []) + } + + /// Locates the given place and applies `Discriminant`, if it exists in the tree. pub fn find_discr(&self, place: PlaceRef<'_>) -> Option { - let index = self.find(place)?; - self.apply(index, TrackElem::Discriminant) + self.find_extra(place, [TrackElem::Discriminant]) } /// Iterate over all direct children. @@ -763,14 +774,14 @@ impl Map { /// /// `tail_elem` allows to support discriminants that are not a place in MIR, but that we track /// as such. - fn for_each_aliasing_place( + pub fn for_each_aliasing_place( &self, place: PlaceRef<'_>, tail_elem: Option, f: &mut impl FnMut(PlaceIndex), ) { let Some(&Some(mut index)) = self.locals.get(place.local) else { - // The local is not tracked at all, nothing to invalidate. + // The local is not tracked at all, so it does not alias anything. return; }; let elems = place @@ -782,7 +793,7 @@ impl Map { let Ok(elem) = elem else { return }; let sub = self.apply(index, elem); if let TrackElem::Variant(..) | TrackElem::Discriminant = elem { - // Writing to an enum variant field invalidates the other variants and the discriminant. + // Enum variant fields and enum discriminants alias each another. self.for_each_variant_sibling(index, sub, f); } if let Some(sub) = sub { @@ -795,7 +806,7 @@ impl Map { } /// Invoke the given function on all the descendants of the given place, except one branch. - pub fn for_each_variant_sibling( + fn for_each_variant_sibling( &self, parent: PlaceIndex, preserved_child: Option, From 67a8c16fe285dc5dc3ca8a0c74fb1bcfa58ce8dc Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 30 Jan 2023 17:37:56 +0000 Subject: [PATCH 08/17] Complete for_each_aliasing_place. --- compiler/rustc_middle/src/mir/mod.rs | 8 ++++++++ compiler/rustc_mir_dataflow/src/value_analysis.rs | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 10ac7e0d39af6..e272c90e0cdaf 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1640,6 +1640,14 @@ impl<'tcx> PlaceRef<'tcx> { } } + /// Returns `true` if this `Place` contains a `Deref` projection. + /// + /// If `Place::is_indirect` returns false, the caller knows that the `Place` refers to the + /// same region of memory as its base. + pub fn is_indirect(&self) -> bool { + self.projection.iter().any(|elem| elem.is_indirect()) + } + /// If MirPhase >= Derefered and if projection contains Deref, /// It's guaranteed to be in the first place pub fn has_deref(&self) -> bool { diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 353b8d801d57b..f24280e218716 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -780,6 +780,10 @@ impl Map { tail_elem: Option, f: &mut impl FnMut(PlaceIndex), ) { + if place.is_indirect() { + // We do not track indirect places. + return; + } let Some(&Some(mut index)) = self.locals.get(place.local) else { // The local is not tracked at all, so it does not alias anything. return; @@ -790,6 +794,9 @@ impl Map { .map(|&elem| elem.try_into()) .chain(tail_elem.map(Ok).into_iter()); for elem in elems { + // A field aliases the parent place. + f(index); + let Ok(elem) = elem else { return }; let sub = self.apply(index, elem); if let TrackElem::Variant(..) | TrackElem::Discriminant = elem { From df889c9821970020abb8dbb1ed9b0014e38c6137 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 3 Feb 2023 16:39:10 +0000 Subject: [PATCH 09/17] Rename assign_idx methods. --- .../rustc_mir_dataflow/src/value_analysis.rs | 55 +++++++++++-------- .../src/dataflow_const_prop.rs | 15 +++-- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index f24280e218716..2da7cdd02a7f8 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -24,7 +24,7 @@ //! - The bottom state denotes uninitialized memory. Because we are only doing a sound approximation //! of the actual execution, we can also use this state for places where access would be UB. //! -//! - The assignment logic in `State::assign_place_idx` assumes that the places are non-overlapping, +//! - The assignment logic in `State::insert_place_idx` assumes that the places are non-overlapping, //! or identical. Note that this refers to place expressions, not memory locations. //! //! - Currently, places that have their reference taken cannot be tracked. Although this would be @@ -470,6 +470,28 @@ impl State { self.flood_discr_with(place, map, V::top()) } + /// Low-level method that assigns to a place. + /// This does nothing if the place is not tracked. + /// + /// The target place must have been flooded before calling this method. + pub fn insert_idx(&mut self, target: PlaceIndex, result: ValueOrPlace, map: &Map) { + match result { + ValueOrPlace::Value(value) => self.insert_value_idx(target, value, map), + ValueOrPlace::Place(source) => self.insert_place_idx(target, source, map), + } + } + + /// Low-level method that assigns a value to a place. + /// This does nothing if the place is not tracked. + /// + /// The target place must have been flooded before calling this method. + pub fn insert_value_idx(&mut self, target: PlaceIndex, value: V, map: &Map) { + let StateData::Reachable(values) = &mut self.0 else { return }; + if let Some(value_index) = map.places[target].value_index { + values[value_index] = value; + } + } + /// Copies `source` to `target`, including all tracked places beneath. /// /// If `target` contains a place that is not contained in `source`, it will be overwritten with @@ -477,52 +499,39 @@ impl State { /// places that are non-overlapping or identical. /// /// The target place must have been flooded before calling this method. - fn assign_place_idx(&mut self, target: PlaceIndex, source: PlaceIndex, map: &Map) { + fn insert_place_idx(&mut self, target: PlaceIndex, source: PlaceIndex, map: &Map) { let StateData::Reachable(values) = &mut self.0 else { return }; - // If both places are tracked, we copy the value to the target. If the target is tracked, - // but the source is not, we have to invalidate the value in target. If the target is not - // tracked, then we don't have to do anything. + // If both places are tracked, we copy the value to the target. + // If the target is tracked, but the source is not, we do nothing, as invalidation has + // already been performed. if let Some(target_value) = map.places[target].value_index { if let Some(source_value) = map.places[source].value_index { values[target_value] = values[source_value].clone(); - } else { - values[target_value] = V::top(); } } for target_child in map.children(target) { // Try to find corresponding child and recurse. Reasoning is similar as above. let projection = map.places[target_child].proj_elem.unwrap(); if let Some(source_child) = map.projections.get(&(source, projection)) { - self.assign_place_idx(target_child, *source_child, map); + self.insert_place_idx(target_child, *source_child, map); } } } + /// Helper method to interpret `target = result`. pub fn assign(&mut self, target: PlaceRef<'_>, result: ValueOrPlace, map: &Map) { self.flood(target, map); if let Some(target) = map.find(target) { - self.assign_idx(target, result, map); + self.insert_idx(target, result, map); } } + /// Helper method for assignments to a discriminant. pub fn assign_discr(&mut self, target: PlaceRef<'_>, result: ValueOrPlace, map: &Map) { self.flood_discr(target, map); if let Some(target) = map.find_discr(target) { - self.assign_idx(target, result, map); - } - } - - /// The target place must have been flooded before calling this method. - pub fn assign_idx(&mut self, target: PlaceIndex, result: ValueOrPlace, map: &Map) { - match result { - ValueOrPlace::Value(value) => { - let StateData::Reachable(values) = &mut self.0 else { return }; - if let Some(value_index) = map.places[target].value_index { - values[value_index] = value; - } - } - ValueOrPlace::Place(source) => self.assign_place_idx(target, source, map), + self.insert_idx(target, result, map); } } diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index bfb1eb8b5fb79..d715e250ca4eb 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -144,7 +144,7 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { .apply(target, TrackElem::Field(Field::from_usize(field_index))) { let result = self.handle_operand(operand, state); - state.assign_idx(field, result, self.map()); + state.insert_idx(field, result, self.map()); } } } @@ -153,12 +153,13 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { { let enum_ty = target.ty(self.local_decls, self.tcx).ty; if let Some(discr_val) = self.eval_disciminant(enum_ty, variant_index) { - state.assign_idx(discr_idx, ValueOrPlace::Value(FlatSet::Elem(discr_val)), &self.map); + state.insert_value_idx(discr_idx, FlatSet::Elem(discr_val), &self.map); } } } } Rvalue::CheckedBinaryOp(op, box (left, right)) => { + // Flood everything now, so we can use `insert_value_idx` directly later. state.flood(target.as_ref(), self.map()); let target = self.map().find(target.as_ref()); @@ -172,7 +173,8 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { let (val, overflow) = self.binary_op(state, *op, left, right); if let Some(value_target) = value_target { - state.assign_idx(value_target, ValueOrPlace::Value(val), self.map()); + // We have flooded `target` earlier. + state.insert_value_idx(value_target, val, self.map()); } if let Some(overflow_target) = overflow_target { let overflow = match overflow { @@ -187,11 +189,8 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { } FlatSet::Bottom => FlatSet::Bottom, }; - state.assign_idx( - overflow_target, - ValueOrPlace::Value(overflow), - self.map(), - ); + // We have flooded `target` earlier. + state.insert_value_idx(overflow_target, overflow, self.map()); } } } From 903ca873f75e3fcea2bf4cf583ae835525e38367 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Sat, 21 Jan 2023 22:09:56 +0000 Subject: [PATCH 10/17] Suggest the correct array length on mismatch --- .../src/infer/error_reporting/mod.rs | 65 +++++++++++++++++++ tests/ui/consts/array-literal-len-mismatch.rs | 4 ++ .../consts/array-literal-len-mismatch.stderr | 11 ++++ tests/ui/consts/const-array-oob-arith.stderr | 8 ++- tests/ui/inference/array-len-mismatch.rs | 12 ++++ tests/ui/inference/array-len-mismatch.stderr | 21 ++++++ 6 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 tests/ui/consts/array-literal-len-mismatch.rs create mode 100644 tests/ui/consts/array-literal-len-mismatch.stderr create mode 100644 tests/ui/inference/array-len-mismatch.rs create mode 100644 tests/ui/inference/array-len-mismatch.stderr diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index 88a0d6def5ec2..e60c86aed012c 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -64,6 +64,7 @@ use rustc_errors::{Applicability, DiagnosticBuilder, DiagnosticStyledString}; use rustc_hir as hir; use rustc_hir::def::DefKind; use rustc_hir::def_id::{DefId, LocalDefId}; +use rustc_hir::intravisit::Visitor; use rustc_hir::lang_items::LangItem; use rustc_hir::Node; use rustc_middle::dep_graph::DepContext; @@ -1975,6 +1976,70 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> { (ty::Bool, ty::Tuple(list)) => if list.len() == 0 { self.suggest_let_for_letchains(&mut err, &trace.cause, span); } + (ty::Array(_, _), ty::Array(_, _)) => 'block: { + let hir = self.tcx.hir(); + let TypeError::FixedArraySize(sz) = terr else { + break 'block; + }; + let tykind = match hir.find_by_def_id(trace.cause.body_id) { + Some(hir::Node::Item(hir::Item { + kind: hir::ItemKind::Fn(_, _, body_id), + .. + })) => { + let body = hir.body(*body_id); + struct LetVisitor<'v> { + span: Span, + result: Option<&'v hir::Ty<'v>>, + } + impl<'v> Visitor<'v> for LetVisitor<'v> { + fn visit_stmt(&mut self, s: &'v hir::Stmt<'v>) { + if self.result.is_some() { + return; + } + // Find a local statement where the initializer has + // the same span as the error and the type is specified. + if let hir::Stmt { + kind: hir::StmtKind::Local(hir::Local { + init: Some(hir::Expr { + span: init_span, + .. + }), + ty: Some(array_ty), + .. + }), + .. + } = s + && init_span == &self.span { + self.result = Some(*array_ty); + } + } + } + let mut visitor = LetVisitor {span, result: None}; + visitor.visit_body(body); + visitor.result.map(|r| &r.peel_refs().kind) + } + Some(hir::Node::Item(hir::Item { + kind: hir::ItemKind::Const(ty, _), + .. + })) => { + Some(&ty.peel_refs().kind) + } + _ => None + }; + + if let Some(tykind) = tykind + && let hir::TyKind::Array(_, length) = tykind + && let hir::ArrayLen::Body(hir::AnonConst { hir_id, .. }) = length + && let Some(span) = self.tcx.hir().opt_span(*hir_id) + { + err.span_suggestion( + span, + "consider specifying the actual array length", + sz.found, + Applicability::MaybeIncorrect, + ); + } + } _ => {} } } diff --git a/tests/ui/consts/array-literal-len-mismatch.rs b/tests/ui/consts/array-literal-len-mismatch.rs new file mode 100644 index 0000000000000..b30ff61a99c53 --- /dev/null +++ b/tests/ui/consts/array-literal-len-mismatch.rs @@ -0,0 +1,4 @@ +const NUMBERS: [u8; 3] = [10, 20]; +//~^ ERROR mismatched types +//~^^ HELP consider specifying the actual array length +fn main() {} diff --git a/tests/ui/consts/array-literal-len-mismatch.stderr b/tests/ui/consts/array-literal-len-mismatch.stderr new file mode 100644 index 0000000000000..22fec638970a7 --- /dev/null +++ b/tests/ui/consts/array-literal-len-mismatch.stderr @@ -0,0 +1,11 @@ +error[E0308]: mismatched types + --> $DIR/array-literal-len-mismatch.rs:1:26 + | +LL | const NUMBERS: [u8; 3] = [10, 20]; + | - ^^^^^^^^ expected an array with a fixed size of 3 elements, found one with 2 elements + | | + | help: consider specifying the actual array length: `2` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. diff --git a/tests/ui/consts/const-array-oob-arith.stderr b/tests/ui/consts/const-array-oob-arith.stderr index f7a55d3ca7210..029d94273fae1 100644 --- a/tests/ui/consts/const-array-oob-arith.stderr +++ b/tests/ui/consts/const-array-oob-arith.stderr @@ -2,13 +2,17 @@ error[E0308]: mismatched types --> $DIR/const-array-oob-arith.rs:5:45 | LL | const BLUB: [i32; (ARR[0] - 40) as usize] = [5]; - | ^^^ expected an array with a fixed size of 2 elements, found one with 1 element + | ---------------------- ^^^ expected an array with a fixed size of 2 elements, found one with 1 element + | | + | help: consider specifying the actual array length: `1` error[E0308]: mismatched types --> $DIR/const-array-oob-arith.rs:8:44 | LL | const BOO: [i32; (ARR[0] - 41) as usize] = [5, 99]; - | ^^^^^^^ expected an array with a fixed size of 1 element, found one with 2 elements + | ---------------------- ^^^^^^^ expected an array with a fixed size of 1 element, found one with 2 elements + | | + | help: consider specifying the actual array length: `2` error: aborting due to 2 previous errors diff --git a/tests/ui/inference/array-len-mismatch.rs b/tests/ui/inference/array-len-mismatch.rs new file mode 100644 index 0000000000000..149d061029bc3 --- /dev/null +++ b/tests/ui/inference/array-len-mismatch.rs @@ -0,0 +1,12 @@ +fn returns_arr() -> [u8; 2] { + [1, 2] +} + +fn main() { + let wrong: [u8; 3] = [10, 20]; + //~^ ERROR mismatched types + //~^^ HELP consider specifying the actual array length + let wrong: [u8; 3] = returns_arr(); + //~^ ERROR mismatched types + //~^^ HELP consider specifying the actual array length +} diff --git a/tests/ui/inference/array-len-mismatch.stderr b/tests/ui/inference/array-len-mismatch.stderr new file mode 100644 index 0000000000000..7358e47839725 --- /dev/null +++ b/tests/ui/inference/array-len-mismatch.stderr @@ -0,0 +1,21 @@ +error[E0308]: mismatched types + --> $DIR/array-len-mismatch.rs:6:26 + | +LL | let wrong: [u8; 3] = [10, 20]; + | ------- ^^^^^^^^ expected an array with a fixed size of 3 elements, found one with 2 elements + | | | + | | help: consider specifying the actual array length: `2` + | expected due to this + +error[E0308]: mismatched types + --> $DIR/array-len-mismatch.rs:9:26 + | +LL | let wrong: [u8; 3] = returns_arr(); + | ------- ^^^^^^^^^^^^^ expected an array with a fixed size of 3 elements, found one with 2 elements + | | | + | | help: consider specifying the actual array length: `2` + | expected due to this + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0308`. From e723e4359048417c30b3eba4fba7417dddb380a9 Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Sun, 12 Feb 2023 16:57:05 -0500 Subject: [PATCH 11/17] Enable #[thread_local] on armv6k-nintendo-3ds Since libctru 2.1.2 was released (https://github.com/devkitPro/libctru/releases/tag/v2.1.2) we should be able to use real #[thread_local] without corruption issues on the 3DS target. --- compiler/rustc_target/src/spec/armv6k_nintendo_3ds.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_target/src/spec/armv6k_nintendo_3ds.rs b/compiler/rustc_target/src/spec/armv6k_nintendo_3ds.rs index 40ec6f961f544..23f4a5abf7795 100644 --- a/compiler/rustc_target/src/spec/armv6k_nintendo_3ds.rs +++ b/compiler/rustc_target/src/spec/armv6k_nintendo_3ds.rs @@ -29,8 +29,7 @@ pub fn target() -> Target { pre_link_args, exe_suffix: ".elf".into(), no_default_libraries: false, - // There are some issues in debug builds with this enabled in certain programs. - has_thread_local: false, + has_thread_local: true, ..Default::default() }, } From 09797a463cd1bc70bc439aaf0c94b7b5a80f5bfb Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 13 Feb 2023 17:01:03 +0000 Subject: [PATCH 12/17] Typo. --- compiler/rustc_mir_transform/src/dataflow_const_prop.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index d715e250ca4eb..f3ca2337e59ca 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -71,7 +71,7 @@ struct ConstAnalysis<'a, 'tcx> { } impl<'tcx> ConstAnalysis<'_, 'tcx> { - fn eval_disciminant( + fn eval_discriminant( &self, enum_ty: Ty<'tcx>, variant_index: VariantIdx, @@ -101,7 +101,7 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { state.flood_discr(place.as_ref(), &self.map); if self.map.find_discr(place.as_ref()).is_some() { let enum_ty = place.ty(self.local_decls, self.tcx).ty; - if let Some(discr) = self.eval_disciminant(enum_ty, variant_index) { + if let Some(discr) = self.eval_discriminant(enum_ty, variant_index) { state.assign_discr( place.as_ref(), ValueOrPlace::Value(FlatSet::Elem(discr)), @@ -152,7 +152,7 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> { && let Some(discr_idx) = self.map().apply(target_idx, TrackElem::Discriminant) { let enum_ty = target.ty(self.local_decls, self.tcx).ty; - if let Some(discr_val) = self.eval_disciminant(enum_ty, variant_index) { + if let Some(discr_val) = self.eval_discriminant(enum_ty, variant_index) { state.insert_value_idx(discr_idx, FlatSet::Elem(discr_val), &self.map); } } From 43a5cc383d9cf96825561b646e5f4686ae45c6c1 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 8 Dec 2022 12:59:02 +0000 Subject: [PATCH 13/17] Separate the lifetime of the session and the arena in the resolver --- compiler/rustc_interface/src/passes.rs | 14 ++++++--- .../rustc_resolve/src/build_reduced_graph.rs | 14 ++++----- compiler/rustc_resolve/src/check_unused.rs | 10 +++---- compiler/rustc_resolve/src/def_collector.rs | 10 +++---- compiler/rustc_resolve/src/diagnostics.rs | 4 +-- .../src/effective_visibilities.rs | 19 +++++++----- compiler/rustc_resolve/src/ident.rs | 2 +- compiler/rustc_resolve/src/imports.rs | 10 +++---- compiler/rustc_resolve/src/late.rs | 28 ++++++++--------- .../rustc_resolve/src/late/diagnostics.rs | 4 +-- compiler/rustc_resolve/src/lib.rs | 30 +++++++++---------- compiler/rustc_resolve/src/macros.rs | 4 +-- 12 files changed, 79 insertions(+), 70 deletions(-) diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index c8d8afae39e0a..e85cc7ad36de6 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -82,7 +82,7 @@ mod boxed_resolver { struct BoxedResolverInner { session: Lrc, resolver_arenas: Option>, - resolver: Option>, + resolver: Option>, _pin: PhantomPinned, } @@ -98,7 +98,10 @@ mod boxed_resolver { impl BoxedResolver { pub(super) fn new( session: Lrc, - make_resolver: impl for<'a> FnOnce(&'a Session, &'a ResolverArenas<'a>) -> Resolver<'a>, + make_resolver: impl for<'a, 'tcx> FnOnce( + &'tcx Session, + &'a ResolverArenas<'a>, + ) -> Resolver<'a, 'tcx>, ) -> BoxedResolver { let mut boxed_resolver = Box::new(BoxedResolverInner { session, @@ -121,7 +124,10 @@ mod boxed_resolver { } } - pub fn access FnOnce(&mut Resolver<'a>) -> R, R>(&mut self, f: F) -> R { + pub fn access FnOnce(&mut Resolver<'a, 'tcx>) -> R, R>( + &mut self, + f: F, + ) -> R { // SAFETY: The resolver doesn't need to be pinned. let mut resolver = unsafe { self.0.as_mut().map_unchecked_mut(|boxed_resolver| &mut boxed_resolver.resolver) @@ -256,7 +262,7 @@ pub fn configure_and_expand( lint_store: &LintStore, mut krate: ast::Crate, crate_name: Symbol, - resolver: &mut Resolver<'_>, + resolver: &mut Resolver<'_, '_>, ) -> Result { trace!("configure_and_expand"); pre_expansion_lint(sess, lint_store, resolver.registered_tools(), &krate, crate_name); diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 44f3bf1be055d..6d3518d53f779 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -65,7 +65,7 @@ impl<'a, Id: Into> ToNameBinding<'a> for (Res, ty::Visibility, Span, } } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined; /// otherwise, reports an error. pub(crate) fn define(&mut self, parent: Module<'a>, ident: Ident, ns: Namespace, def: T) @@ -214,18 +214,18 @@ impl<'a> Resolver<'a> { } } -struct BuildReducedGraphVisitor<'a, 'b> { - r: &'b mut Resolver<'a>, +struct BuildReducedGraphVisitor<'a, 'b, 'tcx> { + r: &'b mut Resolver<'a, 'tcx>, parent_scope: ParentScope<'a>, } -impl<'a> AsMut> for BuildReducedGraphVisitor<'a, '_> { - fn as_mut(&mut self) -> &mut Resolver<'a> { +impl<'a, 'tcx> AsMut> for BuildReducedGraphVisitor<'a, '_, 'tcx> { + fn as_mut(&mut self) -> &mut Resolver<'a, 'tcx> { self.r } } -impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { +impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> { fn resolve_visibility(&mut self, vis: &ast::Visibility) -> ty::Visibility { self.try_resolve_visibility(vis, true).unwrap_or_else(|err| { self.r.report_vis_error(err); @@ -1315,7 +1315,7 @@ macro_rules! method { }; } -impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> { +impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> { method!(visit_expr: ast::Expr, ast::ExprKind::MacCall, walk_expr); method!(visit_pat: ast::Pat, ast::PatKind::MacCall, walk_pat); method!(visit_ty: ast::Ty, ast::TyKind::MacCall, walk_ty); diff --git a/compiler/rustc_resolve/src/check_unused.rs b/compiler/rustc_resolve/src/check_unused.rs index eae4c9992eb08..294fd0a736f37 100644 --- a/compiler/rustc_resolve/src/check_unused.rs +++ b/compiler/rustc_resolve/src/check_unused.rs @@ -49,8 +49,8 @@ impl<'a> UnusedImport<'a> { } } -struct UnusedImportCheckVisitor<'a, 'b> { - r: &'a mut Resolver<'b>, +struct UnusedImportCheckVisitor<'a, 'b, 'tcx> { + r: &'a mut Resolver<'b, 'tcx>, /// All the (so far) unused imports, grouped path list unused_imports: FxIndexMap>, base_use_tree: Option<&'a ast::UseTree>, @@ -58,7 +58,7 @@ struct UnusedImportCheckVisitor<'a, 'b> { item_span: Span, } -impl<'a, 'b> UnusedImportCheckVisitor<'a, 'b> { +impl<'a, 'b, 'tcx> UnusedImportCheckVisitor<'a, 'b, 'tcx> { // We have information about whether `use` (import) items are actually // used now. If an import is not used at all, we signal a lint error. fn check_import(&mut self, id: ast::NodeId) { @@ -94,7 +94,7 @@ impl<'a, 'b> UnusedImportCheckVisitor<'a, 'b> { } } -impl<'a, 'b> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b> { +impl<'a, 'b, 'tcx> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b, 'tcx> { fn visit_item(&mut self, item: &'a ast::Item) { self.item_span = item.span_with_attributes(); @@ -222,7 +222,7 @@ fn calc_unused_spans( } } -impl Resolver<'_> { +impl Resolver<'_, '_> { pub(crate) fn check_unused(&mut self, krate: &ast::Crate) { for import in self.potentially_unused_imports.iter() { match import.kind { diff --git a/compiler/rustc_resolve/src/def_collector.rs b/compiler/rustc_resolve/src/def_collector.rs index 2764a6c28a5e9..e7ff236f84616 100644 --- a/compiler/rustc_resolve/src/def_collector.rs +++ b/compiler/rustc_resolve/src/def_collector.rs @@ -9,7 +9,7 @@ use rustc_span::symbol::sym; use rustc_span::Span; pub(crate) fn collect_definitions( - resolver: &mut Resolver<'_>, + resolver: &mut Resolver<'_, '_>, fragment: &AstFragment, expansion: LocalExpnId, ) { @@ -18,14 +18,14 @@ pub(crate) fn collect_definitions( } /// Creates `DefId`s for nodes in the AST. -struct DefCollector<'a, 'b> { - resolver: &'a mut Resolver<'b>, +struct DefCollector<'a, 'b, 'tcx> { + resolver: &'a mut Resolver<'b, 'tcx>, parent_def: LocalDefId, impl_trait_context: ImplTraitContext, expansion: LocalExpnId, } -impl<'a, 'b> DefCollector<'a, 'b> { +impl<'a, 'b, 'tcx> DefCollector<'a, 'b, 'tcx> { fn create_def(&mut self, node_id: NodeId, data: DefPathData, span: Span) -> LocalDefId { let parent_def = self.parent_def; debug!("create_def(node_id={:?}, data={:?}, parent_def={:?})", node_id, data, parent_def); @@ -81,7 +81,7 @@ impl<'a, 'b> DefCollector<'a, 'b> { } } -impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> { +impl<'a, 'b, 'tcx> visit::Visitor<'a> for DefCollector<'a, 'b, 'tcx> { fn visit_item(&mut self, i: &'a Item) { debug!("visit_item: {:?}", i); diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index a08ae0f184bb2..934d60589d4e2 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -114,7 +114,7 @@ fn reduce_impl_span_to_impl_keyword(sm: &SourceMap, impl_span: Span) -> Span { sm.span_until_whitespace(impl_span) } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { pub(crate) fn report_errors(&mut self, krate: &Crate) { self.report_with_use_injections(krate); @@ -1883,7 +1883,7 @@ impl<'a> Resolver<'a> { } } -impl<'a, 'b> ImportResolver<'a, 'b> { +impl<'a, 'b, 'tcx> ImportResolver<'a, 'b, 'tcx> { /// Adds suggestions for a path that cannot be resolved. pub(crate) fn make_path_suggestion( &mut self, diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index ab68f25a886fe..0079c3e526d62 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -29,8 +29,8 @@ impl ParentId<'_> { } } -pub(crate) struct EffectiveVisibilitiesVisitor<'r, 'a> { - r: &'r mut Resolver<'a>, +pub(crate) struct EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> { + r: &'r mut Resolver<'a, 'tcx>, def_effective_visibilities: EffectiveVisibilities, /// While walking import chains we need to track effective visibilities per-binding, and def id /// keys in `Resolver::effective_visibilities` are not enough for that, because multiple @@ -41,7 +41,7 @@ pub(crate) struct EffectiveVisibilitiesVisitor<'r, 'a> { changed: bool, } -impl Resolver<'_> { +impl Resolver<'_, '_> { fn nearest_normal_mod(&mut self, def_id: LocalDefId) -> LocalDefId { self.get_nearest_non_block_module(def_id.to_def_id()).nearest_parent_mod().expect_local() } @@ -67,18 +67,21 @@ impl Resolver<'_> { } } -impl<'a, 'b> IntoDefIdTree for &'b mut Resolver<'a> { - type Tree = &'b Resolver<'a>; +impl<'a, 'b, 'tcx> IntoDefIdTree for &'b mut Resolver<'a, 'tcx> { + type Tree = &'b Resolver<'a, 'tcx>; fn tree(self) -> Self::Tree { self } } -impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { +impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> { /// Fills the `Resolver::effective_visibilities` table with public & exported items /// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we /// need access to a TyCtxt for that. - pub(crate) fn compute_effective_visibilities<'c>(r: &'r mut Resolver<'a>, krate: &'c Crate) { + pub(crate) fn compute_effective_visibilities<'c>( + r: &'r mut Resolver<'a, 'tcx>, + krate: &'c Crate, + ) { let mut visitor = EffectiveVisibilitiesVisitor { r, def_effective_visibilities: Default::default(), @@ -192,7 +195,7 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } } -impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> { +impl<'r, 'ast, 'tcx> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r, 'tcx> { fn visit_item(&mut self, item: &'ast ast::Item) { let def_id = self.r.local_def_id(item.id); // Update effective visibilities of nested items. diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index d03ccf256fad7..61a48b109b22a 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -28,7 +28,7 @@ use RibKind::*; type Visibility = ty::Visibility; -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { /// A generic scope visitor. /// Visits scopes in order to resolve some identifier in them or perform other actions. /// If the callback returns `Some` result, we stop visiting scopes and return it. diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 8d1c789dea7c5..da3e5095e531d 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -225,7 +225,7 @@ fn pub_use_of_private_extern_crate_hack(import: &Import<'_>, binding: &NameBindi } } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { /// Given a binding and an import that resolves to it, /// return the corresponding binding defined by the import. pub(crate) fn import( @@ -333,7 +333,7 @@ impl<'a> Resolver<'a> { // If the resolution becomes a success, define it in the module's glob importers. fn update_resolution(&mut self, module: Module<'a>, key: BindingKey, f: F) -> T where - F: FnOnce(&mut Resolver<'a>, &mut NameResolution<'a>) -> T, + F: FnOnce(&mut Resolver<'a, 'tcx>, &mut NameResolution<'a>) -> T, { // Ensure that `resolution` isn't borrowed when defining in the module's glob importers, // during which the resolution might end up getting re-defined via a glob cycle. @@ -405,11 +405,11 @@ struct UnresolvedImportError { candidates: Option>, } -pub(crate) struct ImportResolver<'a, 'b> { - pub r: &'a mut Resolver<'b>, +pub(crate) struct ImportResolver<'a, 'b, 'tcx> { + pub r: &'a mut Resolver<'b, 'tcx>, } -impl<'a, 'b> ImportResolver<'a, 'b> { +impl<'a, 'b, 'tcx> ImportResolver<'a, 'b, 'tcx> { // Import resolution // // This is a fixed-point algorithm. We resolve imports until our efforts diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index d4c056f12f62b..d3bcbbabf55e5 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -505,7 +505,7 @@ enum MaybeExported<'a> { } impl MaybeExported<'_> { - fn eval(self, r: &Resolver<'_>) -> bool { + fn eval(self, r: &Resolver<'_, '_>) -> bool { let def_id = match self { MaybeExported::Ok(node_id) => Some(r.local_def_id(node_id)), MaybeExported::Impl(Some(trait_def_id)) | MaybeExported::ImplItem(Ok(trait_def_id)) => { @@ -584,8 +584,8 @@ struct DiagnosticMetadata<'ast> { current_elision_failures: Vec, } -struct LateResolutionVisitor<'a, 'b, 'ast> { - r: &'b mut Resolver<'a>, +struct LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { + r: &'b mut Resolver<'a, 'tcx>, /// The module that represents the current item scope. parent_scope: ParentScope<'a>, @@ -628,7 +628,7 @@ struct LateResolutionVisitor<'a, 'b, 'ast> { } /// Walks the whole crate in DFS order, visiting each item, resolving names as it goes. -impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> { +impl<'a: 'ast, 'ast, 'tcx> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast, 'tcx> { fn visit_attribute(&mut self, _: &'ast Attribute) { // We do not want to resolve expressions that appear in attributes, // as they do not correspond to actual code. @@ -1199,8 +1199,8 @@ impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> { } } -impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { - fn new(resolver: &'b mut Resolver<'a>) -> LateResolutionVisitor<'a, 'b, 'ast> { +impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { + fn new(resolver: &'b mut Resolver<'a, 'tcx>) -> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { // During late resolution we only track the module component of the parent scope, // although it may be useful to track other components as well for diagnostics. let graph_root = resolver.graph_root; @@ -2029,13 +2029,13 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { /// List all the lifetimes that appear in the provided type. fn find_lifetime_for_self(&self, ty: &'ast Ty) -> Set1 { - struct SelfVisitor<'r, 'a> { - r: &'r Resolver<'a>, + struct SelfVisitor<'r, 'a, 'tcx> { + r: &'r Resolver<'a, 'tcx>, impl_self: Option, lifetime: Set1, } - impl SelfVisitor<'_, '_> { + impl SelfVisitor<'_, '_, '_> { // Look for `self: &'a Self` - also desugared from `&'a self`, // and if that matches, use it for elision and return early. fn is_self_ty(&self, ty: &Ty) -> bool { @@ -2053,7 +2053,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { } } - impl<'a> Visitor<'a> for SelfVisitor<'_, '_> { + impl<'a> Visitor<'a> for SelfVisitor<'_, '_, '_> { fn visit_ty(&mut self, ty: &'a Ty) { trace!("SelfVisitor considering ty={:?}", ty); if let TyKind::Ref(lt, ref mt) = ty.kind && self.is_self_ty(&mt.ty) { @@ -4288,13 +4288,13 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> { } } -struct LifetimeCountVisitor<'a, 'b> { - r: &'b mut Resolver<'a>, +struct LifetimeCountVisitor<'a, 'b, 'tcx> { + r: &'b mut Resolver<'a, 'tcx>, } /// Walks the whole crate in DFS order, visiting each item, counting the declared number of /// lifetime generic parameters. -impl<'ast> Visitor<'ast> for LifetimeCountVisitor<'_, '_> { +impl<'ast> Visitor<'ast> for LifetimeCountVisitor<'_, '_, '_> { fn visit_item(&mut self, item: &'ast Item) { match &item.kind { ItemKind::TyAlias(box TyAlias { ref generics, .. }) @@ -4328,7 +4328,7 @@ impl<'ast> Visitor<'ast> for LifetimeCountVisitor<'_, '_> { } } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { pub(crate) fn late_resolve_crate(&mut self, krate: &Crate) { visit::walk_crate(&mut LifetimeCountVisitor { r: self }, krate); let mut late_resolution_visitor = LateResolutionVisitor::new(self); diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index c6d27ec69c512..a3195a6436689 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -166,7 +166,7 @@ impl TypoCandidate { } } -impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> { +impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> { fn def_span(&self, def_id: DefId) -> Option { match def_id.krate { LOCAL_CRATE => self.r.opt_span(def_id), @@ -318,7 +318,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> { span: Span, source: PathSource<'_>, res: Option, - ) -> (DiagnosticBuilder<'a, ErrorGuaranteed>, Vec) { + ) -> (DiagnosticBuilder<'tcx, ErrorGuaranteed>, Vec) { debug!(?res, ?source); let base_error = self.make_base_error(path, span, source, res); let code = source.error_code(res.is_some()); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 3c70e9c93e364..3db3b76fc26a2 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -147,7 +147,7 @@ struct ParentScope<'a> { impl<'a> ParentScope<'a> { /// Creates a parent scope with the passed argument used as the module scope component, /// and other scope components set to default empty values. - fn module(module: Module<'a>, resolver: &Resolver<'a>) -> ParentScope<'a> { + fn module(module: Module<'a>, resolver: &Resolver<'a, '_>) -> ParentScope<'a> { ParentScope { module, expansion: LocalExpnId::ROOT, @@ -528,9 +528,9 @@ impl<'a> ModuleData<'a> { } } - fn for_each_child(&'a self, resolver: &mut R, mut f: F) + fn for_each_child<'tcx, R, F>(&'a self, resolver: &mut R, mut f: F) where - R: AsMut>, + R: AsMut>, F: FnMut(&mut R, Ident, Namespace, &'a NameBinding<'a>), { for (key, name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() { @@ -541,9 +541,9 @@ impl<'a> ModuleData<'a> { } /// This modifies `self` in place. The traits will be stored in `self.traits`. - fn ensure_traits(&'a self, resolver: &mut R) + fn ensure_traits<'tcx, R>(&'a self, resolver: &mut R) where - R: AsMut>, + R: AsMut>, { let mut traits = self.traits.borrow_mut(); if traits.is_none() { @@ -864,8 +864,8 @@ struct MacroData { /// The main resolver class. /// /// This is the visitor that walks the whole crate. -pub struct Resolver<'a> { - session: &'a Session, +pub struct Resolver<'a, 'tcx> { + session: &'tcx Session, /// Item with a given `LocalDefId` was defined during macro expansion with ID `ExpnId`. expn_that_defined: FxHashMap, @@ -949,7 +949,7 @@ pub struct Resolver<'a> { /// Ambiguity errors are delayed for deduplication. ambiguity_errors: Vec>, /// `use` injections are delayed for better placement and deduplication. - use_injections: Vec>, + use_injections: Vec>, /// Crate-local macro expanded `macro_export` referred to by a module-relative path. macro_expanded_macro_export_errors: BTreeSet<(Span, Span)>, @@ -1111,8 +1111,8 @@ impl<'a> ResolverArenas<'a> { } } -impl<'a> AsMut> for Resolver<'a> { - fn as_mut(&mut self) -> &mut Resolver<'a> { +impl<'a, 'tcx> AsMut> for Resolver<'a, 'tcx> { + fn as_mut(&mut self) -> &mut Resolver<'a, 'tcx> { self } } @@ -1134,14 +1134,14 @@ impl DefIdTree for ResolverTree<'_> { } } -impl<'a, 'b> DefIdTree for &'a Resolver<'b> { +impl<'a, 'b, 'tcx> DefIdTree for &'a Resolver<'b, 'tcx> { #[inline] fn opt_parent(self, id: DefId) -> Option { ResolverTree(&self.untracked).opt_parent(id) } } -impl<'a> Resolver<'a> { +impl<'tcx> Resolver<'_, 'tcx> { fn opt_local_def_id(&self, node: NodeId) -> Option { self.node_id_to_def_id.get(&node).copied() } @@ -1200,14 +1200,14 @@ impl<'a> Resolver<'a> { } } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { pub fn new( - session: &'a Session, + session: &'tcx Session, krate: &Crate, crate_name: Symbol, metadata_loader: Box, arenas: &'a ResolverArenas<'a>, - ) -> Resolver<'a> { + ) -> Resolver<'a, 'tcx> { let root_def_id = CRATE_DEF_ID.to_def_id(); let mut module_map = FxHashMap::default(); let graph_root = arenas.new_module( diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 96ad6e96fac12..1c220a81792da 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -160,7 +160,7 @@ fn soft_custom_inner_attributes_gate(path: &ast::Path, invoc: &Invocation) -> bo false } -impl<'a> ResolverExpand for Resolver<'a> { +impl<'a, 'tcx> ResolverExpand for Resolver<'a, 'tcx> { fn next_node_id(&mut self) -> NodeId { self.next_node_id() } @@ -467,7 +467,7 @@ impl<'a> ResolverExpand for Resolver<'a> { } } -impl<'a> Resolver<'a> { +impl<'a, 'tcx> Resolver<'a, 'tcx> { /// Resolve macro path with error reporting and recovery. /// Uses dummy syntax extensions for unresolved macros or macros with unexpected resolutions /// for better error recovery. From 21f4c0723ed871472b050b6031b7c9f5e7e0970d Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 13 Dec 2022 09:46:37 +0000 Subject: [PATCH 14/17] Remove BoxedResolver --- compiler/rustc_interface/src/interface.rs | 1 - compiler/rustc_interface/src/passes.rs | 90 +---------------------- compiler/rustc_interface/src/queries.rs | 29 ++++---- 3 files changed, 18 insertions(+), 102 deletions(-) diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs index 7a5e45ada3f6a..d504aea77d0da 100644 --- a/compiler/rustc_interface/src/interface.rs +++ b/compiler/rustc_interface/src/interface.rs @@ -1,4 +1,3 @@ -pub use crate::passes::BoxedResolver; use crate::util; use rustc_ast::token; diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index e85cc7ad36de6..6a94d19001e11 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -23,9 +23,9 @@ use rustc_parse::{parse_crate_from_file, parse_crate_from_source_str, validate_a use rustc_passes::{self, hir_stats, layout_test}; use rustc_plugin_impl as plugin; use rustc_query_impl::{OnDiskCache, Queries as TcxQueries}; -use rustc_resolve::{Resolver, ResolverArenas}; +use rustc_resolve::Resolver; use rustc_session::config::{CrateType, Input, OutputFilenames, OutputType}; -use rustc_session::cstore::{CrateStoreDyn, MetadataLoader, MetadataLoaderDyn, Untracked}; +use rustc_session::cstore::{CrateStoreDyn, MetadataLoader, Untracked}; use rustc_session::output::filename_for_input; use rustc_session::search_paths::PathKind; use rustc_session::{Limit, Session}; @@ -37,9 +37,7 @@ use rustc_trait_selection::traits; use std::any::Any; use std::ffi::OsString; use std::io::{self, BufWriter, Write}; -use std::marker::PhantomPinned; use std::path::{Path, PathBuf}; -use std::pin::Pin; use std::sync::{Arc, LazyLock}; use std::{env, fs, iter}; @@ -73,90 +71,6 @@ fn count_nodes(krate: &ast::Crate) -> usize { counter.count } -pub use boxed_resolver::BoxedResolver; -mod boxed_resolver { - use super::*; - - pub struct BoxedResolver(Pin>); - - struct BoxedResolverInner { - session: Lrc, - resolver_arenas: Option>, - resolver: Option>, - _pin: PhantomPinned, - } - - // Note: Drop order is important to prevent dangling references. Resolver must be dropped first, - // then resolver_arenas and session. - impl Drop for BoxedResolverInner { - fn drop(&mut self) { - self.resolver.take(); - self.resolver_arenas.take(); - } - } - - impl BoxedResolver { - pub(super) fn new( - session: Lrc, - make_resolver: impl for<'a, 'tcx> FnOnce( - &'tcx Session, - &'a ResolverArenas<'a>, - ) -> Resolver<'a, 'tcx>, - ) -> BoxedResolver { - let mut boxed_resolver = Box::new(BoxedResolverInner { - session, - resolver_arenas: Some(Resolver::arenas()), - resolver: None, - _pin: PhantomPinned, - }); - // SAFETY: `make_resolver` takes a resolver arena with an arbitrary lifetime and - // returns a resolver with the same lifetime as the arena. We ensure that the arena - // outlives the resolver in the drop impl and elsewhere so these transmutes are sound. - unsafe { - let resolver = make_resolver( - std::mem::transmute::<&Session, &Session>(&boxed_resolver.session), - std::mem::transmute::<&ResolverArenas<'_>, &ResolverArenas<'_>>( - boxed_resolver.resolver_arenas.as_ref().unwrap(), - ), - ); - boxed_resolver.resolver = Some(resolver); - BoxedResolver(Pin::new_unchecked(boxed_resolver)) - } - } - - pub fn access FnOnce(&mut Resolver<'a, 'tcx>) -> R, R>( - &mut self, - f: F, - ) -> R { - // SAFETY: The resolver doesn't need to be pinned. - let mut resolver = unsafe { - self.0.as_mut().map_unchecked_mut(|boxed_resolver| &mut boxed_resolver.resolver) - }; - f((&mut *resolver).as_mut().unwrap()) - } - - pub fn into_outputs(mut self) -> ty::ResolverOutputs { - // SAFETY: The resolver doesn't need to be pinned. - let mut resolver = unsafe { - self.0.as_mut().map_unchecked_mut(|boxed_resolver| &mut boxed_resolver.resolver) - }; - resolver.take().unwrap().into_outputs() - } - } -} - -pub fn create_resolver( - sess: Lrc, - metadata_loader: Box, - krate: &ast::Crate, - crate_name: Symbol, -) -> BoxedResolver { - trace!("create_resolver"); - BoxedResolver::new(sess, move |sess, resolver_arenas| { - Resolver::new(sess, krate, crate_name, metadata_loader, resolver_arenas) - }) -} - pub fn register_plugins<'a>( sess: &'a Session, metadata_loader: &'a dyn MetadataLoader, diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index 67886b6b9896e..752c1ec7538e7 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -1,6 +1,6 @@ use crate::errors::{FailedWritingFile, RustcErrorFatal, RustcErrorUnexpectedAnnotation}; use crate::interface::{Compiler, Result}; -use crate::passes::{self, BoxedResolver}; +use crate::passes; use rustc_ast as ast; use rustc_codegen_ssa::traits::CodegenBackend; @@ -13,8 +13,9 @@ use rustc_incremental::DepGraphFuture; use rustc_lint::LintStore; use rustc_middle::arena::Arena; use rustc_middle::dep_graph::DepGraph; -use rustc_middle::ty::{self, GlobalCtxt, TyCtxt}; +use rustc_middle::ty::{self, GlobalCtxt, ResolverOutputs, TyCtxt}; use rustc_query_impl::Queries as TcxQueries; +use rustc_resolve::Resolver; use rustc_session::config::{self, OutputFilenames, OutputType}; use rustc_session::{output::find_crate_name, Session}; use rustc_span::symbol::sym; @@ -87,7 +88,7 @@ pub struct Queries<'tcx> { parse: Query, crate_name: Query, register_plugins: Query<(ast::Crate, Lrc)>, - expansion: Query<(Lrc, BoxedResolver, Lrc)>, + expansion: Query<(Lrc, ResolverOutputs, Lrc)>, dep_graph: Query, // This just points to what's in `gcx_cell`. gcx: Query<&'tcx GlobalCtxt<'tcx>>, @@ -170,23 +171,25 @@ impl<'tcx> Queries<'tcx> { pub fn expansion( &self, - ) -> Result, BoxedResolver, Lrc)>> { + ) -> Result, ResolverOutputs, Lrc)>> { trace!("expansion"); self.expansion.compute(|| { let crate_name = *self.crate_name()?.borrow(); let (krate, lint_store) = self.register_plugins()?.steal(); let _timer = self.session().timer("configure_and_expand"); let sess = self.session(); - let mut resolver = passes::create_resolver( - sess.clone(), - self.codegen_backend().metadata_loader(), + + let arenas = Resolver::arenas(); + let mut resolver = Resolver::new( + sess, &krate, crate_name, + self.codegen_backend().metadata_loader(), + &arenas, ); - let krate = resolver.access(|resolver| { - passes::configure_and_expand(sess, &lint_store, krate, crate_name, resolver) - })?; - Ok((Lrc::new(krate), resolver, lint_store)) + let krate = + passes::configure_and_expand(sess, &lint_store, krate, crate_name, &mut resolver)?; + Ok((Lrc::new(krate), resolver.into_outputs(), lint_store)) }) } @@ -209,13 +212,13 @@ impl<'tcx> Queries<'tcx> { pub fn global_ctxt(&'tcx self) -> Result>> { self.gcx.compute(|| { let crate_name = *self.crate_name()?.borrow(); - let (krate, resolver, lint_store) = self.expansion()?.steal(); + let (krate, resolver_outputs, lint_store) = self.expansion()?.steal(); let ty::ResolverOutputs { untracked, global_ctxt: untracked_resolutions, ast_lowering: untracked_resolver_for_lowering, - } = resolver.into_outputs(); + } = resolver_outputs; let gcx = passes::create_global_ctxt( self.compiler, From d15663814b8ec3b60da9e1c733c90e478ebb3ed0 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 14 Feb 2023 09:38:49 +0000 Subject: [PATCH 15/17] Inline the expansion query --- compiler/rustc_interface/src/queries.rs | 51 +++++++++++-------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index 752c1ec7538e7..fcd399c0540e3 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -13,7 +13,7 @@ use rustc_incremental::DepGraphFuture; use rustc_lint::LintStore; use rustc_middle::arena::Arena; use rustc_middle::dep_graph::DepGraph; -use rustc_middle::ty::{self, GlobalCtxt, ResolverOutputs, TyCtxt}; +use rustc_middle::ty::{self, GlobalCtxt, TyCtxt}; use rustc_query_impl::Queries as TcxQueries; use rustc_resolve::Resolver; use rustc_session::config::{self, OutputFilenames, OutputType}; @@ -88,7 +88,6 @@ pub struct Queries<'tcx> { parse: Query, crate_name: Query, register_plugins: Query<(ast::Crate, Lrc)>, - expansion: Query<(Lrc, ResolverOutputs, Lrc)>, dep_graph: Query, // This just points to what's in `gcx_cell`. gcx: Query<&'tcx GlobalCtxt<'tcx>>, @@ -107,7 +106,6 @@ impl<'tcx> Queries<'tcx> { parse: Default::default(), crate_name: Default::default(), register_plugins: Default::default(), - expansion: Default::default(), dep_graph: Default::default(), gcx: Default::default(), ongoing_codegen: Default::default(), @@ -169,30 +167,6 @@ impl<'tcx> Queries<'tcx> { }) } - pub fn expansion( - &self, - ) -> Result, ResolverOutputs, Lrc)>> { - trace!("expansion"); - self.expansion.compute(|| { - let crate_name = *self.crate_name()?.borrow(); - let (krate, lint_store) = self.register_plugins()?.steal(); - let _timer = self.session().timer("configure_and_expand"); - let sess = self.session(); - - let arenas = Resolver::arenas(); - let mut resolver = Resolver::new( - sess, - &krate, - crate_name, - self.codegen_backend().metadata_loader(), - &arenas, - ); - let krate = - passes::configure_and_expand(sess, &lint_store, krate, crate_name, &mut resolver)?; - Ok((Lrc::new(krate), resolver.into_outputs(), lint_store)) - }) - } - fn dep_graph(&self) -> Result> { self.dep_graph.compute(|| { let sess = self.session(); @@ -212,7 +186,28 @@ impl<'tcx> Queries<'tcx> { pub fn global_ctxt(&'tcx self) -> Result>> { self.gcx.compute(|| { let crate_name = *self.crate_name()?.borrow(); - let (krate, resolver_outputs, lint_store) = self.expansion()?.steal(); + let (krate, resolver_outputs, lint_store) = { + let (krate, lint_store) = self.register_plugins()?.steal(); + let _timer = self.session().timer("configure_and_expand"); + let sess = self.session(); + + let arenas = Resolver::arenas(); + let mut resolver = Resolver::new( + sess, + &krate, + crate_name, + self.codegen_backend().metadata_loader(), + &arenas, + ); + let krate = passes::configure_and_expand( + sess, + &lint_store, + krate, + crate_name, + &mut resolver, + )?; + (Lrc::new(krate), resolver.into_outputs(), lint_store) + }; let ty::ResolverOutputs { untracked, From 241c6a4a616010382aefe297c69a26fb5a22dd06 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 14 Feb 2023 09:50:13 +0000 Subject: [PATCH 16/17] Simplify expansion logic --- compiler/rustc_interface/src/queries.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index fcd399c0540e3..d727efdafc278 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -186,8 +186,8 @@ impl<'tcx> Queries<'tcx> { pub fn global_ctxt(&'tcx self) -> Result>> { self.gcx.compute(|| { let crate_name = *self.crate_name()?.borrow(); - let (krate, resolver_outputs, lint_store) = { - let (krate, lint_store) = self.register_plugins()?.steal(); + let (krate, lint_store) = self.register_plugins()?.steal(); + let (krate, resolver_outputs) = { let _timer = self.session().timer("configure_and_expand"); let sess = self.session(); @@ -206,7 +206,7 @@ impl<'tcx> Queries<'tcx> { crate_name, &mut resolver, )?; - (Lrc::new(krate), resolver.into_outputs(), lint_store) + (Lrc::new(krate), resolver.into_outputs()) }; let ty::ResolverOutputs { From dfd0afb991d4bfbd6fdf86adb631e428c185ac89 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Tue, 14 Feb 2023 19:32:32 +0000 Subject: [PATCH 17/17] Revert to using `RtlGenRandom` This is required due to `BCryptGenRandom` failing to load the necessary dll on some systems. --- library/std/src/sys/windows/c.rs | 13 +-- library/std/src/sys/windows/rand.rs | 121 +++++++--------------------- 2 files changed, 31 insertions(+), 103 deletions(-) diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index 81461de4f721f..f58dcf1287bef 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -295,8 +295,6 @@ pub fn nt_success(status: NTSTATUS) -> bool { status >= 0 } -// "RNG\0" -pub const BCRYPT_RNG_ALGORITHM: &[u16] = &[b'R' as u16, b'N' as u16, b'G' as u16, 0]; pub const BCRYPT_USE_SYSTEM_PREFERRED_RNG: DWORD = 0x00000002; #[repr(C)] @@ -834,6 +832,10 @@ if #[cfg(not(target_vendor = "uwp"))] { #[link(name = "advapi32")] extern "system" { + // Forbidden when targeting UWP + #[link_name = "SystemFunction036"] + pub fn RtlGenRandom(RandomBuffer: *mut u8, RandomBufferLength: ULONG) -> BOOLEAN; + // Allowed but unused by UWP pub fn OpenProcessToken( ProcessHandle: HANDLE, @@ -1258,13 +1260,6 @@ extern "system" { cbBuffer: ULONG, dwFlags: ULONG, ) -> NTSTATUS; - pub fn BCryptOpenAlgorithmProvider( - phalgorithm: *mut BCRYPT_ALG_HANDLE, - pszAlgId: LPCWSTR, - pszimplementation: LPCWSTR, - dwflags: ULONG, - ) -> NTSTATUS; - pub fn BCryptCloseAlgorithmProvider(hAlgorithm: BCRYPT_ALG_HANDLE, dwFlags: ULONG) -> NTSTATUS; } // Functions that aren't available on every version of Windows that we support, diff --git a/library/std/src/sys/windows/rand.rs b/library/std/src/sys/windows/rand.rs index b5a49489d3fb8..cdf37cfe9117b 100644 --- a/library/std/src/sys/windows/rand.rs +++ b/library/std/src/sys/windows/rand.rs @@ -1,106 +1,39 @@ -//! # Random key generation -//! -//! This module wraps the RNG provided by the OS. There are a few different -//! ways to interface with the OS RNG so it's worth exploring each of the options. -//! Note that at the time of writing these all go through the (undocumented) -//! `bcryptPrimitives.dll` but they use different route to get there. -//! -//! Originally we were using [`RtlGenRandom`], however that function is -//! deprecated and warns it "may be altered or unavailable in subsequent versions". -//! -//! So we switched to [`BCryptGenRandom`] with the `BCRYPT_USE_SYSTEM_PREFERRED_RNG` -//! flag to query and find the system configured RNG. However, this change caused a small -//! but significant number of users to experience panics caused by a failure of -//! this function. See [#94098]. -//! -//! The current version falls back to using `BCryptOpenAlgorithmProvider` if -//! `BCRYPT_USE_SYSTEM_PREFERRED_RNG` fails for any reason. -//! -//! [#94098]: https://github.com/rust-lang/rust/issues/94098 -//! [`RtlGenRandom`]: https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom -//! [`BCryptGenRandom`]: https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom +use crate::io; use crate::mem; use crate::ptr; use crate::sys::c; -/// Generates high quality secure random keys for use by [`HashMap`]. -/// -/// This is used to seed the default [`RandomState`]. -/// -/// [`HashMap`]: crate::collections::HashMap -/// [`RandomState`]: crate::collections::hash_map::RandomState pub fn hashmap_random_keys() -> (u64, u64) { - Rng::SYSTEM.gen_random_keys().unwrap_or_else(fallback_rng) + let mut v = (0, 0); + let ret = unsafe { + c::BCryptGenRandom( + ptr::null_mut(), + &mut v as *mut _ as *mut u8, + mem::size_of_val(&v) as c::ULONG, + c::BCRYPT_USE_SYSTEM_PREFERRED_RNG, + ) + }; + if c::nt_success(ret) { v } else { fallback_rng() } } -struct Rng { - algorithm: c::BCRYPT_ALG_HANDLE, - flags: u32, -} -impl Rng { - const SYSTEM: Self = unsafe { Self::new(ptr::null_mut(), c::BCRYPT_USE_SYSTEM_PREFERRED_RNG) }; - - /// Create the RNG from an existing algorithm handle. - /// - /// # Safety - /// - /// The handle must either be null or a valid algorithm handle. - const unsafe fn new(algorithm: c::BCRYPT_ALG_HANDLE, flags: u32) -> Self { - Self { algorithm, flags } - } - - /// Open a handle to the RNG algorithm. - fn open() -> Result { - use crate::sync::atomic::AtomicPtr; - use crate::sync::atomic::Ordering::{Acquire, Release}; - - // An atomic is used so we don't need to reopen the handle every time. - static HANDLE: AtomicPtr = AtomicPtr::new(ptr::null_mut()); - - let mut handle = HANDLE.load(Acquire); - if handle.is_null() { - let status = unsafe { - c::BCryptOpenAlgorithmProvider( - &mut handle, - c::BCRYPT_RNG_ALGORITHM.as_ptr(), - ptr::null(), - 0, - ) - }; - if c::nt_success(status) { - // If another thread opens a handle first then use that handle instead. - let result = HANDLE.compare_exchange(ptr::null_mut(), handle, Release, Acquire); - if let Err(previous_handle) = result { - // Close our handle and return the previous one. - unsafe { c::BCryptCloseAlgorithmProvider(handle, 0) }; - handle = previous_handle; - } - Ok(unsafe { Self::new(handle, 0) }) - } else { - Err(status) - } - } else { - Ok(unsafe { Self::new(handle, 0) }) - } - } +/// Generate random numbers using the fallback RNG function (RtlGenRandom) +/// +/// This is necessary because of a failure to load the SysWOW64 variant of the +/// bcryptprimitives.dll library from code that lives in bcrypt.dll +/// See +#[cfg(not(target_vendor = "uwp"))] +#[inline(never)] +fn fallback_rng() -> (u64, u64) { + let mut v = (0, 0); + let ret = + unsafe { c::RtlGenRandom(&mut v as *mut _ as *mut u8, mem::size_of_val(&v) as c::ULONG) }; - fn gen_random_keys(self) -> Result<(u64, u64), c::NTSTATUS> { - let mut v = (0, 0); - let status = unsafe { - let size = mem::size_of_val(&v).try_into().unwrap(); - c::BCryptGenRandom(self.algorithm, ptr::addr_of_mut!(v).cast(), size, self.flags) - }; - if c::nt_success(status) { Ok(v) } else { Err(status) } - } + if ret != 0 { v } else { panic!("fallback RNG broken: {}", io::Error::last_os_error()) } } -/// Generate random numbers using the fallback RNG function +/// We can't use RtlGenRandom with UWP, so there is no fallback +#[cfg(target_vendor = "uwp")] #[inline(never)] -fn fallback_rng(rng_status: c::NTSTATUS) -> (u64, u64) { - match Rng::open().and_then(|rng| rng.gen_random_keys()) { - Ok(keys) => keys, - Err(status) => { - panic!("RNG broken: {rng_status:#x}, fallback RNG broken: {status:#x}") - } - } +fn fallback_rng() -> (u64, u64) { + panic!("fallback RNG broken: RtlGenRandom() not supported on UWP"); }