Merge pull request #6763 from ethereum-optimism/inphi/mips-todos

cannon: Fix j/jal VM emulation

cannon/mipsevm/evm_test.go

@@ -165,6 +165,43 @@ func TestEVM(t *testing.T) {
 	}
 }
 
+func TestEVMSingleStep(t *testing.T) {
+	contracts, addrs := testContractsSetup(t)
+	var tracer vm.EVMLogger
+	//tracer = SourceMapTracer(t, contracts, addrs)
+
+	type testInput struct {
+		name   string
+		pc     uint32
+		nextPC uint32
+		insn   uint32
+	}
+	cases := []testInput{
+		{"j MSB set target", 0, 4, 0x0A_00_00_02},                         // j 0x02_00_00_02
+		{"j non-zero PC region", 0x10000000, 0x10000004, 0x08_00_00_02},   // j 0x2
+		{"jal MSB set target", 0, 4, 0x0E_00_00_02},                       // jal 0x02_00_00_02
+		{"jal non-zero PC region", 0x10000000, 0x10000004, 0x0C_00_00_02}, // jal 0x2
+	}
+
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			state := &State{PC: tt.pc, NextPC: tt.nextPC, Memory: NewMemory()}
+			state.Memory.SetMemory(tt.pc, tt.insn)
+
+			us := NewInstrumentedState(state, nil, os.Stdout, os.Stderr)
+			stepWitness, err := us.Step(true)
+			require.NoError(t, err)
+
+			evm := NewMIPSEVM(contracts, addrs)
+			evm.SetTracer(tracer)
+			evmPost := evm.Step(t, stepWitness)
+			goPost := us.state.EncodeWitness()
+			require.Equal(t, hexutil.Bytes(goPost).String(), hexutil.Bytes(evmPost).String(),
+				"mipsevm produced different state than EVM")
+		})
+	}
+}
+
 func TestEVMFault(t *testing.T) {
 	contracts, addrs := testContractsSetup(t)
 	var tracer vm.EVMLogger // no-tracer by default, but see SourceMapTracer and MarkdownTracer

cannon/mipsevm/mips.go

@@ -285,13 +285,13 @@ func (m *InstrumentedState) mipsStep() error {
 
 	// j-type j/jal
 	if opcode == 2 || opcode == 3 {
-		// TODO likely bug in original code: MIPS spec says this should be in the "current" region;
-		// a 256 MB aligned region (i.e. use top 4 bits of branch delay slot (pc+4))
 		linkReg := uint32(0)
 		if opcode == 3 {
 			linkReg = 31
 		}
-		return m.handleJump(linkReg, SE(insn&0x03FFFFFF, 26)<<2)
+		// Take top 4 bits of the next PC (its 256 MB region), and concatenate with the 26-bit offset
+		target := (m.state.NextPC & 0xF0000000) | ((insn & 0x03FFFFFF) << 2)
+		return m.handleJump(linkReg, target)
 	}
 
 	// register fetch
@@ -396,7 +396,6 @@ func (m *InstrumentedState) mipsStep() error {
 func execute(insn uint32, rs uint32, rt uint32, mem uint32) uint32 {
 	opcode := insn >> 26 // 6-bits
 	fun := insn & 0x3f   // 6-bits
-	// TODO(CLI-4136): deref the immed into a register
 
 	if opcode < 0x20 {
 		// transform ArithLogI

packages/contracts-bedrock/.gas-snapshot

@@ -298,77 +298,79 @@ LegacyERC20ETH_Test:test_transfer_doesNotExist_reverts() (gas: 10755)
 LegacyMessagePasser_Test:test_passMessageToL1_succeeds() (gas: 34524)
 LibPosition_Test:test_pos_correctness_succeeds() (gas: 38689)
 MIPS_Test:test_add_succeeds() (gas: 121593)
-MIPS_Test:test_addi_succeeds() (gas: 121896)
-MIPS_Test:test_addu_succeeds() (gas: 121645)
-MIPS_Test:test_addui_succeeds() (gas: 121953)
-MIPS_Test:test_and_succeeds() (gas: 121628)
-MIPS_Test:test_andi_succeeds() (gas: 121770)
+MIPS_Test:test_addi_succeeds() (gas: 121918)
+MIPS_Test:test_addu_succeeds() (gas: 121601)
+MIPS_Test:test_addui_succeeds() (gas: 121975)
+MIPS_Test:test_and_succeeds() (gas: 121650)
+MIPS_Test:test_andi_succeeds() (gas: 121792)
 MIPS_Test:test_beq_succeeds() (gas: 202355)
-MIPS_Test:test_bgez_succeeds() (gas: 121507)
-MIPS_Test:test_bgtz_succeeds() (gas: 121428)
-MIPS_Test:test_blez_succeeds() (gas: 121406)
-MIPS_Test:test_bltz_succeeds() (gas: 121482)
-MIPS_Test:test_bne_succeeds() (gas: 121548)
-MIPS_Test:test_branch_inDelaySlot_fails() (gas: 85977)
-MIPS_Test:test_brk_succeeds() (gas: 121509)
+MIPS_Test:test_bgez_succeeds() (gas: 121484)
+MIPS_Test:test_bgtz_succeeds() (gas: 121405)
+MIPS_Test:test_blez_succeeds() (gas: 121361)
+MIPS_Test:test_bltz_succeeds() (gas: 121504)
+MIPS_Test:test_bne_succeeds() (gas: 121570)
+MIPS_Test:test_branch_inDelaySlot_fails() (gas: 85999)
+MIPS_Test:test_brk_succeeds() (gas: 121531)
 MIPS_Test:test_clo_succeeds() (gas: 121991)
 MIPS_Test:test_clone_succeeds() (gas: 121484)
-MIPS_Test:test_clz_succeeds() (gas: 122440)
+MIPS_Test:test_clz_succeeds() (gas: 122462)
 MIPS_Test:test_div_succeeds() (gas: 121806)
-MIPS_Test:test_divu_succeeds() (gas: 121806)
-MIPS_Test:test_exit_succeeds() (gas: 121386)
-MIPS_Test:test_fcntl_succeeds() (gas: 203171)
-MIPS_Test:test_illegal_instruction_fails() (gas: 91153)
-MIPS_Test:test_invalid_root_fails() (gas: 435656)
-MIPS_Test:test_jal_succeeds() (gas: 117399)
+MIPS_Test:test_divu_succeeds() (gas: 121762)
+MIPS_Test:test_exit_succeeds() (gas: 121408)
+MIPS_Test:test_fcntl_succeeds() (gas: 203129)
+MIPS_Test:test_illegal_instruction_fails() (gas: 91175)
+MIPS_Test:test_invalid_root_fails() (gas: 435678)
+MIPS_Test:test_jal_nonzeroRegion_succeeds() (gas: 120492)
+MIPS_Test:test_jal_succeeds() (gas: 120481)
 MIPS_Test:test_jalr_succeeds() (gas: 121349)
-MIPS_Test:test_jr_succeeds() (gas: 121138)
-MIPS_Test:test_jump_inDelaySlot_fails() (gas: 85512)
-MIPS_Test:test_jump_succeeds() (gas: 120353)
+MIPS_Test:test_jr_succeeds() (gas: 121094)
+MIPS_Test:test_jump_inDelaySlot_fails() (gas: 85345)
+MIPS_Test:test_jump_nonzeroRegion_succeeds() (gas: 120236)
+MIPS_Test:test_jump_succeeds() (gas: 120188)
 MIPS_Test:test_lb_succeeds() (gas: 127346)
-MIPS_Test:test_lbu_succeeds() (gas: 127266)
-MIPS_Test:test_lh_succeeds() (gas: 127345)
-MIPS_Test:test_lhu_succeeds() (gas: 127262)
-MIPS_Test:test_ll_succeeds() (gas: 127282)
-MIPS_Test:test_lui_succeeds() (gas: 121531)
+MIPS_Test:test_lbu_succeeds() (gas: 127222)
+MIPS_Test:test_lh_succeeds() (gas: 127367)
+MIPS_Test:test_lhu_succeeds() (gas: 127284)
+MIPS_Test:test_ll_succeeds() (gas: 127304)
+MIPS_Test:test_lui_succeeds() (gas: 121488)
 MIPS_Test:test_lw_succeeds() (gas: 127158)
-MIPS_Test:test_lwl_succeeds() (gas: 241457)
-MIPS_Test:test_lwr_succeeds() (gas: 241767)
+MIPS_Test:test_lwl_succeeds() (gas: 241434)
+MIPS_Test:test_lwr_succeeds() (gas: 241722)
 MIPS_Test:test_mfhi_succeeds() (gas: 121458)
 MIPS_Test:test_mflo_succeeds() (gas: 121484)
-MIPS_Test:test_mmap_succeeds() (gas: 118492)
+MIPS_Test:test_mmap_succeeds() (gas: 118451)
 MIPS_Test:test_movn_succeeds() (gas: 202409)
 MIPS_Test:test_movz_succeeds() (gas: 202313)
-MIPS_Test:test_mthi_succeeds() (gas: 121428)
+MIPS_Test:test_mthi_succeeds() (gas: 121450)
 MIPS_Test:test_mtlo_succeeds() (gas: 121478)
-MIPS_Test:test_mul_succeeds() (gas: 121541)
-MIPS_Test:test_mult_succeeds() (gas: 121645)
-MIPS_Test:test_multu_succeeds() (gas: 121698)
-MIPS_Test:test_nor_succeeds() (gas: 121739)
-MIPS_Test:test_or_succeeds() (gas: 121635)
+MIPS_Test:test_mul_succeeds() (gas: 121563)
+MIPS_Test:test_mult_succeeds() (gas: 121667)
+MIPS_Test:test_multu_succeeds() (gas: 121675)
+MIPS_Test:test_nor_succeeds() (gas: 121697)
+MIPS_Test:test_or_succeeds() (gas: 121657)
 MIPS_Test:test_ori_succeeds() (gas: 121865)
-MIPS_Test:test_preimage_read_succeeds() (gas: 233825)
+MIPS_Test:test_preimage_read_succeeds() (gas: 233847)
 MIPS_Test:test_preimage_write_succeeds() (gas: 126473)
-MIPS_Test:test_prestate_exited_succeeds() (gas: 112970)
+MIPS_Test:test_prestate_exited_succeeds() (gas: 112992)
 MIPS_Test:test_sb_succeeds() (gas: 159993)
 MIPS_Test:test_sc_succeeds() (gas: 160187)
-MIPS_Test:test_sh_succeeds() (gas: 160096)
-MIPS_Test:test_sll_succeeds() (gas: 121434)
-MIPS_Test:test_sllv_succeeds() (gas: 121624)
-MIPS_Test:test_slt_succeeds() (gas: 203244)
+MIPS_Test:test_sh_succeeds() (gas: 160052)
+MIPS_Test:test_sll_succeeds() (gas: 121456)
+MIPS_Test:test_sllv_succeeds() (gas: 121646)
+MIPS_Test:test_slt_succeeds() (gas: 203266)
 MIPS_Test:test_sltu_succeeds() (gas: 121871)
-MIPS_Test:test_sra_succeeds() (gas: 121719)
+MIPS_Test:test_sra_succeeds() (gas: 121763)
 MIPS_Test:test_srav_succeeds() (gas: 121959)
 MIPS_Test:test_srl_succeeds() (gas: 121514)
 MIPS_Test:test_srlv_succeeds() (gas: 121707)
 MIPS_Test:test_step_abi_succeeds() (gas: 57876)
-MIPS_Test:test_sub_succeeds() (gas: 121674)
-MIPS_Test:test_subu_succeeds() (gas: 121682)
-MIPS_Test:test_sw_succeeds() (gas: 160050)
-MIPS_Test:test_swl_succeeds() (gas: 160066)
-MIPS_Test:test_swr_succeeds() (gas: 160141)
+MIPS_Test:test_sub_succeeds() (gas: 121696)
+MIPS_Test:test_subu_succeeds() (gas: 121659)
+MIPS_Test:test_sw_succeeds() (gas: 160027)
+MIPS_Test:test_swl_succeeds() (gas: 160088)
+MIPS_Test:test_swr_succeeds() (gas: 160163)
 MIPS_Test:test_xor_succeeds() (gas: 121685)
-MIPS_Test:test_xori_succeeds() (gas: 121894)
+MIPS_Test:test_xori_succeeds() (gas: 121916)
 MerkleTrie_get_Test:test_get_corruptedProof_reverts() (gas: 5733)
 MerkleTrie_get_Test:test_get_extraProofElements_reverts() (gas: 58889)
 MerkleTrie_get_Test:test_get_invalidDataRemainder_reverts() (gas: 35845)

packages/contracts-bedrock/src/cannon/MIPS.sol

@@ -667,9 +667,9 @@ contract MIPS {
 
             // j-type j/jal
             if (opcode == 2 || opcode == 3) {
-                // TODO(CLI-4136): likely bug in original code: MIPS spec says this should be in the "current" region;
-                // a 256 MB aligned region (i.e. use top 4 bits of branch delay slot (pc+4))
-                return handleJump(opcode == 2 ? 0 : 31, SE(insn & 0x03FFFFFF, 26) << 2);
+                // Take top 4 bits of the next PC (its 256 MB region), and concatenate with the 26-bit offset
+                uint32 target = (state.nextPC & 0xF0000000) | (insn & 0x03FFFFFF) << 2;
+                return handleJump(opcode == 2 ? 0 : 31, target);
             }
 
             // register fetch
@@ -775,7 +775,6 @@ contract MIPS {
         unchecked {
             uint32 opcode = insn >> 26; // 6-bits
             uint32 func = insn & 0x3f; // 6-bits
-            // TODO(CLI-4136): deref the immed into a register
 
             if (opcode < 0x20) {
                 // transform ArithLogI

packages/contracts-bedrock/test/MIPS.t.sol

@@ -986,7 +986,7 @@ contract MIPS_Test is CommonTest {
     }
 
     function test_jump_succeeds() external {
-        uint16 label = 0x2;
+        uint32 label = 0x02_00_00_02; // set the 26th bit to assert no sign extension
         uint32 insn = uint32(0x08_00_00_00) | label; // j label
         (MIPS.State memory state, bytes memory proof) = constructMIPSState(0, insn, 0x4, 0);
 
@@ -1000,23 +1000,51 @@ contract MIPS_Test is CommonTest {
         assertEq(postState, outputState(expect), "unexpected post state");
     }
 
+    function test_jump_nonzeroRegion_succeeds() external {
+        uint32 pcRegion1 = 0x10000000;
+        uint32 label = 0x2;
+        uint32 insn = uint32(0x08_00_00_00) | label; // j label
+        (MIPS.State memory state, bytes memory proof) = constructMIPSState(pcRegion1, insn, 0x4, 0);
+
+        MIPS.State memory expect;
+        expect.memRoot = state.memRoot;
+        expect.pc = state.nextPC;
+        expect.nextPC = (state.nextPC & 0xF0_00_00_00) | (uint32(label) << 2);
+        expect.step = state.step + 1;
+
+        bytes memory witness = encodeState(state);
+        bytes32 postState = mips.step(witness, proof);
+        assertEq(postState, outputState(expect), "unexpected post state");
+    }
+
     function test_jal_succeeds() external {
-        uint32 pc = 0x0;
-        uint16 label = 0x2;
+        uint32 label = 0x02_00_00_02; // set the 26th bit to assert no sign extension
         uint32 insn = uint32(0x0c_00_00_00) | label; // jal label
-        (bytes32 memRoot, bytes memory proof) = ffi.getCannonMemoryProof(pc, insn);
-
-        MIPS.State memory state;
-        state.pc = 0;
-        state.nextPC = 4;
-        state.memRoot = memRoot;
+        (MIPS.State memory state, bytes memory proof) = constructMIPSState(0, insn, 0x4, 0);
 
         MIPS.State memory expect;
         expect.memRoot = state.memRoot;
         expect.pc = state.nextPC;
         expect.nextPC = label << 2;
         expect.step = state.step + 1;
-        expect.registers[31] = state.pc + 8;
+        expect.registers[31] = state.pc + 8; // ra
+
+        bytes32 postState = mips.step(encodeState(state), proof);
+        assertEq(postState, outputState(expect), "unexpected post state");
+    }
+
+    function test_jal_nonzeroRegion_succeeds() external {
+        uint32 pcRegion1 = 0x10000000;
+        uint32 label = 0x2;
+        uint32 insn = uint32(0x0c_00_00_00) | label; // jal label
+        (MIPS.State memory state, bytes memory proof) = constructMIPSState(pcRegion1, insn, 0x4, 0);
+
+        MIPS.State memory expect;
+        expect.memRoot = state.memRoot;
+        expect.pc = state.nextPC;
+        expect.nextPC = (state.nextPC & 0xF0_00_00_00) | (uint32(label) << 2);
+        expect.step = state.step + 1;
+        expect.registers[31] = state.pc + 8; // ra
 
         bytes32 postState = mips.step(encodeState(state), proof);
         assertEq(postState, outputState(expect), "unexpected post state");