fix(avm): cpp addressing

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

barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp

@@ -22,6 +22,25 @@ enum class ValueTag {
     MAX = U128,
 };
 
+template <typename T> ValueTag tag_for_type()
+{
+    if constexpr (std::is_same_v<T, FF>) {
+        return ValueTag::FF;
+    } else if constexpr (std::is_same_v<T, uint1_t>) {
+        return ValueTag::U1;
+    } else if constexpr (std::is_same_v<T, uint8_t>) {
+        return ValueTag::U8;
+    } else if constexpr (std::is_same_v<T, uint16_t>) {
+        return ValueTag::U16;
+    } else if constexpr (std::is_same_v<T, uint32_t>) {
+        return ValueTag::U32;
+    } else if constexpr (std::is_same_v<T, uint64_t>) {
+        return ValueTag::U64;
+    } else if constexpr (std::is_same_v<T, uint128_t>) {
+        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<T>(value)) {
             return std::get<T>(value);
         }
-        throw std::runtime_error("TaggedValue::as(): type mismatch");
+        throw std::runtime_error("TaggedValue::as(): type mismatch. Wanted type " +
+                                 std::to_string(static_cast<uint32_t>(tag_for_type<T>())) + " but got " + to_string());
     }
 
     std::size_t hash() const noexcept { return std::hash<FF>{}(as_ff()); }

barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.cpp

@@ -30,49 +30,72 @@ std::vector<Operand> 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<FF>(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<MemoryAddress>(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<MemoryAddress>());
+                if (!memory.is_valid_address(event.resolved_operands[i])) {
                     throw AddressingException(AddressingEventError::INDIRECT_INVALID_ADDRESS, i);
                 }
-                FF new_address = memory.get(static_cast<MemoryAddress>(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<uint32_t>(static_cast<MemoryAddress>(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;
     }

barretenberg/cpp/src/barretenberg/vm2/simulation/addressing.test.cpp

@@ -0,0 +1,199 @@
+#include "barretenberg/vm2/simulation/addressing.hpp"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#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 <typename T> auto from = ::bb::avm2::simulation::Operand::from<T>;
+
+TEST(AvmSimulationAddressingTest, AllDirectAndNonRelative)
+{
+    InstructionInfoDB instruction_info_db;
+    NoopEventEmitter<AddressingEvent> event_emitter;
+    Addressing addressing(instruction_info_db, event_emitter);
+    MemoryValue zero_u32 = MemoryValue::from<uint32_t>(0);
+
+    {
+        const auto instr = InstructionBuilder(SET_8)
+                               // dstOffset
+                               .operand<uint8_t>(1)
+                               // tag
+                               .operand(MemoryTag::FF)
+                               // value
+                               .operand<uint8_t>(1)
+                               .build();
+
+        StrictMock<MockMemory> 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<MemoryAddress>(1),
+                        // tag is unchanged.
+                        instr.operands.at(1),
+                        // value is unchanged.
+                        instr.operands.at(2)));
+    }
+    {
+        const auto instr = InstructionBuilder(ADD_16)
+                               // aOffset
+                               .operand<uint16_t>(1)
+                               // bOffset
+                               .operand<uint16_t>(2)
+                               // dstOffset
+                               .operand<uint16_t>(3)
+                               .build();
+
+        StrictMock<MockMemory> 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<MemoryAddress>(1), from<MemoryAddress>(2), from<MemoryAddress>(3)));
+    }
+}
+
+TEST(AvmSimulationAddressingTest, RelativeAddressing)
+{
+    InstructionInfoDB instruction_info_db;
+    NoopEventEmitter<AddressingEvent> 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<uint32_t>(100);
+
+    // Set up the ADD_8 instruction with relative addressing
+    const auto instr = InstructionBuilder(ADD_8)
+                           // aOffset
+                           .operand<uint8_t>(10)
+                           .relative()
+                           // bOffset
+                           .operand<uint8_t>(20)
+                           // dstOffset
+                           .operand<uint8_t>(30)
+                           .relative()
+                           .build();
+
+    StrictMock<MockMemory> 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<MemoryAddress>(110),
+                    // bOffset stays the same
+                    from<MemoryAddress>(20),
+                    // dstOffset resolved as base + 30 = 130
+                    from<MemoryAddress>(130)));
+}
+
+TEST(AvmSimulationAddressingTest, IndirectAddressing)
+{
+    InstructionInfoDB instruction_info_db;
+    NoopEventEmitter<AddressingEvent> event_emitter;
+    Addressing addressing(instruction_info_db, event_emitter);
+
+    // For indirect addressing, memory locations contain the actual addresses
+    MemoryValue zero_u32 = MemoryValue::from<uint32_t>(0);
+
+    // Set up the ADD_8 instruction with indirect addressing
+    const auto instr = InstructionBuilder(ADD_8)
+                           // aOffset - address 5 contains the actual address
+                           .operand<uint8_t>(5)
+                           .indirect()
+                           // bOffset
+                           .operand<uint8_t>(10)
+                           // dstOffset - address 15 contains the actual address
+                           .operand<uint8_t>(15)
+                           .indirect()
+                           .build();
+
+    StrictMock<MockMemory> memory;
+    EXPECT_CALL(memory, get(0)).WillOnce(ReturnRef(zero_u32)); // Base address
+    MemoryValue addr_5_value = MemoryValue::from<uint32_t>(50);
+    EXPECT_CALL(memory, get(5)).WillOnce(ReturnRef(addr_5_value));
+    MemoryValue addr_15_value = MemoryValue::from<uint32_t>(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<MemoryAddress>(50),
+                    // bOffset stays the same
+                    from<MemoryAddress>(10),
+                    // dstOffset resolved indirectly: memory[15] = 60
+                    from<MemoryAddress>(60)));
+}
+
+TEST(AvmSimulationAddressingTest, IndirectAndRelativeAddressing)
+{
+    InstructionInfoDB instruction_info_db;
+    NoopEventEmitter<AddressingEvent> event_emitter;
+    Addressing addressing(instruction_info_db, event_emitter);
+
+    // Base address is 100
+    MemoryValue base_addr = MemoryValue::from<uint32_t>(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<uint8_t>(5)
+                           .indirect()
+                           .relative()
+                           // bOffset (indirect only): address 10 contains value
+                           .operand<uint8_t>(10)
+                           .indirect()
+                           // dstOffset (relative only)
+                           .operand<uint8_t>(15)
+                           .relative()
+                           .build();
+
+    StrictMock<MockMemory> 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<uint32_t>(200);
+    EXPECT_CALL(memory, get(105)).WillOnce(ReturnRef(addr_105_value));
+    // Address 10 contains value 60
+    MemoryValue addr_10_value = MemoryValue::from<uint32_t>(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<MemoryAddress>(200),
+                    // bOffset: indirect only (memory[10]=60)
+                    from<MemoryAddress>(60),
+                    // dstOffset: relative only (base+15=115)
+                    from<MemoryAddress>(115)));
+}
+
+} // namespace
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/events/addressing_event.hpp

@@ -3,6 +3,7 @@
 #include <cstdint>
 #include <optional>
 #include <stdexcept>
+#include <string>
 #include <vector>
 
 #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<int>(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)
     {}

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<sizeof...(Ts)>{};
     [f, this, &context, &resolved_operands]<std::size_t... Is>(std::index_sequence<Is...>) {
-        (this->*f)(context, resolved_operands.at(Is).template as<Ts>()...);
+        // FIXME(fcarreiro): we go through FF here.
+        (this->*f)(context, static_cast<Ts>(resolved_operands.at(Is).as_ff())...);
     }(operand_indices);
 }