Skip to content

Commit

Permalink
fix: update indirect for SET opcode
Browse files Browse the repository at this point in the history
  • Loading branch information
@IlyasRidhuan
IlyasRidhuan committed Mar 1, 2024
1 parent 699a32c commit 75a8dbf
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 24 deletions.
21 changes: 13 additions & 8 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_deserialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ std::vector<Instruction> Deserialization::parse(std::vector<uint8_t> const& byte
std::vector<OperandType> 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));
}

Expand All @@ -86,29 +90,30 @@ std::vector<Instruction> Deserialization::parse(std::vector<uint8_t> const& byte
static_cast<uint8_t>(AvmMemoryTag::U32),
static_cast<uint8_t>(AvmMemoryTag::U64),
static_cast<uint8_t>(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<AvmMemoryTag>(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";
Expand Down
16 changes: 9 additions & 7 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_execution.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,24 +114,25 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
case OpCode::SET: {
uint32_t dst_offset = 0;
uint128_t val = 0;
AvmMemoryTag in_tag = std::get<AvmMemoryTag>(inst.operands.at(0));
dst_offset = std::get<uint32_t>(inst.operands.at(2));
// Skip the indirect flag at index 0;
AvmMemoryTag in_tag = std::get<AvmMemoryTag>(inst.operands.at(1));
dst_offset = std::get<uint32_t>(inst.operands.at(3));

switch (in_tag) {
case AvmMemoryTag::U8:
val = std::get<uint8_t>(inst.operands.at(1));
val = std::get<uint8_t>(inst.operands.at(2));
break;
case AvmMemoryTag::U16:
val = std::get<uint16_t>(inst.operands.at(1));
val = std::get<uint16_t>(inst.operands.at(2));
break;
case AvmMemoryTag::U32:
val = std::get<uint32_t>(inst.operands.at(1));
val = std::get<uint32_t>(inst.operands.at(2));
break;
case AvmMemoryTag::U64:
val = std::get<uint64_t>(inst.operands.at(1));
val = std::get<uint64_t>(inst.operands.at(2));
break;
case AvmMemoryTag::U128:
val = std::get<uint128_t>(inst.operands.at(1));
val = std::get<uint128_t>(inst.operands.at(2));
break;
default:
break;
Expand All @@ -142,6 +143,7 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
}
// Control Flow - Contract Calls
case OpCode::RETURN:
// Skip indirect at index 0
trace_builder.return_op(std::get<uint32_t>(inst.operands.at(1)), std::get<uint32_t>(inst.operands.at(2)));
break;
default:
Expand Down
29 changes: 20 additions & 9 deletions barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -120,15 +122,17 @@ TEST_F(AvmExecutionTests, setAndSubOpcodes)
EXPECT_THAT(instructions.at(0),
AllOf(Field(&Instruction::op_code, OpCode::SET),
Field(&Instruction::operands,
ElementsAre(VariantWith<AvmMemoryTag>(AvmMemoryTag::U16),
ElementsAre(VariantWith<uint8_t>(0),
VariantWith<AvmMemoryTag>(AvmMemoryTag::U16),
VariantWith<uint16_t>(47123),
VariantWith<uint32_t>(170)))));

// SET
EXPECT_THAT(instructions.at(1),
AllOf(Field(&Instruction::op_code, OpCode::SET),
Field(&Instruction::operands,
ElementsAre(VariantWith<AvmMemoryTag>(AvmMemoryTag::U16),
ElementsAre(VariantWith<uint8_t>(0),
VariantWith<AvmMemoryTag>(AvmMemoryTag::U16),
VariantWith<uint16_t>(37123),
VariantWith<uint32_t>(51)))));

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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"; //

Expand All @@ -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");
Expand Down

0 comments on commit 75a8dbf

Please sign in to comment.