feat(package): Inject capture calls at source lines with Harmony#1683
Merged
Conversation
The Harmony transpiler patcher (PR 4) needs to know whether each captured parameter/local is a value type before boxing it into the object[] array passed to Capture; boxing a reference-typed value would be incorrect. Mono.Cecil's TypeReference.IsValueType is already resolved from the ELEMENT_TYPE encoding without needing TypeReference.Resolve() (which throws for external assemblies in the Unity Editor, per the PR 2 generic ref struct fix), so it is available at zero extra cost.
Implements SourcePausePointPatcher, injecting a call to SourcePausePointCapture.Capture at a resolved instruction index via a Harmony transpiler and tracking patches per method so multiple pause points can share one method and be added/removed independently. The call site relies on Harmony/MonoMod's skip-visibility DynamicMethod to reach the internal Capture method across assembly boundaries, so no InternalsVisibleTo widening was needed beyond the three test assemblies. Value-type `this` is dereferenced and boxed explicitly (ldobj+box) since ldarg.0 yields a managed pointer for struct instance methods. Displaced branch-target labels and exception-region blocks are moved onto the new first instruction of an injected sequence so loops and try/finally regions keep executing correctly after patching. Covers five method shapes end-to-end (instance, static, struct instance, loop, try/finally) proving the resolved IL argument/local indices line up against the real compiled assemblies.
Adds coverage for the parts of SourcePausePointPatcher's ledger that weren't exercised by the single-injection acceptance tests: two pause points landing in the same method independently, re-patching an already-patched id as a documented no-op, and Unpatch restoring a method's original (uninstrumented) behavior before a later re-patch re-injects the call site.
Adds coverage for SourcePausePointPatcher.CheckPatchable's four gates: abstract methods, extern/internal-call methods (no IL body), methods with an unbound generic parameter of their own or declared inside an open generic type, and methods marked (or declared inside a type marked) [BurstCompile]. Each case is confirmed to fail before Harmony is ever invoked and to carry the manual-marker fallback hint. A test-only shadow of Unity.Burst.BurstCompileAttribute is defined locally since this project does not depend on com.unity.burst; SourcePausePointPatcher only ever compares the attribute's FullName.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Harmony-based source pause point patching, value-type metadata, structured patch results, IL capture injection, patch lifecycle management, and end-to-end tests for method shapes, control flow, and unpatchable methods. ChangesSource pause point patching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SourcePausePointPatcher
participant Harmony
participant PatchedMethod
participant UloopPausePointRegistry
participant SourcePausePointCapture
SourcePausePointPatcher->>SourcePausePointPatcher: Resolve and validate method
SourcePausePointPatcher->>Harmony: Register injection and transpiler
Harmony->>PatchedMethod: Insert capture IL
PatchedMethod->>UloopPausePointRegistry: Check armed state
PatchedMethod->>SourcePausePointCapture: Capture id, instance, parameters, and locals
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs`:
- Around line 63-80: Make the ledger and Harmony updates transactional in the
patch-registration logic around InjectionsByMethod and MethodById: snapshot the
prior dictionary entries and patch state before mutation, perform Unpatch/Patch,
and restore the previous ledger entries and Harmony patch state if either
operation throws, then rethrow the original exception. Apply the same rollback
handling to the related logic at the referenced second location, and do not
swallow unexpected exceptions.
- Around line 162-168: Update CheckPatchable to reject methods whose declaring
type is byref-like before AppendInstanceLoad can emit box, returning a dedicated
SourcePausePointPatchFailureReason and the manual-marker fallback hint; add a
fixture test covering a ref struct declaring type and asserting this failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f64efe6-0094-4fb8-b210-43f0dab8e3a4
⛔ Files ignored due to path filters (15)
Assets/Tests/Editor/SourcePausePointPatcher.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherLoopMethodFixture.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherNormalMethodFixture.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherStaticMethodFixture.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherStructInstanceMethodFixture.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherTryFinallyMethodFixture.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/UnityCLILoop.Tests.Editor.SourcePausePointPatcher.asmdefis excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/UnityCLILoop.Tests.Editor.SourcePausePointPatcher.asmdef.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointResolver/Fixtures/ReferenceTypeLocalFixture.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchFailureReason.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchInjection.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs.metais excluded by none and included by none
📒 Files selected for processing (18)
Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherLoopMethodFixture.csAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherNormalMethodFixture.csAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherStaticMethodFixture.csAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherStructInstanceMethodFixture.csAssets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherTryFinallyMethodFixture.csAssets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.csAssets/Tests/Editor/SourcePausePointResolver/Fixtures/ReferenceTypeLocalFixture.csAssets/Tests/Editor/SourcePausePointResolver/SourcePausePointResolverTests.csPackages/src/Editor/FirstPartyTools/PausePoint/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointLocalVariable.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointParameter.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchFailureReason.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchInjection.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointResolver.csPackages/src/Runtime/PausePoints/AssemblyInfo.cs
Comment on lines
+162
to
+168
| if (HasBurstCompileAttribute(method) || HasBurstCompileAttribute(method.DeclaringType)) | ||
| { | ||
| return SourcePausePointPatchResult.Failure( | ||
| SourcePausePointPatchFailureReason.UnpatchableBurstCompiled, | ||
| $"'{method}' (or its declaring type) is marked [BurstCompile] and cannot be patched.", | ||
| SourcePausePointConstants.ManualMarkerFallbackHint); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm whether byref-like patchability and coverage already exist elsewhere.
rg -n -C4 \
'\bref\s+struct\b|IsByRefLike(Attribute)?|Unpatchable.*(Ref|ByRef)' \
Packages/src/Editor/FirstPartyTools/PausePoint \
Assets/Tests/Editor/SourcePausePointPatcherRepository: hatayama/unity-cli-loop
Length of output: 3621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the patcher and any related tests/guards for byref-like or ref-struct handling.
printf '\n== SourcePausePointPatcher outline ==\n'
ast-grep outline Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs --view expanded
printf '\n== Relevant patcher lines ==\n'
nl -ba Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs | sed -n '1,260p'
printf '\n== PausePoint tests/files mentioning byref-like or ref struct ==\n'
rg -n -C3 '\bref\s+struct\b|IsByRefLike|ManualMarkerFallbackHint|UnpatchableBurstCompiled|AppendInstanceLoad|CheckPatchable' \
Packages/src/Editor/FirstPartyTools/PausePoint \
Assets/Tests/Editor/SourcePausePointPatcherRepository: hatayama/unity-cli-loop
Length of output: 1732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SourcePausePointPatcher relevant body ==\n'
sed -n '120,270p' Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs
printf '\n== Test coverage search ==\n'
rg -n -C4 '\bref\s+struct\b|IsByRefLike|ManualMarkerFallbackHint|UnpatchableBurstCompiled|AppendInstanceLoad|CheckPatchable' \
Assets/Tests/Editor/SourcePausePointPatcher \
Packages/src/Editor/FirstPartyTools/PausePointRepository: hatayama/unity-cli-loop
Length of output: 25242
Reject byref-like declaring types before emitting box. CheckPatchable still lets ref struct declaring types reach AppendInstanceLoad, which boxes value-type this and can emit invalid IL instead of the manual-marker fallback. Add a byref-like guard, a dedicated failure reason, and a fixture test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs`
around lines 162 - 168, Update CheckPatchable to reject methods whose declaring
type is byref-like before AppendInstanceLoad can emit box, returning a dedicated
SourcePausePointPatchFailureReason and the manual-marker fallback hint; add a
fixture test covering a ref struct declaring type and asserting this failure.
Fable 5's PR review flagged that the injected transpiler prologue built and boxed the parameter/local array unconditionally on every call, only relying on SourcePausePointCapture's own IsArmed check afterward. Since that check ran after the allocation-heavy IL had already executed, an inactive (not-yet-armed) pause point still paid the cost on every hit. Move the IsArmed check into the injected IL itself, ahead of the array build, with a new branch label that skips straight to the original instruction when not armed.
Fable 5's PR review flagged that TryResolveMethod trusted a resolved metadata token against whatever assembly currently matched by name, with no check that it was still the same compiled assembly the resolution was taken from. After a recompile/domain reload, a stale token could resolve to a different method than intended. Add an Mvid field to SourcePausePointResolution (populated from Cecil's ModuleDefinition.Mvid) and compare it against the loaded assembly's ModuleVersionId before resolving, failing with a new StaleAssembly reason when they diverge. Also addresses the review's should-items: switch SourcePausePointPatcher's asserts to UnityEngine.Debug.Assert to match the rest of the PausePoint files, add a contract check that local slot order lines up with the resolved slot index, give AssemblyNotLoaded its own hint text instead of reusing the manual-marker fallback message, and cover the try/finally exception-region end boundary with a new test.
If Harmony's Patch/Unpatch call throws mid-call (e.g. building invalid IL for a byref-like `this`), the in-memory ledger (InjectionsByMethod, MethodById) was already updated to claim success before the Harmony call ran. A later caller would then see a false "already patched" state and silently miss the pause point, or Unpatch would leave other ids on a method with no transpiler actually attached. Wrap both call sites in try-finally (not try-catch, per this repo's Fail Fast policy) so the ledger only commits when Harmony's call actually completes; on failure the finally block rolls the ledger back to its honest pre-call shape while letting the original exception propagate unchanged.
Boxing a ref struct's `this` is illegal IL, so patching an instance method declared on a ref struct would otherwise crash at Harmony's JIT time. Detect byref-like declaring types (same reflection-based IsByRefLikeAttribute check style as the existing Burst check) and emit a null instance load instead of ldarg.0/ldobj/box, so locals and parameters are still captured even though the instance's own fields cannot be. Add a Warning field to SourcePausePointPatchResult (via a default parameter on the existing SuccessResult, keeping this repo's no- overloads rule) so callers can surface that the instance was not captured. Add a ref-struct fixture and test covering the degraded capture path.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User Impact
enable-pause-point --file <path> --line <N>will be able to pause execution and report local variables, parameters, and instance fields at that exact line.Changes
SourcePausePointPatcher: a dedicated Harmony instance resolves a method by assembly name + metadata token, checks it is patchable, and injects a call toSourcePausePointCapture.Captureat the resolved instruction index via a transpiler. A per-method injection ledger supports multiple pause points per method, idempotent re-patching of an already-patched id, andUnpatch/UnpatchAll.internalCapturemethod across assembly boundaries because Harmony/MonoMod builds the replacement method as a skip-visibilityDynamicMethod, so noInternalsVisibleTowidening beyond the test assemblies was needed.thisis dereferenced and boxed explicitly (ldobj+box), sinceldarg.0yields a managed pointer for struct instance methods.[BurstCompile]) are rejected up front with aSourcePausePointPatchFailureReasonand a hint pointing at the existing hand-written marker fallback.IsValueTypeto the PR 2 resolver DTOs (SourcePausePointParameter/SourcePausePointLocalVariable), needed to decide whether a captured value must be boxed.Verification
dist/darwin-arm64/uloop compile— 0 errors / 0 warningsdist/darwin-arm64/uloop run-tests(newSourcePausePointPatcherTests, 14/14) — instance/static/struct/loop/try-finally injection, multi-pause-point-per-method, idempotent re-patch, unpatch-then-repatch, and all four patchability-check gatesdist/darwin-arm64/uloop run-tests(full Editor suite) — 1849 tests, 0 failures