cannon: Tweak sign extension logic (#14185)

* cannon: Add some unit tests for SignExtend * cannon: Tweak signExtend logic * cannon: Run semver-lock * cannon: Fix lint error
ethereum-optimism · Feb 7, 2025 · f5b73fc · f5b73fc
1 parent a0131e6
commit f5b73fc
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 15 deletions.
diff --git a/cannon/mipsevm/exec/mips_instructions.go b/cannon/mipsevm/exec/mips_instructions.go
@@ -467,7 +467,7 @@ func ExecuteMipsInstruction(insn uint32, opcode uint32, fun uint32, rs, rt, mem
 }
 
 func SignExtend(dat Word, idx Word) Word {
-	isSigned := (dat >> (idx - 1)) != 0
+	isSigned := (dat>>(idx-1))&1 != 0
 	signed := ((Word(1) << (arch.WordSize - idx)) - 1) << idx
 	mask := (Word(1) << idx) - 1
 	if isSigned {

diff --git a/cannon/mipsevm/exec/mips_instructions32_test.go b/cannon/mipsevm/exec/mips_instructions32_test.go
@@ -108,3 +108,52 @@ func TestStoreSubWord_32bits(t *testing.T) {
 		})
 	}
 }
+
+func TestSignExtend_32bit(t *testing.T) {
+	cases := []struct {
+		name     string
+		data     Word
+		index    Word
+		expected Word
+	}{
+		{name: "idx 1, signed", data: 0x0000_0001, index: 1, expected: 0xFFFF_FFFF},
+		{name: "idx 1, unsigned", data: 0x0000_0000, index: 1, expected: 0x0000_0000},
+		{name: "idx 2, signed", data: 0x0000_0002, index: 2, expected: 0xFFFF_FFFE},
+		{name: "idx 2, unsigned", data: 0x0000_0001, index: 2, expected: 0x0000_0001},
+		{name: "idx 4, signed", data: 0x0000_0008, index: 4, expected: 0xFFFF_FFF8},
+		{name: "idx 4, unsigned", data: 0x0000_0005, index: 4, expected: 0x0000_0005},
+		{name: "idx 8, signed", data: 0x0000_0092, index: 8, expected: 0xFFFF_FF92},
+		{name: "idx 8, unsigned", data: 0x0000_0075, index: 8, expected: 0x0000_0075},
+		{name: "idx 16, signed", data: 0x0000_A123, index: 16, expected: 0xFFFF_A123},
+		{name: "idx 16, unsigned", data: 0x0000_7123, index: 16, expected: 0x0000_7123},
+		{name: "idx 32, signed", data: 0x8123_4567, index: 32, expected: 0x8123_4567},
+		{name: "idx 32, unsigned", data: 0x7123_4567, index: 32, expected: 0x7123_4567},
+		{name: "idx 1, signed, nonzero upper bits", data: 0x1234_5671, index: 1, expected: 0xFFFF_FFFF},
+		{name: "idx 1, unsigned, nonzero upper bits", data: 0x1234_567E, index: 1, expected: 0x0000_0000},
+		{name: "idx 2, signed, nonzero upper bits", data: 0xABCD_EFE6, index: 2, expected: 0xFFFF_FFFE},
+		{name: "idx 2, unsigned, nonzero upper bits", data: 0xABCD_EFED, index: 2, expected: 0x0000_0001},
+		{name: "idx 4, signed, nonzero upper bits", data: 0x1230_0008, index: 4, expected: 0xFFFF_FFF8},
+		{name: "idx 4, unsigned, nonzero upper bits", data: 0xFFF0_0005, index: 4, expected: 0x0000_0005},
+		{name: "idx 8, signed, nonzero upper bits", data: 0x1111_1192, index: 8, expected: 0xFFFF_FF92},
+		{name: "idx 8, unsigned, nonzero upper bits", data: 0xFFFF_FF75, index: 8, expected: 0x0000_0075},
+		{name: "idx 16, signed, nonzero upper bits", data: 0x1234_A123, index: 16, expected: 0xFFFF_A123},
+		{name: "idx 16, unsigned, nonzero upper bits", data: 0x1234_7123, index: 16, expected: 0x0000_7123},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			actual := SignExtend(c.data, c.index)
+			expected := signExtend64(c.expected)
+			require.Equal(t, expected, actual)
+		})
+	}
+}
+
+func signExtend64(w Word) Word {
+	// If bit at index 31 == 1, then sign extend the higher bits on 64-bit architectures
+	if !arch.IsMips32 && (w>>31)&1 == 1 {
+		upperBits := uint64(0xFFFF_FFFF_0000_0000)
+		return Word(upperBits) | w
+	}
+	return w
+}
diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json
@@ -140,16 +140,16 @@
     "sourceCodeHash": "0xf4f83ca89d2519045a2916c670bda66f39b431a13921e639a5342bfc6157b178"
   },
   "src/cannon/MIPS.sol": {
-    "initCodeHash": "0x7d0fc7c7b51b74fa2611aaa8cc1a5967e2e48f0726ea894eb2c43f36b0ff2ab7",
-    "sourceCodeHash": "0xc18f51210c9d75f9bc33e55f879e8f0ab2514924718022264c0a2993134821e0"
+    "initCodeHash": "0x9d8a3c089fb84919159403a961fe0514d8be00f07b3a8be1a13a9289cc0ad11a",
+    "sourceCodeHash": "0x0c4805985e4c5b1c670a6ae057f5d330a1c3755216f61e38fe8b564a89b28da3"
   },
   "src/cannon/MIPS2.sol": {
-    "initCodeHash": "0x4e1dbd0a6ac84873622af9234aca42e6e7b2bfda1186bbfd3ff83081f141ef86",
-    "sourceCodeHash": "0x62c820b22c72399efd7688dcf713c34a6ee6821835ec66d5e7b98f33bbbfb209"
+    "initCodeHash": "0x1f4e7cfdbcf7a8ca0ebac69bc7fe74143286a0e51a06ee9cbd699d68efd26dba",
+    "sourceCodeHash": "0x20256a2196daca39b56bfae1c90b8871349916dc47461b5ca078c2013c067571"
   },
   "src/cannon/MIPS64.sol": {
-    "initCodeHash": "0x6c433a3fdba3af72d2d0399572e25b1b878ae53fb13cd237e2f4c1964b51644f",
-    "sourceCodeHash": "0x9d1af96777dc76b215aec7111c4fab2af4116dfefc734908e661ec641421dae2"
+    "initCodeHash": "0x7c957949e0fa8c20bd6ef2dca20e555ff58496a72f287dfee3acc87ae541ac6e",
+    "sourceCodeHash": "0x8ad227488b687501133454920f22c7c1e0261cf2e5641364d8b58ecebf270f86"
   },
   "src/cannon/PreimageOracle.sol": {
     "initCodeHash": "0x17d3b3df1aaaf7a705b8d48de8a05e6511b910fdafdbe5eb7f7f95ec944fba9a",

diff --git a/packages/contracts-bedrock/src/cannon/MIPS.sol b/packages/contracts-bedrock/src/cannon/MIPS.sol
@@ -47,8 +47,8 @@ contract MIPS is ISemver {
     }
 
     /// @notice The semantic version of the MIPS contract.
-    /// @custom:semver 1.3.0-beta.1
-    string public constant version = "1.3.0-beta.1";
+    /// @custom:semver 1.3.0-beta.2
+    string public constant version = "1.3.0-beta.2";
 
     /// @notice The preimage oracle contract.
     IPreimageOracle internal immutable ORACLE;

diff --git a/packages/contracts-bedrock/src/cannon/MIPS2.sol b/packages/contracts-bedrock/src/cannon/MIPS2.sol
@@ -61,8 +61,8 @@ contract MIPS2 is ISemver {
     }
 
     /// @notice The semantic version of the MIPS2 contract.
-    /// @custom:semver 1.0.0-beta.28
-    string public constant version = "1.0.0-beta.28";
+    /// @custom:semver 1.0.0-beta.29
+    string public constant version = "1.0.0-beta.29";
 
     /// @notice The preimage oracle contract.
     IPreimageOracle internal immutable ORACLE;

diff --git a/packages/contracts-bedrock/src/cannon/MIPS64.sol b/packages/contracts-bedrock/src/cannon/MIPS64.sol
@@ -63,8 +63,8 @@ contract MIPS64 is ISemver {
     }
 
     /// @notice The semantic version of the MIPS64 contract.
-    /// @custom:semver 1.0.0-beta.1
-    string public constant version = "1.0.0-beta.1";
+    /// @custom:semver 1.0.0-beta.2
+    string public constant version = "1.0.0-beta.2";
 
     /// @notice The preimage oracle contract.
     IPreimageOracle internal immutable ORACLE;

diff --git a/packages/contracts-bedrock/src/cannon/libraries/MIPS64Instructions.sol b/packages/contracts-bedrock/src/cannon/libraries/MIPS64Instructions.sol
@@ -594,7 +594,7 @@ library MIPS64Instructions {
     /// @notice Extends the value leftwards with its most significant bit (sign extension).
     function signExtend(uint64 _dat, uint64 _idx) internal pure returns (uint64 out_) {
         unchecked {
-            bool isSigned = (_dat >> (_idx - 1)) != 0;
+            bool isSigned = (_dat >> (_idx - 1)) & 1 != 0;
             uint256 signed = ((1 << (arch.WORD_SIZE - _idx)) - 1) << _idx;
             uint256 mask = (1 << _idx) - 1;
             return uint64(_dat & mask | (isSigned ? signed : 0));

diff --git a/packages/contracts-bedrock/src/cannon/libraries/MIPSInstructions.sol b/packages/contracts-bedrock/src/cannon/libraries/MIPSInstructions.sol
@@ -444,7 +444,7 @@ library MIPSInstructions {
     /// @notice Extends the value leftwards with its most significant bit (sign extension).
     function signExtend(uint32 _dat, uint32 _idx) internal pure returns (uint32 out_) {
         unchecked {
-            bool isSigned = (_dat >> (_idx - 1)) != 0;
+            bool isSigned = (_dat >> (_idx - 1)) & 1 != 0;
             uint256 signed = ((1 << (32 - _idx)) - 1) << _idx;
             uint256 mask = (1 << _idx) - 1;
             return uint32(_dat & mask | (isSigned ? signed : 0));