Repository: aptos-labs/aptos-core (language: python)

Issue:
[Bug][move-compiler-v2] MOVELOC_UNAVAILABLE_ERROR related bugs raised by V2
# 🐛 Bug

## To reproduce

Try to compile the following code with `MVC_EXP=ast-simplify=off aptos move compile --compiler-version v2 --language-version 2` or `MVC_EXP=optimize=off aptos move compile --compiler-version v2 --language-version 2`.

**Code snippet to reproduce**
```move
module 0xCAFE::Module1 {
    struct Struct3 has drop, copy {
        var32: u16,
        var33: u32,
        var34: u8,
        var35: u32,
        var36: u32,
    }

    public fun function6(): Struct3 {
        let var44: u16 =  21859u16;
        let var45: u32 =  1399722001u32;
        Struct3 {
            var32: var44,
            var33: var45,
            var34: 154u8,
            var35: var45,
            var36: var45,
        }
    }
}
```

**Stack trace/error message**
```
bug: bytecode verification failed with unexpected status code `MOVELOC_UNAVAILABLE_ERROR`. This is a compiler bug, consider reporting it.
   ┌─ ~/debug_package/sources/0.move:13:9
   │  
14 │ ╭         Struct3 {
15 │ │             var32: var44,
16 │ │             var33: var45,
17 │ │             var34: 154u8,
18 │ │             var35: var45,
19 │ │             var36: var45,
20 │ │         }
   │ ╰─────────^

{
  "Error": "Move compilation failed: exiting with bytecode verification errors"
}
```

## Expected Behavior

It should compile as it does with `aptos move compile` and `aptos move compile --compiler-version v2 --language-version 2`.

## Additional context
If the last use of `var45` is changed to some number literal, it will compile fine: e.g.
```move
...
            var36: 1399722001u32,
...
```



Discussion / hints:
The fuzzer found another `MOVELOC_UNAVAILABLE_ERROR` case:
```
module 0xCAFE::Module0 {
    public fun function2<T1: drop + store> (var3: T1, var4: T1): T1 {
        if (((var3 == var3) && if (true)  { false } else { true }))  {
        } else {};
        var4
    }
}
```

Compiling with `aptos move compile` gives:
```
error[E06002]: use of unassigned variable
  ┌─ ~/debug_package/sources/repro.move:3:23
  │
3 │         if (((var3 == var3) && if (true)  { false } else { true }))  {
  │               ----    ^^^^ Invalid usage of previously moved variable 'var3'.
  │               │        
  │               The value of 'var3' was previously moved here.
  │               Suggestion: use 'copy var3' to avoid the move.
```

Compiling with `aptos move compile --compiler-version v2 --language-version 2` gives:
```
bug: bytecode verification failed with unexpected status code `MOVELOC_UNAVAILABLE_ERROR`. This is a compiler bug, consider reporting it.
  ┌─ ~/debug_package/sources/repro.move:3:14
  │
3 │         if (((var3 == var3) && if (true)  { false } else { true }))  {
  │              ^^^^^^^^^^^^^^
```

Fix patch:
diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs
index 1bfb4c3faeda3..4ba5f0c9d7ef1 100644
--- a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs
+++ b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs
@@ -942,10 +942,13 @@ impl<'a> FunctionGenerator<'a> {
                 None => {
                     // Copy the temporary if it is copyable and still used after this code point, or
                     // if it appears again in temps_to_push.
-                    if fun_ctx.is_copyable(*temp)
-                        && (ctx.is_alive_after(*temp, true)
-                            || temps_to_push[pos + 1..].contains(temp))
-                    {
+                    if ctx.is_alive_after(*temp, &temps_to_push[pos + 1..], true) {
+                        if !fun_ctx.is_copyable(*temp) {
+                            fun_ctx.module.internal_error(
+                                &ctx.fun_ctx.fun.get_bytecode_loc(ctx.attr_id),
+                                format!("value in `$t{}` expected to be copyable", temp),
+                            )
+                        }
                         self.emit(FF::Bytecode::CopyLoc(local))
                     } else {
                         self.emit(FF::Bytecode::MoveLoc(local));
@@ -981,9 +984,9 @@ impl<'a> FunctionGenerator<'a> {
     /// point are saved to locals. This flushes the stack as deep as needed for this.
     fn save_used_after(&mut self, ctx: &BytecodeContext, temps: &[TempIndex]) {
         let mut stack_to_flush = self.stack.len();
-        for temp in temps {
+        for (i, temp) in temps.iter().enumerate() {
             if let Some(pos) = self.stack.iter().position(|t| t == temp) {
-                if ctx.is_alive_after(*temp, true) {
+                if ctx.is_alive_after(*temp, &temps[i + 1..], true) {
                     // Determine new lowest point to which we need to flush
                     stack_to_flush = std::cmp::min(stack_to_flush, pos);
                 }
@@ -1017,7 +1020,7 @@ impl<'a> FunctionGenerator<'a> {
         while self.stack.len() > top {
             let temp = self.stack.pop().unwrap();
             if before && ctx.is_alive_before(temp)
-                || !before && ctx.is_alive_after(temp, false)
+                || !before && ctx.is_alive_after(temp, &[], false)
                 || self.pinned.contains(&temp)
             {
                 // Only need to save to a local if the temp is still used afterwards
@@ -1144,11 +1147,21 @@ impl<'env> FunctionContext<'env> {
 }
 
 impl<'env> BytecodeContext<'env> {
-    /// Determine whether `temp` is alive (used) in the reachable code after this point.
-    /// When `dest_check` is true, we additionally check if `temp` is also written to
-    /// by the current instruction; if it is, then the definition of `temp` being
-    /// considered here is killed, making it not alive after this point.
-    pub fn is_alive_after(&self, temp: TempIndex, dest_check: bool) -> bool {
+    /// Determine whether `temp` is alive (used) in the reachable code after this point,
+    /// or is part of the remaining argument list. When `dest_check` is true, we additionally
+    /// check if `temp` is also written to by the current instruction; if it is, then the
+    /// definition of `temp` being considered here is killed, making it not alive after this point.
+    pub fn is_alive_after(
+        &self,
+        temp: TempIndex,
+        remaining_args: &[TempIndex],
+        dest_check: bool,
+    ) -> bool {
+        if remaining_args.contains(&temp) {
+            // Temp is used another time in the same argument list of this instruction, and
+            // is alive after even if it is a destination
+            return true;
+        }
         let bc = &self.fun_ctx.fun.data.code[self.code_offset as usize];
         if dest_check && bc.dests().contains(&temp) {
             return false;


Test patch:
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.no-opt.exp
deleted file mode 100644
index e34791244b187..0000000000000
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.no-opt.exp
+++ /dev/null
@@ -1,15 +0,0 @@
-
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `MOVELOC_UNAVAILABLE_ERROR`. This is a compiler bug, consider reporting it.
-   ┌─ tests/reference-safety/bug_13687.move:14:9
-   │
-14 │ ╭         Struct3 {
-15 │ │             var32: var44,
-16 │ │             var33: var45,
-17 │ │             var34: 154u8,
-18 │ │             var35: var45,
-19 │ │             var36: var45,
-20 │ │         }
-   │ ╰─────────^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.exp
similarity index 100%
rename from third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.old.exp
rename to third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.exp
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.move b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.move
similarity index 81%
rename from third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.move
rename to third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.move
index 09f92b4aa8367..2f077cc9fa339 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687.move
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.move
@@ -1,4 +1,3 @@
-// TODO(#13912): after fix, rename file to reflect issue (`bug_nnn.move` to `bug_nnn_<issue>.move`)
 module 0xCAFE::Module1 {
     struct Struct3 has drop, copy {
         var32: u16,
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.no-opt.exp
new file mode 100644
index 0000000000000..90b32906711cc
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.no-opt.exp
@@ -0,0 +1,2 @@
+
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.old.exp
new file mode 100644
index 0000000000000..90b32906711cc
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_13687_repeated_args.old.exp
@@ -0,0 +1,2 @@
+
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.exp
new file mode 100644
index 0000000000000..f566c3107f5c6
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.exp
@@ -0,0 +1,6 @@
+processed 2 tasks
+
+task 1 'run'. lines 24-24:
+return values: { 21859, 1399722001, 154, 1399722001, 1399722001 }
+
+==> Compiler v2 delivered same results!
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.move b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.move
new file mode 100644
index 0000000000000..b5eb1ff3164a4
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_13687_repeated_args.move
@@ -0,0 +1,24 @@
+//# publish
+module 0xCAFE::Module1 {
+    struct Struct3 has drop, copy {
+        var32: u16,
+        var33: u32,
+        var34: u8,
+        var35: u32,
+        var36: u32,
+    }
+
+    public fun function6(): Struct3 {
+        let var44: u16 =  21859u16;
+        let var45: u32 =  1399722001u32;
+        Struct3 {
+            var32: var44,
+            var33: var45,
+            var34: 154u8,
+            var35: var45,
+            var36: var45,
+        }
+    }
+}
+
+//# run 0xCAFE::Module1::function6