megboyzz/nfsmw-online
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Keep name-lookup acceleration unregistered: two failed attempts recorded

Browse Source 
sub_4F3704 (resource lookup by name) is 18.8% of load-time samples and does
a linear strcmp scan over 6232 entries because the game's own hash cache is
disabled (*(self+8) == 0, confirmed by a live probe: 320,000+ calls per load).

Attempt 1 crashed the process: guest strings were read through G2H and
scanned for a NUL with no bounds check, so a bad offset walked off the end
of the mapped region (SIGSEGV, SEGV_ACCERR at a host address).

Attempt 2 was memory-safe (uc_mem_read everywhere, length and count caps,
every cache hit verified against live guest memory, falls through to the
guest on any doubt) but caused a visible frame-rate drop on the prologue
loading screen. The cause was a design flaw the earlier probe log had
already shown and I misread: the cache was keyed on the table ADDRESS, yet
this game reuses one address for different tables (35 and 59 entries
alternating in the log). The descriptor comparison therefore marked the
index stale on nearly every call, and each rebuild re-read all ~6232 entry
strings byte-by-byte - far more work than the scan it replaced.

Kept in the tree, unregistered, with both mistakes documented. The fix for a
third attempt is to key the cache on the DESCRIPTOR CONTENTS rather than the
address, so alternating tables each keep their own index, plus bulk string
reads instead of per-byte.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
megboyzz
2026-09-19 02:21:08 +03:00
co-authored by Claude
parent d5e6037fc7
commit f631a3a0a4
2 changed files with 155 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mpcore/src/main/cpp/emu/guest_engine.cpp
+14 -10
View File
@@ -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
mpcore/src/main/cpp/emu/name_lookup_accel.cpp
+141 -77
View File
@@ -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<std::string, int32_t> byName;
};
std::mutex g_mutex;
std::map<uint32_t, TableCache> g_tables; // keyed by the guest table address
std::map<uint32_t, TableCache> g_tables;
std::atomic<uint64_t> g_hits{0};
std::atomic<uint64_t> g_served{0};
std::atomic<uint64_t> g_fellThrough{0};
std::atomic<uint64_t> 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<const char*>(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<std::mutex> 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