From 3b035a7a5c1512ff4f6e964269d68385a302d5d4 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Tue, 19 Nov 2024 16:24:03 +0000 Subject: [PATCH] [loader-v2] Addressing TODOs --- .../block-executor/src/code_cache_global_manager.rs | 1 - .../src/tests/bad_storage_tests.rs | 3 +-- third_party/move/move-vm/runtime/src/loader/mod.rs | 3 +-- .../move/move-vm/runtime/src/storage/loader.rs | 13 +++++-------- .../entry_points/script_too_few_type_args_inner.exp | 2 +- .../script_too_few_type_args_inner.v2_exp | 2 +- .../script_too_many_type_args_inner.exp | 2 +- .../script_too_many_type_args_inner.v2_exp | 2 +- third_party/move/move-vm/types/src/code/errors.rs | 6 +++--- 9 files changed, 14 insertions(+), 20 deletions(-) diff --git a/aptos-move/block-executor/src/code_cache_global_manager.rs b/aptos-move/block-executor/src/code_cache_global_manager.rs index 5daaf4c0b7e90..f90ada6ba1994 100644 --- a/aptos-move/block-executor/src/code_cache_global_manager.rs +++ b/aptos-move/block-executor/src/code_cache_global_manager.rs @@ -173,7 +173,6 @@ impl AptosModuleCacheManager { AptosModuleCacheManagerGuard::Guard { guard } }, None => { - // TODO(loader_v2): Should we return an error here instead? alert_or_println!("Locking module cache manager failed, fallback to empty caches"); // If this is true, we failed to acquire a lock, and so default storage environment diff --git a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs index cbc19460eca72..922d80ef4775e 100644 --- a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs +++ b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs @@ -792,8 +792,7 @@ fn test_storage_returns_bogus_error_when_loading_resource() { .unwrap_err(); if *error_code == StatusCode::UNKNOWN_VERIFICATION_ERROR { - // MoveVM maps `UNKNOWN_VERIFICATION_ERROR` to `VERIFICATION_ERROR` in - // `maybe_core_dump` function in interpreter.rs. + // MoveVM maps `UNKNOWN_VERIFICATION_ERROR` to `VERIFICATION_ERROR`. assert_eq!(err.major_status(), StatusCode::VERIFICATION_ERROR); } else { assert_eq!(err.major_status(), *error_code); diff --git a/third_party/move/move-vm/runtime/src/loader/mod.rs b/third_party/move/move-vm/runtime/src/loader/mod.rs index 4b74cc881dac6..1777fd5ac9b9b 100644 --- a/third_party/move/move-vm/runtime/src/loader/mod.rs +++ b/third_party/move/move-vm/runtime/src/loader/mod.rs @@ -1303,8 +1303,7 @@ impl<'a> Resolver<'a> { function_name, ) .map_err(|_| { - // TODO(loader_v2): - // Loader V1 implementation uses this error, but it should be a linker error. + // Note: legacy loader implementation used this error, so we need to remap. PartialVMError::new(StatusCode::FUNCTION_RESOLUTION_FAILURE).with_message( format!( "Module or function do not exist for {}::{}::{}", diff --git a/third_party/move/move-vm/runtime/src/storage/loader.rs b/third_party/move/move-vm/runtime/src/storage/loader.rs index 3c484c7f74549..539a439c0c4f5 100644 --- a/third_party/move/move-vm/runtime/src/storage/loader.rs +++ b/third_party/move/move-vm/runtime/src/storage/loader.rs @@ -139,14 +139,11 @@ impl LoaderV2 { .iter() .map(|ty_tag| self.load_ty(code_storage, ty_tag)) .collect::>>() - // TODO(loader_v2): - // Loader V1 implementation returns undefined here, causing some tests to fail. We - // should probably map this to script. - .map_err(|e| e.finish(Location::Undefined))?; + .map_err(|err| err.finish(Location::Script))?; let main = script.entry_point(); Type::verify_ty_arg_abilities(main.ty_param_abilities(), &ty_args) - .map_err(|e| e.finish(Location::Script))?; + .map_err(|err| err.finish(Location::Script))?; Ok(LoadedFunction { owner: LoadedFunctionOwner::Script(script), @@ -205,7 +202,7 @@ impl LoaderV2 { ) -> PartialVMResult> { let module = self .load_module(module_storage, address, module_name) - .map_err(|e| e.to_partial())?; + .map_err(|err| err.to_partial())?; Ok(module .struct_map .get(struct_name) @@ -239,9 +236,9 @@ impl LoaderV2 { st.module.as_ident_str(), st.name.as_ident_str(), ) - .map_err(|e| e.finish(Location::Undefined)) + .map_err(|err| err.finish(Location::Undefined)) }) - .map_err(|e| e.to_partial()) + .map_err(|err| err.to_partial()) } } diff --git a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.exp b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.exp index 8bfde526b7e76..1ee2d970eb9f3 100644 --- a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.exp +++ b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.exp @@ -4,7 +4,7 @@ task 1 'run'. lines 6-10: Error: Script execution failed with VMError: { major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH, sub_status: None, - location: undefined, + location: script, indices: [], offsets: [], } diff --git a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.v2_exp b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.v2_exp index 8bfde526b7e76..1ee2d970eb9f3 100644 --- a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.v2_exp +++ b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.v2_exp @@ -4,7 +4,7 @@ task 1 'run'. lines 6-10: Error: Script execution failed with VMError: { major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH, sub_status: None, - location: undefined, + location: script, indices: [], offsets: [], } diff --git a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.exp b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.exp index 8bfde526b7e76..1ee2d970eb9f3 100644 --- a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.exp +++ b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.exp @@ -4,7 +4,7 @@ task 1 'run'. lines 6-10: Error: Script execution failed with VMError: { major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH, sub_status: None, - location: undefined, + location: script, indices: [], offsets: [], } diff --git a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.v2_exp b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.v2_exp index 8bfde526b7e76..1ee2d970eb9f3 100644 --- a/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.v2_exp +++ b/third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.v2_exp @@ -4,7 +4,7 @@ task 1 'run'. lines 6-10: Error: Script execution failed with VMError: { major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH, sub_status: None, - location: undefined, + location: script, indices: [], offsets: [], } diff --git a/third_party/move/move-vm/types/src/code/errors.rs b/third_party/move/move-vm/types/src/code/errors.rs index c0ad3879111f9..6cd0ae508b4e9 100644 --- a/third_party/move/move-vm/types/src/code/errors.rs +++ b/third_party/move/move-vm/types/src/code/errors.rs @@ -26,9 +26,9 @@ macro_rules! module_storage_error { }; } -// TODO(loader_v2): -// The error message is formatted in the same way as V1, to ensure that replay and tests work in -// the same way, but ideally we should use proper formatting here. +// Note: +// The error message is formatted in the same way as by the legacy loader implementation, to +// ensure that replay and tests work in the same way. #[macro_export] macro_rules! module_linker_error { ($addr:expr, $name:expr) => {