diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_deserialization.cpp index 58c62049bb4..00efac80474 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_deserialization.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_deserialization.cpp @@ -77,7 +77,11 @@ std::vector Deserialization::parse(std::vector const& byte std::vector inst_format; if (opcode == OpCode::SET) { - if (pos == length) { + // Small hack here because of the structure of SET (where Indirect is the first flag). + // Right now pos is pointing to the indirect flag, but we want it to point to the memory tag. + // We cannot increment pos again because we need to read from pos later when parsing the SET opcode + // So we effectively peek at the next pos + if (pos + 1 == length) { throw_or_abort("Operand for SET opcode is missing at position " + std::to_string(pos)); } @@ -86,29 +90,30 @@ std::vector Deserialization::parse(std::vector const& byte static_cast(AvmMemoryTag::U32), static_cast(AvmMemoryTag::U64), static_cast(AvmMemoryTag::U128) }; - uint8_t set_tag_u8 = bytecode.at(pos); + // Peek again here for the mem tag + uint8_t set_tag_u8 = bytecode.at(pos + 1); if (!valid_tags.contains(set_tag_u8)) { - throw_or_abort("Instruction tag for SET opcode is invalid at position " + std::to_string(pos) + + throw_or_abort("Instruction tag for SET opcode is invalid at position " + std::to_string(pos + 1) + " value: " + std::to_string(set_tag_u8)); } auto in_tag = static_cast(set_tag_u8); switch (in_tag) { case AvmMemoryTag::U8: - inst_format = { OperandType::TAG, OperandType::UINT8, OperandType::UINT32 }; + inst_format = { OperandType::INDIRECT, OperandType::TAG, OperandType::UINT8, OperandType::UINT32 }; break; case AvmMemoryTag::U16: - inst_format = { OperandType::TAG, OperandType::UINT16, OperandType::UINT32 }; + inst_format = { OperandType::INDIRECT, OperandType::TAG, OperandType::UINT16, OperandType::UINT32 }; break; case AvmMemoryTag::U32: - inst_format = { OperandType::TAG, OperandType::UINT32, OperandType::UINT32 }; + inst_format = { OperandType::INDIRECT, OperandType::TAG, OperandType::UINT32, OperandType::UINT32 }; break; case AvmMemoryTag::U64: - inst_format = { OperandType::TAG, OperandType::UINT64, OperandType::UINT32 }; + inst_format = { OperandType::INDIRECT, OperandType::TAG, OperandType::UINT64, OperandType::UINT32 }; break; case AvmMemoryTag::U128: - inst_format = { OperandType::TAG, OperandType::UINT128, OperandType::UINT32 }; + inst_format = { OperandType::INDIRECT, OperandType::TAG, OperandType::UINT128, OperandType::UINT32 }; break; default: // This branch is guarded above. std::cerr << "This code branch must have been guarded by the tag validation. \n"; diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_execution.cpp index 8bff2e164b6..98934051ee7 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_execution.cpp @@ -114,24 +114,25 @@ std::vector Execution::gen_trace(std::vector const& instructio case OpCode::SET: { uint32_t dst_offset = 0; uint128_t val = 0; - AvmMemoryTag in_tag = std::get(inst.operands.at(0)); - dst_offset = std::get(inst.operands.at(2)); + // Skip the indirect flag at index 0; + AvmMemoryTag in_tag = std::get(inst.operands.at(1)); + dst_offset = std::get(inst.operands.at(3)); switch (in_tag) { case AvmMemoryTag::U8: - val = std::get(inst.operands.at(1)); + val = std::get(inst.operands.at(2)); break; case AvmMemoryTag::U16: - val = std::get(inst.operands.at(1)); + val = std::get(inst.operands.at(2)); break; case AvmMemoryTag::U32: - val = std::get(inst.operands.at(1)); + val = std::get(inst.operands.at(2)); break; case AvmMemoryTag::U64: - val = std::get(inst.operands.at(1)); + val = std::get(inst.operands.at(2)); break; case AvmMemoryTag::U128: - val = std::get(inst.operands.at(1)); + val = std::get(inst.operands.at(2)); break; default: break; @@ -142,6 +143,7 @@ std::vector Execution::gen_trace(std::vector const& instructio } // Control Flow - Contract Calls case OpCode::RETURN: + // Skip indirect at index 0 trace_builder.return_op(std::get(inst.operands.at(1)), std::get(inst.operands.at(2))); break; default: diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp index 2ed7dbc45dd..b180ef83a75 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp @@ -93,10 +93,12 @@ TEST_F(AvmExecutionTests, basicAddReturn) TEST_F(AvmExecutionTests, setAndSubOpcodes) { std::string bytecode_hex = to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "02" // U16 "B813" // val 47123 "000000AA" // dst_offset 170 + to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "02" // U16 "9103" // val 37123 "00000033" // dst_offset 51 @@ -120,7 +122,8 @@ TEST_F(AvmExecutionTests, setAndSubOpcodes) EXPECT_THAT(instructions.at(0), AllOf(Field(&Instruction::op_code, OpCode::SET), Field(&Instruction::operands, - ElementsAre(VariantWith(AvmMemoryTag::U16), + ElementsAre(VariantWith(0), + VariantWith(AvmMemoryTag::U16), VariantWith(47123), VariantWith(170))))); @@ -128,7 +131,8 @@ TEST_F(AvmExecutionTests, setAndSubOpcodes) EXPECT_THAT(instructions.at(1), AllOf(Field(&Instruction::op_code, OpCode::SET), Field(&Instruction::operands, - ElementsAre(VariantWith(AvmMemoryTag::U16), + ElementsAre(VariantWith(0), + VariantWith(AvmMemoryTag::U16), VariantWith(37123), VariantWith(51))))); @@ -160,11 +164,13 @@ TEST_F(AvmExecutionTests, setAndSubOpcodes) TEST_F(AvmExecutionTests, powerWithMulOpcodes) { std::string bytecode_hex = to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "04" // U64 "00000000" // val 5 higher 32 bits "00000005" // val 5 lower 32 bits "00000000" // dst_offset 0 + to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "04" // U64 "00000000" // val 1 higher 32 bits "00000001" // val 1 lower 32 bits @@ -241,6 +247,7 @@ TEST_F(AvmExecutionTests, powerWithMulOpcodes) TEST_F(AvmExecutionTests, simpleInternalCall) { std::string bytecode_hex = to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "03" // U32 "0D3D2518" // val 222111000 = 0xD3D2518 "00000004" // dst_offset 4 @@ -257,6 +264,7 @@ TEST_F(AvmExecutionTests, simpleInternalCall) "00000000" // ret offset 0 "00000000" // ret size 0 + to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "03" // U32 "075BCD15" // val 123456789 = 0x75BCD15 "00000007" // dst_offset 7 @@ -315,6 +323,7 @@ TEST_F(AvmExecutionTests, nestedInternalCalls) auto setInstructionHex = [](std::string const& val, std::string const& dst_offset) { return to_hex(OpCode::SET) // opcode SET + + "00" // Indirect flag + "01" // U8 + val + "000000" + dst_offset; }; @@ -493,6 +502,7 @@ TEST_F(AvmExecutionTests, ffInstructionTagSetOpcode) "00000009" // addr b 9 "00000001" // addr c 1 + to_hex(OpCode::SET) + // opcode SET + "00" // Indirect flag "06" // tag FF "00002344"; // @@ -503,13 +513,14 @@ TEST_F(AvmExecutionTests, ffInstructionTagSetOpcode) // Negative test detecting SET opcode without any operand. TEST_F(AvmExecutionTests, SetOpcodeNoOperand) { - std::string bytecode_hex = "00" // ADD - "00" // Indirect flag - "05" // U128 - "00000007" // addr a 7 - "00000009" // addr b 9 - "00000001" // addr c 1 - + to_hex(OpCode::SET); // opcode SET + std::string bytecode_hex = "00" // ADD + "00" // Indirect flag + "05" // U128 + "00000007" // addr a 7 + "00000009" // addr b 9 + "00000001" // addr c 1 + + to_hex(OpCode::SET) + // opcode SET + "00"; // Indirect flag auto bytecode = hex_to_bytes(bytecode_hex); EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Operand for SET opcode is missing");