From b12c17702aa1b60a5bdd75a4cf29ae78ccd2c622 Mon Sep 17 00:00:00 2001 From: Rich Lander Date: Thu, 18 Jun 2026 06:09:03 -0700 Subject: [PATCH 1/2] Eliminate return-accumulator temps in EH/lock by sinking returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a method's result is computed inside a try, catch, finally-guarded, or lock body and the ret sits after the region closes, the importer reconstructs a synthetic accumulator local: try { V = x + 1; } finally { ... } return V; The original source was try { return x + 1; } finally { ... }. bare, which recompiles opcode-exact for the simple shapes its DA walk can prove. It deliberately bails on leave and lock, so the two-return try/finally (TryFinallyTwoReturns) and the lock body (ClassicLock) keep diverging, and even the shapes it fixes still render the unnatural temp form rather than the source. ReturnSinkingPass rewrites the accumulator back to source form: it sinks each feeding store into a return in place — adjacent StoreLocal V; return V; pairs, and the terminal fall-through stores of a try-body, catch arm, lock body, or if/else arm a trailing return V follows. It is sound by construction: it acts only on a local that is never address-taken and whose every load is a return's operand, and applies a candidate only when every store and every load is consumed by the plan (all-or-nothing). Switch is excluded: a switch expression is lowered through its own result accumulator, so per-arm returns would diverge. CfgSampleClass compile-back: exact 93 -> 95 (TryFinallyTwoReturns, ClassicLock now exact); the EH/lock return-accumulators render try { return e; }. Compile- check arbiter unchanged across 35k+ BCL methods (no new malformed or binding errors). Gate updated: the five EH/lock shapes pinned exact; KnownDiffs trimmed to the residual docket. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../CompileBackGateTests.cs | 37 ++-- .../IrImporterTests.cs | 34 +++ .../Pipeline/Passes/IrPass.cs | 7 + .../Pipeline/Passes/ReturnSinkingPass.cs | 205 ++++++++++++++++++ 4 files changed, 268 insertions(+), 15 deletions(-) create mode 100644 src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs diff --git a/src/ILInspector.Decompiler.Tests/CompileBackGateTests.cs b/src/ILInspector.Decompiler.Tests/CompileBackGateTests.cs index 918f522b..97f6e078 100644 --- a/src/ILInspector.Decompiler.Tests/CompileBackGateTests.cs +++ b/src/ILInspector.Decompiler.Tests/CompileBackGateTests.cs @@ -19,38 +19,45 @@ public class CompileBackGateTests /// Methods that still recompile to a different opcode stream — the open /// decompiler docket. Each is a tracked defect or a benign over-render; the gate /// tolerates these but fails if a NEW method joins the set. Shrink this list as - /// fixes land. Tracked defects include StaleFieldRead (issue #605), Shadowed - /// (a dropped this.field load), and the definite-assignment default-init - /// over-render (TryFinallyAdd, PowerOfTwo, CatchEverything, SwitchCase, - /// WhileLoop, ClassicLock, TryFinallyTwoReturns, ManualDisposeAsyncInFinally). + /// fixes land. Tracked defects include StaleFieldRead (issue #605), + /// branch-polarity in the string-switch lowering (DayNumber, + /// SmallStringSwitch), and benign codegen choices (BothPositive, NeitherOr, + /// ReverseCopy, the volatile field stub in ReadVolatileFlag). /// static readonly HashSet KnownDiffs = new(StringComparer.Ordinal) { "BothPositive", - "CatchEverything", - "ClassicLock", "DayNumber", - "ManualDisposeAsyncInFinally", "NeitherOr", - "PowerOfTwo", "ReadVolatileFlag", "ReverseCopy", "SmallStringSwitch", "StaleFieldRead", - "SwitchCase", - "TryFinallyAdd", - "TryFinallyTwoReturns", - "WhileLoop", }; /// /// Methods a prior compile-back fix turned opcode-exact. Pinning them guards the /// fix durably: CheckedAdd must keep the overflow check (#604), UnsignedShift /// must keep dropping the redundant width mask (#606), Shadowed must keep - /// qualifying the shadowed this.field load (#607), and .ctor must keep lifting - /// its field initializer ahead of the base call to the field declaration. + /// qualifying the shadowed this.field load (#607), .ctor must keep lifting + /// its field initializer ahead of the base call to the field declaration + /// (#614), and the return-accumulator elimination must keep sinking the + /// result temp out of an EH region or lock (TryFinallyAdd, + /// TryFinallyTwoReturns, CatchEverything, ClassicLock, + /// ManualDisposeAsyncInFinally). /// - static readonly string[] PinnedExact = { "CheckedAdd", "UnsignedShift", "Shadowed", ".ctor" }; + static readonly string[] PinnedExact = + { + "CheckedAdd", + "UnsignedShift", + "Shadowed", + ".ctor", + "TryFinallyAdd", + "TryFinallyTwoReturns", + "CatchEverything", + "ClassicLock", + "ManualDisposeAsyncInFinally", + }; static IReadOnlyList EvaluateFixtures() { diff --git a/src/ILInspector.Decompiler.Tests/IrImporterTests.cs b/src/ILInspector.Decompiler.Tests/IrImporterTests.cs index fcc90c08..22b8f7af 100644 --- a/src/ILInspector.Decompiler.Tests/IrImporterTests.cs +++ b/src/ILInspector.Decompiler.Tests/IrImporterTests.cs @@ -311,6 +311,40 @@ public void Shadowed_QualifiesFieldLoadWhenParameterShadows() Assert.Contains("this._shadowed", CSharpPrinter.PrintRaised(function).Output!); } + [Fact] + public void TryFinallyAdd_SinksReturnAccumulatorIntoTry() + { + // The result is computed inside the try and the ret sits after the + // finally, so the importer reconstructs a synthetic accumulator local. + // Sinking it back to `return x + 1;` inside the try keeps Roslyn from + // re-zero-initing the temp (a dead leading ldc.i4.0/stloc the original + // never emitted) — there must be no accumulator local in the output. + var function = ImportFixture(nameof(CfgSampleClass.TryFinallyAdd)); + IrPasses.Run(function); + + string output = CSharpPrinter.PrintRaised(function).Output!; + Assert.Equal(DecompilationFidelity.Full, function.Fidelity); + Assert.Contains("return x + 1;", output); + Assert.DoesNotContain("V_0", output); + } + + [Fact] + public void TryFinallyTwoReturns_SinksBothReturnsIntoTry() + { + // Both the guarded `return x` and the fall-through `return -1` are + // spilled into one accumulator and returned after the finally. The + // pass must sink both back into the try as distinct returns, leaving no + // accumulator local behind. + var function = ImportFixture(nameof(CfgSampleClass.TryFinallyTwoReturns)); + IrPasses.Run(function); + + string output = CSharpPrinter.PrintRaised(function).Output!; + Assert.Equal(DecompilationFidelity.Full, function.Fidelity); + Assert.Contains("return x;", output); + Assert.Contains("return -1;", output); + Assert.DoesNotContain("V_0", output); + } + [Fact] public void InParameter_SeesThroughModifier_ImportsAtFull() { diff --git a/src/ILInspector.Decompiler/Pipeline/Passes/IrPass.cs b/src/ILInspector.Decompiler/Pipeline/Passes/IrPass.cs index 2ee9b6df..8afcd8ea 100644 --- a/src/ILInspector.Decompiler/Pipeline/Passes/IrPass.cs +++ b/src/ILInspector.Decompiler/Pipeline/Passes/IrPass.cs @@ -81,6 +81,13 @@ public static class IrPasses // printer lifts it to a signature initializer rather than an invalid // base(temp); body call (CS0175). new ConstructorChainArgumentPass(), + // Eliminate return-accumulator temporaries the compiler spilled across + // an EH region or lock (try { V = e; } finally { } return V; back to + // try { return e; }). Runs after structuring and the second inlining so + // the try/catch/finally/lock shells and their tail stores are fully + // formed; expression inlining leaves these temps standing because it + // refuses to move a value across a leave/region edge. + new ReturnSinkingPass(), // Raise any static function-pointer load still standing into &Method // (it feeds a calli, native callback, or delegate*-typed field — all of // which take a function pointer directly). diff --git a/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs b/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs new file mode 100644 index 00000000..b8bb2572 --- /dev/null +++ b/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs @@ -0,0 +1,205 @@ +namespace ILInspector.Decompiler.Pipeline; + +/// +/// Eliminates a return-accumulator temporary — the inverse of the compiler's +/// habit of spilling a method's result into a synthetic local when the value +/// is produced inside an exception region or a lock and the +/// ret sits after the region closes. The decompiler reconstructs +/// try { V = x + 1; } finally { ... } return V;; the original source was +/// try { return x + 1; } finally { ... }. Recompiling the temp form makes +/// Roslyn re-zero-init the synthesized local, so the recompiled IL gains a dead +/// leading ldc.i4.0; stloc the original never had. +/// +/// The pass is sound by construction: it acts only on a local that is never +/// address-taken and whose every load is the direct operand of a +/// , and it applies a candidate only when every store and +/// every load is consumed by the plan (all-or-nothing). A store feeding a +/// return is rewritten into a in place — adjacent +/// StoreLocal V; return V; pairs, and the terminal fall-through stores of +/// a structured statement (try/finally try body, try/catch arms, lock body, +/// if/else arms) the trailing return V; follows. Switch is deliberately +/// excluded: a switch expression is lowered through its own result +/// accumulator, so reproducing per-arm returns would diverge from the original. +/// +public sealed class ReturnSinkingPass : IIrPass +{ + public string Name => "return-sinking"; + + public void Run(IrFunction function) + { + while (SinkOnce(function)) + { + } + } + + static bool SinkOnce(IrFunction function) + { + var stores = new Dictionary>(); + var returnLoads = new Dictionary>(); + var addressTaken = new HashSet(); + var escapingLoad = new HashSet(); + + foreach (var node in function.Descendants) + { + switch (node) + { + case StoreLocal store: + (stores.TryGetValue(store.Index, out var list) ? list : stores[store.Index] = []).Add(store); + break; + case LoadLocalAddress address: + addressTaken.Add(address.Index); + break; + case LoadLocal load when load.Parent is Return ret && ReferenceEquals(ret.Value, load): + (returnLoads.TryGetValue(load.Index, out var rl) ? rl : returnLoads[load.Index] = []).Add(ret); + break; + case LoadLocal load: + // A load that is not a return's operand: the local carries + // its value somewhere else, so it is not a pure return + // accumulator and must be left alone. + escapingLoad.Add(load.Index); + break; + } + } + + foreach (var (index, indexStores) in stores) + { + if (addressTaken.Contains(index) || escapingLoad.Contains(index)) + continue; + if (!returnLoads.TryGetValue(index, out var returns) || returns.Count == 0) + continue; + if (TryPlan(index, indexStores, returns) is { } plan) + { + Apply(plan); + return true; + } + } + return false; + } + + sealed record Plan(List<(StoreLocal Store, Break? Break)> Folds, List Returns); + + static Plan? TryPlan(int index, List indexStores, List returns) + { + var folds = new List<(StoreLocal, Break?)>(); + var consumed = new HashSet(ReferenceEqualityComparer.Instance); + + foreach (var ret in returns) + { + if (ret.Parent is not Block block || ret.ChildIndex == 0) + return null; + var prev = block.Children[ret.ChildIndex - 1]; + + if (prev is StoreLocal adjacent && adjacent.Index == index) + { + if (!consumed.Add(adjacent)) + return null; + folds.Add((adjacent, null)); + continue; + } + + var tails = CollectStmtTails(prev, index); + if (tails is null) + return null; + foreach (var tail in tails) + { + if (!consumed.Add(tail.Store)) + return null; + folds.Add(tail); + } + } + + // Soundness: refuse unless the plan consumes every store of the local. + // Every load is already a return we delete, so this leaves no orphan + // read of an undefined accumulator and no store that should have been + // a return but was missed. + if (consumed.Count != indexStores.Count) + return null; + + return new Plan(folds, returns); + } + + static void Apply(Plan plan) + { + foreach (var (store, brk) in plan.Folds) + { + var value = (IrExpression)store.DetachChildren()[0]; + brk?.Detach(); + store.ReplaceWith(new Return(value)); + } + foreach (var ret in plan.Returns) + ret.Detach(); + } + + /// + /// The terminal fall-through stores of a trailing + /// return index consumes when it follows the structured statement, or + /// null when the statement is not a clean accumulator sink (every reachable + /// fall-through tail must be a StoreLocal index). + /// + static List<(StoreLocal Store, Break? Break)>? CollectStmtTails(IrNode statement, int index) + { + switch (statement) + { + case TryFinally tryFinally: + // The finally cannot produce the returned value; only the try + // body's fall-through tail feeds the trailing return. + return CollectContainerTail(tryFinally.TryBody, index); + case Lock @lock: + return CollectContainerTail(@lock.Body, index); + case TryCatch tryCatch: + { + var result = CollectContainerTail(tryCatch.TryBody, index); + if (result is null) + return null; + foreach (var clause in tryCatch.Clauses) + { + var clauseTail = CollectContainerTail(clause.Body, index); + if (clauseTail is null) + return null; + result.AddRange(clauseTail); + } + return result; + } + case Switch: + // Do not sink into a switch: csc lowers a switch *expression* + // through its own result accumulator (each arm stores, the + // epilogue loads and returns), so reproducing per-arm returns + // diverges from the original stream rather than matching it. + return null; + case IfStatement { HasElse: true } ifStatement: + { + var thenTail = CollectBlockTail(ifStatement.Then, index); + var elseTail = CollectBlockTail(ifStatement.Else!, index); + if (thenTail is null || elseTail is null) + return null; + thenTail.AddRange(elseTail); + return thenTail; + } + default: + return null; + } + } + + static List<(StoreLocal Store, Break? Break)>? CollectContainerTail(BlockContainer container, int index) + { + var blocks = container.Blocks; + return blocks.Count == 0 ? null : CollectBlockTail(blocks[^1], index); + } + + static List<(StoreLocal Store, Break? Break)>? CollectBlockTail(Block block, int index) + { + if (block.Children.Count == 0) + return null; + var last = block.Children[^1]; + if (last is StoreLocal store && store.Index == index) + return [(store, null)]; + if (last is Break brk + && block.Children.Count >= 2 + && block.Children[^2] is StoreLocal stored + && stored.Index == index) + { + return [(stored, brk)]; + } + return CollectStmtTails(last, index); + } +} From b25f9b0ed57dde8ba9bc3b1f5d51ed8bc1b70f40 Mon Sep 17 00:00:00 2001 From: Rich Lander Date: Thu, 18 Jun 2026 06:15:38 -0700 Subject: [PATCH 2/2] Document the #611 division of labor in ReturnSinkingPass header Cross-reference the printer's definite-assignment dead-init drop (#611) as the conservative fallback for accumulators this pass leaves standing, so the two-layer division of labor is discoverable from the code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Pipeline/Passes/ReturnSinkingPass.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs b/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs index b8bb2572..33279e45 100644 --- a/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs +++ b/src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs @@ -20,6 +20,15 @@ namespace ILInspector.Decompiler.Pipeline; /// if/else arms) the trailing return V; follows. Switch is deliberately /// excluded: a switch expression is lowered through its own result /// accumulator, so reproducing per-arm returns would diverge from the original. +/// +/// This is the structural raiser for the pure-return-accumulator subset; the +/// printer's definite-assignment dead-init drop (CSharpPrinter, PR #611) is the +/// conservative fallback for every accumulator this pass deliberately leaves +/// standing — switch-expression results, multi-use locals (a value used past +/// its return), and control flow this pass does not model. Those keep their +/// temp but lose the redundant = default; only here does the temp vanish +/// entirely. The two compose without overlap: a local this pass sinks has no +/// stores or loads left, so the printer never emits a declaration for it. /// public sealed class ReturnSinkingPass : IIrPass {