aptos-labs / aptos-core

Aptos is a layer 1 blockchain built to support the widespread use of blockchain through better technology and user experience.
https://aptosfoundation.org
Other
6.16k stars 3.63k forks source link

[Bug] Copy propagation seems to wrongly deal with `move` #12068

Open wrwg opened 8 months ago

wrwg commented 8 months ago

Consider the simple program:

module 0x32::m {
    fun main() {
        let x = 0;
        while (true) {
            x = x + 1;
            break
        };
        assert!(x == 1, 42);
    }
}

This results in the following byte code with the new ability processor:

============ after AbilityProcessor: ================

[variant baseline]
fun m::main() {
     var $t0: u64
     var $t1: u64
     var $t2: bool
     var $t3: u64
     var $t4: u64
     var $t5: bool
     var $t6: u64
     var $t7: u64
  0: $t1 := 0
  1: $t0 := move($t1)
  2: label L0
  3: $t2 := true
  4: if ($t2) goto 5 else goto 11
  5: label L2
  6: $t4 := 1
  7: $t3 := +($t0, $t4)
  8:  ...
}

After the copy propagation pipeline, we see this:

============ after CopyPropagation: ================

[variant baseline]
fun m::main() {
     var $t0: u64
     var $t1: u64
     var $t2: bool
     var $t3: u64
     var $t4: u64
     var $t5: bool
     var $t6: u64
     var $t7: u64
  0: $t1 := 0
  1: $t0 := move($t1)
  2: label L0
  3: $t2 := true
  4: if ($t2) goto 5 else goto 11
  5: label L2
  6: $t4 := 1
  7: $t3 := +($t1, $t4)        // <--- t1 is moved and invalid
  8: ...
}

This error might have been masked without new ability processor as the new one minimizes copy in favor of move.

vineethk commented 8 months ago

@wrwg Acknowledged, working on this.

brmataptos commented 7 months ago

third_party/move/move-compiler-v2/tests/copy-propagation/call_1.exp shows a similar issue.

vineethk commented 7 months ago

Once #12357 lands, I think we can move this down to be medium priority.

wrwg commented 4 months ago

Need to figure whether copy propagation will become part of standard pipeline