From 5ace65bd9fa76a134ca729b8f0cbc608b40c5734 Mon Sep 17 00:00:00 2001 From: Mohanson Date: Tue, 3 Aug 2021 08:59:04 +0800 Subject: [PATCH] Fix HINTs instruction (#188) * Fix HINTs instruction * Fix ci * Add a simple c.add hint test * Allow some clippy rules --- Makefile | 1 + src/decoder.rs | 10 ++-- src/instructions/b.rs | 2 +- src/instructions/i.rs | 2 +- src/instructions/m.rs | 2 +- src/instructions/mod.rs | 2 +- src/instructions/rvc.rs | 94 +++++++++++++++++++++++++----------- src/machine/aot/mod.rs | 4 +- src/machine/asm/mod.rs | 2 +- src/machine/mod.rs | 2 +- src/machine/trace.rs | 2 +- tests/programs/cadd_hints | Bin 0 -> 960 bytes tests/programs/cadd_hints.S | 6 +++ tests/test_asm.rs | 2 +- tests/test_versions.rs | 16 ++++++ 15 files changed, 104 insertions(+), 43 deletions(-) create mode 100755 tests/programs/cadd_hints create mode 100644 tests/programs/cadd_hints.S diff --git a/Makefile b/Makefile index 0a8c70f3..45abb7b4 100644 --- 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 index cc44f374..d0e256a9 100644 --- a/src/decoder.rs +++ b/src/decoder.rs @@ -14,15 +14,17 @@ const INSTRUCTION_CACHE_SIZE: usize = 4096; pub struct Decoder { factories: Vec, 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(isa: u8) -> Decoder { - let mut decoder = Decoder::new(isa & ISA_MOP != 0); +pub fn build_decoder(isa: u8, version: u32) -> Decoder { + let mut decoder = Decoder::new(isa & ISA_MOP != 0, version); decoder.add_instruction_factory(rvc::factory::); decoder.add_instruction_factory(i::factory::); decoder.add_instruction_factory(m::factory::); diff --git a/src/instructions/b.rs b/src/instructions/b.rs index 91f3d3ff..e344250e 100644 --- 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(instruction_bits: u32) -> Option { +pub fn factory(instruction_bits: u32, _: u32) -> Option { 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 index 4d5353d5..3a4e6e43 100644 --- a/src/instructions/i.rs +++ b/src/instructions/i.rs @@ -31,7 +31,7 @@ impl FenceType { } } -pub fn factory(instruction_bits: u32) -> Option { +pub fn factory(instruction_bits: u32, _: u32) -> Option { 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 index c9e39976..feb441e2 100644 --- 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(instruction_bits: u32) -> Option { +pub fn factory(instruction_bits: u32, _: u32) -> Option { 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 index 716f9b6b..a563e25c 100644 --- 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; +pub type InstructionFactory = fn(instruction_bits: u32, version: u32) -> Option; // 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 index b8e907ce..f0b5b347 100644 --- 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(instruction_bits: u32) -> Option { +pub fn factory(instruction_bits: u32, version: u32) -> Option { let bit_length = R::BITS; if bit_length != 32 && bit_length != 64 { return None; @@ -178,15 +178,27 @@ pub fn factory(instruction_bits: u32) -> Option { 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(instruction_bits: u32) -> Option { 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(instruction_bits: u32) -> Option { ) .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(instruction_bits: u32) -> Option { 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(instruction_bits: u32) -> Option { 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 index 881953ae..5aaa24ee 100644 --- 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::(self.isa()); + let mut decoder = build_decoder::(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 { - let mut decoder = build_decoder::(self.isa()); + let mut decoder = build_decoder::(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 index 7f494d6b..9bba3c9e 100644 --- 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::(self.machine.isa()); + let mut decoder = build_decoder::(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 index 0aebf889..4d7b0b23 100644 --- 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::(self.isa()); + let mut decoder = build_decoder::(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 index 4b9fe06c..c4da905c 100644 --- 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 { - let mut decoder = build_decoder::(self.isa()); + let mut decoder = build_decoder::(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 new file mode 100755 index 0000000000000000000000000000000000000000..0a365e08a4519a5ca060a0fbfcfc091d9b01f634 GIT binary patch literal 960 zcmb_a%}T>S5T2waMDP&tQt>4AElbT&I%n}yum!+bN}eDgyl&%yP%X&9iM3Ev>1MQH(iO8BO9M9zm(%=%!1RjMq~ z{3(Pddlr;a2(BddUyb0+`LuEjJ9}udG#|{t*hbgN<|pm+Xm-fP!VJ*Jq8i)4S!&-^ zNdsu<6;f>F!q-pHl>4dFR>>PsxK+MQyxw1|?Bd)PO@C0+=d6ams^M>H`U7M4qiGzC z*le0kc#^e{345K}4(kRNmwm=C?uH=?`n}SR^PJ^8$~f{>Su%`x1T2{h0lfTa%%d2Q zXA&Qj$CCvIuRMZZQ)g5nKA1^PvIG0^~+sk}nCt@jktchWmlu!b?t0f+b|;X3gYF5!df?v{*n^!b}ei@y4^X{lo^q1XQc2C+-b literal 0 HcmV?d00001 diff --git a/tests/programs/cadd_hints.S b/tests/programs/cadd_hints.S new file mode 100644 index 00000000..469afccb --- /dev/null +++ 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 index 30fa2737..4c58c1f7 100644 --- a/tests/test_asm.rs +++ b/tests/test_asm.rs @@ -402,7 +402,7 @@ pub fn test_asm_step() { .unwrap(); let result = || -> Result { - let mut decoder = build_decoder::(ISA_IMC); + let mut decoder = build_decoder::(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 index 6fe318d5..cd467301 100644 --- 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); +}