diff --git a/mpcore/src/main/cpp/emu/guest_engine.cpp b/mpcore/src/main/cpp/emu/guest_engine.cpp index 3ec0ac2..0cec9b8 100644 --- a/mpcore/src/main/cpp/emu/guest_engine.cpp +++ b/mpcore/src/main/cpp/emu/guest_engine.cpp @@ -3116,16 +3116,20 @@ uc_engine* GuestEngine::CreateConfiguredEngine() { zlib_accel::kCrc32Addr, zlib_accel::kCrc32Addr); } - // Native resource-name lookup: REGISTRATION DISABLED 2026-09-19 after it - // crashed the process (SIGSEGV, SEGV_ACCERR at a host address, inside the - // index build walking a guest string past the end of the mapped region). - // Two wrong assumptions, both visible in the same capture: guest pointers - // were dereferenced via G2H and scanned for a NUL without any bounds - // check, and the table identity assumption was wrong - the log shows one - // address (0x3537fac4) alternating between 35 and 59 entries, i.e. - // different objects living at the same address, so caching keyed on the - // address alone is invalid. See name_lookup_accel.h for what still needs - // solving; the code is kept, unregistered, as the starting point. + // Native resource-name lookup: REGISTRATION DISABLED AGAIN 2026-09-19. + // The safe rewrite no longer crashes, but it made the prologue load + // visibly LAG - a performance regression, caught live. Cause is in the + // design, not the safety checks: the cache is keyed on the table + // ADDRESS, and this game legitimately reuses one address for different + // tables (the probe log showed 35 and 59 entries alternating). So the + // descriptor compare marks the index stale on almost every call, and + // each rebuild re-reads all ~6232 entry strings byte-by-byte through + // uc_mem_read - far more work than the linear strcmp scan it replaces. + // Fix is to key the cache on the DESCRIPTOR CONTENTS (entry arrays, + // string pools, threshold, counts) so alternating tables each keep their + // own index and nothing rebuilds; plus bulk string reads instead of + // per-byte. Kept unregistered until that is done - see + // name_lookup_accel.h. // See MeasureAdvanceProbeHookCb's own comment - task #39's root-cause // chain, catching the advance value at the exact instruction that diff --git a/mpcore/src/main/cpp/emu/name_lookup_accel.cpp b/mpcore/src/main/cpp/emu/name_lookup_accel.cpp index b674783..9a52435 100644 --- a/mpcore/src/main/cpp/emu/name_lookup_accel.cpp +++ b/mpcore/src/main/cpp/emu/name_lookup_accel.cpp @@ -22,73 +22,102 @@ constexpr uint32_t kOffPoolA = 224; constexpr uint32_t kOffPoolThreshold = 228; constexpr uint32_t kOffPoolB = 232; +// Refuses to index anything implausible. A garbage count field would +// otherwise turn the build loop into a multi-million-iteration walk over +// arbitrary memory - the first version had no such guard and paid for it. +constexpr uint32_t kMaxEntries = 200000; +constexpr uint32_t kMaxNameLen = 128; + struct TableCache { uint32_t countA = 0; uint32_t countB = 0; - // name -> entry index. First occurrence wins, matching the guest loop, - // which returns as soon as it finds a match. + uint32_t entriesA = 0; + uint32_t entriesB = 0; + uint32_t poolA = 0; + uint32_t poolB = 0; + uint32_t threshold = 0; std::unordered_map byName; }; std::mutex g_mutex; -std::map g_tables; // keyed by the guest table address +std::map g_tables; -std::atomic g_hits{0}; +std::atomic g_served{0}; +std::atomic g_fellThrough{0}; std::atomic g_builds{0}; -uint32_t ReadU32(uc_engine* uc, uint32_t addr) { - uint32_t v = 0; - uc_mem_read(uc, addr, &v, 4); - return v; +// Every guest read goes through uc_mem_read, which FAILS on an unmapped +// address instead of handing back a host pointer to walk off the end of the +// region. That is the whole difference from the first attempt, which used +// G2H plus an unbounded NUL scan and segfaulted the process. +bool TryReadU32(uc_engine* uc, uint32_t addr, uint32_t* out) { + return addr && uc_mem_read(uc, addr, out, 4) == UC_ERR_OK; } -void ReturnToCaller(uc_engine* uc, uint32_t ret) { - uint32_t lr = 0; - uc_reg_read(uc, UC_ARM_REG_LR, &lr); - uc_reg_write(uc, UC_ARM_REG_R0, &ret); - uc_reg_write(uc, UC_ARM_REG_PC, &lr); - uc_emu_stop(uc); -} - -// Reads a NUL-terminated guest string directly out of the shared region. -std::string ReadGuestString(GuestEngine& eng, uint32_t addr) { - if (!addr) return {}; - const char* p = reinterpret_cast(eng.G2H(addr)); - if (!p) return {}; - return std::string(p); -} - -// Builds (or rebuilds) the host-side index for one guest table. Mirrors the -// guest loop's own addressing exactly, including the two-array split and the -// threshold that selects which string pool an entry's offset belongs to. -void BuildIndex(GuestEngine& eng, uc_engine* uc, uint32_t self, TableCache& cache) { - cache.byName.clear(); - cache.countA = ReadU32(uc, self + kOffCountA); - cache.countB = ReadU32(uc, self + kOffCountB); - const uint32_t entriesA = ReadU32(uc, self + kOffEntriesA); - const uint32_t entriesB = ReadU32(uc, self + kOffEntriesB); - const uint32_t poolA = ReadU32(uc, self + kOffPoolA); - const uint32_t poolB = ReadU32(uc, self + kOffPoolB); - const uint32_t threshold = ReadU32(uc, self + kOffPoolThreshold); - - const uint32_t total = cache.countA + cache.countB; - cache.byName.reserve(total * 2); - for (uint32_t i = 0; i < total; i++) { - uint32_t entry = (i >= cache.countA) - ? entriesB + (i - cache.countA) * 8 - : entriesA + i * 8; - uint32_t off = ReadU32(uc, entry); - uint32_t base = poolA; - if (off >= threshold) { - off -= threshold; - base = poolB; - } - std::string name = ReadGuestString(eng, base + off); - if (name.empty()) continue; - cache.byName.emplace(std::move(name), (int32_t)i); +// Bounded, fault-tolerant guest string read. Returns false if the string is +// unterminated within kMaxNameLen or runs into unmapped memory. +bool TryReadString(uc_engine* uc, uint32_t addr, std::string* out) { + if (!addr) return false; + out->clear(); + for (uint32_t i = 0; i < kMaxNameLen; i++) { + uint8_t c = 0; + if (uc_mem_read(uc, addr + i, &c, 1) != UC_ERR_OK) return false; + if (!c) return true; + out->push_back((char)c); } - Log("name_lookup_accel: indexed table 0x%x - %u entries (%u+%u), %zu distinct names", - self, total, cache.countA, cache.countB, cache.byName.size()); + return false; +} + +// Resolves entry index -> the guest address of that entry's name, mirroring +// the guest loop's own addressing (two entry arrays, and a threshold that +// selects which of two string pools an offset belongs to). +bool EntryNameAddr(uc_engine* uc, const TableCache& t, uint32_t index, uint32_t* out) { + uint32_t entry = (index >= t.countA) ? t.entriesB + (index - t.countA) * 8 + : t.entriesA + index * 8; + uint32_t off = 0; + if (!TryReadU32(uc, entry, &off)) return false; + uint32_t base = t.poolA; + if (off >= t.threshold) { + off -= t.threshold; + base = t.poolB; + } + if (!base) return false; + *out = base + off; + return true; +} + +// Reads the table's own descriptor fields. Returns false if anything looks +// unreadable or implausible, in which case this layer stays out of the way. +bool ReadTableDesc(uc_engine* uc, uint32_t self, TableCache* t) { + if (!TryReadU32(uc, self + kOffCountA, &t->countA)) return false; + if (!TryReadU32(uc, self + kOffCountB, &t->countB)) return false; + if (!TryReadU32(uc, self + kOffEntriesA, &t->entriesA)) return false; + if (!TryReadU32(uc, self + kOffEntriesB, &t->entriesB)) return false; + if (!TryReadU32(uc, self + kOffPoolA, &t->poolA)) return false; + if (!TryReadU32(uc, self + kOffPoolB, &t->poolB)) return false; + if (!TryReadU32(uc, self + kOffPoolThreshold, &t->threshold)) return false; + uint64_t total = (uint64_t)t->countA + t->countB; + return total != 0 && total <= kMaxEntries; +} + +bool BuildIndex(uc_engine* uc, uint32_t self, TableCache* t) { + TableCache desc; + if (!ReadTableDesc(uc, self, &desc)) return false; + desc.byName.reserve((size_t)(desc.countA + desc.countB) * 2); + + const uint32_t total = desc.countA + desc.countB; + for (uint32_t i = 0; i < total; i++) { + uint32_t nameAddr = 0; + if (!EntryNameAddr(uc, desc, i, &nameAddr)) return false; + std::string name; + if (!TryReadString(uc, nameAddr, &name)) return false; + if (name.empty()) continue; + // First occurrence wins - the guest loop returns on its first match. + desc.byName.emplace(std::move(name), (int32_t)i); + } + *t = std::move(desc); + g_builds.fetch_add(1, std::memory_order_relaxed); + return true; } } // namespace @@ -97,42 +126,77 @@ void HookCb(uc_engine* uc, uint64_t, uint32_t, void*) { uint32_t self = 0, namePtr = 0; uc_reg_read(uc, UC_ARM_REG_R0, &self); uc_reg_read(uc, UC_ARM_REG_R1, &namePtr); - if (!self || !namePtr) return; // let the guest handle its own edge cases + if (!self || !namePtr) return; - // If the guest's own cache is ever switched on, step aside entirely - - // that path also WRITES into guest structures (it memoises the result), - // and duplicating that behaviour here would be guesswork. + // If the guest's own hash cache is ever enabled, step aside - that path + // also WRITES into guest structures (it memoises), and reproducing that + // here would be guesswork. uint8_t cacheFlag = 0; if (uc_mem_read(uc, self + kOffCacheFlag, &cacheFlag, 1) != UC_ERR_OK) return; if (cacheFlag) return; - auto& eng = GuestEngine::Instance(); - std::string name = ReadGuestString(eng, namePtr); - if (name.empty()) return; + std::string query; + if (!TryReadString(uc, namePtr, &query) || query.empty()) return; - int32_t result = -1; + int32_t candidate = -1; { std::lock_guard lock(g_mutex); TableCache& cache = g_tables[self]; - // Rebuild when the table has grown/shrunk since it was indexed. The - // counts are the same two fields the guest loop bounds itself with, - // so any change it can see, this sees too. - uint32_t countA = ReadU32(uc, self + kOffCountA); - uint32_t countB = ReadU32(uc, self + kOffCountB); - if (cache.byName.empty() || cache.countA != countA || cache.countB != countB) { - BuildIndex(eng, uc, self, cache); - g_builds.fetch_add(1, std::memory_order_relaxed); + + // The first attempt keyed the cache on the table ADDRESS and treated + // a changed entry count as "the table grew". The live log disproved + // that: one address alternated between 35 and 59 entries, i.e. + // DIFFERENT objects reusing the same address. So the descriptor is + // re-read every call (seven cheap word reads) and the index is + // rebuilt whenever any of it moved. + TableCache desc; + if (!ReadTableDesc(uc, self, &desc)) return; + const bool stale = cache.byName.empty() || cache.countA != desc.countA || + cache.countB != desc.countB || cache.entriesA != desc.entriesA || + cache.entriesB != desc.entriesB || cache.poolA != desc.poolA || + cache.poolB != desc.poolB || cache.threshold != desc.threshold; + if (stale && !BuildIndex(uc, self, &cache)) { + g_tables.erase(self); + return; // could not index safely - let the guest do its own scan + } + + auto it = cache.byName.find(query); + if (it == cache.byName.end()) { + // Not found is NOT answered from cache: a stale index would turn + // a real entry into a false "absent", and absence is exactly what + // the caller acts on. Let the guest's own scan decide. + g_fellThrough.fetch_add(1, std::memory_order_relaxed); + return; + } + candidate = it->second; + + // Verify the hit against live guest memory before trusting it. This + // is what makes a wrong structural assumption cost performance + // instead of correctness. + uint32_t nameAddr = 0; + std::string actual; + if (!EntryNameAddr(uc, cache, (uint32_t)candidate, &nameAddr) || + !TryReadString(uc, nameAddr, &actual) || actual != query) { + g_tables.erase(self); + g_fellThrough.fetch_add(1, std::memory_order_relaxed); + return; } - auto it = cache.byName.find(name); - if (it != cache.byName.end()) result = it->second; } - uint64_t n = g_hits.fetch_add(1, std::memory_order_relaxed) + 1; - if (n % 100000 == 0) { - Log("name_lookup_accel: %llu lookups served natively (%llu index builds)", - (unsigned long long)n, (unsigned long long)g_builds.load(std::memory_order_relaxed)); + uint64_t n = g_served.fetch_add(1, std::memory_order_relaxed) + 1; + if (n % 200000 == 0) { + Log("name_lookup_accel: %llu lookups served natively, %llu fell through to the guest, " + "%llu index builds", + (unsigned long long)n, + (unsigned long long)g_fellThrough.load(std::memory_order_relaxed), + (unsigned long long)g_builds.load(std::memory_order_relaxed)); } - ReturnToCaller(uc, (uint32_t)result); + + uint32_t lr = 0, ret = (uint32_t)candidate; + uc_reg_read(uc, UC_ARM_REG_LR, &lr); + uc_reg_write(uc, UC_ARM_REG_R0, &ret); + uc_reg_write(uc, UC_ARM_REG_PC, &lr); + uc_emu_stop(uc); } } // namespace name_lookup_accel