From ba50eebaaaa3408430b428eb87480b747288a112 Mon Sep 17 00:00:00 2001 From: Stephen Dennis Date: Sat, 25 Jul 2026 19:14:10 -0600 Subject: [PATCH] fix(jit/dbt): dedupe block cache inserts by guest_pc (#1153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Intrinsic blocks are inserted into the block cache twice: try_emit_intrinsic() in the backend inserts before returning, and every caller of dbt_backend_translate_block() (dbt_run, dbt_pretranslate) inserts the result again. dbt_cache_insert had no dedupe, so each intrinsic consumed a second way in its set. Measured over a real blob load with a temporary counter (reverted before commit): 102 duplicate inserts, exactly matching intrinsic_hits=102, spread over 55 sets. 17 of those sets took two or more duplicates and the worst took nine — into a set that holds four ways. This happens once at blob load and is permanent for the process: dbt_reset preserves blob translations. A single duplicate is harmless, which is worth stating because it is why a naive test passes: the way-0 FIFO evicts the original and the copy at way 1 keeps that pc reachable. Harm needs two intrinsics in one set — all four ways then hold two distinct blocks, and the next two distinct blocks evict each other. A set asked to hold only four distinct blocks drops one. The measurement says that is 17 sets, not a hypothetical. The fix is in dbt_cache_insert rather than in the three backends: it is backend-neutral, needs no per-backend change, and closes the whole class rather than the one known double-insert path. The match condition mirrors dbt_cache_lookup (guest_pc equal AND native_code non-null) so an empty way is never mistaken for an entry for pc 0. Re-inserting a known pc updates the entry in place; callers only translate on a lookup miss, so this is reachable today only via the intrinsic path, where the pointer is identical. The redundant insert in try_emit_intrinsic is left in place — it is now harmless, and removing it would touch three backends for no behavioural gain. New tests/dbt_cache unit island, wired into `make test`. It compiles dbt.cpp against four backend stubs, so it needs neither `install` nor --enable-jit and has no skip path. Set membership is discovered by probing rather than by recomputing cache_set(), so the tests survive a retuned hash instead of drifting with it. Verified by reverting the dedupe: 4 assertions fail, including the eviction case, which reports the specific pc that was dropped. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 4 + Makefile | 15 +- mux/modules/engine/dbt.cpp | 16 ++ tests/dbt_cache/.gitignore | 2 + tests/dbt_cache/Makefile | 41 +++++ tests/dbt_cache/test_dbt_cache.cpp | 279 +++++++++++++++++++++++++++++ 6 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 tests/dbt_cache/.gitignore create mode 100644 tests/dbt_cache/Makefile create mode 100644 tests/dbt_cache/test_dbt_cache.cpp diff --git a/CLAUDE.md b/CLAUDE.md index b70fd4e9d..241c510db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,6 +32,10 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co renames — #1152 survived because nothing exercised the affected backend, so host-only coverage is not enough here. Compiles the backend sources directly: needs neither `install` nor `--enable-jit`, and has no skip path. +- DBT block cache: `make test-dbt-cache` (also part of `make test`); + `tests/dbt_cache/` covers `dbt_cache_insert`/`dbt_cache_lookup` dedupe and + FIFO eviction (#1153). Compiles `dbt.cpp` against backend stubs — no + `install`, no `--enable-jit`, no skip path. - Wildcard-capture scenario: `make test-scenario` (opt-in, NOT in `make test`); spins a throwaway netmux and drives `$`-command `%0..%9` captures over a socket (`tests/scenario/`) — the path muxscript can't reach diff --git a/Makefile b/Makefile index 9d789d0a6..81b26542c 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ # make test — run smoke tests (build + install first) # make hooks — install git hooks (done automatically on first build) -.PHONY: all install clean realclean test test-ios test-ganl test-netaddr test-dbt-chain test-alarm test-scenario test-parity213 test-stress test-jit-qreg test-jit-ifelse hooks +.PHONY: all install clean realclean test test-ios test-ganl test-netaddr test-dbt-chain test-dbt-cache test-alarm test-scenario test-parity213 test-stress test-jit-qreg test-jit-ifelse hooks # Install git hooks on first build so all developers get protection # against accidentally editing generated files. @@ -29,11 +29,12 @@ clean: $(MAKE) -C testcases/tools clean $(MAKE) -C mux/ganl/tests clean $(MAKE) -C tests/dbt_chain clean + $(MAKE) -C tests/dbt_cache clean realclean: $(MAKE) -C mux distclean -test: install test-ganl test-netaddr test-dbt-chain test-alarm test-jit-qreg test-jit-ifelse test-ios +test: install test-ganl test-netaddr test-dbt-chain test-dbt-cache test-alarm test-jit-qreg test-jit-ifelse test-ios $(MAKE) -C testcases/tools cd testcases && ./tools/Makesmoke && ./tools/Smoke @@ -89,6 +90,16 @@ test-dbt-chain: @echo "==> Running DBT chain patch encode/decode tests" $(MAKE) -C tests/dbt_chain test +# DBT block cache tests (#1153). The 4-way cache had no dedupe by guest_pc +# while intrinsic blocks are inserted twice (once by try_emit_intrinsic, once +# by every caller of dbt_backend_translate_block), so two intrinsics sharing a +# set filled all four ways with two distinct blocks and evicted live ones. +# Compiles dbt.cpp directly against backend stubs — no `install`, no +# --enable-jit, no skip path. +test-dbt-cache: + @echo "==> Running DBT block cache tests" + $(MAKE) -C tests/dbt_cache test + # mux_alarm unit tests: the per-command wall-clock abort. Guards the lazy # worker-thread start — alarm_clock is a libmux global whose constructor used # to spawn a thread during static init, deadlocking before main in ~14% of diff --git a/mux/modules/engine/dbt.cpp b/mux/modules/engine/dbt.cpp index 5b63ab265..8a83b3d82 100644 --- a/mux/modules/engine/dbt.cpp +++ b/mux/modules/engine/dbt.cpp @@ -124,6 +124,22 @@ block_entry_t *dbt_cache_lookup(dbt_state_t *dbt, uint64_t pc) { void dbt_cache_insert(dbt_state_t *dbt, uint64_t pc, uint8_t *code) { uint32_t set = cache_set(pc); block_entry_t *base = &dbt->cache[set * BLOCK_CACHE_WAYS]; + // An entry for this pc already exists — update it in place rather than + // consuming a second way (#1153). Intrinsic blocks are inserted twice: + // try_emit_intrinsic() in the backend inserts before returning, and + // every caller of dbt_backend_translate_block() inserts the result + // again. Without this, one intrinsic occupied two of the four ways in + // its set, and a set holding two such duplicates could FIFO-evict a + // live block that still had room. The match condition mirrors + // dbt_cache_lookup so an empty way (guest_pc 0, native_code null) is + // never mistaken for an entry. + // + for (size_t w = 0; w < BLOCK_CACHE_WAYS; w++) { + if (base[w].guest_pc == pc && base[w].native_code) { + base[w].native_code = code; + return; + } + } // Use first empty way. for (size_t w = 0; w < BLOCK_CACHE_WAYS; w++) { if (base[w].guest_pc == 0) { diff --git a/tests/dbt_cache/.gitignore b/tests/dbt_cache/.gitignore new file mode 100644 index 000000000..cf4b14254 --- /dev/null +++ b/tests/dbt_cache/.gitignore @@ -0,0 +1,2 @@ +test_dbt_cache +*.o diff --git a/tests/dbt_cache/Makefile b/tests/dbt_cache/Makefile new file mode 100644 index 000000000..d1f59ebfb --- /dev/null +++ b/tests/dbt_cache/Makefile @@ -0,0 +1,41 @@ +# Makefile — unit tests for the DBT block cache (dbt_cache_insert / +# dbt_cache_lookup, #1153). +# +# Links dbt.cpp against a driver that stubs the four dbt_backend_* symbols +# the translation unit references. dbt.cpp is compiled here directly rather +# than taken from the engine build, so this test needs no prior +# `make install` and no --enable-jit. +# +# Build: make +# Run: make test + +CXX = g++ +CXXFLAGS = -std=c++17 -g -O2 +TESTFLAGS = -Wall -Wextra +INCDIR = ../../mux/include +ENGINE = ../../mux/modules/engine +DEFS = -DHAVE_CONFIG_H -DTINYMUX_JIT + +TARGET = test_dbt_cache +OBJS = test_dbt_cache.o dbt_shared.o + +all: $(TARGET) + +test_dbt_cache.o: test_dbt_cache.cpp + $(CXX) $(CXXFLAGS) $(TESTFLAGS) $(DEFS) -I$(INCDIR) -c -o $@ $< + +# Code under test, built under the engine's own warning posture (see the +# same note in tests/dbt_chain/Makefile). +dbt_shared.o: $(ENGINE)/dbt.cpp + $(CXX) $(CXXFLAGS) $(DEFS) -I$(INCDIR) -c -o $@ $< + +$(TARGET): $(OBJS) + $(CXX) $(CXXFLAGS) -o $@ $(OBJS) + +test: $(TARGET) + ./$(TARGET) + +clean: + rm -f *.o $(TARGET) + +.PHONY: all test clean diff --git a/tests/dbt_cache/test_dbt_cache.cpp b/tests/dbt_cache/test_dbt_cache.cpp new file mode 100644 index 000000000..9dd21e58c --- /dev/null +++ b/tests/dbt_cache/test_dbt_cache.cpp @@ -0,0 +1,279 @@ +// Unit tests for the DBT block cache: dbt_cache_insert / dbt_cache_lookup. +// +// The cache is 4-way set associative with a FIFO way-0 eviction. Insert had +// no dedupe by guest_pc, and intrinsic blocks are inserted *twice*: +// try_emit_intrinsic() in the backend inserts before returning, and every +// caller of dbt_backend_translate_block() inserts the result again. So one +// intrinsic occupied two of the four ways in its set, and a set holding +// duplicates could FIFO-evict a live block while it still had room (#1153). +// +// The tests deliberately do not reimplement cache_set(): it is static in +// dbt.cpp, and a test that recomputed the hash would pass even if the hash +// and the test drifted apart together. Instead set membership is discovered +// empirically -- insert a marker and see which set it lands in -- so these +// tests keep working if the hash is ever retuned. +// +// Build/run: make test (no dependency on a built netmux; dbt.cpp is +// compiled directly) + +#include +#include +#include +#include +#include + +#include "dbt.h" +#include "dbt_internal.h" + +// --------------------------------------------------------------- +// Backend stubs +// --------------------------------------------------------------- +// +// Linking dbt.cpp pulls in the whole translation unit, so the four backend +// symbols it references must resolve. The cache pair touches none of them; +// each stub aborts so that a refactor which routes cache work through the +// backend fails loudly instead of quietly testing something else. +// +static void die_unreachable(const char *who) { + fprintf(stderr, "test_dbt_cache: %s was called from the cache path.\n" + "These stubs are no longer safe.\n", who); + abort(); +} + +void dbt_backend_emit_trampoline(dbt_state_t *) { + die_unreachable("dbt_backend_emit_trampoline"); +} + +uint8_t *dbt_backend_translate_block(dbt_state_t *, uint64_t) { + die_unreachable("dbt_backend_translate_block"); + return nullptr; +} + +void dbt_backend_backpatch_jmp(uint8_t *, uint32_t, uint8_t *) { + die_unreachable("dbt_backend_backpatch_jmp"); +} + +uint32_t dbt_backend_decode_jmp_target(const uint8_t *, uint32_t) { + die_unreachable("dbt_backend_decode_jmp_target"); + return 0; +} + +// --------------------------------------------------------------- +// Test framework +// --------------------------------------------------------------- + +static int g_pass = 0; +static int g_fail = 0; + +static void check(bool ok, const char *what) { + if (ok) { + g_pass++; + } else { + g_fail++; + printf("FAIL: %s\n", what); + } +} + +// Distinct non-null code pointers. Never dereferenced -- the cache only +// stores and compares them. +static uint8_t *code_ptr(int n) { + return reinterpret_cast(static_cast(0x100000) + + static_cast(n) * 0x40); +} + +static dbt_state_t *make_state(void) { + dbt_state_t *dbt = new dbt_state_t(); + dbt->cache.assign(BLOCK_CACHE_SIZE, block_entry_t{}); + return dbt; +} + +static void clear_cache(dbt_state_t *dbt) { + dbt->cache.assign(BLOCK_CACHE_SIZE, block_entry_t{}); +} + +// Which set does pc map to? Discovered by probing, not by recomputing the +// hash -- see the file header. +static uint32_t set_of(dbt_state_t *scratch, uint64_t pc) { + clear_cache(scratch); + uint8_t *mark = code_ptr(999); + dbt_cache_insert(scratch, pc, mark); + for (size_t i = 0; i < scratch->cache.size(); i++) { + if (scratch->cache[i].native_code == mark) { + return static_cast(i / BLOCK_CACHE_WAYS); + } + } + return UINT32_MAX; +} + +// Count ways in a set that hold a live entry. +static int occupied(dbt_state_t *dbt, uint32_t set) { + int n = 0; + for (size_t w = 0; w < BLOCK_CACHE_WAYS; w++) { + if (dbt->cache[set * BLOCK_CACHE_WAYS + w].native_code) { + n++; + } + } + return n; +} + +// Find `want` distinct guest PCs that all map to the same set, so the tests +// can fill one set exactly. Guest PCs are 4-aligned (RV64 instructions). +static bool find_colliding(dbt_state_t *scratch, size_t want, + std::vector &out) { + std::vector > by_set(BLOCK_CACHE_SETS); + for (uint64_t pc = 0x1000; pc < 0x1000 + 4 * 40000; pc += 4) { + uint32_t s = set_of(scratch, pc); + if (s == UINT32_MAX) { + continue; + } + by_set[s].push_back(pc); + if (by_set[s].size() >= want) { + out = by_set[s]; + return true; + } + } + return false; +} + +// --------------------------------------------------------------- + +int main(void) { + printf("=== DBT block cache ===\n\n"); + + dbt_state_t *scratch = make_state(); + dbt_state_t *dbt = make_state(); + + std::vector pcs; + if (!find_colliding(scratch, BLOCK_CACHE_WAYS + 1, pcs)) { + printf("FAIL: could not find %zu guest PCs sharing a set\n", + BLOCK_CACHE_WAYS + 1); + return 1; + } + const uint32_t SET = set_of(scratch, pcs[0]); + printf(" using set %u, colliding PCs:", SET); + for (size_t i = 0; i < pcs.size(); i++) { + printf(" 0x%llX", static_cast(pcs[i])); + } + printf("\n\n"); + + // --- 1. The #1153 regression ------------------------------------- + // The intrinsic shape: the same guest_pc inserted twice with the same + // code pointer. It must occupy one way, not two. + { + clear_cache(dbt); + dbt_cache_insert(dbt, pcs[0], code_ptr(0)); + dbt_cache_insert(dbt, pcs[0], code_ptr(0)); // caller's second insert + check(1 == occupied(dbt, SET), + "double insert of one pc must occupy exactly one way"); + block_entry_t *be = dbt_cache_lookup(dbt, pcs[0]); + check(be && be->native_code == code_ptr(0), + "double-inserted pc must still look up to its code"); + } + + // --- 2. The consequence the issue names -------------------------- + // Capacity loss needs TWO intrinsics in one set, not one. With a single + // duplicate the way-0 FIFO evicts the original and the copy at way 1 + // keeps that pc reachable, so nothing is lost -- an earlier draft of + // this test used one intrinsic and passed with the bug still in. + // + // Two duplicates fill all four ways with two distinct blocks. The next + // two distinct blocks then evict each other, so a set that was asked to + // hold only four distinct blocks drops one. + { + clear_cache(dbt); + dbt_cache_insert(dbt, pcs[0], code_ptr(0)); + dbt_cache_insert(dbt, pcs[0], code_ptr(0)); // intrinsic A, twice + dbt_cache_insert(dbt, pcs[1], code_ptr(1)); + dbt_cache_insert(dbt, pcs[1], code_ptr(1)); // intrinsic B, twice + dbt_cache_insert(dbt, pcs[2], code_ptr(2)); + dbt_cache_insert(dbt, pcs[3], code_ptr(3)); + + check(BLOCK_CACHE_WAYS == static_cast(occupied(dbt, SET)), + "a set holding two intrinsics must still fill four ways"); + bool all_resident = true; + for (size_t i = 0; i < BLOCK_CACHE_WAYS; i++) { + block_entry_t *be = dbt_cache_lookup(dbt, pcs[i]); + if (!be || be->native_code != code_ptr(static_cast(i))) { + all_resident = false; + printf(" pc 0x%llX was evicted\n", + static_cast(pcs[i])); + } + } + check(all_resident, + "four distinct blocks must all fit a 4-way set, " + "however many times each was inserted"); + } + + // --- 3. Re-insert updates in place ------------------------------- + // Retranslation to a new address must move the entry, not duplicate it. + { + clear_cache(dbt); + dbt_cache_insert(dbt, pcs[0], code_ptr(1)); + dbt_cache_insert(dbt, pcs[0], code_ptr(2)); + check(1 == occupied(dbt, SET), + "re-insert at a new address must not add a way"); + block_entry_t *be = dbt_cache_lookup(dbt, pcs[0]); + check(be && be->native_code == code_ptr(2), + "re-insert must publish the new code pointer"); + } + + // --- 4. Distinct PCs are still distinct --------------------------- + // The dedupe must key on guest_pc, not collapse a whole set. + { + clear_cache(dbt); + for (size_t i = 0; i < BLOCK_CACHE_WAYS; i++) { + dbt_cache_insert(dbt, pcs[i], code_ptr(static_cast(i))); + } + check(BLOCK_CACHE_WAYS == static_cast(occupied(dbt, SET)), + "four distinct pcs must occupy four ways"); + bool ok = true; + for (size_t i = 0; i < BLOCK_CACHE_WAYS; i++) { + block_entry_t *be = dbt_cache_lookup(dbt, pcs[i]); + if (!be || be->native_code != code_ptr(static_cast(i))) { + ok = false; + } + } + check(ok, "each distinct pc must look up to its own code"); + } + + // --- 5. Eviction still happens when genuinely full ---------------- + // The fix must not turn a full set into a set that refuses new blocks. + { + clear_cache(dbt); + for (size_t i = 0; i <= BLOCK_CACHE_WAYS; i++) { + dbt_cache_insert(dbt, pcs[i], code_ptr(static_cast(i))); + } + check(BLOCK_CACHE_WAYS == static_cast(occupied(dbt, SET)), + "an over-full set must still hold exactly 4 ways"); + block_entry_t *be = dbt_cache_lookup(dbt, pcs[BLOCK_CACHE_WAYS]); + check(be && be->native_code + == code_ptr(static_cast(BLOCK_CACHE_WAYS)), + "the newest block must be resident after eviction"); + check(nullptr == dbt_cache_lookup(dbt, pcs[0]), + "way 0 must be the one evicted (FIFO)"); + } + + // --- 6. guest_pc 0 is the empty sentinel -------------------------- + // The dedupe match mirrors dbt_cache_lookup (native_code non-null) so an + // empty way is never mistaken for an entry for pc 0. Registration skips + // guest_addr 0, but the cache must not corrupt itself if it ever sees it. + { + clear_cache(dbt); + const uint32_t s0 = set_of(scratch, 0); + dbt_cache_insert(dbt, 0, code_ptr(7)); + check(1 == occupied(dbt, s0), + "insert of pc 0 must occupy exactly one way"); + dbt_cache_insert(dbt, 0, code_ptr(8)); + check(1 == occupied(dbt, s0), + "re-insert of pc 0 must dedupe, not consume a second way"); + block_entry_t *be = dbt_cache_lookup(dbt, 0); + check(be && be->native_code == code_ptr(8), + "pc 0 must look up to its latest code"); + } + + delete dbt; + delete scratch; + + printf("\n=== dbt cache: %d passed, %d failed ===\n", g_pass, g_fail); + return (g_fail > 0) ? 1 : 0; +}