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 all commits
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 @@ -512,7 +512,16 @@ impl<'a> FunctionGenerator<'a> {
) {
let fun_ctx = ctx.fun_ctx;
self.abstract_push_args(ctx, source);
if inst.is_empty() {
if let Some(opcode) = ctx.fun_ctx.module.get_well_known_function_code(
Copy link
Contributor

Choose a reason for hiding this comment

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

I note that V1 code checks (in fake_natives.rs) for this check for the existence of an attribute (AttributeName_::Known(KnownAttribute::Native(NativeAttribute::BytecodeInstruction))) before trying to match a "fake native"/"well-known function". I might agree with their comment about developer sanity here:

//! This module verifies the usage of the "fake native" functions. These functions are declared
//! as 'native`, but do not appear in the compiled module. For developer sanity, they must be marked
//! with the `FAKE_NATIVE_ATTR`

If someday our descendants decide to add a new well-known function they might have an easier time if they get a warning from the compiler that it sees the #[bytecode_instruction] annotation on the function definition but doesn't know how to translate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether there is any value to this attribute, and I remember the author wasn't sure either. The attribute would have to at least also denote the bytecode which is going to be used. In fact, I think those functions should not appear in the first place in vector, a refactoring we may want to do at a later point. (Specification builtin functions do not have source code.) They have already introduced quite some confusion including security incidents.

&ctx.fun_ctx.loc,
id,
Some(
self.gen
.signature(&ctx.fun_ctx.module, &ctx.fun_ctx.loc, inst.to_vec()),
),
) {
self.emit(opcode)
} else if inst.is_empty() {
let idx = self.gen.function_index(
&fun_ctx.module,
&fun_ctx.loc,
Expand All @@ -523,7 +532,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 @@ -16,6 +16,7 @@ use move_binary_format::{
};
use move_core_types::{account_address::AccountAddress, identifier::Identifier};
use move_model::{
ast::Address,
model::{
FieldEnv, FunId, FunctionEnv, GlobalEnv, Loc, ModuleEnv, ModuleId, Parameter, QualifiedId,
StructEnv, StructId, TypeParameter, TypeParameterKind,
Expand Down Expand Up @@ -623,4 +624,41 @@ impl<'env> ModuleContext<'env> {
value as FF::TableIndex
}
}

/// Get the file format opcode for a well-known function. This applies currently to a set
/// vector functions which have builtin opcodes. Gets passed an optional type instantiation
/// in form of a signature.
pub fn get_well_known_function_code(
&self,
loc: &Loc,
qid: QualifiedId<FunId>,
inst_sign: Option<FF::SignatureIndex>,
) -> Option<FF::Bytecode> {
let fun = self.env.get_function(qid);
let mod_name = fun.module_env.get_name();
if mod_name.addr() != &Address::Numerical(AccountAddress::ONE) {
return None;
}
let pool = self.env.symbol_pool();
if pool.string(mod_name.name()).as_str() == "vector" {
if let Some(inst) = inst_sign {
match pool.string(fun.get_name()).as_str() {
"empty" => Some(FF::Bytecode::VecPack(inst, 0)),
"length" => Some(FF::Bytecode::VecLen(inst)),
"borrow" => Some(FF::Bytecode::VecImmBorrow(inst)),
"borrow_mut" => Some(FF::Bytecode::VecMutBorrow(inst)),
"push_back" => Some(FF::Bytecode::VecPushBack(inst)),
"pop_back" => Some(FF::Bytecode::VecPopBack(inst)),
"destroy_empty" => Some(FF::Bytecode::VecUnpack(inst, 0)),
"swap" => Some(FF::Bytecode::VecSwap(inst)),
_ => None,
}
} else {
self.internal_error(loc, "expected type instantiation for vector operation");
None
}
} else {
None
}
}
}
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)
}
}
Loading