Skip to content

Commit

Permalink
[compiler v2] Fix equalities, FreezeRef conversion (#12162)
Browse files Browse the repository at this point in the history 
* [compiler-v2] Refactoring of ability instrumentation and checking

This refactors instrumentation of copy and drop instructions, and checking of abilities in general, closing #11223, which lead us to generate unnecessary copies. Copy instrumentation is now moved after reference safety as borrow information is needed to compute it. The existing ability checking and explicit drop processors have been marged into a new `ability_processor` which deals with instrumenting copies and drops as well as checking ability conformance.

* Addresssing reviewer comments

* Addressing reviewer comments

* Fixing #11924, addressing reviewer comments.

* [compiler v2] Fix equalities, FreezeRef conversion

This enables the comparison of references with mixed mutability (`&x == &mut y`), and introduces generation of the `FreezeRef` operation when an argument is widned from mutable to immutable references. The later applies for any kind of function call, not only equalities. Without freeze, certain scenarios produce borrow errors in reference safety and/or the bytecode verifier.

Note that this PR is intended to work together with the new reference safety analysis, so some tests do not yet work without it.

Fixes #12151
Fixes #11738
Fixes #11434

* Rebasing

* Adapting freeze to new analysis, fixing a bug in reference analysis

* Removing left-over move inference from reference analysis
  • Loading branch information
@wrwg
wrwg authored Feb 27, 2024
1 parent 8003356 commit 997d925
Show file tree
Hide file tree
Showing 38 changed files with 534 additions and 809 deletions.
101 changes: 58 additions & 43 deletions third_party/move/move-compiler-v2/src/pipeline/reference_safety_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,6 @@ pub struct LifetimeState {
temp_to_label_map: BTreeMap<TempIndex, LifetimeLabel>,
/// A map from globals to labels. Represents root states of the active graph.
global_to_label_map: BTreeMap<QualifiedInstId<StructId>, LifetimeLabel>,
/// Contains the set of variables whose values may have been moved to somewhere else.
moved: SetDomain<TempIndex>,
}

/// Represents a node of the borrow graph.
Expand Down Expand Up @@ -216,6 +214,8 @@ enum BorrowEdgeKind {
/// call outcome can be different, they are distinguished by code offset -- two call edges are never the
/// same.
Call(bool, Operation, CodeOffset),
/// Freezes a mutable reference.
Freeze,
}

impl BorrowEdgeKind {
Expand All @@ -226,6 +226,7 @@ impl BorrowEdgeKind {
| BorrowGlobal(is_mut)
| BorrowField(is_mut, _)
| Call(is_mut, _, _) => *is_mut,
Freeze => false,
}
}
}
Expand Down Expand Up @@ -304,8 +305,6 @@ impl AbstractDomain for LifetimeState {
change = JoinResult::Changed;
}
self.check_graph_consistency();

change = change.combine(self.moved.join(&other.moved));
change
}
}
Expand Down Expand Up @@ -1010,7 +1009,15 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
///
/// If we walk this graph now from the root to the leaves, we can determine safety by directly comparing
/// hyper edge siblings.
fn check_borrow_safety(&mut self, temps: &BTreeSet<TempIndex>) {
fn check_borrow_safety(&mut self, temps_vec: &[TempIndex]) {
// First check direct duplicates
for (i, temp) in temps_vec.iter().enumerate() {
if temps_vec[i + 1..].contains(temp) {
self.exclusive_access_direct_dup_error(*temp)
}
}
// Now build and analyze the hyper graph
let temps = &temps_vec.iter().cloned().collect::<BTreeSet<_>>();
let filtered_leaves = self
.state
.leaves()
Expand Down Expand Up @@ -1073,9 +1080,9 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
let target = edge.target;
targets.insert(target);
if edge.kind.is_mut() {
if let Some(temps) = filtered_leaves.get(&target) {
if let Some(ts) = filtered_leaves.get(&target) {
let mut inter =
temps.intersection(temps).cloned().collect::<BTreeSet<_>>();
ts.intersection(temps).cloned().collect::<BTreeSet<_>>();
if !inter.is_empty() {
if !self.state.is_leaf(&target) {
// A mut leaf node must have exclusive access
Expand All @@ -1088,7 +1095,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
}
if mapped_temps.len() > 1 {
// We cannot associate the same mut node with more than one local
self.exclusive_access_dup_error(&hyper, &mapped_temps)
self.exclusive_access_indirect_dup_error(&hyper, &mapped_temps)
}
hyper_nodes.insert(targets);
}
Expand Down Expand Up @@ -1161,8 +1168,9 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
}

/// Reports an error about exclusive access requirement for duplicate usage. See safety
/// condition (d) in the file header documentation.
fn exclusive_access_dup_error(
/// condition (d) in the file header documentation. This handles the case were the
/// same node is used by multiple temps
fn exclusive_access_indirect_dup_error(
&self,
labels: &BTreeSet<LifetimeLabel>,
temps: &BTreeSet<TempIndex>,
Expand All @@ -1172,7 +1180,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
self.error_with_hints(
self.cur_loc(),
format!(
"same mutable reference in {} is also used in other {} in same argument list",
"same mutable reference in {} is also used in other {} in argument list",
self.display(*ts[0]),
self.display(*ts[1])
),
Expand All @@ -1181,6 +1189,21 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
)
}

/// Reports an error about exclusive access requirement for duplicate usage. See safety
/// condition (d) in the file header documentation. This handles the case were the
/// same local is used multiple times.
fn exclusive_access_direct_dup_error(&self, temp: TempIndex) {
self.error_with_hints(
self.cur_loc(),
format!(
"same mutable reference in {} is used again in argument list",
self.display(temp),
),
"requirement enforced here",
iter::empty(),
)
}

/// Reports an error together with hints
fn error_with_hints(
&self,
Expand Down Expand Up @@ -1252,6 +1275,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
BorrowGlobal(_) => "global borrow",
BorrowField(..) => "field borrow",
Call(..) => "call result",
Freeze => "freeze",
},),
)
}
Expand Down Expand Up @@ -1341,11 +1365,6 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
self.check_read_local(src, mode);
self.check_write_local(dest);
}
// Track whether the variable content is moved
if kind == AssignKind::Move {
self.state.moved.insert(src);
}
self.state.moved.remove(&dest);
}

/// Process a borrow local instruction.
Expand All @@ -1370,7 +1389,6 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
label,
BorrowEdge::new(BorrowEdgeKind::BorrowGlobal(is_mut), loc, child),
);
self.state.moved.remove(&dest);
}

/// Process a borrow field instruction.
Expand All @@ -1391,7 +1409,6 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
label,
BorrowEdge::new(BorrowEdgeKind::BorrowField(is_mut, field_id), loc, child),
);
self.state.moved.remove(&dest);
}

/// Process a function call. For now we implement standard Move semantics, where every
Expand Down Expand Up @@ -1446,11 +1463,17 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
}
}
}
// All sources are moved into a call
self.state.moved.extend(srcs.iter().cloned());
for dest in dests {
self.state.moved.remove(dest);
}
}

/// Process a FreezeRef instruction.
fn freeze_ref(&mut self, code_offset: CodeOffset, dest: TempIndex, src: TempIndex) {
let label = *self.state.label_for_temp(src).expect("label for reference");
let target = self.state.replace_ref(dest, code_offset, 0);
self.state.add_edge(label, BorrowEdge {
kind: BorrowEdgeKind::Freeze,
loc: self.cur_loc(),
target,
})
}

/// Process a MoveFrom instruction.
Expand All @@ -1468,7 +1491,6 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
self.borrow_info(label, |_| true).into_iter(),
)
}
self.state.moved.remove(&dest);
}

/// Process a return instruction.
Expand Down Expand Up @@ -1510,15 +1532,13 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
}
}
}
self.state.moved.extend(srcs.iter().cloned())
}

/// Process a ReadRef instruction.
fn read_ref(&mut self, dest: TempIndex, src: TempIndex) {
debug_assert!(self.is_ref(src));
self.check_write_local(dest);
self.check_read_local(src, ReadMode::Argument);
self.state.moved.remove(&dest);
}

/// Process a WriteRef instruction.
Expand Down Expand Up @@ -1579,14 +1599,21 @@ impl<'env> TransferFunctions for LifeTimeAnalysis<'env> {
| Operation::Function(..)
| Operation::Eq
| Operation::Neq => {
let exclusive_refs =
srcs.iter().filter(|t| step.is_ref(**t)).cloned().collect();
let exclusive_refs = srcs
.iter()
.filter(|t| step.is_ref(**t))
.cloned()
.collect_vec();
step.check_borrow_safety(&exclusive_refs)
},
_ => {},
},
Ret(_, srcs) => {
let exclusive_refs = srcs.iter().filter(|t| step.is_ref(**t)).cloned().collect();
let exclusive_refs = srcs
.iter()
.filter(|t| step.is_ref(**t))
.cloned()
.collect_vec();
step.check_borrow_safety(&exclusive_refs)
},
_ => {},
Expand Down Expand Up @@ -1618,6 +1645,7 @@ impl<'env> TransferFunctions for LifeTimeAnalysis<'env> {
},
ReadRef => step.read_ref(dests[0], srcs[0]),
WriteRef => step.write_ref(srcs[0], srcs[1]),
FreezeRef => step.freeze_ref(code_offset, dests[0], srcs[0]),
MoveFrom(mid, sid, inst) => {
step.move_from(dests[0], &mid.qualified_inst(*sid, inst.clone()), srcs[0])
},
Expand Down Expand Up @@ -1682,11 +1710,6 @@ impl LifetimeInfoAtCodeOffset {
.filter(|t| !self.after.temp_to_label_map.contains_key(t))
.cloned()
}

/// Returns true if the value in the variable has been moved at this program point.
pub fn is_moved(&self, temp: TempIndex) -> bool {
self.after.moved.contains(&temp)
}
}

impl FunctionTargetProcessor for ReferenceSafetyProcessor {
Expand Down Expand Up @@ -1780,6 +1803,7 @@ impl<'a> Display for BorrowEdgeDisplay<'a> {
BorrowGlobal(is_mut) => write!(f, "borrow_global({})", is_mut),
BorrowField(is_mut, _) => write!(f, "borrow_field({})", is_mut),
Call(is_mut, _, _) => write!(f, "call({})", is_mut),
Freeze => write!(f, "freeze"),
})?;
if display_child {
write!(f, " -> {}", edge.target)
Expand Down Expand Up @@ -1860,7 +1884,6 @@ impl<'a> Display for LifetimeStateDisplay<'a> {
graph,
temp_to_label_map,
global_to_label_map,
moved,
} = &self.1;
let pool = self.0.global_env().symbol_pool();
writeln!(
Expand All @@ -1887,14 +1910,6 @@ impl<'a> Display for LifetimeStateDisplay<'a> {
.iter()
.map(|(str, label)| format!("{}={}", self.0.global_env().display(str), label))
.join(",")
)?;
writeln!(
f,
"moved: {{{}}}",
moved
.iter()
.map(|t| self.0.get_local_raw_name(*t).display(pool).to_string())
.join(",")
)
}
}
Expand Down
Loading

0 comments on commit 997d925

Please sign in to comment.