[compiler-v2] Enum types in the VM #13812

wrwg · 2024-06-25T15:46:59Z

Description

In #13725 support for enums was added to the stackless bytecode IR. This PR extends the Move VM's representation of bytecode ('file format') by struct variants. It implements

representation in file format
serialization/deserialization of file format
bytecode verifier
code generation in compiler v2
interpreter inclusive of paranoid mode

Compatibility checking, as well as certain other minor features (as marked by #13806 in the code), are not yet implemented by this PR.

On Move bytecode level, there are 5 * 2 new instructions (each instruction has a dual generic version):

TestVariant(StructVariantHandleIndex)
	and TestVariantGeneric(StructVariantInstantiationIndex)
PackVariant(StructVariantHandleIndex)
	and PackVariantGeneric(StructVariantInstantiationIndex)
UnpackVariant(StructVariantHandleIndex)
	and UnpackVariantGeneric(StructVariantInstantiationIndex)
ImmBorrowVariantField(VariantFieldHandleIndex)
	and ImmBorrowVariantFieldGeneric(VariantFieldInstantiationIndex)
MutBorrowVariantField(VariantFieldHandleIndex)
	and MutBorrowVariantFieldGeneric(VariantFieldInstantiationIndex)

For the indices used in those instructions, 4 new tables have been added to the file format holding the associated data.

There is a lot of boilerplate code to support the new instructions and tables. Some refactoring of existing code has been done to avoid too much copy and paste, specifically in the serializers and in the bytecode verifier.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Apart of passing existing tests, there is a new test in move-compiler-v2/tests/file-format-generator/struct_variants.move which shows the disassembeled output of various match expressions.

There are also new e2e transactional tests in move-compiler-v2/transactional-tests/tests/enun. To add negative tests for the bytecode verifier and serializers, we first need a better way to build code with injected tests, see also #13871

trunk-io · 2024-06-25T15:47:04Z

⏱️ 5h 31m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
test-fuzzers	2h 26m	🟩 🟩 🟩 🟩
rust-targeted-unit-tests	26m	🟥
forge-compat-test / forge	19m	🟥 🟥
rust-move-tests	16m	⬜ 🟩
rust-move-unit-coverage	14m	⬜ 🟩
run-tests-main-branch	14m	🟩 🟩 🟩
rust-images / rust-all	13m	🟩
rust-move-tests	13m	🟩
rust-move-unit-coverage	11m	🟩
rust-lints	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	9m	🟩
rust-move-unit-coverage	8m	🟩
cli-e2e-tests / run-cli-tests	7m	🟩
general-lints	5m	🟩 🟩 🟩
test-target-determinator	3m	🟩
check-dynamic-deps	3m	🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	42s	🟩 🟩 🟩 🟩
permission-check	20s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	18s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	15s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	15s	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	13s	🟩
determine-docker-build-metadata	1s	🟩

🚨 4 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
rust-targeted-unit-tests	26m	14m
rust-move-tests	16m	9m
rust-lints	10m	6m
rust-move-unit-coverage	14m	12m

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

vgao1996 · 2024-06-25T20:03:13Z

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

+            Bytecode::PackVariant(a) => write!(f, "PackVariant({})", a),
+            Bytecode::TestVariant(a) => write!(f, "TestVariant({})", a),
+            Bytecode::PackVariantGeneric(a) => write!(f, "PackVariantGeneric({})", a),
+            Bytecode::TestVariantGeneric(a) => write!(f, "TestVariantGeneric({})", a),


I highly doubt if it is still a good idea to have generic and non-generic versions of the same instructions.

Back in the old days, we split some instructions into generic & non-generic versions, hoping that this would make the bytecode more optimized for the VM to execute. However now it's clear this point is totally moot.

There is a strong need for the bytecode format to be a stable, backward compatible storage format, which conflicts with being optimized for execution

A more suitable solution to this problem is to introduce some VM-internal microcodes that are designed to be unstable, adapting to VM changes to the VM's execution engine. Notice that this is exactly how modern CPUs architectures work.

Because of the two versions, we ended up having to duplicate a ton of code. This has been a maintenance nightmare and greatly increased the chance we screw something up (e.g. making changes in only one version)

This PR is no redesigning the bytecode architecture, but following along what we have. One principle of the current design is that almost every instruction has exactly one argument, therefore fits into three bytes in most cases (one for the bytecode, one for the argument, one terminator). This is not for efficiency of execution but for the size of the bytecode. (You are right that for performance of execution it makes little sense.) The entire way how the file format is designed is for this.

If we want to change this, we can't do it just for two out of dozens instructions. The resulting design would look very, very odd. So we have BorrowField and BorrowFieldGeneric, and symmetrically, we have BorrowVariantField and BorrowVariantFieldGeneric. I don't think that any other approach leads to a satisfactory design.

I see your point. Agreed that we could try to come up with a more appropriate solution to this problem later.

codecov · 2024-06-26T17:26:30Z

Codecov Report

Attention: Patch coverage is 76.01822% with 895 lines in your changes missing coverage. Please review.

Project coverage is 59.1%. Comparing base (6e394fd) to head (6c84279).
Report is 5 commits behind head on main.

Files	Patch %	Lines
...move-binary-format/src/proptest_types/functions.rs	0.0%	235 Missing ⚠️
...party/move/move-bytecode-verifier/src/signature.rs	0.0%	74 Missing ⚠️
...y/move/tools/move-disassembler/src/disassembler.rs	84.7%	45 Missing ⚠️
...hird_party/move/move-vm/runtime/src/interpreter.rs	85.8%	44 Missing ⚠️
...ty/move/move-binary-format/src/check_complexity.rs	0.0%	43 Missing ⚠️
third_party/move/move-core/types/src/value.rs	0.0%	43 Missing ⚠️
..._party/move/move-binary-format/src/deserializer.rs	88.2%	38 Missing ⚠️
...d_party/move/move-binary-format/src/file_format.rs	48.4%	34 Missing ⚠️
...ytecode-verifier/src/ability_field_requirements.rs	0.0%	33 Missing ⚠️
third_party/move/move-vm/runtime/src/loader/mod.rs	78.8%	33 Missing ⚠️
... and 29 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #13812       +/-   ##
===========================================
- Coverage    70.8%    59.1%    -11.8%     
===========================================
  Files        2332      824     -1508     
  Lines      462140   200490   -261650     
===========================================
- Hits       327540   118635   -208905     
+ Misses     134600    81855    -52745

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brmataptos

A few real problems: Redundant rep in file_format could expand space as well as being a potential attack vector. Somewhere I found an inconsistency in one of your boilerplate patterns. Others are minor.

brmataptos · 2024-06-30T19:23:29Z

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

@@ -449,6 +594,13 @@ impl<'a> BoundsChecker<'a> {
                        )?;
                    }
                },
+                PackVariantGeneric(idx) | UnpackVariantGeneric(idx) | TestVariantGeneric(idx) => {


The previous clause (PackGeneric(idx)|...) does a check of type parameters. Don't you need something similar here?

Good catch, done.

brmataptos · 2024-06-30T19:26:31Z

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

 pub const STRUCT_DEF_INST_INDEX_MAX: u64 = TABLE_INDEX_MAX;
 pub const CONSTANT_INDEX_MAX: u64 = TABLE_INDEX_MAX;
+pub const STRUCT_VARIANT_HANDLE_INDEX_MAX: u64 = TABLE_INDEX_MAX;
+pub const STRUCT_VARIANT_INST_INDEX_MAX: u64 = TABLE_INDEX_MAX;


It would be nice if we had a canonical ordering for these declarations, so it would be easier to see if one was missing. But I guess that's not easy, unless we just go alphabetical.

brmataptos · 2024-06-30T19:29:26Z

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

@@ -305,6 +305,11 @@ impl Struct {
            StructFieldInformation::Declared(fields) => {
                fields.iter().map(|f| Field::new(m, f)).collect()
            },
+            StructFieldInformation::DeclaredVariants(..) => {
+                // TODO(#13806): consider implementing (probably not as long we do not


I think the editorial comment ("probably not as long as we do not know whether normalized.rs will be retired") should go in the Issue for more discussion, not the comment. This comment may live forever. And maybe you want a more specific issue here.

The issue is a cluster tracking bug, the idea is we search the code for TODO(#nnn). This method is common at Google and Meta which have good web code search, works also well in my IDE.

I've also seen panic!("TODO(#13806): blah blah") which i did like :)
no action needed here - just a comment

brmataptos · 2024-06-30T19:49:42Z

third_party/move/move-compiler/src/interface_generator.rs

@@ -201,6 +201,11 @@ fn write_struct_def(ctx: &mut Context, sdef: &StructDefinition) -> String {
            return out;
        },
        StructFieldInformation::Declared(fields) => fields,
+        StructFieldInformation::DeclaredVariants(..) => {
+            // TODO(#13806): consider implement if interface generator will be


Don't we need an interface generator to build against on-chain dependences which are only available as bytecode? Better if these issues can be written somewhere (like dedicated issues) for more discussion. But I guess they'll all be removed by the time you're done.

As said the tracking bug is a better way to deal with this at this stage, IMO. The code is always better than a secondary artifact (learned this at Google).

At this point we do not even know whether this code is used anywhere in a serious manner. As discussed in the design review, API folks will look at how they adapt once the feature is ready here. I think their API generator directly works on file format representation, and not compiler v1 stuff.

brmataptos · 2024-06-30T20:12:43Z

third_party/move/move-vm/integration-tests/src/tests/instantiation_tests.rs

@@ -101,6 +101,10 @@ fn instantiation_err() {
                ],
            }),
        }],
+        struct_variant_handles: vec![],


I'm not sure what the deal is with move-vm/integration-tests/, but it seems worthwhile to have some demo variant here eventually, so add a comment to add one.

brmataptos · 2024-06-30T20:12:56Z

third_party/move/move-vm/integration-tests/src/tests/vm_arguments_tests.rs

@@ -215,6 +215,10 @@ fn make_module_with_function(
                code: vec![Bytecode::LdU64(0), Bytecode::Abort],
            }),
        }],
+        struct_variant_handles: vec![],


I'm not sure what the deal is with move-vm/integration-tests/, but it seems worthwhile to have some demo variant here eventually, so add a comment to add one.

brmataptos · 2024-06-30T20:13:59Z

third_party/move/move-model/src/model.rs

+        variant: Option<Symbol>,
+        offset: usize,
+    ) -> FieldEnv<'env> {
+        // TODO: speed this up via a cache RefCell<BTreeMap<(variant, offset), FieldId>>


Related issue?

It is not worth an issue, so I removed the TODO. and just left as a comment that this can be sped up. It's a linear search over less then a dozen elements in average, so its OK for the model.

brmataptos · 2024-06-30T20:16:39Z

third_party/move/move-ir-compiler/move-ir-to-bytecode/src/compiler.rs

@@ -485,6 +485,10 @@ pub fn compile_module<'a>(
        metadata: vec![],
        struct_defs,
        function_defs,
+        struct_variant_handles: vec![],


Add a TODO and point to an issue "If we are keeping the move-ir-compiler we need to do something here".

Actually, I see you have a lot of changes to move-bytecode-source-map. Why there and not here?

The source map is used all over our products (its also stored on chain), but Move IR is nowhere used anymore besides some legay tests. I'm not sure whether we want to keep it. Added a TODO linking to the tracking bug.

brmataptos · 2024-06-30T20:23:02Z

third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs

@@ -402,6 +406,19 @@ impl AbstractState {
        Ok(AbstractValue::NonReference)
    }

+    pub fn test_variant(


What is this function testing? Add a comment line, please.

You are right, this function is not needed. It behaves like read_ref (function before, also not documented), but we better call just directly read_ref, because that is the abstract semantics of testing a variant (the reference is used to read access the tag in the variant data)

brmataptos · 2024-06-30T21:42:08Z

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

+    pub owner: StructDefinitionIndex,
+    /// The sequence of variants which share the field at the given
+    /// field offset.
+    pub variants: Vec<VariantCount>,


This variants field is redundant with the VariantDefinition field fields, as it should be the inverse. If you keep both, you need a check for that on all input paths. (Maybe you do have, this PR is too big to tell). OTOH, if you get rid of this one, you can reconstruct it at deserialization and reduce the file size a bit.

Currently, the operation is more targeted. It can express selecting a field from just one given variant or from a set of variants. For example, if used in a match expression context, the list would contain only the variant we have tested for.

It is possible we relax this and just allow it where ever possible based on the StructDef metadata. Lets consider this an option for a later refinement when we understand all the relevant use cases on source level.

@wrwg So this means that we can have multiple VariantFieldHandle with the same owner? Just confirming

Yes, we can if the variant count/index sets are different. I'm open to change this, as said, after we land the PR.

wrwg

Thanks for the review @brmataptos . Please check my answer in the comments about the redundancy problem. I agree this can be a problem.

wrwg · 2024-07-02T03:05:02Z

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

@@ -449,6 +594,13 @@ impl<'a> BoundsChecker<'a> {
                        )?;
                    }
                },
+                PackVariantGeneric(idx) | UnpackVariantGeneric(idx) | TestVariantGeneric(idx) => {


Good catch, done.

wrwg · 2024-07-02T03:20:50Z

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

+    pub owner: StructDefinitionIndex,
+    /// The sequence of variants which share the field at the given
+    /// field offset.
+    pub variants: Vec<VariantCount>,


Currently, the operation is more targeted. It can express selecting a field from just one given variant or from a set of variants. For example, if used in a match expression context, the list would contain only the variant we have tested for.

It is possible we relax this and just allow it where ever possible based on the StructDef metadata. Lets consider this an option for a later refinement when we understand all the relevant use cases on source level.

wrwg · 2024-07-02T03:27:50Z

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

@@ -305,6 +305,11 @@ impl Struct {
            StructFieldInformation::Declared(fields) => {
                fields.iter().map(|f| Field::new(m, f)).collect()
            },
+            StructFieldInformation::DeclaredVariants(..) => {
+                // TODO(#13806): consider implementing (probably not as long we do not


The issue is a cluster tracking bug, the idea is we search the code for TODO(#nnn). This method is common at Google and Meta which have good web code search, works also well in my IDE.

wrwg · 2024-07-02T03:46:01Z

third_party/move/move-bytecode-verifier/src/reference_safety/abstract_state.rs

@@ -402,6 +406,19 @@ impl AbstractState {
        Ok(AbstractValue::NonReference)
    }

+    pub fn test_variant(


You are right, this function is not needed. It behaves like read_ref (function before, also not documented), but we better call just directly read_ref, because that is the abstract semantics of testing a variant (the reference is used to read access the tag in the variant data)

wrwg · 2024-07-02T04:23:03Z

third_party/move/move-compiler/src/interface_generator.rs

@@ -201,6 +201,11 @@ fn write_struct_def(ctx: &mut Context, sdef: &StructDefinition) -> String {
            return out;
        },
        StructFieldInformation::Declared(fields) => fields,
+        StructFieldInformation::DeclaredVariants(..) => {
+            // TODO(#13806): consider implement if interface generator will be


As said the tracking bug is a better way to deal with this at this stage, IMO. The code is always better than a secondary artifact (learned this at Google).

At this point we do not even know whether this code is used anywhere in a serious manner. As discussed in the design review, API folks will look at how they adapt once the feature is ready here. I think their API generator directly works on file format representation, and not compiler v1 stuff.

wrwg · 2024-07-02T04:34:49Z

third_party/move/move-ir-compiler/move-ir-to-bytecode/src/compiler.rs

@@ -485,6 +485,10 @@ pub fn compile_module<'a>(
        metadata: vec![],
        struct_defs,
        function_defs,
+        struct_variant_handles: vec![],


The source map is used all over our products (its also stored on chain), but Move IR is nowhere used anymore besides some legay tests. I'm not sure whether we want to keep it. Added a TODO linking to the tracking bug.

wrwg · 2024-07-02T04:37:48Z

third_party/move/move-model/src/model.rs

+        variant: Option<Symbol>,
+        offset: usize,
+    ) -> FieldEnv<'env> {
+        // TODO: speed this up via a cache RefCell<BTreeMap<(variant, offset), FieldId>>


It is not worth an issue, so I removed the TODO. and just left as a comment that this can be sped up. It's a linear search over less then a dozen elements in average, so its OK for the model.

wrwg · 2024-07-02T04:39:37Z

third_party/move/move-vm/integration-tests/src/tests/instantiation_tests.rs

@@ -101,6 +101,10 @@ fn instantiation_err() {
                ],
            }),
        }],
+        struct_variant_handles: vec![],


wrwg · 2024-07-02T04:39:57Z

third_party/move/move-vm/integration-tests/src/tests/vm_arguments_tests.rs

@@ -215,6 +215,10 @@ fn make_module_with_function(
                code: vec![Bytecode::LdU64(0), Bytecode::Abort],
            }),
        }],
+        struct_variant_handles: vec![],


ziaptos

Only commented on files I have viewed.

ziaptos · 2024-07-02T20:18:59Z

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

+        Ok(())
+    }
+
+    fn check_table<T>(


+1 for breaking the convention of repeating code.
Would be even better if rust supported generic closures so that we can have this as a lambda inside the function above.

ziaptos · 2024-07-02T20:43:44Z

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

@@ -104,6 +112,10 @@ pub enum TableType {
    FIELD_INST              = 0xE,
    FRIEND_DECLS            = 0xF,
    METADATA                = 0x10,
+    VARIANT_FIELD_HANDLE    = 0x11,


Most tables are named XYZ_HANDLES (only one FIELD_HANDLE - signular)

I renamed them all (both mine and the old ones)

ziaptos · 2024-07-02T20:46:36Z

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

@@ -305,6 +305,11 @@ impl Struct {
            StructFieldInformation::Declared(fields) => {
                fields.iter().map(|f| Field::new(m, f)).collect()
            },
+            StructFieldInformation::DeclaredVariants(..) => {
+                // TODO(#13806): consider implementing (probably not as long we do not


I've also seen panic!("TODO(#13806): blah blah") which i did like :)
no action needed here - just a comment

ziaptos · 2024-07-02T20:54:15Z

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

+///   - `field_count` as a ULEB128 (number of fields defined in the type)
+///   - `fields` as a ULEB128 (index into the `FieldDefinition` table)
+/// - if DeclaredVariants (since v7):
+///   - `field_count` as a ULEB128 (number of fields defined in the type)


Something wrong with this comment :)

Not only my one, but also the existing one... There is no FieldDefinitionTable,, rather field definitions are embeed in the StructDef.

Removed the entire comment block. If someone wants to know the serialization format, they have to look at the code anyway. Other functions do not have this comment either.

ziaptos · 2024-07-02T21:17:05Z

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

-        });
-    }
-    Ok(())
+fn load_variant(cursor: &mut VersionedCursor) -> BinaryLoaderResult<VariantDefinition> {


nit: I'd have this inline in load_variants.

I'd agree, just following convention in this module, where every type has its own loader. But I added #[inline(always)] ;-)

ziaptos · 2024-07-02T21:25:16Z

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

+    pub owner: StructDefinitionIndex,
+    /// The sequence of variants which share the field at the given
+    /// field offset.
+    pub variants: Vec<VariantCount>,


@wrwg So this means that we can have multiple VariantFieldHandle with the same owner? Just confirming

ziaptos · 2024-07-02T21:26:02Z

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

+    pub owner: StructDefinitionIndex,
+    /// The sequence of variants which share the field at the given
+    /// field offset.
+    pub variants: Vec<VariantCount>,


nit: Maybe define VariantIndex,Vec<VariantCount> reads weird (misleading)

Agreed. The existing code uses FieldCount for the index, not every bad thing need to be adapted. Renamed.

runtian-zhou

How are we testing the ser/de logic? IIRC we had a proptest logic that randomly generates bytecode and we test ser/de round trips with those generated bytecode. Should we update the generation logic? ( I didn't see the changes in the PR)

runtian-zhou · 2024-07-03T04:59:28Z

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

+#[cfg_attr(any(test, feature = "fuzzing"), proptest(no_params))]
+#[cfg_attr(feature = "fuzzing", derive(arbitrary::Arbitrary))]
+pub struct StructVariantHandle {
+    pub struct_index: StructDefinitionIndex,


Hmmm I'm not sure if I get this. The match instruction takes a StructVariantHandleIndex, which refers to StructDefinitionIndex. Does that mean that the match instruction could only be used at the defining module? As the StructDefinition should only be available for the defining module.

Yes, right now, match can only happen in the defining module.

We should make public/public structs together in one strike. In that case, StructDefinition would be imported from another module and embedded in the current module. Then everything we do today locally for structs (and enums) should become available publicly.

Got it. So public struct/enum will NOT be part of this PR and do you suggest we should bundle this feature in the same release or in a future release?

Yes. They are not part of this PR.

runtian-zhou · 2024-07-03T05:02:00Z

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

+        assert ty == &mut struct_ty
+        ty_stack << &mut field_ty
+    "#]
+    ImmBorrowVariantFieldGeneric(VariantFieldInstantiationIndex),


So we are using test instruction to pair with destruct so that we don't need to implement a jump table instead right? I think victor suggested that this could have some gas implication as later variant might cost more than the earlier ones as you have more instructions to test.

I actually have Jump tables in my original design doc, and they could be added at a later stage. However, it is not that obvious how they would help, and we need the basic instructions anyway. Consider

match (e) { C1(C2(_)) => e1, C1(C3(x)) if p(x) => e2, C2(C3(_)) => e3. etc }

Not that obvious how to create a jump table for stuff like this.

The basic underlying implementation right now are decision trees based on predicates. We can add jump tables on top of that for the cases we can identify an advantage, but the basic operations are needed.

runtian-zhou · 2024-07-03T05:15:28Z

api/types/src/bytecode.rs

@@ -109,6 +109,10 @@ pub trait Bytecode {
                    .map(|f| self.new_move_struct_field(f))
                    .collect(),
            ),
+            StructFieldInformation::DeclaredVariants(..) => {
+                // TODO(#13806): implement for struct variants
+                panic!("enums/struct variants not yet supported by the API")


Will panic crash the node?

Good point. I added a default behavior instead.

runtian-zhou · 2024-07-03T05:16:03Z

aptos-move/aptos-gas-schedule/src/gas_schedule/instr.rs

@@ -45,6 +45,15 @@ crate::gas_schedule::macros::define_gas_parameters!(
        [mut_borrow_field: InternalGas, "mut_borrow_field", 735],
        [imm_borrow_field_generic: InternalGas, "imm_borrow_field_generic", 735],
        [mut_borrow_field_generic: InternalGas, "mut_borrow_field_generic", 735],
+        [imm_borrow_variant_field: InternalGas, "imm_borrow_variant_field", 835],


Are we ok with those constants or we need to run another round of gas calibration?

We probably do need to calibrate this, though I derived them from the existing field operations, adding some 12% extra cost for the need of variant tag check.

runtian-zhou · 2024-07-03T05:23:15Z

aptos-move/framework/src/natives/string_utils.rs

@@ -308,6 +308,31 @@ fn native_format_impl(
            )?;
            out.push('}');
        },
+        MoveTypeLayout::Struct(MoveStructLayout::RuntimeVariants(variants)) => {
+            let strct = val.value_as::<Struct>()?;


Do we need to feature gate this change? I think we don't because the variant is only going to be relavent once bytecode v7 is enabled. By the time v7 flag is enabled we should expect all nodes to have this change already. Just wanting to make sure my reasoning is correct here.

I assume right now we do not, since as you said, this is guarded by the v7 flag, which behaves like a feature gate here.

runtian-zhou · 2024-07-03T06:07:50Z

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

@@ -729,3 +735,10 @@ impl_view_internals!(FieldDefinitionView, FieldDefinition, field_def);
 impl_view_internals!(TypeSignatureView, TypeSignature, type_signature);
 impl_view_internals!(SignatureView, Signature, signature);
 impl_view_internals!(SignatureTokenView, SignatureToken, token);
+
+/// A type represent eoither a FieldHandleIndex or a VariantFieldHandleIndex.


georgemitenkov

Made almost full pass, mainly looked at VM-related code

georgemitenkov · 2024-07-03T08:27:53Z

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

@@ -729,3 +735,10 @@ impl_view_internals!(FieldDefinitionView, FieldDefinition, field_def);
 impl_view_internals!(TypeSignatureView, TypeSignature, type_signature);
 impl_view_internals!(SignatureView, Signature, signature);
 impl_view_internals!(SignatureTokenView, SignatureToken, token);
+
+/// A type represent eoither a FieldHandleIndex or a VariantFieldHandleIndex.


Tbh, I am not a fan of such comments because the name is sufficient, and if variants are changed/renamed, it is no longer correct. Maybe remove?

I'm a fan of it, and I do it in my own code with every public type/function even if redundant.

My experience is if you don't, you establish ambiguous code style. The next guy come and change the code will not add any comments even though they should have. A simple rule 'add /// comment to every public thing' is easy to understand, a code style 'document when it is not obvious' is open for all kinds of interpretation.

georgemitenkov · 2024-07-03T08:33:02Z

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

@@ -300,6 +336,11 @@ struct ModuleSerializer {
    field_handles: (u32, u32),
    field_instantiations: (u32, u32),
    friend_decls: (u32, u32),
+    // Since v7


Please add more info, like "bytecode version 7". New person looking at this has no context what v7 is. I think applies in a few other places as well

Good point. Did a global replace and now its // Since bytecode version 7 everywhere

georgemitenkov · 2024-07-03T08:36:22Z

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

@@ -606,7 +669,7 @@ fn serialize_struct_def_instantiation(
    Ok(())
 }

-/// Serializes `FieldDefinition` within a struct.
+/// Serializes `FieldDefinition` list within.


Comment seems off, why remove "a struct"?

georgemitenkov · 2024-07-03T08:36:56Z

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

@@ -616,11 +679,6 @@ fn serialize_field_definitions(binary: &mut BinaryData, fields: &[FieldDefinitio
 }

 /// Serializes a `FieldDefinition`.
-///


nit: remove this the first line as well

georgemitenkov · 2024-07-03T08:56:09Z

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

@@ -84,6 +90,8 @@ pub const SIGNATURE_TOKEN_DEPTH_MAX: usize = 256;
 ///
 /// The binary contains a subset of those tables. A table specification is a tuple (table type,
 /// start offset, byte count) for a given table.
+///
+/// Notice there must not be more than `max(u8) - 1` tables.


Is there a test which asserts this? if not, we should probably have one so that such an invariant is always enforced

No not such tests. At least I added this comment which wasn't there before ;-) (While the issue was there before as well).

BTW, its not really an invariant more like a restricting of the current design

georgemitenkov · 2024-07-03T15:33:52Z

third_party/move/move-vm/runtime/src/loader/mod.rs

            },
            BinaryType::Script(_) => unreachable!("Scripts cannot have field instructions"),
        }
    }

+    pub(crate) fn create_struct_type(&self, struct_ty: &Arc<StructType>) -> Type {


Let's use ty for types? So create_struct_ty instead, I think in a couple of other places as well.

georgemitenkov · 2024-07-03T15:34:35Z

third_party/move/move-vm/runtime/src/loader/mod.rs

+            .create_struct_ty(struct_ty.idx, AbilityInfo::struct_(struct_ty.abilities))
+    }
+
+    pub(crate) fn create_struct_type_instantiation(


Same as above

georgemitenkov · 2024-07-03T15:38:36Z

third_party/move/move-vm/runtime/src/loader/mod.rs

+        let formulas = match &struct_type.layout {
+            StructLayout::Single(fields) => fields
+                .iter()
+                .map(|(_, field_type)| self.calculate_depth_of_type(field_type, module_store))


nit: field --> field_ty?

georgemitenkov · 2024-07-03T15:44:18Z

third_party/move/move-vm/runtime/src/loader/mod.rs

+                layout
+            },
+            StructLayout::Variants(variants) => {
+                // We do not support variants to have direct identifier mappings,


Is this enforced? Can I have

enum Foo { Bar(Aggregator), }

?

Well you tell me, this code is not well documented, and the aggregator integration is a mystery (mostly).

For structs someone left this comment:

// For aggregators / snapshots, the first field should be lifted. if let Some(kind) = &maybe_mapping {

I'm lost on whether this logic applies to variants as well. And what is the logic? Every first field of a struct which is of type Aggregator?

This non-standard semantics should have been documented here.

georgemitenkov · 2024-07-03T15:45:00Z

third_party/move/move-vm/runtime/src/loader/modules.rs

    pub(crate) definition_struct_type: Arc<StructType>,
 }

 // A field instantiation. The offset is the only used information when operating on a field
 #[derive(Clone, Debug)]
 pub(crate) struct FieldInstantiation {
    pub(crate) offset: usize,
+    pub(crate) uninstantiated_ty: Type,
+    pub(crate) definition_struct_type: Arc<StructType>,


Would be nice to have consistency between type and ty. The latter seems better to me

wrwg

Thanks for the reviews! Addressed most comments. Open question for @georgemitenkov is how to correctly deal with aggregators and struct variants and this type lifting (see code).

wrwg · 2024-07-03T22:24:15Z

api/types/src/bytecode.rs

@@ -109,6 +109,10 @@ pub trait Bytecode {
                    .map(|f| self.new_move_struct_field(f))
                    .collect(),
            ),
+            StructFieldInformation::DeclaredVariants(..) => {
+                // TODO(#13806): implement for struct variants
+                panic!("enums/struct variants not yet supported by the API")


Good point. I added a default behavior instead.

wrwg · 2024-07-03T22:25:18Z

aptos-move/aptos-gas-schedule/src/gas_schedule/instr.rs

@@ -45,6 +45,15 @@ crate::gas_schedule::macros::define_gas_parameters!(
        [mut_borrow_field: InternalGas, "mut_borrow_field", 735],
        [imm_borrow_field_generic: InternalGas, "imm_borrow_field_generic", 735],
        [mut_borrow_field_generic: InternalGas, "mut_borrow_field_generic", 735],
+        [imm_borrow_variant_field: InternalGas, "imm_borrow_variant_field", 835],


We probably do need to calibrate this, though I derived them from the existing field operations, adding some 12% extra cost for the need of variant tag check.

wrwg · 2024-07-03T22:26:33Z

aptos-move/framework/src/natives/string_utils.rs

@@ -308,6 +308,31 @@ fn native_format_impl(
            )?;
            out.push('}');
        },
+        MoveTypeLayout::Struct(MoveStructLayout::RuntimeVariants(variants)) => {
+            let strct = val.value_as::<Struct>()?;


I assume right now we do not, since as you said, this is guarded by the v7 flag, which behaves like a feature gate here.

wrwg · 2024-07-03T22:29:51Z

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

-        });
-    }
-    Ok(())
+fn load_variant(cursor: &mut VersionedCursor) -> BinaryLoaderResult<VariantDefinition> {


I'd agree, just following convention in this module, where every type has its own loader. But I added #[inline(always)] ;-)

wrwg · 2024-07-03T22:32:45Z

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

+    pub owner: StructDefinitionIndex,
+    /// The sequence of variants which share the field at the given
+    /// field offset.
+    pub variants: Vec<VariantCount>,


Yes, we can if the variant count/index sets are different. I'm open to change this, as said, after we land the PR.

wrwg · 2024-07-04T00:08:05Z

third_party/move/move-vm/runtime/src/loader/modules.rs

+pub(crate) struct StructVariantInfo {
+    // struct field count
+    pub(crate) field_count: u16,
+    pub(crate) variant: VariantCount,


I already merged instantiation and non-instantiation for variant fields, integration now the existing field stuff into this is too heavy weight and not the purpose of this PR.

The problem is that a lot of this stuff here is index based, and the indices are fully typed, so you cannot easily share APIs. This should be refactored heavily but in a different PR.

wrwg · 2024-07-04T00:09:31Z

third_party/move/move-vm/runtime/src/loader/modules.rs

 // A field handle. The offset is the only used information when operating on a field
 #[derive(Clone, Debug)]
 pub(crate) struct FieldHandle {
    pub(crate) offset: usize,
+    pub(crate) ty: Type,


wrwg · 2024-07-04T00:09:40Z

third_party/move/move-vm/runtime/src/loader/modules.rs

    pub(crate) definition_struct_type: Arc<StructType>,
 }

 // A field instantiation. The offset is the only used information when operating on a field
 #[derive(Clone, Debug)]
 pub(crate) struct FieldInstantiation {
    pub(crate) offset: usize,
+    pub(crate) uninstantiated_ty: Type,
+    pub(crate) definition_struct_type: Arc<StructType>,


wrwg · 2024-07-04T00:16:01Z

third_party/move/move-vm/types/src/loaded_data/runtime_types.rs

 impl StructType {
+    /// Get the fields from this struct type. If this is a proper struct, the `variant`


Its a good question what is better. If you continue to allow struct_def.fields() (or field_count) you can easily make an error as the struct can be a variant. With this additional parameter, you have to think about whether you are dealing with a struct or with variants. The convention worked well in the compiler, and is continued here.

wrwg · 2024-07-04T00:16:44Z

third_party/move/move-vm/types/src/loaded_data/runtime_types.rs

+            {
+                variants[variant as usize].1.as_slice()
+            },
+            _ => &[],


We can't return an error, and we should not crash. I think this is OK.

wrwg · 2024-07-04T00:34:07Z

How are we testing the ser/de logic? IIRC we had a proptest logic that randomly generates bytecode and we test ser/de round trips with those generated bytecode. Should we update the generation logic? ( I didn't see the changes in the PR)

I actually added things to the proptest_types modules, but I'm seeing now that might have not complete. Let me followup on this.

Regardless, the problem remains to test bytecode verifier. Proptest does not provide a test oracle, so that is not the helpful (only in cases liike deserialize(serialize(x)) ~~ x it works well)

In #13725 support for enums was added to the stackless bytecode IR. This PR extends the Move VM's representation of bytecode ('file format') by struct variants. It also implements - representation in file format - serialization/deserialization of file format - bytecode verifier - code generation in compiler v2 - intepreter and paranoid mode The runtime semantics (intepreter, runtime types, compatibility checking), as well as certain other features (as marked by #13806 in the code), are not yet implemented by this PR. On Move bytecode level, there are `5 * 2` new instructions (each instruction has a dual generic version): ``` TestVariant(StructVariantHandleIndex) and TestVariantGeneric(StructVariantInstantiationIndex) PackVariant(StructVariantHandleIndex) and PackVariantGeneric(StructVariantInstantiationIndex) UnpackVariant(StructVariantHandleIndex) and UnpackVariantGeneric(StructVariantInstantiationIndex) ImmBorrowVariantField(VariantFieldHandleIndex) and ImmBorrowVariantFieldGeneric(VariantFieldInstantiationIndex) MutBorrowVariantField(VariantFieldHandleIndex) and MutBorrowVariantFieldGeneric(VariantFieldInstantiationIndex) ``` For the indices used in those instructions, 4 new tables have been added to the file format holding the associated data. There is a lot of boilerplate code to support the new instructions and tables. Some refactoring of existing code has been done to avoid too much copy and paste, specifically in the serializers and in the bytecode verifier. Apart of passing existing tests, there is a new test in move-compiler-v2/tests/file-format-generator/struct_variants.move which shows the disassembeled output of various match expressions. There are also new e2e transactional tests in move-compiler-v2/transactional-tests/tests/enun. To add negative tests for the bytecode verifier and serializers, we first need a better way to build code with injected faults. See also #14074 and #13812.

github-actions · 2024-07-23T19:08:45Z

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6c84279403b75712eb7383bb466beb90df261952`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6c84279403b75712eb7383bb466beb90df261952 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 5711.758533619057 txn/s, latency: 5132.7647499797395 ms, (p50: 3800 ms, p90: 8400 ms, p99: 21700 ms), latency samples: 246780
2. Upgrading first Validator to new version: 6c84279403b75712eb7383bb466beb90df261952
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6574.191700149651 txn/s, latency: 4200.560355648536 ms, (p50: 4300 ms, p90: 4800 ms, p99: 5200 ms), latency samples: 124280
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6584.931886836997 txn/s, latency: 4820.035543941153 ms, (p50: 4500 ms, p90: 8100 ms, p99: 9200 ms), latency samples: 258300
3. Upgrading rest of first batch to new version: 6c84279403b75712eb7383bb466beb90df261952
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 5997.500749214328 txn/s, latency: 4327.791187169312 ms, (p50: 4900 ms, p90: 5800 ms, p99: 6000 ms), latency samples: 120960
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6550.964154640732 txn/s, latency: 4805.041388717768 ms, (p50: 4900 ms, p90: 6800 ms, p99: 7200 ms), latency samples: 227260
4. upgrading second batch to new version: 6c84279403b75712eb7383bb466beb90df261952
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10816.72260670966 txn/s, latency: 2863.9709848936595 ms, (p50: 3000 ms, p90: 3900 ms, p99: 4600 ms), latency samples: 201240
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 9348.61640611297 txn/s, latency: 3402.4852632437724 ms, (p50: 3300 ms, p90: 4100 ms, p99: 9200 ms), latency samples: 367720
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6c84279403b75712eb7383bb466beb90df261952 passed
Test Ok

github-actions · 2024-07-23T19:09:03Z

✅ Forge suite `realistic_env_max_load` success on `6c84279403b75712eb7383bb466beb90df261952`

two traffics test: inner traffic : committed: 9159.31571999977 txn/s, submitted: 9353.70593606598 txn/s, failed submission: 40.98834464021085 txn/s, expired: 194.3902160662114 txn/s, latency: 14519.79624980654 ms, (p50: 12100 ms, p90: 26600 ms, p99: 56100 ms), latency samples: 3482647
two traffics test : committed: 99.98273319391039 txn/s, latency: 1939.305 ms, (p50: 2000 ms, p90: 2200 ms, p99: 2400 ms), latency samples: 2000
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.241, avg: 0.219", "QsPosToProposal: max: 1.342, avg: 0.796", "ConsensusProposalToOrdered: max: 0.320, avg: 0.295", "ConsensusOrderedToCommit: max: 0.394, avg: 0.375", "ConsensusProposalToCommit: max: 0.689, avg: 0.670"]
Max round gap was 1 [limit 4] at version 1962363. Max no progress secs was 5.819472 [limit 15] at version 1962363.
Test Ok

github-actions · 2024-07-23T19:14:26Z

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6c84279403b75712eb7383bb466beb90df261952`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6c84279403b75712eb7383bb466beb90df261952 (PR)
Upgrade the nodes to version: 6c84279403b75712eb7383bb466beb90df261952
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1138.775486296672 txn/s, submitted: 1140.3996567131505 txn/s, failed submission: 1.624170416478546 txn/s, expired: 1.624170416478546 txn/s, latency: 2884.5041462917684 ms, (p50: 2100 ms, p90: 5400 ms, p99: 11100 ms), latency samples: 98160
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1039.119805057946 txn/s, submitted: 1041.9830007904075 txn/s, failed submission: 2.863195732461487 txn/s, expired: 2.863195732461487 txn/s, latency: 2894.797594319627 ms, (p50: 2100 ms, p90: 5400 ms, p99: 9900 ms), latency samples: 94360
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6c84279403b75712eb7383bb466beb90df261952 passed
Upgrade the remaining nodes to version: 6c84279403b75712eb7383bb466beb90df261952
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1056.7136603021427 txn/s, submitted: 1058.9472550184864 txn/s, failed submission: 2.2335947163435694 txn/s, expired: 2.2335947163435694 txn/s, latency: 2977.1683470725006 ms, (p50: 2100 ms, p90: 5400 ms, p99: 9100 ms), latency samples: 94620
Test Ok

wrwg requested review from vineethk, runtian-zhou, rahxephon89, vgao1996, georgemitenkov, brmataptos and ziaptos June 25, 2024 15:46

wrwg requested review from banool, gregnazario and 0xmaayan as code owners June 25, 2024 15:47

vgao1996 reviewed Jun 25, 2024

View reviewed changes

wrwg force-pushed the wrwg/enum-vm branch 2 times, most recently from 179576d to da7e31d Compare June 26, 2024 17:08

wrwg requested a review from vgao1996 June 28, 2024 04:25

brmataptos reviewed Jun 30, 2024

View reviewed changes

wrwg requested review from davidiw and movekevin as code owners July 1, 2024 18:01

wrwg requested a review from fEst1ck July 1, 2024 19:41

wrwg commented Jul 2, 2024

View reviewed changes

wrwg requested a review from brmataptos July 2, 2024 14:07

ziaptos reviewed Jul 2, 2024

View reviewed changes

runtian-zhou reviewed Jul 3, 2024

View reviewed changes

georgemitenkov reviewed Jul 3, 2024

View reviewed changes

wrwg commented Jul 4, 2024

View reviewed changes

wrwg requested review from runtian-zhou and ziaptos July 10, 2024 01:26

This comment has been minimized.

Sign in to view

wrwg force-pushed the wrwg/enum-vm branch from 4f37733 to d4fdc57 Compare July 23, 2024 14:48

This comment has been minimized.

Sign in to view

wrwg force-pushed the wrwg/enum-vm branch from d4fdc57 to 2ffd9b3 Compare July 23, 2024 17:59

This comment has been minimized.

Sign in to view

wrwg force-pushed the wrwg/enum-vm branch from 2ffd9b3 to 6c84279 Compare July 23, 2024 18:39

This comment has been minimized.

Sign in to view

wrwg merged commit 777f7bd into main Jul 23, 2024
45 of 48 checks passed

wrwg deleted the wrwg/enum-vm branch July 23, 2024 19:28

		impl StructType {
		/// Get the fields from this struct type. If this is a proper struct, the `variant`

[compiler-v2] Enum types in the VM #13812

[compiler-v2] Enum types in the VM #13812

Conversation

wrwg commented Jun 25, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

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

vgao1996 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2024 • edited Loading

Codecov Report

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

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

wrwg 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

ziaptos 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

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

wrwg commented Jun 25, 2024 •

edited

Loading

trunk-io bot commented Jun 25, 2024 •

edited

Loading

vgao1996 Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading