Fix HINTs instruction (#188)

* Fix HINTs instruction * Fix ci * Add a simple c.add hint test * Allow some clippy rules
nervosnetwork · Aug 3, 2021 · 5ace65b · 5ace65b
1 parent 89a3779
commit 5ace65b
Show file tree

Hide file tree

Showing 15 changed files with 104 additions and 43 deletions.
diff --git a/Makefile b/Makefile
@@ -22,6 +22,7 @@ fmt:
 clippy_rule = -D warnings \
 	-D clippy::clone_on_ref_ptr \
 	-D clippy::enum_glob_use \
+	-A clippy::collapsible-else-if \
 	-A clippy::upper_case_acronyms \
 	-A clippy::unusual_byte_groupings \
 	-A clippy::inconsistent_digit_grouping \

diff --git a/src/decoder.rs b/src/decoder.rs
@@ -14,15 +14,17 @@ const INSTRUCTION_CACHE_SIZE: usize = 4096;
 pub struct Decoder {
     factories: Vec<InstructionFactory>,
     mop: bool,
+    version: u32,
     // use a cache of instructions to avoid decoding the same instruction twice, pc is the key and the instruction is the value
     instructions_cache: [(u64, u64); INSTRUCTION_CACHE_SIZE],
 }
 
 impl Decoder {
-    pub fn new(mop: bool) -> Decoder {
+    pub fn new(mop: bool, version: u32) -> Decoder {
         Decoder {
             factories: vec![],
             mop,
+            version,
             instructions_cache: [(RISCV_MAX_MEMORY as u64, 0); INSTRUCTION_CACHE_SIZE],
         }
     }
@@ -96,7 +98,7 @@ impl Decoder {
         }
         let instruction_bits = self.decode_bits(memory, pc)?;
         for factory in &self.factories {
-            if let Some(instruction) = factory(instruction_bits) {
+            if let Some(instruction) = factory(instruction_bits, self.version) {
                 self.instructions_cache[instruction_cache_key] = (pc, instruction);
                 return Ok(instruction);
             }
@@ -550,8 +552,8 @@ impl Decoder {
     }
 }
 
-pub fn build_decoder<R: Register>(isa: u8) -> Decoder {
-    let mut decoder = Decoder::new(isa & ISA_MOP != 0);
+pub fn build_decoder<R: Register>(isa: u8, version: u32) -> Decoder {
+    let mut decoder = Decoder::new(isa & ISA_MOP != 0, version);
     decoder.add_instruction_factory(rvc::factory::<R>);
     decoder.add_instruction_factory(i::factory::<R>);
     decoder.add_instruction_factory(m::factory::<R>);

diff --git a/src/instructions/b.rs b/src/instructions/b.rs
@@ -6,7 +6,7 @@ use ckb_vm_definitions::instructions as insts;
 use super::utils::{self, funct3, funct7, opcode, rd, rs1, rs2};
 use super::{set_instruction_length_4, Instruction, Itype, Register, Rtype};
 
-pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
+pub fn factory<R: Register>(instruction_bits: u32, _: u32) -> Option<Instruction> {
     let bit_length = R::BITS;
     if bit_length != 32 && bit_length != 64 {
         return None;

diff --git a/src/instructions/i.rs b/src/instructions/i.rs
@@ -31,7 +31,7 @@ impl FenceType {
     }
 }
 
-pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
+pub fn factory<R: Register>(instruction_bits: u32, _: u32) -> Option<Instruction> {
     let bit_length = R::BITS;
     if bit_length != 32 && bit_length != 64 {
         return None;

diff --git a/src/instructions/m.rs b/src/instructions/m.rs
@@ -3,7 +3,7 @@ use ckb_vm_definitions::instructions as insts;
 use super::utils::{funct3, funct7, opcode, rd, rs1, rs2};
 use super::{set_instruction_length_4, Instruction, Register, Rtype};
 
-pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
+pub fn factory<R: Register>(instruction_bits: u32, _: u32) -> Option<Instruction> {
     let bit_length = R::BITS;
     if bit_length != 32 && bit_length != 64 {
         return None;

diff --git a/src/instructions/mod.rs b/src/instructions/mod.rs
@@ -25,7 +25,7 @@ pub fn extract_opcode(i: Instruction) -> InstructionOpcode {
     (((i >> 8) & 0xff00) | (i & 0x00ff)) as u16
 }
 
-pub type InstructionFactory = fn(instruction_bits: u32) -> Option<Instruction>;
+pub type InstructionFactory = fn(instruction_bits: u32, version: u32) -> Option<Instruction>;
 
 // Blank instructions need no register indices nor immediates, they only have opcode
 // and module bit set.

diff --git a/src/instructions/rvc.rs b/src/instructions/rvc.rs
@@ -90,7 +90,7 @@ fn b_immediate(instruction_bits: u32) -> i32 {
 }
 
 #[allow(clippy::cognitive_complexity)]
-pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
+pub fn factory<R: Register>(instruction_bits: u32, version: u32) -> Option<Instruction> {
     let bit_length = R::BITS;
     if bit_length != 32 && bit_length != 64 {
         return None;
@@ -178,15 +178,27 @@ pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
         0b_000_00000000000_01 => {
             let nzimm = immediate(instruction_bits);
             let rd = rd(instruction_bits);
-            if nzimm != 0 && rd != 0 {
+            if rd != 0 {
                 // C.ADDI
-                Some(Itype::new_s(insts::OP_ADDI, rd, rd, nzimm).0)
-            } else if nzimm == 0 && rd == 0 {
-                // C.NOP
-                Some(nop())
+                if nzimm != 0 {
+                    Some(Itype::new_s(insts::OP_ADDI, rd, rd, nzimm).0)
+                } else if version >= 1 {
+                    // HINTs
+                    Some(nop())
+                } else {
+                    None
+                }
             } else {
-                // Invalid instruction
-                None
+                // C.NOP
+                #[allow(clippy::if_same_then_else)]
+                if nzimm == 0 {
+                    Some(nop())
+                } else if version >= 1 {
+                    // HINTs
+                    Some(nop())
+                } else {
+                    None
+                }
             }
         }
         0b_001_00000000000_01 => {
@@ -208,6 +220,9 @@ pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
             if rd != 0 {
                 // C.LI
                 Some(Itype::new_s(insts::OP_ADDI, rd, 0, immediate(instruction_bits)).0)
+            } else if version >= 1 {
+                // HINTs
+                Some(nop())
             } else {
                 None
             }
@@ -232,11 +247,16 @@ pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
                         )
                         .0,
                     )
-                } else if rd != 0 {
-                    // C.LUI
-                    Some(Utype::new_s(insts::OP_LUI, rd, imm).0)
                 } else {
-                    None
+                    // C.LUI
+                    if rd != 0 {
+                        Some(Utype::new_s(insts::OP_LUI, rd, imm).0)
+                    } else if version >= 1 {
+                        // HINTs
+                        Some(nop())
+                    } else {
+                        None
+                    }
                 }
             } else {
                 None
@@ -365,15 +385,14 @@ pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
         0b_000_00000000000_10 => {
             let uimm = uimmediate(instruction_bits);
             let rd = rd(instruction_bits);
-            if rd == 0 {
-                // Reserved
-                None
-            } else if uimm != 0 {
+            if rd != 0 && uimm != 0 {
                 // C.SLLI
                 Some(Itype::new(insts::OP_SLLI, rd, rd, uimm & u32::from(R::SHIFT_MASK)).0)
-            } else {
-                // C.SLLI64
+            } else if version >= 1 {
+                // HINTs
                 Some(nop())
+            } else {
+                None
             }
         }
         0b_010_00000000000_10 => {
@@ -405,28 +424,45 @@ pub fn factory<R: Register>(instruction_bits: u32) -> Option<Instruction> {
                 0b_0_00000_00000_00 => {
                     let rd = rd(instruction_bits);
                     let rs2 = c_rs2(instruction_bits);
-                    if rd == 0 {
-                        None
-                    } else if rs2 == 0 {
-                        // C.JR
-                        Some(Itype::new_s(insts::OP_JALR, 0, rd, 0).0)
+                    if rs2 == 0 {
+                        if rd != 0 {
+                            // C.JR
+                            Some(Itype::new_s(insts::OP_JALR, 0, rd, 0).0)
+                        } else {
+                            // Reserved
+                            None
+                        }
                     } else {
-                        // C.MV
-                        Some(Rtype::new(insts::OP_ADD, rd, 0, rs2).0)
+                        if rd != 0 {
+                            // C.MV
+                            Some(Rtype::new(insts::OP_ADD, rd, 0, rs2).0)
+                        } else if version >= 1 {
+                            // HINTs
+                            Some(nop())
+                        } else {
+                            None
+                        }
                     }
                 }
                 0b_1_00000_00000_00 => {
                     let rd = rd(instruction_bits);
                     let rs2 = c_rs2(instruction_bits);
                     match (rd, rs2) {
-                        // C.BREAK
+                        // C.EBREAK
                         (0, 0) => Some(blank_instruction(insts::OP_EBREAK)),
                         // C.JALR
                         (rs1, 0) => Some(Itype::new_s(insts::OP_JALR, 1, rs1, 0).0),
                         // C.ADD
-                        (rd, rs2) if rd != 0 => Some(Rtype::new(insts::OP_ADD, rd, rd, rs2).0),
-                        // Invalid instruction
-                        _ => None,
+                        (rd, rs2) => {
+                            if rd != 0 {
+                                Some(Rtype::new(insts::OP_ADD, rd, rd, rs2).0)
+                            } else if version >= 1 {
+                                // HINTs
+                                Some(nop())
+                            } else {
+                                None
+                            }
+                        }
                     }
                 }
                 _ => unreachable!(),

diff --git a/src/machine/aot/mod.rs b/src/machine/aot/mod.rs
@@ -200,7 +200,7 @@ impl LabelGatheringMachine {
     }
 
     pub fn gather(&mut self) -> Result<(), Error> {
-        let mut decoder = build_decoder::<u64>(self.isa());
+        let mut decoder = build_decoder::<u64>(self.isa(), self.version());
         for i in 0..self.sections.len() {
             let (section_start, section_end) = self.sections[i];
             self.pc = Value::from_u64(section_start);
@@ -565,7 +565,7 @@ impl AotCompilingMachine {
     }
 
     pub fn compile(&mut self) -> Result<AotCode, Error> {
-        let mut decoder = build_decoder::<u64>(self.isa());
+        let mut decoder = build_decoder::<u64>(self.isa(), self.version());
         let mut instructions = [Instruction::default(); MAXIMUM_INSTRUCTIONS_PER_BLOCK];
         for i in 0..self.sections.len() {
             let (section_start, section_end) = self.sections[i];

diff --git a/src/machine/asm/mod.rs b/src/machine/asm/mod.rs
@@ -444,7 +444,7 @@ impl<'a> AsmMachine<'a> {
         if self.machine.isa() & ISA_MOP != 0 && self.machine.version() == VERSION0 {
             return Err(Error::InvalidVersion);
         }
-        let mut decoder = build_decoder::<u64>(self.machine.isa());
+        let mut decoder = build_decoder::<u64>(self.machine.isa(), self.machine.version());
         self.machine.set_running(true);
         while self.machine.running() {
             if self.machine.reset_signal() {

diff --git a/src/machine/mod.rs b/src/machine/mod.rs
@@ -566,7 +566,7 @@ impl<'a, Inner: SupportMachine> DefaultMachine<'a, Inner> {
         if self.isa() & ISA_MOP != 0 && self.version() == VERSION0 {
             return Err(Error::InvalidVersion);
         }
-        let mut decoder = build_decoder::<Inner::REG>(self.isa());
+        let mut decoder = build_decoder::<Inner::REG>(self.isa(), self.version());
         self.set_running(true);
         while self.running() {
             if self.reset_signal() {

diff --git a/src/machine/trace.rs b/src/machine/trace.rs
@@ -102,7 +102,7 @@ impl<'a, Inner: SupportMachine> TraceMachine<'a, Inner> {
     }
 
     pub fn run(&mut self) -> Result<i8, Error> {
-        let mut decoder = build_decoder::<Inner::REG>(self.isa());
+        let mut decoder = build_decoder::<Inner::REG>(self.isa(), self.version());
         self.machine.set_running(true);
         // For current trace size this is acceptable, however we might want
         // to tweak the code here if we choose to use a larger trace size or

diff --git a/tests/programs/cadd_hints b/tests/programs/cadd_hints
diff --git a/tests/programs/cadd_hints.S b/tests/programs/cadd_hints.S
@@ -0,0 +1,6 @@
+.global _start
+_start:
+  c.add zero, a0
+  li a0, 0
+  li a7, 93
+  ecall
diff --git a/tests/test_asm.rs b/tests/test_asm.rs
@@ -402,7 +402,7 @@ pub fn test_asm_step() {
         .unwrap();
 
     let result = || -> Result<i8, Error> {
-        let mut decoder = build_decoder::<u64>(ISA_IMC);
+        let mut decoder = build_decoder::<u64>(ISA_IMC, VERSION0);
         machine.machine.set_running(true);
         while machine.machine.running() {
             machine.step(&mut decoder)?;

diff --git a/tests/test_versions.rs b/tests/test_versions.rs
@@ -464,3 +464,19 @@ pub fn test_asm_version1_goblin_overflow_elf() {
     let machine = create_asm_machine("goblin_overflow_elf".to_string(), VERSION1);
     assert_eq!(machine.machine.version(), VERSION1);
 }
+
+#[test]
+pub fn test_asm_version0_cadd_hints() {
+    let mut machine = create_rust_machine("cadd_hints".to_string(), VERSION0);
+    let result = machine.run();
+    assert!(result.is_err());
+    assert_eq!(result.unwrap_err(), Error::InvalidInstruction(36906));
+}
+
+#[test]
+pub fn test_asm_version1_cadd_hints() {
+    let mut machine = create_rust_machine("cadd_hints".to_string(), VERSION1);
+    let result = machine.run();
+    assert!(result.is_ok());
+    assert_eq!(result.unwrap(), 0);
+}