From ddada05d2011115831905ec86bd1e6d21199cc69 Mon Sep 17 00:00:00 2001 From: paulostation Date: Fri, 14 Aug 2026 02:18:59 +0000 Subject: [PATCH] feat(headless): pump the streamer in WARMUP + add bOnSideStand differential test Two changes plus reproduction docs, on top of j0y base 5650884. 1. game_tests.cpp: the WARMUP phase now services the on-demand streaming queue (CStreaming::LoadRequestedModels() per frame, LoadAllRequestedModels(false) flush before tests). Without this the headless frame loop never drives the queue -- requested models never stream in (census loaded_bikes=0), silently blocking every test that needs a streamed model. Safe here: WARMUP runs before SuspendOtherThreads, so CdStreamThread is still alive. Same calls the game itself uses (Pools.cpp, Game.cpp). 2. test_CCarCtrl_GetNewVehicle_diff.cpp: behavioural differential test for CCarCtrl::GetNewVehicleDependingOnCarModel (0x421440, paulo-sysframe PR #1). Runs the ORIGINAL machine code (via HookDisableGuard) vs the reversed C++ on each loaded bike and compares bikeFlags.bOnSideStand, with vacuous-pass guards (compared/bikesSeen/onSideStandTrue/origSetFlag all > 0). 3. docs/: reproduction manifest (exact commands, GAME_TEST_REQUEST_MODELS + WARMUP_FRAMES, expected 34-assertion PASS, and a falsification check), the PR#1 CarCtrl reversal diff for reference, and the write-up. Result: PASS CCarCtrl/Diff_GetNewVehicleDependingOnCarModel (34 assertions), STATUS=PASS, reproduced twice. PR #1's bOnSideStand write proven faithful to the original, headless. --- docs/CarCtrl-pr1-reversal.diff | 76 ++++++ docs/REPRODUCE-bonsidestand.md | 108 +++++++++ docs/story-bike-flag-green.md | 145 ++++++++++++ headless_stubs/game_tests.cpp | 59 +++++ .../test_CCarCtrl_GetNewVehicle_diff.cpp | 216 ++++++++++++++++++ 5 files changed, 604 insertions(+) create mode 100644 docs/CarCtrl-pr1-reversal.diff create mode 100644 docs/REPRODUCE-bonsidestand.md create mode 100644 docs/story-bike-flag-green.md create mode 100644 headless_stubs/tests/test_CCarCtrl_GetNewVehicle_diff.cpp diff --git a/docs/CarCtrl-pr1-reversal.diff b/docs/CarCtrl-pr1-reversal.diff new file mode 100644 index 0000000..3e01229 --- /dev/null +++ b/docs/CarCtrl-pr1-reversal.diff @@ -0,0 +1,76 @@ +diff --git a/source/game_sa/CarCtrl.cpp b/source/game_sa/CarCtrl.cpp +index d8ceaf50..b8c3439d 100644 +--- a/source/game_sa/CarCtrl.cpp ++++ b/source/game_sa/CarCtrl.cpp +@@ -37,7 +37,7 @@ void CCarCtrl::InjectHooks() + RH_ScopedInstall(CreateCarForScript, 0x431F80); + RH_ScopedInstall(ChooseBoatModel, 0x421970); + RH_ScopedInstall(ChooseCarModelToLoad, 0x421900); +- RH_ScopedInstall(GetNewVehicleDependingOnCarModel, 0x421440, { .reversed = false }); ++ RH_ScopedInstall(GetNewVehicleDependingOnCarModel, 0x421440); + RH_ScopedInstall(IsAnyoneParking, 0x42C250); + RH_ScopedInstall(IsThisVehicleInteresting, 0x423EA0); + RH_ScopedInstall(JoinCarWithRoadAccordingToMission, 0x432CB0); +@@ -422,32 +422,45 @@ void CCarCtrl::GetAIPlaneToDoDogFightAgainstPlayer(CAutomobile* automobile) { + + // 0x421440 + CVehicle* CCarCtrl::GetNewVehicleDependingOnCarModel(int32 modelId, uint8 createdBy) { +- return plugin::CallAndReturn(modelId, createdBy); +- /* ++ const auto createdByEnum = static_cast(createdBy); ++ + switch (CModelInfo::GetModelInfo(modelId)->AsVehicleModelInfoPtr()->m_nVehicleType) { + case VEHICLE_TYPE_MTRUCK: +- return new CMonsterTruck(modelId, createdBy); ++ return new CMonsterTruck(modelId, createdByEnum); + case VEHICLE_TYPE_QUAD: +- return new CQuadBike(modelId, createdBy); ++ return new CQuadBike(modelId, createdByEnum); + case VEHICLE_TYPE_HELI: +- return new CHeli(modelId, createdBy); ++ return new CHeli(modelId, createdByEnum); + case VEHICLE_TYPE_PLANE: +- return new CPlane(modelId, createdBy); ++ return new CPlane(modelId, createdByEnum); + case VEHICLE_TYPE_BOAT: +- return new CBoat(modelId, createdBy); ++ return new CBoat(modelId, createdByEnum); + case VEHICLE_TYPE_TRAIN: +- return new CTrain(modelId, createdBy); +- case VEHICLE_TYPE_BIKE: +- return new CBike(modelId, createdBy); +- case VEHICLE_TYPE_BMX: +- return new CBmx(modelId, createdBy); ++ return new CTrain(modelId, createdByEnum); ++ case VEHICLE_TYPE_BIKE: { ++ // 0x42151B: `or byte ptr [eax + 0x614], 0x10` on the newly built bike. ++ // 0x614 is CBike's bikeFlags union and 0x10 is bOnSideStand. The ++ // original emits this write twice more -- at 0x42155C for the BMX case ++ // below, and at 0x421575, which is the `operator new` returned-NULL arm ++ // and is not reproducible in modern C++ (new throws instead). ++ auto* bike = new CBike(modelId, createdByEnum); ++ bike->bikeFlags.bOnSideStand = true; ++ return bike; ++ } ++ case VEHICLE_TYPE_BMX: { ++ // 0x42155C: the same write on the BMX path (CBmx inherits the field). ++ auto* bmx = new CBmx(modelId, createdByEnum); ++ bmx->bikeFlags.bOnSideStand = true; ++ return bmx; ++ } + case VEHICLE_TYPE_TRAILER: +- return new CTrailer(modelId, createdBy); +- case VEHICLE_TYPE_AUTOMOBILE: +- return new CAutomobile(modelId, createdBy, 1); ++ return new CTrailer(modelId, createdByEnum); ++ default: ++ // The original decrements the type before its jump table, so ++ // VEHICLE_TYPE_AUTOMOBILE (0) underflows the `cmp eax, 0xa` bound and ++ // lands in the default arm at 0x4216BE. ++ return new CAutomobile(modelId, createdByEnum, true); + } +- return nullptr; +- */ + } + + // 0x42C250 diff --git a/docs/REPRODUCE-bonsidestand.md b/docs/REPRODUCE-bonsidestand.md new file mode 100644 index 0000000..a007958 --- /dev/null +++ b/docs/REPRODUCE-bonsidestand.md @@ -0,0 +1,108 @@ +# Auditing the bOnSideStand differential test + +Everything needed to reproduce and scrutinise the green run described in +[../stories/2026-08-14-bike-flag-green.md](../stories/2026-08-14-bike-flag-green.md), +and to check that the test proves what it claims rather than passing vacuously. + +## Exact base (what to check out) + +| Repo | Commit | +| --- | --- | +| outer (`j0y/gta-reversed-diff-test`) | `5650884d9bfae876f4d51101b8295661f280e50c` | +| nested (`gta-reversed`, detached) | `b1399d66e9c3a958ba92227c1f97d076e136ec5a` | +| docker image `gta-reversed-build:latest` | `a7ef7b4a70b5` (12.3 GB) | + +The game binary (`gta_sa_compact.exe`) and `GTASA/` data are copyrighted and not +in any repo; an auditor needs their own copy on CT 103 at +`/opt/difftest/gta-reversed-diff-test/`. + +## The three changes on top of that base + +Apply these to the clean checkout. Each is one concern; nothing else in the +working tree (deleted CColStore/CStats tests, .ini tweaks) is required. + +1. **`01-game_tests-streaming-pump.diff`** — the harness bug fix. WARMUP now + pumps `CStreaming::LoadRequestedModels()` per frame and flushes with + `LoadAllRequestedModels(false)` before tests run. Without it, requested models + never stream in headless (census `loaded_bikes=0`); with it, all 8 load. This + is the only change that touches shared harness code — audit it hardest. +2. **`02-CarCtrl-pr1-reversal.diff`** — PR #1 itself. The reversal of + `GetNewVehicleDependingOnCarModel` (`0x421440`), including the + `bikeFlags.bOnSideStand = true` writes and dropping `{ .reversed = false }` so + the hook is actually installed. This is the *code under test*. +3. **`03-test_CCarCtrl_GetNewVehicle_diff.cpp`** — the test (an untracked file; + drop it into `headless_stubs/tests/`). This is the *auditor's instrument*; + read it before trusting its verdict. + +## Reproduce + +```bash +# on CT 103, from /opt/difftest/gta-reversed-diff-test +export SSH_AUTH_SOCK=$(find /tmp -maxdepth 2 -type s -name 'agent.*' | head -1) # if driving remotely; see ADR 0001 + +# 1. build the DLL with the three changes applied +./scripts/docker-build.sh build-tests # expect 0 errors, fresh build-output/gta_reversed.asi + +# 2. run the bike test, requesting the 8 bike/BMX models and pumping enough +# warmup frames for the streamer to deliver them +docker run --rm \ + -v "$PWD/GTASA:/game:ro" -v "$PWD/build-output:/build:ro" \ + -v "$PWD/gamebin/gta_sa_compact.exe:/gamebin/gta_sa_compact.exe:ro" \ + -v "$PWD/scripts:/scripts:ro" -v "$PWD/configs:/configs:ro" \ + -v /tmp/wine-logs:/tmp/wine-logs \ + -e GAME_TEST_ENABLE=1 \ + -e GAME_TEST_FILTER=CCarCtrl \ + -e GAME_TEST_REQUEST_MODELS=481,509,510,448,461,463,468,521 \ + -e GAME_TEST_WARMUP_FRAMES=600 \ + -e TIMEOUT=300 \ + gta-reversed-build bash -c '/scripts/run-headless.sh > /tmp/wine-logs/audit-run.log 2>&1; cp /opt/wine-gtasa/drive_c/*.txt /tmp/wine-logs/ 2>/dev/null; cp /opt/wine-gtasa/drive_c/Games/GTASA/*.txt /tmp/wine-logs/ 2>/dev/null; true' + +# 3. read the verdict (authoritative — NOT the test's printf, which does not reach the file) +grep -E 'GetNewVehicleDependingOnCarModel|PASSED=|FAILED=|STATUS=' /tmp/wine-logs/game_test_results.txt +``` + +`bike_iter.sh` (in the harness scratch / repo) wraps steps 2–3 and prints a JSON +verdict; the manual commands above are the ground truth it automates. + +## Expected output + +``` + PASS CCarCtrl/Diff_GetNewVehicleDependingOnCarModel (34 assertions) +PASSED=20 FAILED=0 STATUS=PASS +``` + +`34 = 10 loaded vehicles × 3 EXPECT_EQ (created, type, onSideStand) + 4 EXPECT_GT +guards`. A full sweep with no early bail. + +## How to check the test is not lying (the important part) + +A green run is only as trustworthy as the test. Verify each of these against +`03-...cpp`: + +- **The reference is the original, not a constant.** The `orig` side runs inside + `HookDisableGuard guard(kHookPath)`, which un-hooks the reversed function so the + original machine code at `0x421440` executes. Grep for `HookDisableGuard`. If it + compared against a hardcoded `true`, it would prove nothing. +- **Falsification test — break it on purpose.** In + `02-CarCtrl-pr1-reversal.diff`, delete the two `bike->bikeFlags.bOnSideStand = + true;` lines, rebuild, rerun. The test **must** flip to FAIL (the reversed side + now reads false while the original still sets it). If it stays green, the test + is vacuous. This is the single most convincing check. +- **The four guards fire.** `EXPECT_GT(compared,0)`, `EXPECT_GT(bikesSeen,0)`, + `EXPECT_GT(onSideStandTrue,0)`, `EXPECT_GT(origSetFlag,0)`. The last two are + counted separately so that `EXPECT_EQ(orig,rev)` cannot pass by *both* sides + reading false — `origSetFlag>0` proves the original genuinely sets the flag. +- **Bikes actually loaded.** Confirm `ids=[448 461 463 468 481 509 510 521]` in a + census run, or that the sweep's `bikesSeen` reached the two bike arms. With + `GAME_TEST_WARMUP_FRAMES` too low (e.g. the default 100) the streamer delivers + nothing and the run would be vacuous — the pump fix (change 1) is what avoids + this. + +## Scope of the claim (what a green does and does not establish) + +- **Does** prove: our reversed `0x421440`, called at construction, sets + `bOnSideStand` identically to the original, on every loaded bike/BMX. +- **Does not** prove: that ambient traffic organically reaches this function, nor + that the flag has its intended downstream physics effect. Those are separate + validations (organic differential trace; end-to-end play). See the story's + closing section. diff --git a/docs/story-bike-flag-green.md b/docs/story-bike-flag-green.md new file mode 100644 index 0000000..125215f --- /dev/null +++ b/docs/story-bike-flag-green.md @@ -0,0 +1,145 @@ +# How a bike finally stood on its side-stand headless + +A short account of getting `test_CCarCtrl_GetNewVehicle_diff` to a meaningful +green on CT 103 — and what "green" actually asserts. + +## The question + +PR #1 of `paulo-sysframe/gta-reversed` reverses one function, +`CCarCtrl::GetNewVehicleDependingOnCarModel` (`0x421440`). Its one +behaviourally-load-bearing act: when it builds a `CBike`/`CBmx`, it sets +`bikeFlags.bOnSideStand = true`, reproducing the original's +`or byte ptr [eax+0x614], 0x10`. A newly created bike stands on its kickstand. + +The static gate the project runs — compile + hook_ok + signature_ok — cannot see +that. It has waved through a body of `return train;`, an inverted `!`, and +reimplementations that dropped this very flag write. So the question was never +"does it compile" but "does it *behave* like the original." Answering that means +running the reversed function on a real bike and watching the flag. + +## Why it wasn't already answered + +To run the function on a bike, a bike model has to exist in the world. Headless, +almost nothing is streamed in. The test asks the game to stream eight bike/BMX +models via `GAME_TEST_REQUEST_MODELS`, then sweeps loaded models and compares. + +That request had, it turned out, **never once delivered a model.** + +## The three dead ends and the one real cause + +**Iteration 0 (warmup=100):** census `loaded_vehicles=1, loaded_bikes=0`. No +bikes. The request *fired* (`requested 8 model(s)` in the log), so wiring was +fine. Something else. + +**Iteration 1 (warmup=600):** identical — `1 vehicle, 0 bikes`. Six times the +warmup frames changed *nothing*. That was the tell. A timing problem gets better +with more time; this didn't move at all. So it wasn't a budget knob. + +**The diagnosis.** The requested IDs were all real bikes in `vehicles.ide` +(pcj600, freeway, sanchez, bmx…). None ever `ConvertBufferToObject`'d. And the +streaming log showed only `CStreaming::Init` / `InitImageList` — **never** +`Update` or `LoadRequestedModels`. Map/object models streamed in fine through a +different, pumped path; on-demand `RequestModel` for a vehicle sat queued +forever. The warmup frame just `return`ed to "let the game process the frame +normally" — but the headless frame loop never drives the streamer's on-demand +queue. Nobody was turning the crank. + +## The fix + +In `game_tests.cpp`, WARMUP phase, service the queue explicitly: + +```cpp +CStreaming::LoadRequestedModels(); // each warmup frame +... +CStreaming::LoadAllRequestedModels(false); // final flush before tests run +``` + +These are the exact calls the game itself uses (`Pools.cpp`, `Game.cpp`). They +are safe *here* precisely because WARMUP runs **before** `SuspendOtherThreads` — +`CdStreamThread` is still alive. The deadlock the original author warned about +only bites *inside* tests, after that thread is parked. So the fix respects the +constraint instead of fighting it. + +Rebuild. Re-run. Census: **`loaded_vehicles=10, loaded_bikes=8, +ids=[448 461 463 468 481 509 510 521]`.** All eight bikes, resident. + +This was a genuine harness bug, not a test quirk: on-demand streaming had never +worked headless, which silently blocked *every* test that needs a streamed +model — not just this one. + +## One last scaffold + +The test still reported FAIL. Not a real failure — a temporary census block the +author had left in, calling `RecordFailure` unconditionally to print the census +to the results file, with a comment: *"removed once the census is answered."* +The census was now answered. Removed the block; the real sweep ran. + +## The docker image + +None of this touched the container. `gta-reversed-build:latest` (12.3 GB) is the +same image throughout: Wine + a **null `d3d9.dll`** + a `/dev/null`-style +framebuffer under Xvfb, running the real `gta_sa.exe` with `gta_reversed.asi` +hooked in. It was already working — it boots the game, streams the world, runs +the in-process test registry. What was broken lived in the harness C++ *inside* +the DLL, not in the image. The image never needed a fix; the diagnosis just +kept looking like it might, until the streaming log named the real culprit. + +## How we assert it works + +"Green" here is not "the process exited 0." The test is **differential**: for +each loaded model it runs the function twice from the same state — + +```cpp +{ + HookDisableGuard guard(kHookPath); // <- original 2004 machine code + orig = BuildAndObserve(modelId, RANDOM_VEHICLE); +} +rev = BuildAndObserve(modelId, RANDOM_VEHICLE); // <- our reversed C++ + +EXPECT_EQ(orig.created, rev.created); // both built a vehicle +EXPECT_EQ(orig.type, rev.type); // same switch arm taken +EXPECT_EQ(orig.onSideStand, rev.onSideStand); // same flag — the write under test +``` + +`HookDisableGuard` temporarily un-hooks the reversed function so the *original* +machine code at `0x421440` runs; then the reversed version runs. The reference is +the original itself, never a value we hand-wrote — a test that trusts the +candidate to say what it does would just agree with the code it is meant to +check. + +Then four guards make a green mean something: + +| Guard | Stops the false pass where… | +| --- | --- | +| `EXPECT_GT(compared, 0)` | nothing loaded, so nothing was tested | +| `EXPECT_GT(bikesSeen, 0)` | only an automobile loaded, so the bike arm never ran | +| `EXPECT_GT(onSideStandTrue, 0)` | **our** code silently dropped the flag write | +| `EXPECT_GT(origSetFlag, 0)` | recorded separately — proves the **original** sets it too, so `EXPECT_EQ` isn't "both agree on false" | + +That last pair is the crux. `EXPECT_EQ(orig.onSideStand, rev.onSideStand)` alone +would pass if *neither* side set the flag. Counting `origSetFlag` and +`onSideStandTrue` separately turns "they agree" into "they agree, and the flag +was actually set, by both." + +## The result + +``` +PASS CCarCtrl/Diff_GetNewVehicleDependingOnCarModel (34 assertions) +FAILED=0 STATUS=PASS +``` + +34 = 10 loaded vehicles × 3 `EXPECT_EQ` + 4 `EXPECT_GT` guards. A full sweep, no +early bail, reproduced across two runs. Eight bikes built through the reversed +`0x421440`; every one set `bOnSideStand` exactly as the original does. + +PR #1 is not just *matched* — it is **exercised and faithful**, proven against +the original machine code on real bikes, headless, with no framebuffer. + +## The principle underneath + +The goal invoked the Totalitarian Principle — *what is not forbidden is +compulsory.* A bike created through this function is not forbidden, so it *must* +be reachable. But the useful reading turned out not to be "wait long enough and a +bike appears." It was: **this state is not forbidden — so find what is forbidding +it.** What forbade it was a streamer that never pumped. Remove that, and the +compulsory thing happens on the very next frame. diff --git a/headless_stubs/game_tests.cpp b/headless_stubs/game_tests.cpp index 6041526..542deec 100644 --- a/headless_stubs/game_tests.cpp +++ b/headless_stubs/game_tests.cpp @@ -20,6 +20,8 @@ static FILE* s_resultFile = nullptr; + + static void TestLog(const char* fmt, ...) { char buf[1024]; va_list args; @@ -40,6 +42,47 @@ static void TestLog(const char* fmt, ...) { } } +// =================================================================== +// Model pre-loading (WARMUP phase only) +// =================================================================== +// +// GAME_TEST_REQUEST_MODELS=481,461,... -- ask the streamer for these model IDs +// at the START of warmup, then let the warmup frames drive the load. +// +// This exists because tests run inside SuspendOtherThreads (see RUN_TESTS +// below), which parks CdStreamThread. A blocking LoadAllRequestedModels() from +// within a test therefore never returns -- the loader waits for streaming +// channels that nothing is pumping. Requesting here instead costs nothing: the +// frames were already going to elapse. +// +// Deliberately does NOT block or verify. A model that fails to arrive is the +// test's problem to notice and report, not this hook's to force. +static void RequestTestModels() { + const char* env = getenv("GAME_TEST_REQUEST_MODELS"); + if (!env || !env[0]) { + return; + } + char buf[512]; + strncpy(buf, env, sizeof(buf) - 1); + buf[sizeof(buf) - 1] = '\0'; + + int requested = 0; + for (char* tok = strtok(buf, ","); tok; tok = strtok(nullptr, ",")) { + while (*tok == ' ') tok++; + const int id = atoi(tok); + if (id <= 0) { + continue; + } + if (!CStreaming::IsModelLoaded(id)) { + // GAME_REQUIRED only. KEEP_IN_MEMORY would pin these for the whole + // run and perturb later tests' streaming budget. + CStreaming::RequestModel(id, STREAMING_GAME_REQUIRED); + requested++; + } + } + TestLog("GAME_TEST_REQUEST_MODELS: requested %d model(s)", requested); +} + // SEH wrapper — must be in a separate function from C++ destructors (MSVC limitation) static bool RunSingleTest(void (*fn)()) { __try { @@ -70,6 +113,9 @@ void GameTestRunnerOnFrame() { // Get warmup frame count from env (default 100 — enough for population to spawn) const char* envFrames = getenv("GAME_TEST_WARMUP_FRAMES"); s_warmupFrames = envFrames ? atoi(envFrames) : 100; + // Ask for any test-required models NOW, so the warmup frames below + // stream them in while CdStreamThread is still running. + RequestTestModels(); s_phase = RunnerPhase::WARMUP; } break; @@ -78,8 +124,21 @@ void GameTestRunnerOnFrame() { // Let the game run to populate world with ambient peds/vehicles if (s_warmupFrames > 0) { s_warmupFrames--; + // Pump the streamer explicitly. The headless frame loop does not + // drive CStreaming's on-demand queue (the log shows only Init / + // InitImageList, never Update / LoadRequestedModels), so models + // asked for by RequestTestModels() sit requested-but-unloaded no + // matter how many warmup frames elapse. Servicing it here is safe: + // CdStreamThread is still running during WARMUP -- the deadlock the + // RequestTestModels comment warns about only occurs INSIDE tests, + // after SuspendOtherThreads parks that thread. Same call the game + // itself uses (Pools.cpp:224, Game.cpp:459). + CStreaming::LoadRequestedModels(); return; // let game process this frame normally } + // Final flush: block until everything still queued is resident, so the + // tests see a fully-loaded set rather than a partially-streamed one. + CStreaming::LoadAllRequestedModels(false); s_phase = RunnerPhase::RUN_TESTS; break; diff --git a/headless_stubs/tests/test_CCarCtrl_GetNewVehicle_diff.cpp b/headless_stubs/tests/test_CCarCtrl_GetNewVehicle_diff.cpp new file mode 100644 index 0000000..f20bb7d --- /dev/null +++ b/headless_stubs/tests/test_CCarCtrl_GetNewVehicle_diff.cpp @@ -0,0 +1,216 @@ +// test_CCarCtrl_GetNewVehicle_diff.cpp -- behavioural test for +// CCarCtrl::GetNewVehicleDependingOnCarModel @ 0x421440. +// +// A SEPARATE FILE from test_CCarCtrl.cpp, deliberately: that file holds six +// working tests and an upstream update must not silently drop ours, nor our +// mistake break theirs. build-tests.sh globs test_*.cpp so the name is enough. +// +// WHY THIS FUNCTION +// +// It is the one the C5 run reversed and PR #1 proposes upstream, and it has +// never been executed. The gate it passed is static -- compile + hook_ok + +// signature_ok -- and the project's own record shows that gate accepting a body +// of `return train;` (run 7) and an inverted `!` (upstream #1247). +// +// It is also the sharper case. Runs C4 and C5 passed the static gate on this +// address while performing NEITHER of the two flag writes the original +// performs, and run C3 wrote a REAL field (`vehicleFlags.bUseCarCheats`) at an +// impossible offset -- CVehicle ends at 0x5A0, 116 bytes short of the 0x614 +// this function writes. An invented name fails at compile; a misplaced real one +// compiles, hooks, passes the signature check, and is wrong at runtime. +// +// WHAT IS ACTUALLY BEING COMPARED +// +// The function's whole observable effect is the OBJECT it returns: +// +// 1. the concrete type dispatched to (the switch), and +// 2. bikeFlags.bOnSideStand on the two bike arms (the writes at 0x42151B +// and 0x42155C). +// +// So the comparison is on the vehicle's type and flag state, not on a return +// value. m_nVehicleType identifies the arm taken without RTTI, which the game +// is built without. +// +// THE THIRD WRITE IS NOT TESTED, AND CANNOT BE +// +// There is a third `or byte ptr [eax + 0x614], 0x10` at 0x421575. It is the +// `operator new` NULL-return arm: 0x42150C and 0x42154D both `je` there after +// `test eax, eax`, and 0x421573 zeroes eax before the write. The original's C++ +// set the flag without checking new for NULL, so 2004 MSVC emitted the write on +// both paths. Modern C++ cannot reach it -- new throws rather than returning +// NULL -- so a faithful reimplementation performs TWO writes, and no test can +// exercise the third without forcing an allocation failure. +// +// EACH CALL ALLOCATES +// +// Unlike a scalar-returning diff test, every call here news a vehicle. They are +// not added to CWorld, so `delete` is the whole cleanup, but it must happen on +// every path or a sweep leaks a vehicle per iteration and perturbs the pool the +// next test observes. +// +// A NULL RETURN IS NOT A PASS +// +// If a model is not loaded, both sides may return nullptr and agree vacuously. +// The test records how many comparisons actually carried a vehicle and fails if +// that count is zero -- the same reasoning as the difftest-agent refusing to +// read a missing results file as a pass. + +#include "StdInc.h" +#include "TestFramework.h" +#include "ScenarioHelpers.h" + +namespace { + +constexpr const char* kHookPath = "Global/CCarCtrl/GetNewVehicleDependingOnCarModel"; + +// What the function is observed to produce. Compared field by field so a +// failure names which half diverged rather than just "not equal". +struct Built { + bool created; // did it return anything at all + int32 type; // m_nVehicleType -- which switch arm was taken + bool isBikeish; // BIKE or BMX: the two arms carrying the write + bool onSideStand; // bikeFlags.bOnSideStand -- the write under test +}; + +Built Observe(CVehicle* v) { + Built b{}; + if (!v) { + return b; + } + b.created = true; + b.type = static_cast(v->m_nVehicleType); + + // bikeFlags lives on CBike. Reading it off anything else would read past + // the object on, say, a CBoat -- so gate on the type, not on the pointer. + // Both arms matter: CBmx derives from CBike and 0x42155C is its write. + b.isBikeish = v->IsBike() || v->IsBMX(); + if (b.isBikeish) { + b.onSideStand = static_cast(v)->bikeFlags.bOnSideStand; + } + return b; +} + +// Build once through whichever implementation is installed, observe it, and +// free it. Returning the observation rather than the pointer keeps the lifetime +// in one place. +Built BuildAndObserve(int32 modelId, uint8 createdBy) { + CVehicle* v = CCarCtrl::GetNewVehicleDependingOnCarModel(modelId, createdBy); + Built b = Observe(v); + delete v; // not CWorld::Add'ed, so this is the whole cleanup + return b; +} + + +// Bike/BMX model IDs, for reference and for the run script's +// GAME_TEST_REQUEST_MODELS list. The two arms under test are reachable no other +// way: at game state 9 under FastLoader almost nothing is streamed in, and the +// first version of this test compared exactly ONE model (an automobile), so +// bOnSideStand was never touched and it passed anyway. +// +// This file deliberately does NOT request them itself. Tests run inside +// SuspendOtherThreads, which parks CdStreamThread, so any streaming call from +// here deadlocks -- measured 2026-08-12, LoadAllRequestedModels entered and +// never returned at both a 200s and a 600s budget, stopping at the identical +// iteration both times. The request happens during WARMUP instead (see +// RequestTestModels in game_tests.cpp), where the frame loop still pumps the +// streamer. +// +// GAME_TEST_REQUEST_MODELS=481,509,510,448,461,463,468,521 +// +// Types are VERIFIED at runtime below rather than trusted, so a wrong ID +// degrades coverage instead of testing the wrong thing. + +} // namespace + +GAME_DIFF_TEST(CCarCtrl, GetNewVehicleDependingOnCarModel) { + // Sweep every loaded vehicle model rather than one: the interesting arms + // are BIKE and BMX, and which models are streamed in at game state 9 is not + // under this test's control. A single model would probably be an + // automobile and would never reach the write being tested. + int32 compared = 0; + int32 bikesSeen = 0; + int32 onSideStandTrue = 0; + int32 origSetFlag = 0; // the ORIGINAL's own behaviour, recorded separately + + // Census FIRST. If no bike is loaded the sweep below proves nothing, and a + // bare assertion failure would not say WHY. Naming which of the requested + // models arrived distinguishes "the streamer never ran" from "it ran and + // these specific IDs were not among what it delivered". + // (census scaffold removed: the streamed-model question is answered -- + // 8 bike models load with the WARMUP streamer pump. The real sweep follows.) + + for (int32 modelId = 400; modelId <= 611; modelId++) { + if (!CStreaming::IsModelLoaded(modelId)) { + continue; + } + + // Original first, then reversed, on the same model. + Built orig; + { + HookDisableGuard guard(kHookPath); + orig = BuildAndObserve(modelId, RANDOM_VEHICLE); + } + Built rev = BuildAndObserve(modelId, RANDOM_VEHICLE); + + EXPECT_EQ(orig.created, rev.created); + if (!orig.created || !rev.created) { + continue; // nothing built on either side; nothing to compare + } + + // Which switch arm was taken. + EXPECT_EQ(orig.type, rev.type); + + // The write this function was reversed for. Only meaningful on the bike + // arms; on every other type both sides read false and it is a no-op. + EXPECT_EQ(orig.onSideStand, rev.onSideStand); + + compared++; + if (orig.isBikeish) { + bikesSeen++; + // The write under test actually fired. Counted so a pass can be + // distinguished from "agreed that nothing happened". + if (rev.onSideStand) { + onSideStandTrue++; + } + // Recorded SEPARATELY from the reversed side. EXPECT_EQ only proves + // the two agree; it cannot tell "both set it" from "neither did". + // This is the evidence that the write is the ORIGINAL's behaviour, + // which is the claim PR #1 actually makes. + if (orig.onSideStand) { + origSetFlag++; + } + } + } + + // Vacuous-pass guards. A green result must mean something was compared AND + // that the write this function was reversed for actually executed. + // + // compared > 0: nothing loaded means nothing was tested. + EXPECT_GT(compared, 0); + + // bikesSeen > 0: an EXPECT rather than a printed note, because the run + // script requests eight bike/BMX models via GAME_TEST_REQUEST_MODELS. If + // none arrives, the streamer never delivered them and the two arms at + // 0x42151B and 0x42155C were never entered -- so the run says nothing about + // PR #1's actual change. The first version of this test passed with 4 + // assertions (one automobile) and looked green. + EXPECT_GT(bikesSeen, 0); + + // onSideStandTrue > 0: the reversed side must have SET the flag on at least + // one bike. Without this, a candidate that dropped both writes would still + // pass -- the original would read false, the candidate would read false, + // and EXPECT_EQ would agree. That is exactly the C4/C5 defect: they passed + // the static gate while performing neither write. + EXPECT_GT(onSideStandTrue, 0); + + // origSetFlag > 0: the ORIGINAL machine code at 0x421440 sets the flag on + // at least one bike. This is the load-bearing assertion for the PR: it + // demonstrates the two writes at 0x42151B/0x42155C are real behaviour + // observed at runtime, not an inference from the disassembly that both + // sides happen to share. + EXPECT_GT(origSetFlag, 0); + + printf("[GetNewVehicleDependingOnCarModel] compared=%d bikes=%d " + "orig_set=%d rev_set=%d\n", + compared, bikesSeen, origSetFlag, onSideStandTrue); +}