Walk the whole decompiler pipeline in the Debug Steps pane - #4029
Walk the whole decompiler pipeline in the Debug Steps pane#4029siegfriedpammer wants to merge 1 commit into
Conversation
The pane used to split the pipeline across two languages: the ILAst language stepped the IL transforms, the C# language stepped the AST transforms, and nothing showed the seam between them, so a step index meant a different thing depending on which language happened to be selected. Recording both halves into one Stepper makes an index replayable across the whole pipeline; a limit that lands in the IL phase has no C# to print, so the halted function is rendered as ILAst instead. Retention stays opt-in because every kept step pins the ILAst it captured - affordable for the single type the pane shows, not for a whole-module decompile. What was left of the ILAst language is its typed-IL dump, which runs no transforms at all; it stays as TypedILLanguage, and IDebugStepProvider was down to one implementation. Assisted-by: Claude:claude-opus-5:Claude Code
christophwille
left a comment
christophwille
left a comment
There was a problem hiding this comment.
Review summary (recall-biased pass, findings verified individually).
The core idea is sound: one Stepper across the IL and AST halves keeps step indices replayable, and the halted-IL rendering path (StepLimitHaltedFunction / TryWriteILAst) is wired consistently. Step numbering is identical between the full run and the step-limited replay as far as I can trace. Findings, most severe first (inline comments on the respective lines):
- Bug -
Show state afteron a non-last member'sConvert ILAst to C#step or member group halts at the next member'sStepStartGroup(method.FullName)and renders that member's raw, untransformed IL with no highlight (CSharpDecompiler.cs:2396). - Debug-build regression -
RecordILTransformSteps = trueon every UI C# decompile retains every IL step of every member (each Node pins ILAst incl. removed instructions) on the sharedCSharpLanguage.stepper, andStepNodeViewModel.Wrapeagerly builds a VM per node on the UI thread even with the pane hidden (CSharpLanguage.cs:333). - Lost capability - the deleted ILAst language's
ILAst after the crashview is unreachable via the pane: replaying the crashed group'sstate afterre-throws before any step reaches the limit (CSharpDecompiler.cs:2435). - Test -
AFailingTransformDoesNotNestLaterMembersUnderItasserts onDecompileProject, which is declared beforeCleanUpFileNamein metadata order, so the sibling assertion passes with or withoutEndOpenGroups(DebugStepRecordingTests.cs:145). - Pre-existing limitation now advertised as replayable - a step limit hit in a detached helper function (ProxyCallReplacer's proxyFunction, nested functions before attachment) attributes the halt to the top-level function, whose ILAst does not contain the halted instruction (CSharpDecompiler.cs:2424).
- Reuse - the per-transform loop duplicates
ILFunction.RunTransformsnaming/invariant logic (CSharpDecompiler.cs:2368). - Simplification -
x:CompileBindings="False"+Optionsforwarder + runtime binding test could be{Binding Options.UseFieldSugar}etc. with compiled bindings (DebugSteps.axaml:22). - Test duplication -
StripStepNumbertwice in DebugStepsTests.cs;CreateDecompiler/ThrowingILTransformcopied from DecompilationErrorRecoveryTests.cs. - Post-merge stale docs (not in this diff) - master's CLAUDE.md:94 (
like the UI's ILAst language), ICSharpCode.ILSpyCmd/ILAstDumper.cs:40/47 and IlspyCmdProgram.cs:122 still point at the retired ILAst language; worth rewording on rebase. - Conventions - comments describing the change relative to the previous version (
which the C# path used to discard,used to live on a separate ILAst language), andbehaviour(en-US per CLAUDE.md) in DebugStepRecordingTests.cs.
Minor, not commented inline: Should().Equal(astTransformNames) weakened to EndWith, so no test pins the exact top-level tree shape; the ref bool handled partial in WriteCode could be an inline StepLimitHaltedFunction is { } halted branch; EndOpenGroups() popping to zero relies on nothing ever opening a group around DecompileBody (a depth-restoring close or a finally would be more robust).
Review by an AI agent (Claude) on Christoph's behalf.
| // The seam between the two halves of the pipeline. Besides marking where the IL steps | ||
| // end and the C# AST steps begin, it is the only handle on the fully transformed | ||
| // ILAst: every IL step shows the state before some transform, never after the last. | ||
| context.Step("Convert ILAst to C#", function.Body); |
There was a problem hiding this comment.
Bug: Show state after on this seam step (or on the member group) of a non-last member shows the wrong member.
This Step records index N and increments to N+1; StepEndGroup then sets the member group's EndStep = N+1. The pane replays state after with StepLimit = EndStep = N+1, and the next Step call in the replay is the next member's context.StepStartGroup(method.FullName) (after its ReadIL), which throws. StepLimitHaltedFunction becomes the next member's raw, untransformed ILFunction, TryWriteILAst replaces the whole document with it, and HighlightStep = N (this member's function.Body) is not in the rendered NodeLookup, so there is no highlight either. Only for the last member of the type does after land in the AST phase and print C#.
A cheap fix: treat a halt that lands exactly on a member's opening group step as state after the previous member (e.g. remember the previous member's transformed function before opening the next group and render that when the halted step is a group opener).
| // The Debug Steps pane walks the whole pipeline, so the IL transforms have to record into | ||
| // the same stepper as the C# AST transforms. Inert unless the decompiler was built with the | ||
| // STEP symbol, which only its debug builds define. | ||
| decompiler.RecordILTransformSteps = true; |
There was a problem hiding this comment.
Debug-build regression: every C# decompile now retains all IL steps and eagerly wraps them on the UI thread, whether or not the pane is open.
RecordILTransformSteps = true is unconditional, OnCSharpDecompiled keeps decompiler.Stepper on the MEF-shared CSharpLanguage.stepper until the next full decompile, and DebugStepsPaneModel (materialised by ToolPaneRegistry regardless of IsVisibleByDefault=false) reacts to StepperUpdated with Dispatcher.UIThread.Post(() => SetStepsSource(...)) -> StepNodeViewModel.Wrap, which recursively allocates one VM per node. Pre-PR the IL steps went to a throwaway per-member stepper and only the (small) AST steps were retained/wrapped. For a large type (e.g. System.Linq.Enumerable incl. nested iterators) that is tens of thousands of Stepper.Nodes (each with three lists + ancestor chain, pinning removed ILAst subtrees) plus as many VMs, on every selection in Debug builds.
Cheaper: opt in only while the Debug Steps pane is visible (flip a flag from the pane's show/hide and re-decompile on open), and/or drop stepper on DetachFromLanguage, and wrap children lazily on expansion.
| errors.Add(innerException as DecompilerException ?? new DecompilerException(module, method, innerException)); | ||
| // The unwind left this member's step groups open; close them so the members after it are | ||
| // recorded as its siblings instead of disappearing into the group that failed. | ||
| Stepper.EndOpenGroups(); |
There was a problem hiding this comment.
Lost debugging capability: the ILAst at the point a transform crashed is no longer reachable.
The deleted ILAst language printed the exception plus ILAst after the crash: + the half-transformed function. Here the crash becomes a C# error comment and EndOpenGroups() sets the group's EndStep to the counter at unwind. Replaying that group's state after (StepLimit = EndStep) re-runs the transform, which throws again before any Step reaches the limit, so StepLimitHaltedFunction stays null for that member; the next member's opening group step then hits the limit and its raw IL is shown instead (or, for the last member, the C# error comment again). Only state before the crashing transform is reachable.
If this is intended, fine, but consider recording the crashed function (e.g. set StepLimitHaltedFunction = function here too when a step limit is set, or expose a CrashedFunction the UI can render) so a developer debugging a throwing transform can still see what it had already mutated.
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(crashed.EndStep, Is.GreaterThan(crashed.BeginStep + 1), "the abandoned group was closed at the step it stopped on"); | ||
| Assert.That(decompiler.Stepper.Steps, Has.Some.Matches<Stepper.Node>(n => n.Description.Contains("DecompileProject")), |
There was a problem hiding this comment.
Vacuous assertion. WholeProjectDecompiler is a single non-partial class and members are decompiled in metadata (= source) order; both DecompileProject overloads (~L230/239) precede CleanUpFileName (~L843), so their groups are top-level whether or not Stepper.EndOpenGroups() runs. Removing the EndOpenGroups() call from the general catch keeps this assertion green; only the EndStep assertion above detects the fix. Assert on a member declared after the crash, e.g. CleanUpDirectoryName or CleanUpPath.
| // rather than the C# it would otherwise print. This clause has to stay above the general | ||
| // handler below, which would turn the halt into an error comment and report the member as | ||
| // a decompilation failure. | ||
| StepLimitHaltedFunction = function; |
There was a problem hiding this comment.
Note (pre-existing in the ILAst language, but now advertised as a whole-pipeline replay): when the limit is hit inside transforms run on a detached helper function that shares this Stepper (ProxyCallReplacer's proxyFunction, LocalFunctionDecompiler/DelegateConstruction nested functions before they are attached), function is the enclosing top-level ILFunction, whose WriteTo never prints the halted instruction, so TryResolve finds no candidate/seam/ancestor and the view shows a different tree with no highlight. Recording the halted function at the exception site (e.g. context.Function carried in a StepLimitReachedException field) would let the UI render the tree the step actually mutated.
| CancellationToken.ThrowIfCancellationRequested(); | ||
| // Named the way ILFunction.RunTransforms names them, so the same transform reads the | ||
| // same in every step tree. | ||
| context.StepStartGroup(transform is BlockILTransform blockTransform |
There was a problem hiding this comment.
Reuse: this loop body (group naming BlockILTransform ? ToString() : GetType().Name, Run, CheckInvariant, StepEndGroup(keepIfEmpty: true)) is a copy of ILFunction.RunTransforms (ILFunction.cs ~L402), kept in sync only by the comment. RunTransforms also emits DecompilerEventSource.ILTransformExecuted tracing that this loop lacks (pre-existing gap, but now the two loops share the group contract). Consider function.RunTransforms(localSettings.DecompileMemberBodies ? ilTransforms : ilTransforms.TakeWhile(...up to and including AsyncAwaitDecompiler), context), or at least a shared StepStartGroup(IILTransform) helper used by both.
| </DataTemplate> | ||
| </ContentControl.DataTemplates> | ||
| </ContentControl> | ||
| <StackPanel Orientation="Horizontal" DataContext="{Binding Options}" x:CompileBindings="False"> |
There was a problem hiding this comment.
Simplification: Options is now statically typed (ILAstWritingOptions, INotifyPropertyChanged), so the four checkboxes can stay on compiled bindings from the pane's existing x:DataType: IsChecked="{Binding Options.UseFieldSugar, Mode=TwoWay}" etc. (or x:DataType="il:ILAstWritingOptions" on this StackPanel). That drops x:CompileBindings="False", the DataContext swap, and the need for the runtime Writing_Option_CheckBoxes_Are_Bound_To_The_ILAst_Writing_Options test that only exists to catch renames the compiler would otherwise catch.
| tab.Text.Should().NotBeNullOrWhiteSpace("replaying the state after an IL-phase step must still emit output"); | ||
| AssertPreciseHighlight(tab, "an IL-phase replay must locate the changed instruction"); | ||
|
|
||
| static string StripStepNumber(string description) |
There was a problem hiding this comment.
Duplicate of the identical StripStepNumber local function at L187; hoist one to a fixture-level static next to AssertPreciseHighlight. Similarly, DebugStepRecordingTests.CreateDecompiler and ThrowingILTransform are byte-for-byte copies of the helpers in DecompilationErrorRecoveryTests.cs (~L128-143) - make those internal and reuse them.
| /// <summary> | ||
| /// The Debug Steps view replays a decompilation by index, so a single <see cref="Stepper"/> has to | ||
| /// span the whole pipeline: the IL transforms, the ILAst-to-C# conversion, and the C# AST | ||
| /// transforms. These tests pin the IL half, which the C# path used to discard. |
There was a problem hiding this comment.
CLAUDE.md: Comments must stand on their own, with no memory of how the code was written ... never reference the previous version. which the C# path used to discard (and used to live on a separate ILAst language ... now belongs to C# in DebugStepsTests.cs ~L277) describe the change rather than the code; reword to what the C# path does now. Also behaviour (L113) -> behavior (en-US per CLAUDE.md).
2 participants
The pane used to split the pipeline across two languages: the ILAst language stepped the IL
transforms, the C# language stepped the AST transforms, and nothing showed the seam between
them. A step index therefore meant a different thing depending on which language happened to be
selected.
Recording both halves into one
Steppermakes an index replayable across the whole pipeline. Alimit that lands in the IL phase has no C# to print, so the halted function is rendered as ILAst
instead.
Retention stays opt-in: every kept step pins the ILAst it captured, which is affordable for the
single type the pane shows but not for a whole-module decompile.
What is left of the ILAst language is its typed-IL dump, which runs no transforms at all. That
stays, as
TypedILLanguage.IDebugStepProviderwas down to a single implementation and isremoved.
Verification
Merged with current master locally: the merge is clean, with one overlapping file
(
ILSpy/ViewModels/DebugStepsPaneModel.cs, which master also changed in 27f5e2b). The mergedresult builds
ILSpy.Desktop.slnfand the fullILSpy.Testssuite passes - 1211 passed, 3skipped, 0 failed - including the 20 Debug Steps tests.
Written by an AI agent (Claude) on Siegfried's behalf.