fix: Clean up post-redesign technical debt

Four fixes identified by codebase audit:

1. EpochCheckpoint.salt no longer misused for insn_index_in_bb.
   Added dedicated saved_insn_index field to EpochCheckpoint.
   CALL_VM/RET_VM handlers updated to use the new field.

2. Extracted enc_state replay from vm_engine.hpp step() into
   pipeline::replay_enc_state() non-template function.
   step() reduced from ~40 lines of inline SipHash chain replay
   to a single function call.  Replay logic properly handles
   REKEY instructions during chain advancement.

3. Removed unused MockEditor class from test_loader.cpp (93 lines
   of dead code).  Replaced stale TODO in vm_runtime_init.cpp with
   proper documentation of hardcoded section name rationale.

4. Added 11 atomic handler tests: LOCK_ADD (basic + zero + overflow),
   XCHG (basic + same-value), CMPXCHG (success + failure + combined),
   ATOMIC_LOAD (basic + zero + different registers).  All handlers
   verified for encode/decode correctness through composition tables.

51 comprehensive tests, 696 total tests passing.

Author: scc-tw

common/include/vm/vm_context.hpp

@@ -53,7 +53,8 @@ struct EpochCheckpoint {
     uint64_t vm_ip;
     uint32_t bb_id;
     uint8_t  epoch_seed[32];
-    uint64_t salt;
+    uint64_t salt;                ///< TRNG salt for epoch derivation
+    uint32_t saved_insn_index;    ///< insn_index_in_bb at CALL_VM time (for resume after RET_VM)
     uint64_t encoded_regs_snapshot[16];
 };
 

loader/tests/test_loader.cpp

@@ -342,99 +342,6 @@ TEST(PayloadBuilder, MultipleRegionsLayout) {
     }
 }
 
-// ============================================================================
-// MockEditor — DI for Loader::patch() tests
-// ============================================================================
-
-class MockEditor : public Loader::EditorBase<MockEditor> {
-    friend class Loader::EditorBase<MockEditor>;
-
-public:
-    struct Config {
-        Loader::TextSectionInfo text = {0x1000, 0x2000};
-        uint64_t next_va = 0x10000;
-        bool fail_open = false;
-        bool fail_add_segment = false;
-        bool fail_overwrite = false;
-        bool fail_save = false;
-    };
-    static Config cfg;
-
-    struct Calls {
-        bool saved = false;
-        int overwrite_count = 0;
-        std::vector<uint64_t> overwrite_vas;
-    };
-    static Calls calls;
-
-    static void reset() { cfg = {}; calls = {}; }
-
-    Loader::TextSectionInfo text_section_impl() const noexcept {
-        return {cfg.text.base_addr, cfg.text.size};
-    }
-
-    uint64_t next_segment_va_impl(uint64_t) const noexcept {
-        return cfg.next_va;
-    }
-
-    tl::expected<Loader::NewSegmentInfo, DC>
-    add_segment_impl(std::string_view, const std::vector<uint8_t>& payload,
-                     uint64_t, Common::DiagnosticCollector& diag) noexcept {
-        if (cfg.fail_add_segment) {
-            diag.error("mock", DC::PatchSegmentCreationFailed, "fail");
-            return tl::unexpected(DC::PatchSegmentCreationFailed);
-        }
-        return Loader::NewSegmentInfo{cfg.next_va, payload.size()};
-    }
-
-    bool cfi_enforced_impl() const noexcept { return false; }
-
-    std::vector<Loader::TextGap>
-    find_text_gaps_impl(std::size_t) const noexcept { return {}; }
-
-    tl::expected<Loader::NewSegmentInfo, DC>
-    extend_text_impl(const std::vector<uint8_t>& data, uint64_t,
-                     Common::DiagnosticCollector&) noexcept {
-        return Loader::NewSegmentInfo{cfg.next_va, data.size()};
-    }
-
-    tl::expected<void, DC>
-    overwrite_text_impl(uint64_t va, const uint8_t*, size_t,
-                        Common::DiagnosticCollector& diag) noexcept {
-        calls.overwrite_count++;
-        calls.overwrite_vas.push_back(va);
-        if (cfg.fail_overwrite) {
-            diag.error("mock", DC::PatchSegmentCreationFailed, "fail");
-            return tl::unexpected(DC::PatchSegmentCreationFailed);
-        }
-        return {};
-    }
-
-    tl::expected<void, DC>
-    add_runtime_dep_impl(std::string_view, Common::DiagnosticCollector&) noexcept {
-        return {};
-    }
-
-    void invalidate_signature_impl() noexcept {}
-
-    tl::expected<void, DC>
-    save_impl(const std::string&, Common::DiagnosticCollector& diag) noexcept {
-        calls.saved = true;
-        if (cfg.fail_save) {
-            diag.error("mock", DC::PatchBinaryWriteFailed, "fail");
-            return tl::unexpected(DC::PatchBinaryWriteFailed);
-        }
-        return {};
-    }
-};
-
-MockEditor::Config MockEditor::cfg;
-MockEditor::Calls MockEditor::calls;
-
-// NOTE: Loader::patch() uses open_binary() which creates real editors.
-// For MockEditor DI tests, we test PayloadBuilder + StubEmitter directly
-// since those are the core logic. Integration tests use real binaries.
-
 // ============================================================================
 // DiagnosticCode helpers
 // ============================================================================

runtime/include/handler_impls.hpp

@@ -627,7 +627,7 @@ struct HandlerTraits<VmOpcode::CALL_VM, P> {
         cp.vm_ip = e.vm_ip;
         cp.bb_id = e.current_bb_id;
         std::memcpy(cp.epoch_seed, ep.reg.encode[0], 32);
-        cp.salt = static_cast<uint64_t>(e.insn_index_in_bb);  // save insn index for resume
+        cp.saved_insn_index = e.insn_index_in_bb;
         for (int r = 0; r < VM_REG_COUNT; ++r)
             cp.encoded_regs_snapshot[r] = e.regs[r].bits;
         e.shadow_depth++;
@@ -653,11 +653,11 @@ struct HandlerTraits<VmOpcode::RET_VM, P> {
         e.branch_target_bb = cp.bb_id;
         e.branch_taken = true;
         // Resume AFTER the CALL_VM instruction (cp.vm_ip is CALL's ip,
-        // cp.salt stores the insn_index_in_bb at CALL time).
+        // cp.saved_insn_index stores the insn_index_in_bb at CALL time).
         // The dispatcher will use these to override enter_basic_block's
         // default vm_ip = entry_ip.
         e.return_resume_ip = cp.vm_ip + 1;
-        e.return_resume_insn_idx = static_cast<uint32_t>(cp.salt) + 1;
+        e.return_resume_insn_idx = cp.saved_insn_index + 1;
         return {};
     }
 };

runtime/include/pipeline.hpp

@@ -80,6 +80,11 @@ verify_bb_mac(const VmImmutable& imm,
               const VmExecution& exec,
               const VmEpoch& epoch) noexcept;
 
+/// Replay enc_state SipHash chain from BB entry to a specific instruction index.
+/// Used by RET_VM to resume execution after CALL_VM.
+void replay_enc_state(VmExecution& exec, const VmEpoch& epoch,
+                      const VmImmutable& imm, uint32_t target_insn_idx) noexcept;
+
 /// Get the current BB's instruction count (for boundary detection).
 [[nodiscard]] uint32_t current_bb_insn_count(
     const VmImmutable& imm,

runtime/include/vm_engine.hpp

@@ -412,53 +412,7 @@ VmEngine<Policy, Oram>::step() noexcept
         if (exec_.return_resume_ip != 0) {
             exec_.vm_ip = exec_.return_resume_ip;
             exec_.insn_index_in_bb = exec_.return_resume_insn_idx;
-            // Re-derive enc_state to match the resume position:
-            // We need to replay the SipHash chain from entry_ip to resume_ip.
-            // The enc_state was reset by enter_basic_block to bb_enc_seed.
-            // We replay by re-decrypting instructions [entry..resume) to
-            // advance enc_state.  This is expensive but correct (Performance Never).
-            uint8_t enc_seed[8];
-            const auto& bb = imm_->bb_metadata[exec_.current_bb_index];
-            uint8_t msg[7];
-            std::memcpy(msg, "enc", 3);
-            std::memcpy(msg + 3, &bb.bb_id, 4);
-            Common::VM::Crypto::blake3_keyed_hash(imm_->stored_seed, msg, 7, enc_seed, 8);
-            uint64_t es = 0;
-            std::memcpy(&es, enc_seed, 8);
-
-            auto insns = imm_->blob.instructions();
-            for (uint32_t j = 0; j < exec_.return_resume_insn_idx; ++j) {
-                uint64_t encrypted = 0;
-                std::memcpy(&encrypted, &insns[bb.entry_ip + j], 8);
-                uint64_t ks = Common::VM::Crypto::siphash_keystream(
-                    imm_->fast_key, es, j);
-                uint64_t plain_u64 = encrypted ^ ks;
-                Common::VM::VmInsn vi{};
-                std::memcpy(&vi, &plain_u64, 8);
-
-                // Check for REKEY
-                uint8_t enc_alias = static_cast<uint8_t>(vi.opcode & 0xFF);
-                uint8_t alias = epoch_->opcode_perm_inv[enc_alias];
-                uint8_t sem = imm_->alias_lut[alias];
-                if (sem == static_cast<uint8_t>(Common::VM::VmOpcode::REKEY)) {
-                    uint32_t cnt = vi.aux;
-                    uint8_t rk[9]; std::memcpy(rk, "rekey", 5);
-                    std::memcpy(rk+5, &cnt, 4);
-                    uint8_t mat[16];
-                    Common::VM::Crypto::blake3_kdf(imm_->stored_seed,
-                        reinterpret_cast<const char*>(rk), 9, mat, 16);
-                    uint8_t esb[8]; std::memcpy(esb, &es, 8);
-                    es = Common::VM::Crypto::siphash_2_4(mat, esb, 8);
-                }
-
-                uint8_t key16[16] = {};
-                std::memcpy(key16, &es, 8);
-                uint8_t msg6[6];
-                std::memcpy(msg6, &vi.opcode, 2);
-                std::memcpy(msg6+2, &vi.aux, 4);
-                es = Common::VM::Crypto::siphash_2_4(key16, msg6, 6);
-            }
-            exec_.enc_state = es;
+            pipeline::replay_enc_state(exec_, *epoch_, *imm_, exec_.return_resume_insn_idx);
             exec_.return_resume_ip = 0;
             exec_.return_resume_insn_idx = 0;
         }

runtime/src/pipeline.cpp

@@ -292,6 +292,61 @@ verify_bb_mac(const VmImmutable& imm,
     return {};
 }
 
+// ─────────────────────────────────────────────────────────────────────────────
+// replay_enc_state (RET_VM resume)
+// ─────────────────────────────────────────────────────────────────────────────
+
+void replay_enc_state(VmExecution& exec, const VmEpoch& epoch,
+                      const VmImmutable& imm,
+                      uint32_t target_insn_idx) noexcept {
+
+    const auto& bb = imm.bb_metadata[exec.current_bb_index];
+
+    // Derive bb_enc_seed from scratch (enter_basic_block already set enc_state,
+    // but we need the seed for the keystream replay).
+    uint8_t enc_seed_bytes[8];
+    derive_bb_enc_seed(imm.stored_seed, bb.bb_id, enc_seed_bytes);
+
+    uint64_t es = 0;
+    std::memcpy(&es, enc_seed_bytes, 8);
+
+    // Replay SipHash chain: decrypt each instruction [0..target_insn_idx) and
+    // advance enc_state, handling REKEY mutations along the way.
+    auto insns = imm.blob.instructions();
+    for (uint32_t j = 0; j < target_insn_idx; ++j) {
+        uint64_t encrypted = 0;
+        std::memcpy(&encrypted, &insns[bb.entry_ip + j], 8);
+
+        uint64_t ks = siphash_keystream(imm.fast_key, es, j);
+        uint64_t plain_u64 = encrypted ^ ks;
+
+        VmInsn vi{};
+        std::memcpy(&vi, &plain_u64, 8);
+
+        // Check for REKEY — must replay its enc_state mutation
+        uint8_t enc_alias = static_cast<uint8_t>(vi.opcode & 0xFF);
+        uint8_t alias = epoch.opcode_perm_inv[enc_alias];
+        uint8_t sem = imm.alias_lut[alias];
+        if (sem == static_cast<uint8_t>(VmOpcode::REKEY)) {
+            uint32_t cnt = vi.aux;
+            uint8_t rk_ctx[9];
+            std::memcpy(rk_ctx, "rekey", 5);
+            std::memcpy(rk_ctx + 5, &cnt, 4);
+            uint8_t rk_mat[16];
+            blake3_kdf(imm.stored_seed,
+                       reinterpret_cast<const char*>(rk_ctx), 9,
+                       rk_mat, 16);
+            uint8_t esb[8];
+            std::memcpy(esb, &es, 8);
+            es = siphash_2_4(rk_mat, esb, 8);
+        }
+
+        es = update_enc_state_impl(es, vi.opcode, vi.aux);
+    }
+
+    exec.enc_state = es;
+}
+
 // ─────────────────────────────────────────────────────────────────────────────
 // current_bb_insn_count
 // ─────────────────────────────────────────────────────────────────────────────

runtime/src/vm_runtime_init.cpp

@@ -46,8 +46,10 @@
 #include <cstdint>
 #include <cstring>
 
-// The section name the loader uses (must match PlatformTraits).
-// TODO: make this configurable or discover via a magic marker.
+// The section name the loader uses — must match FormatConfig::section_name
+// in PlatformTraits.hpp.  Hardcoded because the runtime is compiled once
+// per platform while FormatConfig is a loader-side constexpr; keeping them
+// in sync is enforced by integration tests (test_loader + test_extend_text).
 #if defined(__APPLE__)
 static constexpr const char* VMPILOT_SEGMENT = "__VMPILOT";
 #elif defined(_WIN32)