Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dataflow-based const validation #64470

Merged
merged 31 commits into from
Sep 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
717c64e
Silence "skipping const checks" if outside a const context
ecstatic-morse Sep 17, 2019
457c3aa
Add additional `const` tests
ecstatic-morse Sep 17, 2019
e81297d
Add analysis to determine if a local is indirectly mutable
ecstatic-morse Sep 17, 2019
83a3e04
Don't treat locals as mutably borrowed after they're dropped
ecstatic-morse Sep 17, 2019
eec93ca
Copy `Qualif` to start work on new const validator
ecstatic-morse Sep 17, 2019
3a5442a
Remove unnecessary method
ecstatic-morse Sep 17, 2019
c2e121d
Make new qualifs public
ecstatic-morse Sep 17, 2019
48d3843
Add requisite imports and bitset to hold qualifs
ecstatic-morse Sep 17, 2019
3758e38
Remove reference to `Mode::NonConstFn` in qualifs
ecstatic-morse Sep 17, 2019
908dcb8
Control whether a `Qualif` is cleared on move
ecstatic-morse Sep 17, 2019
3698d04
Pass current qualification state in a separate parameter
ecstatic-morse Sep 17, 2019
fc92d3b
Add dataflow-based const validation
ecstatic-morse Sep 17, 2019
c990243
Run new validator in compare mode
ecstatic-morse Sep 17, 2019
670c84d
Fix tests broken by more consistent miri unleashed warnings
ecstatic-morse Sep 17, 2019
27bd849
Correct list of miri-supported operations
ecstatic-morse Sep 20, 2019
f2ff425
Add rationale for `suppress_errors` flag
ecstatic-morse Sep 20, 2019
e296436
Remember to replace ICE with some form of warning
ecstatic-morse Sep 20, 2019
b3e59bb
Move non-const ops into their own module
ecstatic-morse Sep 20, 2019
93ee779
Explain why `visit_terminator` does nothing for `IndirectlyMutableLoc…
ecstatic-morse Sep 20, 2019
bc7928a
Trigger ICE on nightly if validators disagree
ecstatic-morse Sep 24, 2019
406ac2e
Give usage instructions `IndirectlyMutableLocals` docs
ecstatic-morse Sep 25, 2019
2f5ea63
Return a `bool` from `in_any_value_of_ty`
ecstatic-morse Sep 25, 2019
dcecefc
Use conservative, type-based qualifcation for statics
ecstatic-morse Sep 25, 2019
1a14d17
Require `fmt::Debug` to implement `NonConstOp`
ecstatic-morse Sep 25, 2019
713ec15
Share `IndirectlyMutableLocals` results via reference
ecstatic-morse Sep 25, 2019
ff6faab
Add description for every module in `check_consts`
ecstatic-morse Sep 25, 2019
a302055
Mask results from flow-sensitive resolver with `in_any_value_of_ty`
ecstatic-morse Sep 25, 2019
8bfe82b
Correct `IndirectlyMutableLocals` docs
ecstatic-morse Sep 25, 2019
f2e7faf
Revert "Use conservative, type-based qualifcation for statics"
ecstatic-morse Sep 25, 2019
ff4158a
Bless miri unleashed output
ecstatic-morse Sep 25, 2019
0bf1a80
Rename `sty` to `kind`
ecstatic-morse Sep 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1359,6 +1359,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"describes how to render the `rendered` field of json diagnostics"),
unleash_the_miri_inside_of_you: bool = (false, parse_bool, [TRACKED],
"take the breaks off const evaluation. NOTE: this is unsound"),
suppress_const_validation_back_compat_ice: bool = (false, parse_bool, [TRACKED],
"silence ICE triggered when the new const validator disagrees with the old"),
osx_rpath_install_name: bool = (false, parse_bool, [TRACKED],
"pass `-install_name @rpath/...` to the macOS linker"),
sanitizer: Option<Sanitizer> = (None, parse_sanitizer, [TRACKED],
Expand Down
148 changes: 148 additions & 0 deletions src/librustc_mir/dataflow/impls/indirect_mutation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
use rustc::mir::visit::Visitor;
use rustc::mir::{self, Local, Location};
use rustc::ty::{self, TyCtxt};
use rustc_data_structures::bit_set::BitSet;
use syntax_pos::DUMMY_SP;

use crate::dataflow::{self, GenKillSet};

/// Whether a borrow to a `Local` has been created that could allow that `Local` to be mutated
/// indirectly. This could either be a mutable reference (`&mut`) or a shared borrow if the type of
/// that `Local` allows interior mutability. Operations that can mutate local's indirectly include:
/// assignments through a pointer (`*p = 42`), function calls, drop terminators and inline assembly.
///
/// If this returns false for a `Local` at a given statement (or terminator), that `Local` could
/// not possibly have been mutated indirectly prior to that statement.
#[derive(Copy, Clone)]
pub struct IndirectlyMutableLocals<'mir, 'tcx> {
body: &'mir mir::Body<'tcx>,
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
}

impl<'mir, 'tcx> IndirectlyMutableLocals<'mir, 'tcx> {
pub fn new(
tcx: TyCtxt<'tcx>,
body: &'mir mir::Body<'tcx>,
param_env: ty::ParamEnv<'tcx>,
) -> Self {
IndirectlyMutableLocals { body, tcx, param_env }
}

fn transfer_function<'a>(
&self,
trans: &'a mut GenKillSet<Local>,
) -> TransferFunction<'a, 'mir, 'tcx> {
TransferFunction {
body: self.body,
tcx: self.tcx,
param_env: self.param_env,
trans
}
}
}

impl<'mir, 'tcx> dataflow::BitDenotation<'tcx> for IndirectlyMutableLocals<'mir, 'tcx> {
type Idx = Local;

fn name() -> &'static str { "mut_borrowed_locals" }

fn bits_per_block(&self) -> usize {
self.body.local_decls.len()
}

fn start_block_effect(&self, _entry_set: &mut BitSet<Local>) {
// Nothing is borrowed on function entry
}

fn statement_effect(
&self,
trans: &mut GenKillSet<Local>,
loc: Location,
) {
let stmt = &self.body[loc.block].statements[loc.statement_index];
self.transfer_function(trans).visit_statement(stmt, loc);
}

fn terminator_effect(
&self,
trans: &mut GenKillSet<Local>,
loc: Location,
) {
let terminator = self.body[loc.block].terminator();
self.transfer_function(trans).visit_terminator(terminator, loc);
}

fn propagate_call_return(
&self,
_in_out: &mut BitSet<Local>,
_call_bb: mir::BasicBlock,
_dest_bb: mir::BasicBlock,
_dest_place: &mir::Place<'tcx>,
) {
// Nothing to do when a call returns successfully
}
}

impl<'mir, 'tcx> dataflow::BottomValue for IndirectlyMutableLocals<'mir, 'tcx> {
// bottom = unborrowed
const BOTTOM_VALUE: bool = false;
}

/// A `Visitor` that defines the transfer function for `IndirectlyMutableLocals`.
struct TransferFunction<'a, 'mir, 'tcx> {
trans: &'a mut GenKillSet<Local>,
body: &'mir mir::Body<'tcx>,
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
}

impl<'tcx> Visitor<'tcx> for TransferFunction<'_, '_, 'tcx> {
fn visit_rvalue(
&mut self,
rvalue: &mir::Rvalue<'tcx>,
location: Location,
) {
if let mir::Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue {
let is_mut = match kind {
mir::BorrowKind::Mut { .. } => true,

| mir::BorrowKind::Shared
| mir::BorrowKind::Shallow
| mir::BorrowKind::Unique
=> {
!borrowed_place
.ty(self.body, self.tcx)
.ty
.is_freeze(self.tcx, self.param_env, DUMMY_SP)
}
};

if is_mut {
match borrowed_place.base {
mir::PlaceBase::Local(borrowed_local) if !borrowed_place.is_indirect()
=> self.trans.gen(borrowed_local),

_ => (),
}
}
}

self.super_rvalue(rvalue, location);
}


fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) {
// This method purposely does nothing except call `super_terminator`. It exists solely to
// document the subtleties around drop terminators.

self.super_terminator(terminator, location);

if let mir::TerminatorKind::Drop { location: _, .. }
| mir::TerminatorKind::DropAndReplace { location: _, .. } = &terminator.kind
{
// Although drop terminators mutably borrow the location being dropped, that borrow
// cannot live beyond the drop terminator because the dropped location is invalidated.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
8 changes: 4 additions & 4 deletions src/librustc_mir/dataflow/impls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ use super::drop_flag_effects_for_function_entry;
use super::drop_flag_effects_for_location;
use super::on_lookup_result_bits;

mod storage_liveness;

pub use self::storage_liveness::*;

mod borrowed_locals;
mod indirect_mutation;
mod storage_liveness;

pub use self::borrowed_locals::*;
pub use self::indirect_mutation::IndirectlyMutableLocals;
pub use self::storage_liveness::*;

pub(super) mod borrows;

Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/dataflow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub use self::impls::DefinitelyInitializedPlaces;
pub use self::impls::EverInitializedPlaces;
pub use self::impls::borrows::Borrows;
pub use self::impls::HaveBeenBorrowedLocals;
pub use self::impls::IndirectlyMutableLocals;
pub use self::at_location::{FlowAtLocation, FlowsAtLocation};
pub(crate) use self::drop_flag_effects::*;

Expand Down
52 changes: 52 additions & 0 deletions src/librustc_mir/transform/check_consts/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//! Check the bodies of `const`s, `static`s and `const fn`s for illegal operations.
//!
//! This module will eventually replace the parts of `qualify_consts.rs` that check whether a local
//! has interior mutability or needs to be dropped, as well as the visitor that emits errors when
//! it finds operations that are invalid in a certain context.

use rustc::hir::def_id::DefId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving the module (and submodules) some docs (explaining the overall structure of it, the purpose, etc.) would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I'll work on this when we get closer to the end and things are more stable.

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a short description in 60ea470aefdd9a3234c83803d3a06cd49559920a.

use rustc::mir;
use rustc::ty::{self, TyCtxt};

pub use self::qualifs::Qualif;

pub mod ops;
mod qualifs;
mod resolver;
pub mod validation;

/// Information about the item currently being validated, as well as a reference to the global
/// context.
pub struct Item<'mir, 'tcx> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this isn't the best name? But I don't have any suggestions right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the equivalent data structure in the old code was ConstCx, although it was also used for doing promotion in NonConstFns. I like Item better, but I agree it's still pretty vague.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ItemValidationInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type appears in a lot of function signatures (even more once this alias disappears), so brevity is desirable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit shorter CheckCtx?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to others, but I'm not enthusiastic about any of the alternatives. Writing item.body and item.tcx are clearer to me than cx.body and cx.tcx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icx: ItemCtxt? Just like we have fcx: FnCtxt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that spirit, maybe ccx: ConstCtxt cause it signifies the analysis rather than the item.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but fcx signifies a function context, so icx being an item context seems fine 😆

I don't have an opinion either way, just wanted to say why it would be consistent.

Copy link
Contributor

@Centril Centril Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wanted to say why it would be consistent.

👍 -- you are right, I think I was considering LoweringContext. :)

body: &'mir mir::Body<'tcx>,
tcx: TyCtxt<'tcx>,
def_id: DefId,
param_env: ty::ParamEnv<'tcx>,
mode: validation::Mode,
}

impl Item<'mir, 'tcx> {
pub fn new(
tcx: TyCtxt<'tcx>,
def_id: DefId,
body: &'mir mir::Body<'tcx>,
) -> Self {
let param_env = tcx.param_env(def_id);
let mode = validation::Mode::for_item(tcx, def_id)
.expect("const validation must only be run inside a const context");

Item {
body,
tcx,
def_id,
param_env,
mode,
}
}
}


fn is_lang_panic_fn(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool {
Some(def_id) == tcx.lang_items().panic_fn() ||
Some(def_id) == tcx.lang_items().begin_panic_fn()
}
Loading