From 30e2464f2edfa68a3ca12048642bf67f4d7128d6 Mon Sep 17 00:00:00 2001 From: Jason Eckhardt Date: Wed, 14 Jun 2023 12:14:30 -0500 Subject: [PATCH 1/2] Fix generic funcion dep defect; rework function discovery approach. We have previously discovered through experience that some of the model-provided information we once depended on to discover all module functions, called functions, and concrete instantiations are not always consistent or reliable. For this reason, we now take a different approach and seed our discovery with just the list of functions provided by `ModuleEnv::get_functions`. For any other called functions (this module or foreign) and for any generic instantiations, we will expand the seed frontier incrementally by gleaning the remaining information from a visitation of every function call instruction (recursively) in every seed function. This also fixes a dependence problem in the previous discovery approach where some transitively expanded generics were never seen or declared. No new tests were added as the previous generics work covers this. We now, however, can eliminate the dependence defect work around that was originally in the moption01 test case. It now runs without the work around successfully. --- .../src/stackless/translate.rs | 202 +++++++++--------- .../tests/rbpf-tests/moption01.move | 7 - 2 files changed, 102 insertions(+), 107 deletions(-) diff --git a/language/tools/move-mv-llvm-compiler/src/stackless/translate.rs b/language/tools/move-mv-llvm-compiler/src/stackless/translate.rs index 9af0faba0a..7956dfe6d2 100644 --- a/language/tools/move-mv-llvm-compiler/src/stackless/translate.rs +++ b/language/tools/move-mv-llvm-compiler/src/stackless/translate.rs @@ -448,122 +448,121 @@ impl<'mm, 'up> ModuleContext<'mm, 'up> { /// Create LLVM function decls for all local functions and /// all extern functions that might be called. fn declare_functions(&mut self) { - use move_binary_format::access::ModuleAccess; let mod_env = self.env.clone(); // fixme bad clone + + // We have previously discovered through experience that some of the model-provided + // information we once depended on to discover all module functions, called functions, + // and concrete instantiations are not always consistent or reliable. + // + // For this reason, we now take a different approach and seed our discovery with just the + // list of functions provided by `ModuleEnv::get_functions`. For any other called functions + // (this module or foreign) and for any generic instantiations, we will expand the seed + // frontier incrementally by gleaning the remaining information from a visitation of every + // function call instruction (recursively) in every seed function. + // + // While this results in yet another linear walk over all the code, it seems to be the + // simplest way to work around the model inconsistencies. + for fn_env in mod_env.get_functions() { + self.declare_functions_walk(&mod_env, &fn_env, vec![]); + } + } + + fn declare_functions_walk( + &mut self, + mod_env: &mm::ModuleEnv, + curr_fn_env: &mm::FunctionEnv, + curr_type_vec: Vec, + ) { let g_env = &mod_env.env; - let mut foreign_fns = BTreeSet::new(); - let mut fn_instantiations: BTreeMap, Vec>> = - BTreeMap::new(); + // Do not process a previously declared function/expansion. + let fn_name = if curr_fn_env.is_native() { + curr_fn_env.llvm_native_fn_symbol_name() + } else if curr_fn_env.get_type_parameter_count() == 0 { + curr_fn_env.llvm_symbol_name(&[]) + } else { + curr_fn_env.llvm_symbol_name(&curr_type_vec) + }; - // First collect any generic instantiations and map to qualified ids. - // Each generic function handle represents potentially multiple concrete functions. - // This map supplies the type parameter vector for each concrete instance. - let this_module_data = &g_env.module_data[mod_env.get_id().to_usize()]; - let cm = &this_module_data.module; - for fi_view in cm.function_instantiations() { - let tyvec = mod_env.get_type_actuals(Some(fi_view.type_parameters)); - let fn_env = mod_env.get_used_function(fi_view.handle); - fn_instantiations - .entry(fn_env.get_qualified_id()) - .or_insert_with(Vec::new) - .push(tyvec); + if self.fn_decls.get(&fn_name).is_some() { + return; } - for fn_env in mod_env.get_functions() { - let linkage = fn_env.llvm_linkage(); - - // For native functions and concrete Move functions, declare a single function. - // - // For a generic Move function, declare all concrete expansions. The generic function - // itself will not be emitted. - let fn_qid = fn_env.get_qualified_id(); - if !fn_env.is_native() && fn_env.get_type_parameter_count() > 0 { - if !fn_instantiations.contains_key(&fn_qid) { - continue; - } - for fi in &fn_instantiations[&fn_qid] { - // Do not attempt to instantiate generics whose type parameters themselves - // are generic. Those cannot be expanded until a function containing them - // is instantiated, resolving the type parameters. - let inst_is_generic = fi.iter().any(|t| t.is_open()); - if inst_is_generic { - continue; - } - self.declare_function(&fn_env, fi, linkage); - let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, fi.to_vec()); - self.expanded_functions.push(fn_qiid); - } - } else { - self.declare_function(&fn_env, &[], linkage); - let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, vec![]); - if !fn_env.is_native() { - self.expanded_functions.push(fn_qiid); - } - } + let fn_data = StacklessBytecodeGenerator::new(curr_fn_env).generate_function(); - for called_fn in fn_env.get_transitive_closure_of_called_functions() { - // Pull in not just the directly called functions, but the transitive closure - // of called functions. Note that all directly called functions that are not - // in our module are public by definition. But those can transitively invoke - // private functions outside of our module, so exclude those. - let is_foreign_mod = called_fn.module_id != mod_env.get_id(); - if is_foreign_mod && g_env.get_function(called_fn).is_exposed() { - foreign_fns.insert(called_fn); - } - } + // If the current function is either a native function or a concrete Move function, + // we have all the information needed to declare a corresponding single function. + // + // If the current function is a generic Move function, we will defer declaring its + // concrete expansions until a call path leading to a particular call site is visited. + // At that point, the type parameters are either resolved or the function is not used + // in the module. The generic function itself will not be emitted. + let curr_fn_qid = curr_fn_env.get_qualified_id(); + if curr_fn_env.is_native() { + // Declare the native and return early--- there is no function body to visit. + self.declare_native_function(curr_fn_env, &fn_data, curr_fn_env.llvm_linkage()); + return; + } else if curr_fn_env.get_type_parameter_count() == 0 { + let curr_fn_qiid = curr_fn_qid.module_id.qualified_inst(curr_fn_qid.id, vec![]); + self.declare_move_function(curr_fn_env, &[], &fn_data, curr_fn_env.llvm_linkage()); + if curr_fn_qid.module_id != mod_env.get_id() { + // True foreign functions are only declared in our module, don't process further. + return; + } + self.expanded_functions.push(curr_fn_qiid); + } else { + // Determine whether any of the type parameters for this generic function are still + // unresolved. If so, then function is not a concrete instance and we defer it until + // a call path containing it is expanded. + assert!(curr_fn_env.get_type_parameter_count() > 0); + let inst_is_generic = curr_type_vec.iter().any(|t| t.is_open()); + if curr_type_vec.is_empty() || inst_is_generic { + return; + } + + // Note that we may be declaring a foreign function here. But since it is being + // expanded into our current module, its linkage is effectively private. + let curr_fn_qiid = curr_fn_qid + .module_id + .qualified_inst(curr_fn_qid.id, curr_type_vec.clone()); + self.declare_move_function( + curr_fn_env, + &curr_type_vec, + &fn_data, + llvm::LLVMLinkage::LLVMPrivateLinkage, + ); + self.expanded_functions.push(curr_fn_qiid); } - for fn_qid in foreign_fns { - let called_fn_env = g_env.get_function(fn_qid); - if !called_fn_env.is_native() && called_fn_env.get_type_parameter_count() > 0 { - if !fn_instantiations.contains_key(&fn_qid) { - continue; - } - for fi in &fn_instantiations[&fn_qid] { - // Do not attempt to instantiate generics whose type parameters themselves - // are generic. Those cannot be expanded until a function containing them - // is instantiated, resolving the type parameters. - let inst_is_generic = fi.iter().any(|t| t.is_open()); - if inst_is_generic { - continue; - } - self.declare_function( - &called_fn_env, - fi, - llvm::LLVMLinkage::LLVMPrivateLinkage, - ); - let fn_qiid = fn_qid.module_id.qualified_inst(fn_qid.id, fi.to_vec()); - assert!(called_fn_env.get_qualified_id() == fn_qid); - self.expanded_functions.push(fn_qiid); - } - } else { - self.declare_function(&called_fn_env, &[], called_fn_env.llvm_linkage()); + // Visit every call site in the current function, instantiate their type parameters, + // and then recursively grow the frontier. + for instr in &fn_data.code { + if let sbc::Bytecode::Call( + _, + _, + sbc::Operation::Function(mod_id, fun_id, types), + _, + None, + ) = instr + { + // Instantiate any type parameters at the current call site with the + // enclosing type parameter scope `curr_type_vec`. + let types = mty::Type::instantiate_vec(types.to_vec(), &curr_type_vec); + + // Recursively discover/declare more functions on this call path. + let called_fn_env = g_env.get_function((*mod_id).qualified(*fun_id)); + self.declare_functions_walk(mod_env, &called_fn_env, types); } } } - fn declare_function( - &mut self, - fn_env: &mm::FunctionEnv, - tyvec: &[mty::Type], - linkage: llvm::LLVMLinkage, - ) { - if fn_env.is_native() { - self.declare_native_function(fn_env, linkage) - } else { - self.declare_move_function(fn_env, tyvec, linkage) - } - } - fn declare_move_function( &mut self, fn_env: &mm::FunctionEnv, tyvec: &[mty::Type], + fn_data: &FunctionData, linkage: llvm::LLVMLinkage, ) { - let fn_data = StacklessBytecodeGenerator::new(fn_env).generate_function(); - let ll_sym_name = fn_env.llvm_symbol_name(tyvec); let ll_fn = { let ll_fnty = { @@ -610,11 +609,14 @@ impl<'mm, 'up> ModuleContext<'mm, 'up> { /// At some point we might want to factor out the platform-specific ABI /// decisions, but for now there are only a few ABI concerns, and we may /// never support another platform for which the ABI is different. - fn declare_native_function(&mut self, fn_env: &mm::FunctionEnv, linkage: llvm::LLVMLinkage) { + fn declare_native_function( + &mut self, + fn_env: &mm::FunctionEnv, + fn_data: &FunctionData, + linkage: llvm::LLVMLinkage, + ) { assert!(fn_env.is_native()); - let fn_data = StacklessBytecodeGenerator::new(fn_env).generate_function(); - let ll_native_sym_name = fn_env.llvm_native_fn_symbol_name(); let ll_fn = { let ll_fnty = { diff --git a/language/tools/move-mv-llvm-compiler/tests/rbpf-tests/moption01.move b/language/tools/move-mv-llvm-compiler/tests/rbpf-tests/moption01.move index 92b1993fb3..dc2c13bf6b 100644 --- a/language/tools/move-mv-llvm-compiler/tests/rbpf-tests/moption01.move +++ b/language/tools/move-mv-llvm-compiler/tests/rbpf-tests/moption01.move @@ -301,13 +301,6 @@ module 0x300::option_tests { use 0x10::option; use 0x10::vector; - fun phony() { - // Work around dependency problem. Remove when that is gone. - let v = vector::singleton(31); - assert!(vector::contains(&v, &31), 0); - assert!(vector::is_empty(&vector::empty()), 0); - } - public fun option_none_is_none() { let none = option::none(); assert!(option::is_none(&none), 0); From 0fde817d401c2722b97ad64514fb13d63d6f8b25 Mon Sep 17 00:00:00 2001 From: Jason Eckhardt Date: Wed, 14 Jun 2023 12:40:11 -0500 Subject: [PATCH 2/2] NFC: Remaster move-ir-tests. The differences are due to a different function output order. --- .../modules/0_M2.expected.ll | 24 ++++---- .../modules/1_M11.expected.ll | 56 +++++++++---------- .../scripts/main.expected.ll | 2 - .../modules/1_UseIt.expected.ll | 8 +-- 4 files changed, 44 insertions(+), 46 deletions(-) diff --git a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func01-build/modules/0_M2.expected.ll b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func01-build/modules/0_M2.expected.ll index c04a2a934f..549b162fe5 100644 --- a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func01-build/modules/0_M2.expected.ll +++ b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func01-build/modules/0_M2.expected.ll @@ -18,32 +18,32 @@ entry: ret %struct.M2__Coin_M2__Bitcoin_ %retval1 } -define %struct.M2__Coin_M2__Sol_ @M2__mint_concrete(i64 %0) { +define private %struct.M2__Coin_M2__Bitcoin_ @M2__mint_generic_M2__Bitcoin(i64 %0) { entry: %local_0 = alloca i64, align 8 %local_1__value = alloca i64, align 8 - %local_2 = alloca %struct.M2__Coin_M2__Sol_, align 8 + %local_2 = alloca %struct.M2__Coin_M2__Bitcoin_, align 8 store i64 %0, ptr %local_0, align 4 %load_store_tmp = load i64, ptr %local_0, align 4 store i64 %load_store_tmp, ptr %local_1__value, align 4 %fv.0 = load i64, ptr %local_1__value, align 4 - %insert_0 = insertvalue %struct.M2__Coin_M2__Sol_ undef, i64 %fv.0, 0 - store %struct.M2__Coin_M2__Sol_ %insert_0, ptr %local_2, align 4 - %retval = load %struct.M2__Coin_M2__Sol_, ptr %local_2, align 4 - ret %struct.M2__Coin_M2__Sol_ %retval + %insert_0 = insertvalue %struct.M2__Coin_M2__Bitcoin_ undef, i64 %fv.0, 0 + store %struct.M2__Coin_M2__Bitcoin_ %insert_0, ptr %local_2, align 4 + %retval = load %struct.M2__Coin_M2__Bitcoin_, ptr %local_2, align 4 + ret %struct.M2__Coin_M2__Bitcoin_ %retval } -define %struct.M2__Coin_M2__Bitcoin_ @M2__mint_generic_M2__Bitcoin(i64 %0) { +define %struct.M2__Coin_M2__Sol_ @M2__mint_concrete(i64 %0) { entry: %local_0 = alloca i64, align 8 %local_1__value = alloca i64, align 8 - %local_2 = alloca %struct.M2__Coin_M2__Bitcoin_, align 8 + %local_2 = alloca %struct.M2__Coin_M2__Sol_, align 8 store i64 %0, ptr %local_0, align 4 %load_store_tmp = load i64, ptr %local_0, align 4 store i64 %load_store_tmp, ptr %local_1__value, align 4 %fv.0 = load i64, ptr %local_1__value, align 4 - %insert_0 = insertvalue %struct.M2__Coin_M2__Bitcoin_ undef, i64 %fv.0, 0 - store %struct.M2__Coin_M2__Bitcoin_ %insert_0, ptr %local_2, align 4 - %retval = load %struct.M2__Coin_M2__Bitcoin_, ptr %local_2, align 4 - ret %struct.M2__Coin_M2__Bitcoin_ %retval + %insert_0 = insertvalue %struct.M2__Coin_M2__Sol_ undef, i64 %fv.0, 0 + store %struct.M2__Coin_M2__Sol_ %insert_0, ptr %local_2, align 4 + %retval = load %struct.M2__Coin_M2__Sol_, ptr %local_2, align 4 + ret %struct.M2__Coin_M2__Sol_ %retval } diff --git a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func02-build/modules/1_M11.expected.ll b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func02-build/modules/1_M11.expected.ll index 358a8f01b2..67a7ca1c60 100644 --- a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func02-build/modules/1_M11.expected.ll +++ b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/generic-func02-build/modules/1_M11.expected.ll @@ -19,6 +19,19 @@ entry: ret i64 %retval1 } +define private i64 @Coins__get_value_generic_M11__USDC(%struct.Coins__Coin_M11__USDC_ %0) { +entry: + %local_0 = alloca %struct.Coins__Coin_M11__USDC_, align 8 + %local_1 = alloca %struct.Coins__Coin_M11__USDC_, align 8 + %local_2__value = alloca i64, align 8 + store %struct.Coins__Coin_M11__USDC_ %0, ptr %local_0, align 4 + %srcval = load %struct.Coins__Coin_M11__USDC_, ptr %local_0, align 4 + %ext_0 = extractvalue %struct.Coins__Coin_M11__USDC_ %srcval, 0 + store i64 %ext_0, ptr %local_2__value, align 4 + %retval = load i64, ptr %local_2__value, align 4 + ret i64 %retval +} + define private { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } @M11__mint_2coins_usdc_and_eth(i64 %0, i64 %1) { entry: %local_0 = alloca i64, align 8 @@ -47,34 +60,6 @@ entry: ret { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } %insert_1 } -define private %struct.Coins__Coin_M11__USDC_ @M11__mint_usdc(i64 %0) { -entry: - %local_0 = alloca i64, align 8 - %local_1 = alloca i64, align 8 - %local_2 = alloca %struct.Coins__Coin_M11__USDC_, align 8 - store i64 %0, ptr %local_0, align 4 - %load_store_tmp = load i64, ptr %local_0, align 4 - store i64 %load_store_tmp, ptr %local_1, align 4 - %call_arg_0 = load i64, ptr %local_1, align 4 - %retval = call %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %call_arg_0) - store %struct.Coins__Coin_M11__USDC_ %retval, ptr %local_2, align 4 - %retval1 = load %struct.Coins__Coin_M11__USDC_, ptr %local_2, align 4 - ret %struct.Coins__Coin_M11__USDC_ %retval1 -} - -define private i64 @Coins__get_value_generic_M11__USDC(%struct.Coins__Coin_M11__USDC_ %0) { -entry: - %local_0 = alloca %struct.Coins__Coin_M11__USDC_, align 8 - %local_1 = alloca %struct.Coins__Coin_M11__USDC_, align 8 - %local_2__value = alloca i64, align 8 - store %struct.Coins__Coin_M11__USDC_ %0, ptr %local_0, align 4 - %srcval = load %struct.Coins__Coin_M11__USDC_, ptr %local_0, align 4 - %ext_0 = extractvalue %struct.Coins__Coin_M11__USDC_ %srcval, 0 - store i64 %ext_0, ptr %local_2__value, align 4 - %retval = load i64, ptr %local_2__value, align 4 - ret i64 %retval -} - define private { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } @Coins__mint_2coins_generic_M11__USDC_M11__Eth(i64 %0, i64 %1) { entry: %local_0 = alloca i64, align 8 @@ -102,6 +87,21 @@ entry: ret { %struct.Coins__Coin_M11__USDC_, %struct.Coins__Coin_M11__Eth_ } %insert_1 } +define private %struct.Coins__Coin_M11__USDC_ @M11__mint_usdc(i64 %0) { +entry: + %local_0 = alloca i64, align 8 + %local_1 = alloca i64, align 8 + %local_2 = alloca %struct.Coins__Coin_M11__USDC_, align 8 + store i64 %0, ptr %local_0, align 4 + %load_store_tmp = load i64, ptr %local_0, align 4 + store i64 %load_store_tmp, ptr %local_1, align 4 + %call_arg_0 = load i64, ptr %local_1, align 4 + %retval = call %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %call_arg_0) + store %struct.Coins__Coin_M11__USDC_ %retval, ptr %local_2, align 4 + %retval1 = load %struct.Coins__Coin_M11__USDC_, ptr %local_2, align 4 + ret %struct.Coins__Coin_M11__USDC_ %retval1 +} + define private %struct.Coins__Coin_M11__USDC_ @Coins__mint_generic_M11__USDC(i64 %0) { entry: %local_0 = alloca i64, align 8 diff --git a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/multi-module-build/scripts/main.expected.ll b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/multi-module-build/scripts/main.expected.ll index 26d2a574b1..fce9f3f43f 100644 --- a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/multi-module-build/scripts/main.expected.ll +++ b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/multi-module-build/scripts/main.expected.ll @@ -17,6 +17,4 @@ entry: ret void } -declare i8 @Test2__test2(i8, i8) - declare i8 @Test1__test1(i8, i8) diff --git a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/struct02-build/modules/1_UseIt.expected.ll b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/struct02-build/modules/1_UseIt.expected.ll index 6d9df67cf1..899889de4e 100644 --- a/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/struct02-build/modules/1_UseIt.expected.ll +++ b/language/tools/move-mv-llvm-compiler/tests/move-ir-tests/struct02-build/modules/1_UseIt.expected.ll @@ -48,12 +48,12 @@ entry: ret void } -declare i8 @Country__dropit(%struct.Country__Country) - -declare i8 @Country__get_id(ptr) +declare %struct.Country__Country @Country__new_country(i8, i64) declare i64 @Country__get_pop(%struct.Country__Country) -declare %struct.Country__Country @Country__new_country(i8, i64) +declare i8 @Country__get_id(ptr) declare void @Country__set_id(ptr, i8) + +declare i8 @Country__dropit(%struct.Country__Country)