Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions docs/CarCtrl-pr1-reversal.diff
Original file line number Diff line number Diff line change
@@ -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<CVehicle*, 0x421440, int32, uint8>(modelId, createdBy);
- /*
+ const auto createdByEnum = static_cast<eVehicleCreatedBy>(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
108 changes: 108 additions & 0 deletions docs/REPRODUCE-bonsidestand.md
Original file line number Diff line number Diff line change
@@ -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.
145 changes: 145 additions & 0 deletions docs/story-bike-flag-green.md
Original file line number Diff line number Diff line change
@@ -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.
Loading