Review: harden function override boundaries - #19
Conversation
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/tests/test_func_override_routes.py">
<violation number="1" location="runtime/tests/test_func_override_routes.py:32">
P3: The negative emitter assertion can never fail, so it gives no real guard. It searches full_function_emitter.cpp source text for `g_psx_func_override_hook(cpu, addr & 0x1FFFFFFFu)) {\n cpu->pc = cpu->gpr[31];` — a literal that never occurs: the override path is emitted as `func_override_try_dispatch(cpu, addr, cpu->gpr[31])`, and generated code is built from concatenated C++ string literals separated by `";\n out += ", so a `) {`-then-newline-`cpu->pc` run can never exist contiguously. The test therefore reports success even if the emitter regressed to overwriting non-local continuations. Check the generated output (build the emitter and assert on its produced C) instead of raw source text.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "JR $ra returns must not be treated as function tail entries") | ||
| require("func_override_try_dispatch(cpu, addr, cpu->gpr[31])" in EMITTER, | ||
| "generated dispatch must use the continuation-preserving helper") | ||
| require("g_psx_func_override_hook(cpu, addr & 0x1FFFFFFFu)) {\n" |
There was a problem hiding this comment.
P3: The negative emitter assertion can never fail, so it gives no real guard. It searches full_function_emitter.cpp source text for g_psx_func_override_hook(cpu, addr & 0x1FFFFFFFu)) {\n cpu->pc = cpu->gpr[31]; — a literal that never occurs: the override path is emitted as func_override_try_dispatch(cpu, addr, cpu->gpr[31]), and generated code is built from concatenated C++ string literals separated by ";\n out += ", so a ) {-then-newline-cpu->pc` run can never exist contiguously. The test therefore reports success even if the emitter regressed to overwriting non-local continuations. Check the generated output (build the emitter and assert on its produced C) instead of raw source text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/tests/test_func_override_routes.py, line 32:
<comment>The negative emitter assertion can never fail, so it gives no real guard. It searches full_function_emitter.cpp source text for `g_psx_func_override_hook(cpu, addr & 0x1FFFFFFFu)) {\n cpu->pc = cpu->gpr[31];` — a literal that never occurs: the override path is emitted as `func_override_try_dispatch(cpu, addr, cpu->gpr[31])`, and generated code is built from concatenated C++ string literals separated by `";\n out += ", so a `) {`-then-newline-`cpu->pc` run can never exist contiguously. The test therefore reports success even if the emitter regressed to overwriting non-local continuations. Check the generated output (build the emitter and assert on its produced C) instead of raw source text.</comment>
<file context>
@@ -0,0 +1,39 @@
+ "JR $ra returns must not be treated as function tail entries")
+ require("func_override_try_dispatch(cpu, addr, cpu->gpr[31])" in EMITTER,
+ "generated dispatch must use the continuation-preserving helper")
+ require("g_psx_func_override_hook(cpu, addr & 0x1FFFFFFFu)) {\n"
+ " cpu->pc = cpu->gpr[31];" not in EMITTER,
+ "generated dispatch must not overwrite non-local continuations")
</file context>
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 23 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/MOD_PACKAGES.md">
<violation number="1" location="docs/MOD_PACKAGES.md:336">
P2: The claim that a later successful commit clears the whole failure state is not accurate for disc patches. On a late arming error mod_runtime.cpp sets disc_enabled=false; a subsequent mod_runtime_commit clears main_applied, disc_guard_failed, and the plan but never restores disc_enabled, and mod_runtime_patch_disc_sector returns early while it is false. disc_enabled is only re-enabled by mod_runtime_enable_disc_patches, which is called once at initial boot (main.cpp:13326), so disc overlays remain disabled after a later successful commit/rematch.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| activation. Activation arms the complete set before it runs any activation | ||
| callback. An unexpected arming error removes the complete package set. | ||
| It also disables all main-memory writes, disc patches, and callbacks from the | ||
| selected plan. A later successful commit clears this failure state. Invalid |
There was a problem hiding this comment.
P2: The claim that a later successful commit clears the whole failure state is not accurate for disc patches. On a late arming error mod_runtime.cpp sets disc_enabled=false; a subsequent mod_runtime_commit clears main_applied, disc_guard_failed, and the plan but never restores disc_enabled, and mod_runtime_patch_disc_sector returns early while it is false. disc_enabled is only re-enabled by mod_runtime_enable_disc_patches, which is called once at initial boot (main.cpp:13326), so disc overlays remain disabled after a later successful commit/rematch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/MOD_PACKAGES.md, line 336:
<comment>The claim that a later successful commit clears the whole failure state is not accurate for disc patches. On a late arming error mod_runtime.cpp sets disc_enabled=false; a subsequent mod_runtime_commit clears main_applied, disc_guard_failed, and the plan but never restores disc_enabled, and mod_runtime_patch_disc_sector returns early while it is false. disc_enabled is only re-enabled by mod_runtime_enable_disc_patches, which is called once at initial boot (main.cpp:13326), so disc overlays remain disabled after a later successful commit/rematch.</comment>
<file context>
@@ -333,7 +333,8 @@ set for address collisions and capacity. A collision rejects the plan before
callback. An unexpected arming error removes the complete package set.
It also disables all main-memory writes, disc patches, and callbacks from the
-selected plan. Invalid plugin ids fail during registration.
+selected plan. A later successful commit clears this failure state. Invalid
+plugin ids fail during registration.
</file context>
1 participant
Review base
This review compares against
mstan/psxrecomp#174at6524ded0db50537bf0feb68a37f68150ae4fedf1.The review head is
ec7fccb13e9d0611faa3161dcaf489662404ac7e. It contains seven commits and changes 23 files.Purpose
This branch hardens function-override publication, lookup, reset, unload, and dynamic-code entry.
It also adds portable tests for each accepted publication route and for reset or unload behavior.
Changed files
Runtime behavior and public interfaces:
runtime/include/func_override.h: define the shared function-override interface and lifecycle operations.runtime/include/mod_packages.h: expose plug-in identifier validation.runtime/include/mod_plugins.h: expose exact code-range override registration.runtime/include/overlay_loader.h: expose shared static code-identity validation.runtime/src/debug_server.c: route debug-server publication through the shared override registry.runtime/src/dirty_ram_interp.c: check the override registry before dynamic-code interpretation.runtime/src/func_override.c: implement publication, lookup, deduplication, and removal behavior.runtime/src/mod_packages.cpp: clear package overrides during package reset.runtime/src/mod_runtime.cpp: publish and remove package and plug-in overrides safely.runtime/src/overlay_loader.c: publish overlay override pairs through the shared registry.Build and test wiring:
runtime/CMakeLists.txt: build the lifecycle and route tests.recompiler/CMakeLists.txt: build the full-function emitter test with portable paths.Source-owned tests:
runtime/tests/overlay_pair_dedup_harness.c: cover real loader publication and deduplication.runtime/tests/test_func_override.c: cover registry boundaries and lifecycle operations.runtime/tests/test_func_override_routes.py: cover five dynamic publication routes.runtime/tests/test_mod_runtime.cpp: cover package and plug-in reset behavior.runtime/tests/test_overlay_pair_dedup_runtime.py: run the loader harness portably.recompiler/tests/full_function_emitter_test.cpp: cover accepted full-function emission.recompiler/src/full_function_emitter.cpp: make the generated full-function boundary consistent with runtime lookup.Documentation:
TCP_COMMANDS.md: document the corrected debug publication command behavior.docs/MOD_PACKAGES.md: document override ownership and reset behavior.docs/TESTING.md: document the new source-owned test routes.tools/aot_overlay_spike/README.md: update the overlay publication procedure.Validation
git diff --checkpassed.python tools/gen_tcp_commands.py --checkpassed and confirmed 311 current commands.All committed fixtures are source-owned. This branch contains no game, BIOS, generated retail, or private data.
Review fixes
The first Cubic review found five issues. Commit
7c5f6294a4dece7527f187f1557d5977c7c64f32fixes all five.JandJRpaths now require a known function entry before they consult an override.The second Cubic review found two issues in that fix. Commit
15f336252dbb4c4e8410e6f25a4c2cbee16af1cefixes both.The third Cubic review found two issues in the provenance fix. Commit
2183cc99605b3e1cc254a30ed4fc28d2079100bffixes both.The fourth Cubic review found one recovery issue. Commit
345f1a1a88d29551a46e1fea6a4c828170e0d583fixes it.The automatic review of that fix found one enable-state issue. Commit
ec7fccb13e9d0611faa3161dcaf489662404ac7efixes it.Developed with AI assistance; validated as described (test evidence in PR body). AI writes the code and the PR, but I always test before I send something up. Happy to iterate on this process with your feedback.