Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387

brmataptos · 2024-11-25T04:54:21Z

Description

This PR started with a cherry-pick commit of Wolfgang's draft PR 15171 = commit #d85b919.

On top of that, I

Remove the Mask feature since it complicates review,
Extend and rename the opcodes to make them more general and clear
- Any function value can have arguments bound to it in advance,
  not requiring a defined function handle except in LdFunction[Generic].
Tried to avoid unnecessary changes, but may have accidentally done so.
- It took me a while to realize that we do indeed need a type parameter to
  EarlyBind and Invoke because of the way the verifier works today.
add some semantics comment to file_format.rs which I'd worked out previously.

The current operations are:
- LdFunction, LdFunctionGeneric = generate a closure from a defined function
- EarlyBind = bind more arguments to any closure
- Invoke = call a closure with final arguments

As noted by Wolfgang in his commit, we are still lacking:

function value representation and interpreter
function value serialization and deserialization
some connections with front-end

How Has This Been Tested?

Added tests for feature gating. Should be safe to merge.

Key Areas to Review

Did I lose any verifier support when modfiying Wolfgang's opcodes?

trunk-io · 2024-11-25T04:54:25Z

⏱️ 1h 30m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟥
rust-move-tests	12m	🟥
rust-cargo-deny	10m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	9m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	4m	🟥
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	21s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	16s	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-branch-prefix	1s	🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
check-dynamic-deps	5m	1m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

brmataptos · 2024-11-25T04:54:32Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

third_party/move/move-binary-format/src/file_format_common.rs

third_party/move/move-vm/test-utils/src/gas_schedule.rs

runtian-zhou

I will need some more time to double check the bytecode verification logics.

runtian-zhou · 2024-11-25T22:04:08Z

third_party/move/move-binary-format/src/file_format.rs

+            assert ty == locals[#args -  i - 1]
+    "#]
+    #[gas_type_creation_tier_1 = "closure_ty"]
+    Invoke(SignatureIndex),


I would rename it to CallFunctionPointer just to align with the Call instruction we had previously?

I don't think that is useful, as it is not really a pointer. I'd be tempted to rename it Apply as a nod to Lisp's nomenclature, but I don't think it really matters here. Does Invoke confuse you?

Leaving it.

runtian-zhou · 2024-12-02T13:17:14Z

third_party/move/move-binary-format/src/file_format.rs

@@ -1254,6 +1261,8 @@ pub enum SignatureToken {
    Signer,
    /// Vector
    Vector(Box<SignatureToken>),
+    /// Function, with n argument types and m result types, and an associated ability set.
+    Function(Vec<SignatureToken>, Vec<SignatureToken>, AbilitySet),


I think it's better to use named struct instead of tuple here.

Also for the AbilitySet, I suppose we will need some subtyping rules here in the type checking phase?

Changing to a struct causes a lot of little changes, hope other reviewers don't mind all the changed code.

Already have it in functions invoke_function and early_bind_function in move-bytecode-verifier/src/type_safety.rs. Do we need more?

Using a struct-style enum variant here causes those mysterious Clippy problems. I think it's related to one of the derive macros on SignatureToken declaration (probably dearbitrary::Dearbitrary) lacking handling of struct-style enum variant. In the interest of moving faster, I'm going back to the tuple-style enum variant instead, rather than trying to fix that 3d-party code. :-)

runtian-zhou · 2024-12-02T13:21:15Z

third_party/move/move-binary-format/src/serializer.rs

@@ -800,6 +800,9 @@ fn serialize_signature_token_single_node_impl(
            binary.push(SerializedType::TYPE_PARAMETER as u8)?;
            serialize_type_parameter_index(binary, *idx)?;
        },
+        SignatureToken::Function(..) => {
+            unimplemented!("serialization of function types")


Are you planning to fix this part for this PR?

Coming in a later PR.

runtian-zhou · 2024-12-02T13:25:02Z

third_party/move/move-binary-format/src/file_format.rs

+            assert ty == locals[#args -  i - 1]
+    "#]
+    #[gas_type_creation_tier_1 = "closure_ty"]
+    InvokeFunction(SignatureIndex),


With the instructions proposed, we will require closures to have their type arguments being fully provided at LdFunction time, not at invocation time. Is this an intentional design? Asking this because the current dynamic dispatch code (for dispatchable fungible asset) actually performs type binding at call time.

Why? LdFunction does not need to load until use? We store idx/name in a function value, and only when we call Invoke we resolve that index/name and load the function?

The SignatureIndex here is a static type, and is needed for verifier to allow the stack depth checker to know how many parameters/results are being popped/pushed on the stack, since the stack depth analysis is independent of the type analysis in the verifier (if these 2 analyses were combined, then this need not be included in the instruction, as it should be easy to derive from context; I really don't know why there are bunch of independent passes in the verifier).

Execution will have to look at the function value (1) to get the early-bound parameters to pass, and (2) to get the FunctionHandle/FunctionInstantiation to (a) get its type to figure out the number and type of parameters needed to pop off the stack for this call, and (b) determine which function is being called.

The dynamic types will have to be checked at some point, either when the value is created (LdFunction/EarlyBind/Deserialization) or called (InvokeFunction). If we think it will be called at least once, then earlier type-checking would make sense, but if pulling the function data into cache is the slow part of type checking, we might want to do it at Call time.

Marking this replied for bookkeeping.

runtian-zhou · 2024-12-02T13:30:36Z

third_party/move/move-binary-format/src/constant.rs

@@ -17,6 +17,10 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> {
        SignatureToken::U128 => Some(MoveTypeLayout::U128),
        SignatureToken::U256 => Some(MoveTypeLayout::U256),
        SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))),
+        SignatureToken::Function(..) => {
+            // TODO(LAMBDA): do we need representation in MoveTypeLayout?


I don't think we need it here? It's a good question however. MoveTypeLayout is used for ser/de in the VM so it would be dependent on our implementation there?

We do not need it here (function values cannot be constants), but we do need it otherwise. I have a PR adding it: #15442

It's possible that we could allow function values as constants in the future. But yeah, maybe not worth it now.

Well, for structs it is also possible but we did not do it yet, so None is a way to go :)

runtian-zhou · 2024-12-02T13:39:27Z

third_party/move/move-binary-format/src/file_format.rs

+        func param types match provided parameter types
+    "#]
+    #[gas_type_creation_tier_0 = "closure_ty"]
+    EarlyBindFunction(SignatureIndex, u8),


Would it make more sense to bind by mask? i.e: EarlyBindFunction will only bind one argument at a time and the u8 will be used to specify which argument to bind. This would allow more flexibility?

Any function generated for the body of an lambda expression can order free variables before lambda parameters so that it is natural to early-bind the k first parameters. If the user wants to use an existing function with parameters in the wrong order, the user can add a wrapper function as necessary.

The mask idea and the code Wolfgang and I had written to deal with more arbitrary permutations of early/late-bound parameters was very "clever" and therefore hard to review and really an unnecessary security risk.

replied already

runtian-zhou · 2024-12-02T13:41:57Z

third_party/move/move-binary-format/src/file_format.rs

+        ..
+        stack >> arg_k
+
+        old_conservative_invocation_mode = conservative_invocation_mode


What is this invocation mode?

For LAMBDA MVP, we are playing it very safe and disallowing any call to module m if there is any dynamic call on the stack. This is enforced by a VM state flag conservative_invocation_mode, which is set if we are in a dynamic call.

This is because dynamic calls violate the static dependency guarantees. After we have the MVP implemented we will see how far we can prove dynamic safety, e.g., using access specifiers.

There was an error here in that the dynamic call also needs to always check that its module is not already on the stack. Fixed.

runtian-zhou · 2024-12-02T13:46:48Z

third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs

+        // TODO(LAMBDA) In the future this may change.
+        let function_handle = self.module.function_handle_at(fh_idx);
+        let function_acquired_resources = self.function_acquired_resources(function_handle, fh_idx);
+        if !function_acquired_resources.is_empty() {


I don't think this is a necessary change nor this is safe. Acquire list is only a local concept for now. Imagine a public move function that has acquire list, you can easily create another move module that invokes this function. In that module, the acquire list would be empty. In this sense you would be able to bypass the constraints here easily.

We disallow both reentrancy and nonempty acquires lists for now, so it's definitely safe, if a bit limited. Your foreign move module can't call back into this one, since you're already on the stack.

In the future we can add more general access_specifiers to functions and function types, and check that the dynamic call doesn't exceed the accesses of the caller. I'd rather work that out first, but people are anxious about an MVP deadline.

I was being overly cautious here. You're right, as long as we disallow all dynamic reentrancy we can ignore the acquires list of the function here.

Let's discuss post-MVP approaches to loosen constraints later.

runtian-zhou · 2024-12-02T13:47:43Z

third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs

+        Ok(())
+    }
+
+    fn invoke_acquire(&mut self, _offset: CodeOffset) -> PartialVMResult<()> {


The strategy I was taking previously is indeed making invoke instruction to be a nil-op but check re-entrancy at runime.

Not sure what you mean by "nil-op" here. Did you mean what I have done here?

georgemitenkov · 2024-12-02T09:02:08Z

third_party/move/move-bytecode-verifier/src/dependencies.rs

+            compare_cross_module_signatures(context, args1, args2, def_module)?;
+            compare_cross_module_signatures(context, result1, result2, def_module)?;
+            if ab1 != ab2 {
+                Err(PartialVMError::new(StatusCode::TYPE_MISMATCH))


Can we add an error message here? i.e., PartialVMError::new(StatusCode::TYPE_MISMATCH).with_message(...)

Done, although:

Do you realize that most type mismatches will show up below, which has no message. Printing the types might be interesting, if we had Display for SignatureToken.

Well, we can use debug and format via "{:?}"? Any extra information is better than a status code. Imagine you have debug a type mismatch - there are so many places it can come from, it is difficult to bisect, error messages allows us to pinpoint the location (if they are propagated correctly 😄)

Below is some old code I do not know who wrote, but I would love to see error messages as well there.

third_party/move/move-bytecode-verifier/src/dependencies.rs

third_party/move/move-binary-format/src/proptest_types/types.rs

georgemitenkov · 2024-12-02T10:01:55Z

third_party/move/move-binary-format/src/file_format.rs

            SignatureToken::MutableReference(boxed) => write!(f, "MutableReference({:?})", boxed),
            SignatureToken::TypeParameter(idx) => write!(f, "TypeParameter({:?})", idx),
        }
    }
 }

 impl SignatureToken {
-    /// Returns true if the token is an integer type.
+    // Returns `true` if the `SignatureToken` is an integer type.


Why this change? Bad rebase?

Not sure what happened to the /// but the backquotes make sense here. Not going to rewrite the whole file, though.

I would prefer to have less changed lines in diff, but up to you

georgemitenkov · 2024-12-02T10:03:13Z

third_party/move/move-binary-format/src/file_format.rs

@@ -1456,6 +1482,13 @@ impl SignatureToken {
        }
    }

+    /// Returns true if the `SignatureToken` is a function.


nit: personally, I am not a fan of "", if the type name changes, comment can easily be outdated. if the tokenorif the [SignatureToken]` can be better?

The comment style I used matches the rest of the file (see lines 1363, 1428, 1446, 1453, 1460, 1467). If we want to change this, we should do them all. Changing just 1 detracts from readability.

Suggest an off-line rustdoc style discussion and global style fix. But not in this PR.

I disagree: off-line discussions will lead to nothing because no one will do the scan across all aptos-core to fix wrong/outdated comments or their style. We should fix these things incrementally as we touch the code.

For this specific issue, I am fine with keeping `` for now. But this is bad in my opinion, and leads to things like:

/// `InterpreterImpl` instances can execute Move functions. /// /// An `Interpreter` instance is a stand alone execution context for a function. /// It mimics execution on a single thread, with an call stack and an operand stack. pub(crate) struct InterpreterImpl { ...

Anyone who cares can do the scan and fix. It's much easier to review a style cleanup as an independent PR.

georgemitenkov · 2024-12-02T17:06:33Z

third_party/move/move-binary-format/src/constant.rs

@@ -17,6 +17,10 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> {
        SignatureToken::U128 => Some(MoveTypeLayout::U128),
        SignatureToken::U256 => Some(MoveTypeLayout::U256),
        SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))),
+        SignatureToken::Function(..) => {
+            // TODO(LAMBDA): do we need representation in MoveTypeLayout?


We do not need it here (function values cannot be constants), but we do need it otherwise. I have a PR adding it: #15442

brmataptos

Addressed comments, thanks.

third_party/move/move-bytecode-verifier/src/dependencies.rs

brmataptos · 2024-12-04T04:29:15Z

third_party/move/move-bytecode-verifier/src/dependencies.rs

+            compare_cross_module_signatures(context, args1, args2, def_module)?;
+            compare_cross_module_signatures(context, result1, result2, def_module)?;
+            if ab1 != ab2 {
+                Err(PartialVMError::new(StatusCode::TYPE_MISMATCH))


Done, although:

Do you realize that most type mismatches will show up below, which has no message. Printing the types might be interesting, if we had Display for SignatureToken.

brmataptos · 2024-12-04T04:29:57Z

third_party/move/move-binary-format/src/file_format.rs

+            assert ty == locals[#args -  i - 1]
+    "#]
+    #[gas_type_creation_tier_1 = "closure_ty"]
+    Invoke(SignatureIndex),


Leaving it.

third_party/move/move-binary-format/src/proptest_types/types.rs

brmataptos · 2024-12-04T04:33:34Z

third_party/move/move-binary-format/src/file_format.rs

            SignatureToken::MutableReference(boxed) => write!(f, "MutableReference({:?})", boxed),
            SignatureToken::TypeParameter(idx) => write!(f, "TypeParameter({:?})", idx),
        }
    }
 }

 impl SignatureToken {
-    /// Returns true if the token is an integer type.
+    // Returns `true` if the `SignatureToken` is an integer type.


Not sure what happened to the /// but the backquotes make sense here. Not going to rewrite the whole file, though.

brmataptos · 2024-12-04T04:59:55Z

third_party/move/move-binary-format/src/constant.rs

@@ -17,6 +17,10 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> {
        SignatureToken::U128 => Some(MoveTypeLayout::U128),
        SignatureToken::U256 => Some(MoveTypeLayout::U256),
        SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))),
+        SignatureToken::Function(..) => {
+            // TODO(LAMBDA): do we need representation in MoveTypeLayout?


It's possible that we could allow function values as constants in the future. But yeah, maybe not worth it now.

brmataptos · 2024-12-04T05:00:19Z

third_party/move/move-binary-format/src/file_format.rs

+        func param types match provided parameter types
+    "#]
+    #[gas_type_creation_tier_0 = "closure_ty"]
+    EarlyBindFunction(SignatureIndex, u8),


replied already

brmataptos · 2024-12-04T05:02:54Z

third_party/move/move-binary-format/src/file_format.rs

+        ..
+        stack >> arg_k
+
+        old_conservative_invocation_mode = conservative_invocation_mode


For LAMBDA MVP, we are playing it very safe and disallowing any call to module m if there is any dynamic call on the stack. This is enforced by a VM state flag conservative_invocation_mode, which is set if we are in a dynamic call.

This is because dynamic calls violate the static dependency guarantees. After we have the MVP implemented we will see how far we can prove dynamic safety, e.g., using access specifiers.

There was an error here in that the dynamic call also needs to always check that its module is not already on the stack. Fixed.

brmataptos · 2024-12-04T05:27:17Z

third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs

+        // TODO(LAMBDA) In the future this may change.
+        let function_handle = self.module.function_handle_at(fh_idx);
+        let function_acquired_resources = self.function_acquired_resources(function_handle, fh_idx);
+        if !function_acquired_resources.is_empty() {


I was being overly cautious here. You're right, as long as we disallow all dynamic reentrancy we can ignore the acquires list of the function here.

Let's discuss post-MVP approaches to loosen constraints later.

brmataptos · 2024-12-04T05:28:44Z

third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs

+        Ok(())
+    }
+
+    fn invoke_acquire(&mut self, _offset: CodeOffset) -> PartialVMResult<()> {


Not sure what you mean by "nil-op" here. Did you mean what I have done here?

brmataptos · 2024-12-06T03:47:21Z

@georgemitenkov I've added feature gating and tests to verify that it's being done. I see some is done in e2e-tests but I can't do it there because the compiler doesn't have complete codegen yet. I've added a test in third_party/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/feature_function-values_tests.rs which manually constructs the bytecode file. Fun!

PTAL.

[move-vm] Types and opcodes for closures This PR implements the extensions needed in the file format for representing closures: - The type (SignatureToken) has a new variant `Function(args, result, abilities)` to represent a function type - The opcodes are extendeed by operations `ClosPack`, `ClosPackGeneric`, and `ClosEval` This supports bit masks for specifyinng which arguments of a function are captured when creating a closure. Bytecode verification is extended to support the new types and opcodes. The implementation of consistency, type, and reference safety should be complete. What is missing are: - File format serialization - Interpreter and value representation - Value serialization - Connection of the new types with the various other type representations

…rser work: - `LdFunction`, `LdFunctionGeneric` = generate a closure from a defined function - `EarlyBind` = bind more arguments to any closure - `Invoke` = call a closure with final arguments Add some description/semantics to `file_format.rs` to better describe the implementation which will be needed. Get rid of complex Mask calculations in favor of simpler early bninding of `k` initial arguments. Hopefully keep all bytecode verifier operations working the same.

…eFunction based on George's suggestion

…simplify AcuiresVerifier::ld_function_acquire - comments cleanup, especially file_format semantics for new bytecodes

…ed by verifier. adding tests required serialization/desrialization of SignatureToken::Function.

…nce that yields Clippy warnings, apparently due to a hidden bug in one of the derived trait macros invoked here (dearbitrary)

georgemitenkov

Full pass. Do we have a way to test the verifier changes? I saw tests for feature verification, but I do not know where we test other passes.

georgemitenkov · 2024-12-09T15:46:01Z

third_party/move/move-binary-format/src/constant.rs

@@ -17,6 +17,10 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> {
        SignatureToken::U128 => Some(MoveTypeLayout::U128),
        SignatureToken::U256 => Some(MoveTypeLayout::U256),
        SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))),
+        SignatureToken::Function(..) => {


Can you return None here (one less TODO)? We do not expect function value constant any time soon. Let's also add a unit test for that?

georgemitenkov · 2024-12-09T15:53:16Z

third_party/move/move-binary-format/src/check_bounds.rs

+                | Reference(_)
+                | MutableReference(_)
+                | Vector(_)
+                | Function { .. } => (),


Should be Function(..)?

georgemitenkov · 2024-12-09T15:54:33Z

third_party/move/move-binary-format/src/check_bounds.rs

@@ -683,8 +685,20 @@ impl<'a> BoundsChecker<'a> {

        for ty in ty.preorder_traversal() {


can we rename this ty to tok? It is annoying that we have tags, tokens and types and sometimes even layouts all as types....

georgemitenkov · 2024-12-09T15:55:13Z

third_party/move/move-binary-format/src/check_complexity.rs

+                | Address
+                | Bool
+                | Vector(_)
+                | Function { .. }


Function(..)?

georgemitenkov · 2024-12-09T16:01:50Z

third_party/move/move-binary-format/src/file_format.rs

@@ -1010,6 +1010,13 @@ impl AbilitySet {
    pub fn into_u8(self) -> u8 {
        self.0
    }
+
+    pub fn to_string_concise(self) -> String {


I don't think this adds much value. Display implementation is the same, only adds 2 spaces. And having multiple conversions into string is just confusing.

I didn't want to change all existing outputs to look different and make review difficult. This is used for ability display just for functions.

georgemitenkov · 2024-12-09T19:43:35Z

third_party/move/move-bytecode-verifier/src/type_safety.rs

+    if &closure_ty != expected_ty {
+        return Err(verifier
+            .error(StatusCode::CALL_TYPE_MISMATCH_ERROR, offset)
+            .with_message("closure type mismatch".to_owned()));


Add debug formats of types "{:?}, expected_ty, ..." Otherwise the message is useless?

georgemitenkov · 2024-12-09T19:45:14Z

third_party/move/move-bytecode-verifier/src/type_safety.rs

+        safe_assert!(false);
+        unreachable!()
+    };
+    if !abilities.is_subset(closure_abilities) {


I am not sure this works? Before you do &closure_ty != expected_ty, but that compares abilities by equality as well? I think with match you can compare args/return types by equality, and check subset here.

georgemitenkov · 2024-12-09T19:51:50Z

third_party/move/move-bytecode-verifier/src/type_safety.rs

+        safe_assert!(false);
+        unreachable!()
+    };
+    if !abilities.is_subset(closure_abilities) {


This is duplicated with early bind, can we factor the above into a helper function?

georgemitenkov · 2024-12-09T19:54:09Z

third_party/move/move-bytecode-verifier/src/signature_v2.rs

+                                return map_err(Err(PartialVMError::new(
+                                    StatusCode::NUMBER_OF_ARGUMENTS_MISMATCH,
+                                )
+                                .with_message("in EarlyBindFunction".to_string())));


Can we make the message more detailed? What is received count, what is the expected, etc.

georgemitenkov · 2024-12-09T19:56:14Z

third_party/move/move-bytecode-verifier/src/signature_v2.rs

+                        checked_early_bind_insts.entry((*idx, *count))
+                    {
+                        // Note non-function case is checked in `verify_fun_sig_idx` above.
+                        if let Some(SignatureToken::Function(args, ..)) =


nit: you can use if matches!(Some(...) if condition) { .. }

georgemitenkov · 2024-12-09T20:00:39Z

@brmataptos tests are failing? I see these on CI:

2024-12-09T04:44:56.0568444Z thread 'garbage_inputs' panicked at third_party/move/move-binary-format/serializer-tests/tests/serializer_tests.rs:45:14:
2024-12-09T04:44:56.0569258Z deserialization should work: PartialVMError { major_status: UNKNOWN_OPCODE, sub_status: None, message: None, exec_state: None, indices: [], offsets: [] }

brmataptos mentioned this pull request Nov 25, 2024

[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

Merged

22 tasks

This was referenced Nov 25, 2024

[compiler-v2] add test cases for lambda #14791

Merged

[move-compiler-v2] add parser code for lambda types #14792

Merged

[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

Merged

brmataptos changed the title ~~Verbatim cherry-pick of Wolfgang's draft PR 15171 = commit #d85b919~~ Wolfgang's VM closure support + reconciliation with lambda simplifications (removing mask, clarifying ops) Nov 25, 2024

brmataptos changed the title ~~Wolfgang's VM closure support + reconciliation with lambda simplifications (removing mask, clarifying ops)~~ Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) Nov 25, 2024

brmataptos marked this pull request as ready for review November 25, 2024 05:07

brmataptos requested review from banool, gregnazario and 0xmaayan as code owners November 25, 2024 05:07

brmataptos requested review from georgemitenkov, runtian-zhou, vgao1996 and wrwg November 25, 2024 05:08

georgemitenkov reviewed Nov 25, 2024

View reviewed changes

third_party/move/move-binary-format/src/file_format_common.rs Outdated Show resolved Hide resolved

brmataptos mentioned this pull request Nov 25, 2024

[move-vm] Types and opcodes for closures #15171

Draft

16 tasks

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 42421fe to 92ae56c Compare November 27, 2024 10:24

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from ebb0fcc to 8154bb7 Compare November 27, 2024 10:24

brmataptos requested a review from georgemitenkov November 27, 2024 17:49

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 92ae56c to 3c02ba1 Compare November 27, 2024 22:41

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from d52d75a to f49f228 Compare November 27, 2024 22:42

rahxephon89 reviewed Nov 30, 2024

View reviewed changes

third_party/move/move-vm/test-utils/src/gas_schedule.rs Show resolved Hide resolved

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 3c02ba1 to ae4444b Compare December 1, 2024 05:57

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from f49f228 to ab7e327 Compare December 1, 2024 05:57

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from ae4444b to 8b37927 Compare December 1, 2024 06:46

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from ab7e327 to 84b10f6 Compare December 1, 2024 06:46

runtian-zhou reviewed Dec 2, 2024

View reviewed changes

georgemitenkov reviewed Dec 2, 2024

View reviewed changes

brmataptos removed request for banool and 0xmaayan December 4, 2024 04:08

brmataptos commented Dec 4, 2024

View reviewed changes

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 02fb9ec to f2281b4 Compare December 4, 2024 07:23

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from 1d8a057 to d382ca0 Compare December 4, 2024 07:23

brmataptos requested a review from rahxephon89 December 4, 2024 07:25

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from d382ca0 to 2fa8bde Compare December 4, 2024 07:54

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from d329f76 to 2734528 Compare December 4, 2024 22:49

brmataptos requested a review from areshand as a code owner December 4, 2024 22:49

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from 2fa8bde to d7ded7a Compare December 4, 2024 22:50

Base automatically changed from 11-21-lambda-compiler-completion to main December 4, 2024 23:23

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from d7ded7a to 844871b Compare December 6, 2024 03:39

brmataptos requested review from davidiw and movekevin as code owners December 6, 2024 03:39

wrwg and others added 6 commits December 7, 2024 14:12

rename EarlyBind and Invoke operations to EarlyBindFunction and Invok…

bfc73ac

…eFunction based on George's suggestion

Address review comments: - use struct for SignatureToken::Function - …

d3cd457

…simplify AcuiresVerifier::ld_function_acquire - comments cleanup, especially file_format semantics for new bytecodes

add ENABLE_FUNCTION_VALUES feature flag and tests to show it is block…

fd9e1e9

…ed by verifier. adding tests required serialization/desrialization of SignatureToken::Function.

don't use named fields for enum SignatureVariant variant Function, si…

e8107d7

…nce that yields Clippy warnings, apparently due to a hidden bug in one of the derived trait macros invoked here (dearbitrary)

brmataptos force-pushed the 11-23-vm_changes_merged_in branch from fdd0ca9 to e8107d7 Compare December 7, 2024 22:13

fix clippy warning in last commit

be210b7

brmataptos removed request for areshand, davidiw and movekevin December 7, 2024 23:18

brmataptos added 2 commits December 8, 2024 20:13

add feature flag which was somehow dropped

76b3042

fix more lints again

4f10a35

georgemitenkov reviewed Dec 9, 2024

View reviewed changes

		@@ -683,8 +685,20 @@ impl<'a> BoundsChecker<'a> {

		for ty in ty.preorder_traversal() {

Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387

Are you sure you want to change the base?

Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387

Conversation

brmataptos commented Nov 25, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

trunk-io bot commented Nov 25, 2024 • edited Loading

brmataptos commented Nov 25, 2024 • edited Loading

runtian-zhou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgemitenkov Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgemitenkov Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgemitenkov Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos commented Dec 6, 2024

georgemitenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgemitenkov commented Dec 9, 2024

brmataptos commented Nov 25, 2024 •

edited

Loading

trunk-io bot commented Nov 25, 2024 •

edited

Loading

brmataptos commented Nov 25, 2024 •

edited

Loading

georgemitenkov Dec 2, 2024 •

edited

Loading

georgemitenkov Dec 4, 2024 •

edited

Loading

georgemitenkov Dec 2, 2024 •

edited

Loading