diff --git a/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.cpp b/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.cpp index 9ea8f9265146..1b402d051f43 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.cpp @@ -250,7 +250,7 @@ std::string TaggedValue::to_string() const [](const uint1_t& val) -> std::string { return val.value() == 0 ? "0" : "1"; }, [](auto&& val) -> std::string { return std::to_string(val); } }, value); - return "TaggedValue(" + v + ", " + std::to_string(get_tag()) + ")"; + return std::to_string(get_tag()) + "(" + v + ")"; } } // namespace bb::avm2 diff --git a/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp b/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp index 247d5e00ef95..e2d62579099f 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp @@ -22,6 +22,25 @@ enum class ValueTag { MAX = U128, }; +template ValueTag tag_for_type() +{ + if constexpr (std::is_same_v) { + return ValueTag::FF; + } else if constexpr (std::is_same_v) { + return ValueTag::U1; + } else if constexpr (std::is_same_v) { + return ValueTag::U8; + } else if constexpr (std::is_same_v) { + return ValueTag::U16; + } else if constexpr (std::is_same_v) { + return ValueTag::U32; + } else if constexpr (std::is_same_v) { + return ValueTag::U64; + } else if constexpr (std::is_same_v) { + return ValueTag::U128; + } +} + class TaggedValue { public: // We are using variant to avoid heap allocations at the cost of a bigger memory footprint. @@ -70,7 +89,8 @@ class TaggedValue { if (std::holds_alternative(value)) { return std::get(value); } - throw std::runtime_error("TaggedValue::as(): type mismatch"); + throw std::runtime_error("TaggedValue::as(): type mismatch. Wanted type " + + std::to_string(static_cast(tag_for_type())) + " but got " + to_string()); } std::size_t hash() const noexcept { return std::hash{}(as_ff()); } diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.cpp index 02cfb1b0151d..0e5d7ee2d456 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.cpp @@ -30,49 +30,72 @@ std::vector Addressing::resolve(const Instruction& instruction, MemoryI assert(spec.num_addresses <= instruction.operands.size()); // We retrieve, cache first because this is probably what we'll do in the circuit. - // However, we can't check the value and tag yet! This should be done only if it's used. + // However, we can't check that the base address is valid* yet! This should be done only if it's used. // This is because the first few instructions might not YET have a valid stack pointer. - auto base_address = memory.get(0); + // *valid = valid_address and valid_tag. + MemoryValue base_address = memory.get(0); event.base_address = base_address; - // First process relative addressing for all the addresses. + // First, we make sure that the operands themselves are valid addresses. + for (size_t i = 0; i < spec.num_addresses; ++i) { + // We are a bit flexible here, we don't check the tag, because we might get a U8 or U16 from the + // wire format but would want to interpret it as a MemoryAddress. We only need to check that "it fits". + // That's why we use as_ff() here. + if (!memory.is_valid_address(instruction.operands[i].as_ff())) { + throw AddressingException(AddressingEventError::OPERAND_INVALID_ADDRESS, i); + } + } + + // Guarantees by this point: + // - all operands are valid addresses IF interpreted as a MemoryAddress. + + // Then, we process relative addressing for all the addresses. + // That is, if relative addressing is used, after_relative[i] = base_address + operands[i]. + // We fist store the operands as is, and then we'll update them if they are relative. event.after_relative = instruction.operands; for (size_t i = 0; i < spec.num_addresses; ++i) { - if ((instruction.indirect >> i) & 1) { + if ((instruction.indirect >> (i + spec.num_addresses)) & 1) { if (!memory.is_valid_address(base_address)) { throw AddressingException(AddressingEventError::BASE_ADDRESS_INVALID_ADDRESS, i); } + // We extend the address to FF to avoid overflows. FF offset = event.after_relative[i]; - offset = offset + base_address; - event.after_relative[i] = Operand::from(offset); + offset += base_address; + // We store the offset as an FF operand. If the circuit needs to prove overflow, it will + // need the full value. + event.after_relative[i] = Operand::from(offset); if (!memory.is_valid_address(offset)) { + // If this happens, it means that the relative computation overflowed. However both the base and + // operand addresses by themselves were valid. throw AddressingException(AddressingEventError::RELATIVE_COMPUTATION_OOB, i); } } + // Now that we are sure that the offset is valid, we can update the value to be of the right type. + event.after_relative[i] = Operand::from(static_cast(event.after_relative[i].as_ff())); } + // Guarantees by this point: + // - all operands are valid addresses IF interpreted as MemoryAddress. + // - all after_relative values are valid addresses. + // Then indirection. + // That is, if indirection is used, resolved_operands[i] = memory[after_relative[i]]. + // We first store the after_relative values as is, and then we'll update them if they are indirect. event.resolved_operands = event.after_relative; for (size_t i = 0; i < spec.num_addresses; ++i) { - if ((instruction.indirect >> (i + spec.num_addresses)) & 1) { - FF offset = event.resolved_operands[i]; - if (!memory.is_valid_address(offset)) { + if ((instruction.indirect >> i) & 1) { + event.resolved_operands[i] = memory.get(event.after_relative[i].as()); + if (!memory.is_valid_address(event.resolved_operands[i])) { throw AddressingException(AddressingEventError::INDIRECT_INVALID_ADDRESS, i); } - FF new_address = memory.get(static_cast(offset)); - event.resolved_operands[i] = Operand::from(new_address); } } - // We guarantee that the resolved operands are valid addresses. - for (size_t i = 0; i < spec.num_addresses; ++i) { - if (!memory.is_valid_address(MemoryValue(event.resolved_operands[i]))) { - throw AddressingException(AddressingEventError::FINAL_ADDRESS_INVALID, i); - } - event.resolved_operands[i] = - Operand::from(static_cast(event.resolved_operands[i].as_ff())); - } + // Guarantees by this point: + // - all operands are valid addresses. + // - all after_relative values are valid addresses. + // - all resolved_operands are valid addresses. } catch (const AddressingException& e) { event.error = e; } diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.test.cpp new file mode 100644 index 000000000000..20f79d9b4f85 --- /dev/null +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.test.cpp @@ -0,0 +1,199 @@ +#include "barretenberg/vm2/simulation/addressing.hpp" + +#include +#include + +#include "barretenberg/vm2/common/memory_types.hpp" +#include "barretenberg/vm2/common/opcodes.hpp" +#include "barretenberg/vm2/simulation/events/event_emitter.hpp" +#include "barretenberg/vm2/simulation/lib/instruction_info.hpp" +#include "barretenberg/vm2/simulation/lib/serialization.hpp" +#include "barretenberg/vm2/simulation/memory.hpp" +#include "barretenberg/vm2/simulation/testing/mock_memory.hpp" +#include "barretenberg/vm2/testing/instruction_builder.hpp" + +namespace bb::avm2::simulation { +namespace { + +using ::bb::avm2::testing::InstructionBuilder; +using ::bb::avm2::testing::OperandBuilder; +using enum ::bb::avm2::WireOpCode; +using ::testing::ElementsAre; +using ::testing::ReturnRef; +using ::testing::StrictMock; + +template auto from = ::bb::avm2::simulation::Operand::from; + +TEST(AvmSimulationAddressingTest, AllDirectAndNonRelative) +{ + InstructionInfoDB instruction_info_db; + NoopEventEmitter event_emitter; + Addressing addressing(instruction_info_db, event_emitter); + MemoryValue zero_u32 = MemoryValue::from(0); + + { + const auto instr = InstructionBuilder(SET_8) + // dstOffset + .operand(1) + // tag + .operand(MemoryTag::FF) + // value + .operand(1) + .build(); + + StrictMock memory; + EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(zero_u32)); // call to get the base address. + + const auto operands = addressing.resolve(instr, memory); + EXPECT_THAT(operands, + ElementsAre( + // dstOffset has been resolved and is now a MemoryAddress. + from(1), + // tag is unchanged. + instr.operands.at(1), + // value is unchanged. + instr.operands.at(2))); + } + { + const auto instr = InstructionBuilder(ADD_16) + // aOffset + .operand(1) + // bOffset + .operand(2) + // dstOffset + .operand(3) + .build(); + + StrictMock memory; + EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(zero_u32)); // call to get the base address. + + const auto operands = addressing.resolve(instr, memory); + EXPECT_THAT(operands, ElementsAre(from(1), from(2), from(3))); + } +} + +TEST(AvmSimulationAddressingTest, RelativeAddressing) +{ + InstructionInfoDB instruction_info_db; + NoopEventEmitter event_emitter; + Addressing addressing(instruction_info_db, event_emitter); + + // For relative addressing, we need a base address other than 0 + // Base pointer at address 100 + MemoryValue base_addr = MemoryValue::from(100); + + // Set up the ADD_8 instruction with relative addressing + const auto instr = InstructionBuilder(ADD_8) + // aOffset + .operand(10) + .relative() + // bOffset + .operand(20) + // dstOffset + .operand(30) + .relative() + .build(); + + StrictMock memory; + EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(base_addr)); + + const auto operands = addressing.resolve(instr, memory); + + EXPECT_THAT(operands, + ElementsAre( + // aOffset resolved as base + 10 = 110 + from(110), + // bOffset stays the same + from(20), + // dstOffset resolved as base + 30 = 130 + from(130))); +} + +TEST(AvmSimulationAddressingTest, IndirectAddressing) +{ + InstructionInfoDB instruction_info_db; + NoopEventEmitter event_emitter; + Addressing addressing(instruction_info_db, event_emitter); + + // For indirect addressing, memory locations contain the actual addresses + MemoryValue zero_u32 = MemoryValue::from(0); + + // Set up the ADD_8 instruction with indirect addressing + const auto instr = InstructionBuilder(ADD_8) + // aOffset - address 5 contains the actual address + .operand(5) + .indirect() + // bOffset + .operand(10) + // dstOffset - address 15 contains the actual address + .operand(15) + .indirect() + .build(); + + StrictMock memory; + EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(zero_u32)); // Base address + MemoryValue addr_5_value = MemoryValue::from(50); + EXPECT_CALL(memory, get(5)).WillOnce(ReturnRef(addr_5_value)); + MemoryValue addr_15_value = MemoryValue::from(60); + EXPECT_CALL(memory, get(15)).WillOnce(ReturnRef(addr_15_value)); + + const auto operands = addressing.resolve(instr, memory); + + // Expect indirect offsets to be resolved by looking up their values in memory + EXPECT_THAT(operands, + ElementsAre( + // aOffset resolved indirectly: memory[5] = 50 + from(50), + // bOffset stays the same + from(10), + // dstOffset resolved indirectly: memory[15] = 60 + from(60))); +} + +TEST(AvmSimulationAddressingTest, IndirectAndRelativeAddressing) +{ + InstructionInfoDB instruction_info_db; + NoopEventEmitter event_emitter; + Addressing addressing(instruction_info_db, event_emitter); + + // Base address is 100 + MemoryValue base_addr = MemoryValue::from(100); + + // Set up the ADD_8 instruction with both indirect and relative addressing + const auto instr = InstructionBuilder(ADD_8) + // aOffset (indirect and relative): address base+5 contains value + .operand(5) + .indirect() + .relative() + // bOffset (indirect only): address 10 contains value + .operand(10) + .indirect() + // dstOffset (relative only) + .operand(15) + .relative() + .build(); + + StrictMock memory; + EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(base_addr)); // Base address (100) + // Address 105 (base+5) contains value 200 + MemoryValue addr_105_value = MemoryValue::from(200); + EXPECT_CALL(memory, get(105)).WillOnce(ReturnRef(addr_105_value)); + // Address 10 contains value 60 + MemoryValue addr_10_value = MemoryValue::from(60); + EXPECT_CALL(memory, get(10)).WillOnce(ReturnRef(addr_10_value)); + + const auto operands = addressing.resolve(instr, memory); + + // Expect combined indirect and relative addressing + EXPECT_THAT(operands, + ElementsAre( + // aOffset: relative (base+5=105) and indirect (memory[105]=200) + from(200), + // bOffset: indirect only (memory[10]=60) + from(60), + // dstOffset: relative only (base+15=115) + from(115))); +} + +} // namespace +} // namespace bb::avm2::simulation diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/events/addressing_event.hpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/events/addressing_event.hpp index d3c292f80e6a..ff80962ab0ca 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/events/addressing_event.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/events/addressing_event.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "barretenberg/vm2/common/instruction_spec.hpp" @@ -12,16 +13,36 @@ namespace bb::avm2::simulation { enum class AddressingEventError { + // The operand is not a valid address. + OPERAND_INVALID_ADDRESS, + // The base address is not a valid address. BASE_ADDRESS_INVALID_ADDRESS, + // The relative computation overflowed. RELATIVE_COMPUTATION_OOB, + // The address obtained after applying indirection is not a valid address. INDIRECT_INVALID_ADDRESS, - FINAL_ADDRESS_INVALID, }; +inline std::string to_string(AddressingEventError e) +{ + switch (e) { + case AddressingEventError::OPERAND_INVALID_ADDRESS: + return "OPERAND_INVALID_ADDRESS"; + case AddressingEventError::BASE_ADDRESS_INVALID_ADDRESS: + return "BASE_ADDRESS_INVALID_ADDRESS"; + case AddressingEventError::RELATIVE_COMPUTATION_OOB: + return "RELATIVE_COMPUTATION_OOB"; + case AddressingEventError::INDIRECT_INVALID_ADDRESS: + return "INDIRECT_INVALID_ADDRESS"; + } + + // Only to please the compiler. + return "UNKNOWN_ADDRESSING_ERROR"; +} + struct AddressingException : public std::runtime_error { explicit AddressingException(AddressingEventError e, size_t operand_idx = 0) - : std::runtime_error("Addressing error: " + std::to_string(static_cast(e)) + " at operand " + - std::to_string(operand_idx)) + : std::runtime_error("Addressing error: " + to_string(e) + " at operand " + std::to_string(operand_idx)) , error(e) , operand_idx(operand_idx) {} diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp index b78b77ca983f..bd643be86e53 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp @@ -196,7 +196,8 @@ inline void Execution::call_with_operands(void (Execution::*f)(ContextInterface& assert(resolved_operands.size() == sizeof...(Ts)); auto operand_indices = std::make_index_sequence{}; [f, this, &context, &resolved_operands](std::index_sequence) { - (this->*f)(context, resolved_operands.at(Is).template as()...); + // FIXME(fcarreiro): we go through FF here. + (this->*f)(context, static_cast(resolved_operands.at(Is).as_ff())...); }(operand_indices); } diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.cpp b/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.cpp new file mode 100644 index 000000000000..4c559e2b7106 --- /dev/null +++ b/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.cpp @@ -0,0 +1,27 @@ +#include "barretenberg/vm2/testing/instruction_builder.hpp" + +namespace bb::avm2::testing { + +simulation::Instruction InstructionBuilder::build() const +{ + // First we compute the indirect and relative contributions of each operand. + uint16_t indirect = 0; + for (size_t i = 0; i < operands.size(); ++i) { + if (operands[i].is_relative) { + indirect |= static_cast(1 << (i + operands.size())); + } + if (operands[i].is_indirect) { + indirect |= static_cast(1 << i); + } + } + + // Then we build the instruction. + std::vector operands_vec; + operands_vec.reserve(operands.size()); + for (const auto& operand : operands) { + operands_vec.push_back(operand.operand); + } + return simulation::Instruction(opcode, indirect, std::move(operands_vec)); +} + +} // namespace bb::avm2::testing diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.hpp b/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.hpp new file mode 100644 index 000000000000..d988089708e5 --- /dev/null +++ b/barretenberg/cpp/src/barretenberg/vm2/testing/instruction_builder.hpp @@ -0,0 +1,70 @@ +#pragma once + +#include "barretenberg/vm2/common/opcodes.hpp" +#include "barretenberg/vm2/simulation/lib/serialization.hpp" + +namespace bb::avm2::testing { + +struct OperandBuilder { + OperandBuilder(simulation::Operand operand) + : operand(operand) + {} + + template static OperandBuilder from(T value) + { + return OperandBuilder(simulation::Operand::from(value)); + } + + OperandBuilder& indirect() + { + is_indirect = true; + return *this; + } + OperandBuilder& relative() + { + is_relative = true; + return *this; + } + + simulation::Operand operand; + bool is_relative = false; + bool is_indirect = false; +}; + +class InstructionBuilder { + private: + WireOpCode opcode; + std::vector operands; + + public: + InstructionBuilder(WireOpCode opcode) + : opcode(opcode) + {} + + InstructionBuilder& operand(OperandBuilder operand) + { + operands.push_back(operand); + return *this; + } + template InstructionBuilder& operand(T value) { return operand(OperandBuilder::from(value)); } + InstructionBuilder& operand(MemoryTag tag) + { + return operand(OperandBuilder::from(static_cast(tag))); + } + + // We forward these to the last operand built, for convenience. + InstructionBuilder& indirect() + { + operands.back().indirect(); + return *this; + } + InstructionBuilder& relative() + { + operands.back().relative(); + return *this; + } + + simulation::Instruction build() const; +}; + +} // namespace bb::avm2::testing