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

[compiler v2] Fix bugs around compilation of vector code #9636

Merged
merged 2 commits into from
Aug 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
42 changes: 40 additions & 2 deletions third_party/move/move-compiler-v2/src/bytecode_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ impl<'env> Generator<'env> {
// Dispatcher

impl<'env> Generator<'env> {
/// Generate code, for the given expression, and store the result in the given temporary.
fn gen(&mut self, targets: Vec<TempIndex>, exp: &Exp) {
match exp.as_ref() {
ExpData::Invalid(id) => self.internal_error(*id, "invalid expression"),
Expand Down Expand Up @@ -720,7 +719,25 @@ impl<'env> Generator<'env> {
.env()
.get_node_instantiation_opt(id)
.unwrap_or_default();
let args = self.gen_arg_list(args);
// Function calls can have implicit conversion of &mut to &, need to compute implicit
// conversions.
let param_types: Vec<Type> = self
.env()
.get_function(fun)
.get_parameters()
.into_iter()
.map(|Parameter(_, ty)| ty.instantiate(&type_args))
.collect();
if args.len() != param_types.len() {
self.internal_error(id, "inconsistent type arity");
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the advantage of this C style errors versus Rust errors?

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 actually neither or, its an error with source location information added to the compilers global environment, which can contain many of them. Its marked as 'internal' because it is unexpected. Nevertheless will be reported with possible errors at the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+possible other errors at the source

return;
}
let args = args
.iter()
.zip(param_types.into_iter())
.map(|(e, t)| self.maybe_convert(e, &t))
.collect::<Vec<_>>();
let args = self.gen_arg_list(&args);
self.emit_with(id, |attr| {
Bytecode::Call(
attr,
Expand All @@ -732,6 +749,27 @@ impl<'env> Generator<'env> {
})
}

/// Convert the expression so it matches the expected type. This is currently only needed
/// for `&mut` to `&` conversion, in which case we need to to introduce a Freeze operation.
fn maybe_convert(&self, exp: &Exp, expected_ty: &Type) -> Exp {
let id = exp.node_id();
let exp_ty = self.env().get_node_type(id);
if let (
Type::Reference(ReferenceKind::Mutable, _),
Type::Reference(ReferenceKind::Immutable, et),
) = (exp_ty, expected_ty)
{
let freeze_id = self
.env()
.new_node(self.env().get_node_loc(id), expected_ty.clone());
self.env()
.set_node_instantiation(freeze_id, vec![et.as_ref().clone()]);
ExpData::Call(freeze_id, Operation::Freeze, vec![exp.clone()]).into_exp()
} else {
exp.clone()
}
}

fn gen_arg_list(&mut self, exps: &[Exp]) -> Vec<TempIndex> {
exps.iter().map(|exp| self.gen_arg(exp)).collect()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ impl<'a> FunctionGenerator<'a> {
let idx = self.gen.function_instantiation_index(
&fun_ctx.module,
&fun_ctx.loc,
fun_ctx.fun.func_env,
&fun_ctx.module.env.get_function(id),
inst.to_vec(),
);
self.emit(FF::Bytecode::CallGeneric(idx))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ impl FunctionTargetProcessor for LiveVarAnalysisProcessor {
mut data: FunctionData,
_scc_opt: Option<&[FunctionEnv]>,
) -> FunctionData {
if fun_env.is_native() {
brmataptos marked this conversation as resolved.
Show resolved Hide resolved
return data;
}
// Call the existing live-var analysis from the move-prover.
let target = FunctionTarget::new(fun_env, &data);
let offset_to_live_refs = livevar_analysis::LiveVarAnnotation::from_map(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// ---- Model Dump
module 0x42::reference_conversion {
private fun deref(r: &u64) {
Deref(r)
}
private fun use_it() {
{
let x: u64 = 42;
{
let r: &mut u64 = Borrow(Mutable)(x);
r = 43;
reference_conversion::deref(r)
}
}
}
} // end 0x42::reference_conversion

============ initial bytecode ================

[variant baseline]
fun reference_conversion::deref($t0: &u64): u64 {
var $t1: u64
0: $t1 := read_ref($t0)
1: return $t1
}


[variant baseline]
fun reference_conversion::use_it(): u64 {
var $t0: u64
var $t1: u64
var $t2: u64
var $t3: &mut u64
var $t4: &mut u64
var $t5: u64
var $t6: &u64
0: $t2 := 42
1: $t1 := move($t2)
2: $t4 := borrow_local($t1)
3: $t3 := move($t4)
4: $t5 := 43
5: write_ref($t3, $t5)
6: $t6 := freeze_ref($t3)
7: $t0 := reference_conversion::deref($t6)
8: return $t0
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module 0x42::reference_conversion {

fun deref(r: &u64): u64 {
*r
}

fun use_it(): u64 {
let x = 42;
let r = &mut x;
*r = 43;
deref(r)
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
============ initial bytecode ================

[variant baseline]
fun Test::foo($t0: u64): u64 {
var $t1: u64
0: $t1 := Test::identity<u64>($t0)
1: return $t1
}


[variant baseline]
fun Test::identity<#0>($t0: #0): #0 {
var $t1: #0
0: $t1 := move($t0)
1: return $t1
}

============ after LiveVarAnalysisProcessor: ================

[variant baseline]
fun Test::foo($t0: u64): u64 {
var $t1: u64
# live vars: $t0
0: $t1 := Test::identity<u64>($t0)
# live vars: $t1
1: return $t1
}


[variant baseline]
fun Test::identity<#0>($t0: #0): #0 {
var $t1: #0
# live vars: $t0
0: $t1 := move($t0)
# live vars: $t1
1: return $t1
}


============ disassembled file-format ==================
// Move bytecode v6
module 42.Test {


foo(Arg0: u64): u64 {
B0:
0: MoveLoc[0](Arg0: u64)
1: Call identity<u64>(u64): u64
2: Ret
}
identity<Ty0>(Arg0: Ty0): Ty0 {
B0:
0: MoveLoc[0](Arg0: Ty0)
1: StLoc[1](loc0: Ty0)
2: MoveLoc[1](loc0: Ty0)
3: Ret
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0x42::Test {
fun identity<T>(x: T): T {
x
}

fun foo(x: u64): u64 {
identity(x)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
processed 2 tasks

==> Compiler v2 delivered same results!
brmataptos marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
//# publish
module 0x42::heap {
use std::vector;

fun create1(): vector<u64> {
vector<u64>[3, 2, 1, 5, 8, 4]
}

fun create2(): vector<u64> {
vector<u64>[1, 2, 3, 4, 5, 8]
}

fun vcopy(x: &vector<u64>): vector<u64> {
let y : vector<u64> = vector::empty<u64>();
let i : u64 = 0;
let l : u64 = vector::length<u64>(x);
while (i < l) {
vector::push_back<u64>(&mut y, *vector::borrow<u64>(x, i));
i = i + 1;
};
y
}

fun sort(x: &mut vector<u64>) {
let i: u64 = 0;
while (i < vector::length<u64>(x)) {
let j: u64 = i + 1;
while (j < vector::length<u64>(x)) {
if (*vector::borrow<u64>(x, i) > *vector::borrow<u64>(x, j)) {
vector::swap<u64>(x, i, j)
};
j = j + 1;
};
i = i + 1;
}
}

fun array_equals(x: &vector<u64>, y: &vector<u64>): bool {
let l1: u64 = vector::length<u64>(x);
let l2: u64 = vector::length<u64>(y);
if (l1 != l2) {
return false
};
let i: u64 = 0;
while (i < l1) {
if (*vector::borrow<u64>(x, i) != *vector::borrow<u64>(y, i)) {
return false
};
i = i + 1;
};
true
}

public fun main() {
let x: vector<u64> = create1();
let y: vector<u64> = create2();
let z: vector<u64> = vcopy(&x);
assert!(array_equals(&x, &z), 23);
assert!(array_equals(&y, &y), 29);
sort(&mut x);
assert!(array_equals(&y, &x), 31);
assert!(array_equals(&x, &y), 29);
assert!(!array_equals(&x, &z), 31);
}
}

//# run
script {
use 0x42::heap::main;
fun mymain() {
main();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
processed 2 tasks

==> Compiler v2 delivered same results!
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//# publish
module 0x42::test {
public fun two_args(x: u64, b: bool): u64 {
if (b) {
x
} else {
0
}
}
}

//# run
script {
use 0x42::test::two_args;
fun mymain() {
assert!(two_args(42, true) == 42, 1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ pub trait MoveTestAdapter<'a>: Sized {
// Run the V2 compiler if requested
SyntaxChoice::Source if run_config == TestRunConfig::CompilerV2 => {
let ((module, _), warning_opt) = compile_source_unit_v2(
state.pre_compiled_deps,
state.named_address_mapping.clone(),
&state.source_files().cloned().collect::<Vec<_>>(),
data_path.to_owned(),
Expand Down Expand Up @@ -326,6 +327,7 @@ pub trait MoveTestAdapter<'a>: Sized {
// Run the V2 compiler if requested.
SyntaxChoice::Source if run_config == TestRunConfig::CompilerV2 => {
let ((_, script), warning_opt) = compile_source_unit_v2(
state.pre_compiled_deps,
state.named_address_mapping.clone(),
&state.source_files().cloned().collect::<Vec<_>>(),
data_path.to_owned(),
Expand Down Expand Up @@ -603,16 +605,34 @@ impl<'a> CompiledState<'a> {
}

fn compile_source_unit_v2(
pre_compiled_deps: Option<&FullyCompiledProgram>,
named_address_mapping: BTreeMap<String, NumericalAddress>,
deps: &[String],
path: String,
) -> Result<(
(Option<CompiledModule>, Option<CompiledScript>),
Option<String>,
)> {
let deps = if let Some(p) = pre_compiled_deps {
// The v2 compiler does not (and perhaps never) supports precompiled programs, so
// compile from the sources again, computing the directories where they are found.
let mut dirs: BTreeSet<_> = p
.files
.iter()
.filter_map(|(_, (file_name, _))| {
Path::new(file_name.as_str())
.parent()
.map(|p| p.to_string_lossy().to_string())
})
.collect();
dirs.extend(deps.iter().cloned());
dirs.into_iter().collect()
} else {
deps.to_vec()
};
let options = move_compiler_v2::Options {
sources: vec![path],
dependencies: deps.to_vec(),
dependencies: deps,
named_address_mapping: named_address_mapping
.into_iter()
.map(|(alias, addr)| format!("{}={}", alias, addr))
Expand Down