From 84ec59d62657b19939769297507fa2ce9398b35d Mon Sep 17 00:00:00 2001 From: Saty <5752427+SatyPardus@users.noreply.github.com> Date: Wed, 21 Oct 2026 06:21:21 +0200 Subject: [PATCH] Stale baselines and file-local statics no longer bounce batches Before a check counts regressions, the units it would blame are compiled as main has them (once per main commit) and their recorded status is updated: a header change merged later could leave a unit building differently on main than recorded (FrameScript_Sprintf 85.9% vs 86.1%, Spell_C_NeedsCooldownEvent recorded exact, built at 89.7%), and every recompile of the unit was counted as a regression. A file-local static is resolved by object and name (Foo_Scripts.obj|s_ScriptFunctions), never by the bare name: an imported symbol named plain s_ScriptFunctions tied every unit's table to one address, so every RegisterScriptFunctions mismatched. Co-Authored-By: Claude Opus 5.5 --- src/server/merge.js | 1 + src/server/verify.js | 36 ++++++++++++++++++++++++++++++++++++ src/shared/compare.js | 14 ++++++++++++-- test/compare.test.js | 17 +++++++++++++++++ test/regressions.test.js | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/server/merge.js b/src/server/merge.js index 9c8c04d..0dffc28 100644 --- a/src/server/merge.js +++ b/src/server/merge.js @@ -302,6 +302,7 @@ export class Merger { return; } } + await this.verifier.refreshBaselines(check, merged.files, `merge-${batch.id}`); const targets = new Set(attempts.map((a) => a.symbol_id)); // The batch a split part came from holds the other functions of these files (still in review, // their source put back as on main); shared changes in the file may change their compiled code. diff --git a/src/server/verify.js b/src/server/verify.js index 4c51760..f3a8fb5 100644 --- a/src/server/verify.js +++ b/src/server/verify.js @@ -318,6 +318,41 @@ export class Verifier { return { check, rec: reconcile(check, symbols), changedHeaders: saved.changedHeaders, reused: check.units.length - part.check.units.length, compiled: part.check.units.length }; } + /** + * A recorded status can be older than main: a header change merged later recompiles a unit only + * where it mentions what changed (affected.js), so a unit's functions can build differently on + * main than recorded. Before a check counts regressions, the units it would blame are compiled + * as main has them (once per main commit) and their recorded status is brought up to date, so a + * submission is compared with what main actually builds. + */ + async refreshBaselines(check, files, jobName) { + const found = this.regressions(check, files); + if (!found.length) return 0; + const head = this.repo.head(); + this.baselineAt ??= new Map(); + const units = [...new Set(found.map((r) => r.file))].filter((u) => isUnit(u) && this.baselineAt.get(u) !== head); + if (!units.length) return 0; + const { check: main } = await this.check({}, `baseline-${jobName}`, { onlyUnits: units, lint: false }); + if (this.repo.head() !== head) return 0; // main moved meanwhile: the next check measures again + const compiled = new Set(main.units.filter((u) => u.compiled).map((u) => u.file)); + const ids = new Set(found.map((r) => r.id)); + let changed = 0; + for (const [id, { f, unit }] of bestResults(main)) { + if (!ids.has(id) || !compiled.has(unit)) continue; + const old = this.store.get('SELECT status, score, code_hash FROM func_status WHERE symbol_id = ?', id); + if (!old || (old.status === f.status && Math.abs(old.score - f.score) <= 1e-4 && old.code_hash === (f.codeHash ?? old.code_hash))) continue; + this.store.run( + 'UPDATE func_status SET file = ?, status = ?, score = ?, code_hash = COALESCE(?, code_hash), commit_hash = ?, updated_at = ? WHERE symbol_id = ?', + unit, f.status, f.score, f.codeHash ?? null, head, now(), id, + ); + this.store.event('baseline_refreshed', null, { id, was: old.status, wasScore: old.score, now: f.status, score: f.score, commit: head }); + this.log.info(`baseline of ${id} was stale (${old.status} ${(old.score * 100).toFixed(1)}%); main builds it as ${f.status} ${(f.score * 100).toFixed(1)}%`); + changed++; + } + for (const u of compiled) this.baselineAt.set(u, head); + return changed; + } + /** Functions that match on main but would no longer match with this change. */ regressions(check, files = {}) { const results = new Map([...bestResults(check)].map(([id, b]) => [id, b.f])); @@ -467,6 +502,7 @@ export class Verifier { // Verify fast: the submitted units (enough to judge and route every submitted function). The // units that only include a changed header are compiled when the batch merges. const { check, rec, changedHeaders } = await this.check(merged.files, `batch-${batch.id}`, { dependents: false, targets: attempts.map((a) => a.symbol_id) }); + await this.refreshBaselines(check, merged.files, `batch-${batch.id}`); const targets = new Set(attempts.map((a) => a.symbol_id)); const outside = this.changedOutside(check, merged.files, targets, batch.user_id); let failed = 0; diff --git a/src/shared/compare.js b/src/shared/compare.js index f149778..33fecb3 100644 --- a/src/shared/compare.js +++ b/src/shared/compare.js @@ -140,8 +140,18 @@ export function bindAnnotations({ listing, file, annotations, nameIndex = new Ma export function compareObject({ obj, listing, file, annotations, symbols, nameIndex = new Map(), pe }) { const objFns = new Map(obj.functions().map((f) => [f.name, f])); const sizes = new Map([...objFns].map(([n, f]) => [n, f.bytes.length])); - const { byAnn, bindings, errors } = bindAnnotations({ listing, file, annotations, nameIndex, symbols, sizes }); + const found = bindAnnotations({ listing, file, annotations, nameIndex, symbols, sizes }); + const { byAnn, errors } = found; + // A file-local static (s_ScriptFunctions, a function's static local) has the same name in every + // unit that defines one: it is known by its object and name (Foo_Scripts.obj|s_ScriptFunctions), + // never by the name alone, which would tie every unit's table to one address. + const unitObj = file.replace(/\.[^./\\]+$/, '') + '.obj'; + const local = new Set(obj.dataSymbols().filter((s) => s.storageClass === 3 && !obj.literal(s)).map((s) => s.name)); + const qualify = (name) => (local.has(name) ? `${unitObj}|${name}` : name); + const bindings = new Map([...found.bindings].map(([sym, id]) => [qualify(sym), id])); const resolve = (name) => bindings.get(name) ?? nameIndex.get(name) ?? null; + const withLocalNames = (fn) => + fn.relocs.some((r) => local.has(r.symbol?.name)) ? { ...fn, relocs: fn.relocs.map((r) => (local.has(r.symbol?.name) ? { ...r, symbol: { ...r.symbol, name: qualify(r.symbol.name) } } : r)) } : fn; const procsBySymbol = new Map(listing.procs.map((p) => [p.symbol, p])); const functions = []; @@ -162,7 +172,7 @@ export function compareObject({ obj, listing, file, annotations, symbols, nameIn continue; } const proc = procsBySymbol.get(symbol); - const r = compareFunction({ fn, addr: sym.addr, origSize: sym.size, pe, resolve, symbols, proc, literalOf: (s) => obj.literal(s) }); + const r = compareFunction({ fn: withLocalNames(fn), addr: sym.addr, origSize: sym.size, pe, resolve, symbols, proc, literalOf: (s) => obj.literal(s) }); inferred.push(...r.inferred); delete r.inferred; functions.push({ ...base, addr: sym.addr, ...r, asm: procText(proc) }); diff --git a/test/compare.test.js b/test/compare.test.js index b06bb33..07c65aa 100644 --- a/test/compare.test.js +++ b/test/compare.test.js @@ -127,3 +127,20 @@ test('lcsRatio', () => { assert.equal(lcsRatio(Buffer.from('abcd'), Buffer.from('abxd')), 0.75); assert.equal(lcsRatio(Buffer.alloc(0), Buffer.alloc(0)), 1); }); + +// Every unit's s_ScriptFunctions is its own table: a file-local static is known by object and name. +test('a file-local static resolves by its unit, never by the bare name', () => { + const f = fixture(); + const { annotations } = parseAnnotations(f.source, FILE); + const obj = new CoffObject(f.obj); + const real = obj.dataSymbols.bind(obj); + obj.dataSymbols = () => [...real(), { name: '?g_val@@3HA', section: 2, storageClass: 3 }]; + const syms = symbols([{ id: 'gl_09999', kind: 'global', addr: 0x405000, size: 4, decorated: '?g_val@@3HA' }]); + const compare = (nameIndex) => + compareObject({ obj, listing: parseListing(f.listing), file: FILE, annotations, symbols: syms, nameIndex, pe: new PEImage(f.pe) }).functions[0]; + // Another unit's static of the same name (at another address) does not make this one wrong. + const other = compare(new Map([['?Callee@@YAXXZ', 'fn_00002'], ['?g_val@@3HA', 'gl_09999']])); + assert.notEqual(other.status, STATUS.MISMATCH); + const own = compare(new Map([['?Callee@@YAXXZ', 'fn_00002'], ['?g_val@@3HA', 'gl_09999'], ['src/player.obj|?g_val@@3HA', 'gl_00001']])); + assert.equal(own.status, STATUS.EXACT); +}); diff --git a/test/regressions.test.js b/test/regressions.test.js index 6d123cb..76a6cb7 100644 --- a/test/regressions.test.js +++ b/test/regressions.test.js @@ -25,3 +25,36 @@ test('header-defined copies are not regressions while the header is unchanged', const files = { 'Engine/A.cpp': '', 'Engine/H/stpl.h': header, 'Engine/H/other.h': '' }; assert.deepEqual(v.withoutHeaderCopies(found, results, files).map((r) => r.id), ['fn_00001', 'fn_00003', 'fn_00004']); }); + +// A recorded status older than main (a header change merged since): main is compiled once and the +// record brought up to date, so the submission is compared with what main builds. +test('a stale baseline is measured again on main before it counts as a regression', async () => { + const rows = new Map([['fn_02237', { status: 'mismatch', score: 0.8589, code_hash: 'old', confirmed: 1 }]]); + const events = []; + const v = Object.create(Verifier.prototype); + v.log = { info() {} }; + v.repo = { head: () => 'c0ffee' }; + v.store = { + get: (_, id) => rows.get(id), + run: (_, file, status, score, hash, commit, at, id) => rows.set(id, { ...rows.get(id), file, status, score, code_hash: hash }), + event: (...e) => events.push(e), + }; + const fn = (score, codeHash) => ({ units: [{ file: 'Engine/FrameScript.cpp', compiled: true, functions: [{ id: 'fn_02237', status: 'mismatch', score, codeHash }] }] }); + v.regressions = (check) => { + const now_ = check.units[0].functions[0]; + const row = rows.get('fn_02237'); + return Math.abs(row.score - now_.score) > 1e-4 ? [{ id: 'fn_02237', file: 'Engine/FrameScript.cpp' }] : []; + }; + let compiles = 0; + v.check = async () => (compiles++, { check: fn(0.8612, 'main') }); + const submission = fn(0.8612, 'main'); + assert.equal(await v.refreshBaselines(submission, {}, 'batch-1'), 1); + assert.equal(rows.get('fn_02237').score, 0.8612); + assert.equal(rows.get('fn_02237').confirmed, 1, 'the confirmation stays'); + assert.deepEqual(v.regressions(submission), [], 'no regression against what main builds'); + // A real regression is measured on main once per commit, not on every batch. + assert.equal(await v.refreshBaselines(fn(0.5, 'worse'), {}, 'batch-2'), 0); + assert.equal(await v.refreshBaselines(fn(0.5, 'worse'), {}, 'batch-3'), 0); + assert.equal(compiles, 1); + assert.equal(events.length, 1); +});