From 6757295b3b10ba900e4ec5ff7c5ab01a01f88a04 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Wed, 2 Jun 2021 22:40:28 +0200 Subject: [PATCH 1/5] Fix #2379: Keep `return` statements around in original form for ConditionDetection, only transform to fall-through block-exit at the end of the transform pipeline. This fixes an issue where `return` statements within try-blocks could turn into `goto` statements. --- .../TestCases/Pretty/ExceptionHandling.cs | 24 ++- .../TestCases/Pretty/ReduceNesting.cs | 27 ++- .../CSharp/CSharpDecompiler.cs | 3 +- .../ICSharpCode.Decompiler.csproj | 1 + .../IL/ControlFlow/ConditionDetection.cs | 2 - .../IL/ControlFlow/ExitPoints.cs | 5 +- .../IL/ControlFlow/RemoveRedundantReturn.cs | 185 ++++++++++++++++++ .../IL/Instructions/InstructionCollection.cs | 16 +- .../IL/Instructions/Leave.cs | 6 +- .../IL/Transforms/HighLevelLoopTransform.cs | 26 ++- .../IL/Transforms/ReduceNestingTransform.cs | 10 +- ILSpy/LoadedAssembly.cs | 2 +- 12 files changed, 265 insertions(+), 42 deletions(-) create mode 100644 ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExceptionHandling.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExceptionHandling.cs index e4ae2c2eb..a8165c833 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExceptionHandling.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExceptionHandling.cs @@ -246,6 +246,28 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } + internal void EarlyReturnInTryBlock(bool a, bool b) + { + try + { + if (a) + { + Console.WriteLine("a"); + } + else if (b) + { + // #2379: The only goto-free way of representing this code is to use a return statement + return; + } + + Console.WriteLine("a || !b"); + } + finally + { + Console.WriteLine("finally"); + } + } + #if ROSLYN || !OPT // TODO Non-Roslyn compilers create a second while loop inside the try, by inverting the if // This is fixed in the non-optimised version by the enabling the RemoveDeadCode flag @@ -476,4 +498,4 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } #endif } -} \ No newline at end of file +} diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs index f08a49b8a..8bbc09ee4 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs @@ -198,17 +198,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty case 1: Console.WriteLine("case 1"); return; + default: + if (B(1)) + { + Console.WriteLine(1); + } + return; } - - if (B(1)) - { - Console.WriteLine(1); - } - } - else - { - Console.WriteLine("else"); } + Console.WriteLine("else"); } // nesting should not be reduced as maximum nesting level is 1 @@ -348,12 +346,13 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty Console.WriteLine(); - if (B(1)) + if (!B(1)) { - for (int i = 0; i < 10; i++) - { - Console.WriteLine(i); - } + return; + } + for (int i = 0; i < 10; i++) + { + Console.WriteLine(i); } } catch diff --git a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs index 6a00c714d..6dc218393 100644 --- a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs +++ b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs @@ -117,7 +117,7 @@ namespace ICSharpCode.Decompiler.CSharp } }, // re-run DetectExitPoints after loop detection - new DetectExitPoints(canIntroduceExitForReturn: true), + new DetectExitPoints(canIntroduceExitForReturn: false), new BlockILTransform { // per-block transforms PostOrderTransforms = { new ConditionDetection(), @@ -159,6 +159,7 @@ namespace ICSharpCode.Decompiler.CSharp new TransformDisplayClassUsage(), new HighLevelLoopTransform(), new ReduceNestingTransform(), + new RemoveRedundantReturn(), new IntroduceDynamicTypeOnLocals(), new IntroduceNativeIntTypeOnLocals(), new AssignVariableNames(), diff --git a/ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj b/ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj index 80c9977ca..8d7057411 100644 --- a/ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj +++ b/ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj @@ -79,6 +79,7 @@ + diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs b/ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs index 764130eaa..474e3031d 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs @@ -16,7 +16,6 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; @@ -366,7 +365,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow Debug.Assert(ifInst.Parent == block); //assert then block terminates - var trueExitInst = GetExit(ifInst.TrueInst); var exitInst = GetExit(block); context.Step($"InvertIf at IL_{ifInst.StartILOffset:x4}", ifInst); diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs index fd76f3a27..4b4232047 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs @@ -16,7 +16,6 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -using System; using System.Collections.Generic; using System.Diagnostics; using System.Threading; @@ -76,7 +75,9 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow else if (slot == TryInstruction.TryBlockSlot || slot == TryCatchHandler.BodySlot || slot == TryCatch.HandlerSlot - || slot == PinnedRegion.BodySlot) + || slot == PinnedRegion.BodySlot + || slot == UsingInstruction.BodySlot + || slot == LockInstruction.BodySlot) { return GetExit(inst.Parent); } diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs b/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs new file mode 100644 index 000000000..4759b9a62 --- /dev/null +++ b/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs @@ -0,0 +1,185 @@ +// Copyright (c) Daniel Grunwald +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +#nullable enable + +using System.Diagnostics; +using System.Linq; + +using ICSharpCode.Decompiler.IL.Transforms; + +namespace ICSharpCode.Decompiler.IL.ControlFlow +{ + /// + /// Similar to , but acts only on leave instructions + /// leaving the whole function (return/yield break) that can be made implicit + /// without using goto. + /// + class RemoveRedundantReturn : IILTransform + { + public void Run(ILFunction function, ILTransformContext context) + { + foreach (var lambda in function.Descendants.OfType()) + { + if (lambda.Body is BlockContainer c && ((lambda.AsyncReturnType ?? lambda.ReturnType).Kind == TypeSystem.TypeKind.Void || lambda.IsIterator)) + { + Block lastBlock = c.Blocks.Last(); + if (lastBlock.Instructions.Last() is Leave { IsLeavingFunction: true }) + { + ConvertReturnToFallthrough(lastBlock.Instructions.SecondToLastOrDefault()); + } + else + { + if (ConvertReturnToFallthrough(lastBlock.Instructions.Last())) + { + lastBlock.Instructions.Add(new Leave(c)); + } + } + } + } + } + + private static bool ConvertReturnToFallthrough(ILInstruction? inst) + { + bool result = false; + switch (inst) + { + case BlockContainer c: + if (c.Kind != ContainerKind.Normal) + { + // loop or switch: turn all "return" into "break" + result |= ReturnToLeaveInContainer(c); + } + else + { + // body of try block, or similar: recurse into last instruction in container + Block lastBlock = c.Blocks.Last(); + if (lastBlock.Instructions.Last() is Leave { IsLeavingFunction: true, Value: Nop } leave) + { + leave.TargetContainer = c; + result = true; + } + else if (ConvertReturnToFallthrough(lastBlock.Instructions.Last())) + { + lastBlock.Instructions.Add(new Leave(c)); + result = true; + } + } + break; + case TryCatch tryCatch: + result |= ConvertReturnToFallthrough(tryCatch.TryBlock); + foreach (var h in tryCatch.Handlers) + { + result |= ConvertReturnToFallthrough(h.Body); + } + break; + case TryFinally tryFinally: + result |= ConvertReturnToFallthrough(tryFinally.TryBlock); + break; + case LockInstruction lockInst: + result |= ConvertReturnToFallthrough(lockInst.Body); + break; + case UsingInstruction usingInst: + result |= ConvertReturnToFallthrough(usingInst.Body); + break; + case PinnedRegion pinnedRegion: + result |= ConvertReturnToFallthrough(pinnedRegion.Body); + break; + case IfInstruction ifInstruction: + result |= ConvertReturnToFallthrough(ifInstruction.TrueInst); + result |= ConvertReturnToFallthrough(ifInstruction.FalseInst); + break; + case Block block when block.Kind == BlockKind.ControlFlow: + { + var lastInst = block.Instructions.LastOrDefault(); + if (lastInst is Leave { IsLeavingFunction: true, Value: Nop }) + { + block.Instructions.RemoveAt(block.Instructions.Count - 1); + result = true; + lastInst = block.Instructions.LastOrDefault(); + } + result |= ConvertReturnToFallthrough(lastInst); + break; + } + } + return result; + } + + /// + /// Transforms + /// loop { ... if (x) return; .. } + /// to + /// loop { ... if (x) break; .. } return; + /// + internal static void ReturnToBreak(Block parentBlock, BlockContainer loopOrSwitch, ILTransformContext context) + { + Debug.Assert(loopOrSwitch.Parent == parentBlock); + // This transform is only possible when the loop/switch doesn't already use "break;" + if (loopOrSwitch.LeaveCount != 0) + return; + // loopOrSwitch with LeaveCount==0 has unreachable exit point and thus must be last in block. + Debug.Assert(parentBlock.Instructions.Last() == loopOrSwitch); + var nearestFunction = parentBlock.Ancestors.OfType().First(); + if (nearestFunction.Body is BlockContainer functionContainer) + { + context.Step("pull return out of loop/switch: " + loopOrSwitch.EntryPoint.Label, loopOrSwitch); + if (ReturnToLeaveInContainer(loopOrSwitch)) + { + // insert a return after the loop + parentBlock.Instructions.Add(new Leave(functionContainer)); + } + } + } + + private static bool ReturnToLeaveInContainer(BlockContainer c) + { + bool result = false; + foreach (var block in c.Blocks) + { + result |= ReturnToLeave(block, c); + } + return result; + } + + private static bool ReturnToLeave(ILInstruction inst, BlockContainer c) + { + if (inst is Leave { IsLeavingFunction: true, Value: Nop } leave) + { + leave.TargetContainer = c; + return true; + } + else if (inst is BlockContainer nested && nested.Kind != ContainerKind.Normal) + { + return false; + } + else if (inst is ILFunction) + { + return false; + } + else + { + bool b = false; + foreach (var child in inst.Children) + { + b |= ReturnToLeave(child, c); + } + return b; + } + } + } +} diff --git a/ICSharpCode.Decompiler/IL/Instructions/InstructionCollection.cs b/ICSharpCode.Decompiler/IL/Instructions/InstructionCollection.cs index 8301c66cb..55c968477 100644 --- a/ICSharpCode.Decompiler/IL/Instructions/InstructionCollection.cs +++ b/ICSharpCode.Decompiler/IL/Instructions/InstructionCollection.cs @@ -16,6 +16,8 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. +#nullable enable + using System; using System.Collections.Generic; using System.Diagnostics; @@ -73,7 +75,7 @@ namespace ICSharpCode.Decompiler.IL public struct Enumerator : IEnumerator { #if DEBUG - ILInstruction parentInstruction; + ILInstruction? parentInstruction; #endif readonly List list; int pos; @@ -140,7 +142,7 @@ namespace ICSharpCode.Decompiler.IL /// Runs in O(1) if the item can be found using the Parent/ChildIndex properties. /// Otherwise, runs in O(N). /// - public int IndexOf(T item) + public int IndexOf(T? item) { if (item == null) { @@ -163,7 +165,7 @@ namespace ICSharpCode.Decompiler.IL /// This method searches the list. /// Usually it's more efficient to test item.Parent instead! /// - public bool Contains(T item) + public bool Contains(T? item) { return IndexOf(item) >= 0; } @@ -395,7 +397,7 @@ namespace ICSharpCode.Decompiler.IL return list[0]; } - public T FirstOrDefault() + public T? FirstOrDefault() { return list.Count > 0 ? list[0] : null; } @@ -405,17 +407,17 @@ namespace ICSharpCode.Decompiler.IL return list[list.Count - 1]; } - public T LastOrDefault() + public T? LastOrDefault() { return list.Count > 0 ? list[list.Count - 1] : null; } - public T SecondToLastOrDefault() + public T? SecondToLastOrDefault() { return list.Count > 1 ? list[list.Count - 2] : null; } - public T ElementAtOrDefault(int index) + public T? ElementAtOrDefault(int index) { if (index >= 0 && index < list.Count) return list[index]; diff --git a/ICSharpCode.Decompiler/IL/Instructions/Leave.cs b/ICSharpCode.Decompiler/IL/Instructions/Leave.cs index 73290f922..5f76b8a58 100644 --- a/ICSharpCode.Decompiler/IL/Instructions/Leave.cs +++ b/ICSharpCode.Decompiler/IL/Instructions/Leave.cs @@ -83,11 +83,15 @@ namespace ICSharpCode.Decompiler.IL } /// - /// Gets whether the leave instruction is leaving the whole ILFunction. + /// Gets whether the leave instruction is directly leaving the whole ILFunction. /// (TargetContainer == main container of the function). /// /// This is only valid for functions returning void (representing value-less "return;"), /// and for iterators (representing "yield break;"). + /// + /// Note: returns false for leave instructions that indirectly leave the function + /// (e.g. leaving a try block, and the try-finally construct is immediately followed + /// by another leave instruction) /// public bool IsLeavingFunction { get { return targetContainer?.Parent is ILFunction; } diff --git a/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs index 40014772e..11e20c33b 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs @@ -20,7 +20,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; -using System.Text; using ICSharpCode.Decompiler.IL.ControlFlow; using ICSharpCode.Decompiler.Util; @@ -39,18 +38,25 @@ namespace ICSharpCode.Decompiler.IL.Transforms { this.context = context; - foreach (var loop in function.Descendants.OfType()) + foreach (var block in function.Descendants.OfType()) { - if (loop.Kind != ContainerKind.Loop) - continue; - if (MatchWhileLoop(loop, out var condition, out var loopBody)) + for (int i = 0; i < block.Instructions.Count; i++) { - if (context.Settings.ForStatement) - MatchForLoop(loop, condition, loopBody); - continue; + var loop = block.Instructions[i] as BlockContainer; + if (loop == null || loop.Kind != ContainerKind.Loop) + continue; + // convert a "return" to a "break" so that we can match the high-level + // loop patterns + RemoveRedundantReturn.ReturnToBreak(block, loop, context); + if (MatchWhileLoop(loop, out var condition, out var loopBody)) + { + if (context.Settings.ForStatement) + MatchForLoop(loop, condition, loopBody); + continue; + } + if (context.Settings.DoWhileStatement && MatchDoWhileLoop(loop)) + continue; } - if (context.Settings.DoWhileStatement && MatchDoWhileLoop(loop)) - continue; } } diff --git a/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs index 062b1cd68..cab4d7d6d 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs @@ -95,7 +95,7 @@ namespace ICSharpCode.Decompiler.IL var inst = block.Instructions[i]; // the next instruction to be executed. Transformations will change the next instruction, so this is a method instead of a variable - ILInstruction NextInsn() => i + 1 < block.Instructions.Count ? block.Instructions[i + 1] : nextInstruction; + ILInstruction NextInsn() => block.Instructions.ElementAtOrDefault(i + 1) ?? nextInstruction; switch (inst) { @@ -103,10 +103,14 @@ namespace ICSharpCode.Decompiler.IL // visit the contents of the container Visit(container, continueTarget); + if (container.Kind == ContainerKind.Switch) + { + RemoveRedundantReturn.ReturnToBreak(block, container, context); + } // reduce nesting in switch blocks if (container.Kind == ContainerKind.Switch && - CanDuplicateExit(NextInsn(), continueTarget, out var keywordExit1) && - ReduceSwitchNesting(block, container, keywordExit1)) + CanDuplicateExit(NextInsn(), continueTarget, out var keywordExit1) && + ReduceSwitchNesting(block, container, keywordExit1)) { RemoveRedundantExit(block, nextInstruction); } diff --git a/ILSpy/LoadedAssembly.cs b/ILSpy/LoadedAssembly.cs index 9d663deaf..ad1dae9dc 100644 --- a/ILSpy/LoadedAssembly.cs +++ b/ILSpy/LoadedAssembly.cs @@ -473,7 +473,7 @@ namespace ICSharpCode.ILSpy return module; } - string file = parent.GetUniversalResolver().FindAssemblyFile(reference); + string? file = parent.GetUniversalResolver().FindAssemblyFile(reference); if (file != null) { From 685a79dc31ec0efed00a489ff44731beb045caab Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 3 Jun 2021 16:53:56 +0200 Subject: [PATCH 2/5] DetectExitPoints: introduce exit points for loops+switch This allows reverting the changes to HighLevelLoopTransform+ReduceNestingTransform from the previous commit, which fixes a bug in loop detection (the previous commit did not handle loops where the loop BlockContainer didn't have a Block as parent). --- .../TestCases/Pretty/ReduceNesting.cs | 26 +++++++++---------- .../CSharp/CSharpDecompiler.cs | 4 +-- .../IL/ControlFlow/ExitPoints.cs | 17 +++++------- .../IL/Transforms/HighLevelLoopTransform.cs | 25 +++++++----------- .../IL/Transforms/ReduceNestingTransform.cs | 4 --- 5 files changed, 30 insertions(+), 46 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs index 8bbc09ee4..ece018663 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs @@ -198,15 +198,16 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty case 1: Console.WriteLine("case 1"); return; - default: - if (B(1)) - { - Console.WriteLine(1); - } - return; + } + if (B(1)) + { + Console.WriteLine(1); } } - Console.WriteLine("else"); + else + { + Console.WriteLine("else"); + } } // nesting should not be reduced as maximum nesting level is 1 @@ -346,13 +347,12 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty Console.WriteLine(); - if (!B(1)) - { - return; - } - for (int i = 0; i < 10; i++) + if (B(1)) { - Console.WriteLine(i); + for (int i = 0; i < 10; i++) + { + Console.WriteLine(i); + } } } catch diff --git a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs index 6dc218393..0b4c212f7 100644 --- a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs +++ b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs @@ -93,7 +93,7 @@ namespace ICSharpCode.Decompiler.CSharp new YieldReturnDecompiler(), // must run after inlining but before loop detection new AsyncAwaitDecompiler(), // must run after inlining but before loop detection new DetectCatchWhenConditionBlocks(), // must run after inlining but before loop detection - new DetectExitPoints(canIntroduceExitForReturn: false), + new DetectExitPoints(), new EarlyExpressionTransforms(), // RemoveDeadVariableInit must run after EarlyExpressionTransforms so that stobj(ldloca V, ...) // is already collapsed into stloc(V, ...). @@ -117,7 +117,7 @@ namespace ICSharpCode.Decompiler.CSharp } }, // re-run DetectExitPoints after loop detection - new DetectExitPoints(canIntroduceExitForReturn: false), + new DetectExitPoints(), new BlockILTransform { // per-block transforms PostOrderTransforms = { new ConditionDetection(), diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs index 4b4232047..d8d570273 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs @@ -52,13 +52,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow static readonly Nop ExitNotYetDetermined = new Nop { Comment = "ExitNotYetDetermined" }; static readonly Nop NoExit = new Nop { Comment = "NoExit" }; - bool canIntroduceExitForReturn; - - public DetectExitPoints(bool canIntroduceExitForReturn) - { - this.canIntroduceExitForReturn = canIntroduceExitForReturn; - } - /// /// Gets the next instruction after is executed. /// Returns NoExit when the next instruction cannot be identified; @@ -214,12 +207,12 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow static ILInstruction ChooseExit(List potentialExits) { ILInstruction first = potentialExits[0]; - if (first is Leave l && l.IsLeavingFunction) + if (first is Leave { IsLeavingFunction: true }) { for (int i = 1; i < potentialExits.Count; i++) { var exit = potentialExits[i]; - if (!(exit is Leave l2 && l2.IsLeavingFunction)) + if (!(exit is Leave { IsLeavingFunction: true })) return exit; } } @@ -256,9 +249,11 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow // we can't introduce any additional exits return false; } - if (inst is Leave l && l.IsLeavingFunction) + if (inst is Leave { IsLeavingFunction: true }) { - return canIntroduceExitForReturn; + // Only convert 'return;' to an exit in a context where we can turn it into 'break;'. + // In other contexts we risk turning it into 'goto'. + return currentContainer.Kind != ContainerKind.Normal; } else { diff --git a/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs index 11e20c33b..d69ae9881 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/HighLevelLoopTransform.cs @@ -38,25 +38,18 @@ namespace ICSharpCode.Decompiler.IL.Transforms { this.context = context; - foreach (var block in function.Descendants.OfType()) + foreach (BlockContainer loop in function.Descendants.OfType()) { - for (int i = 0; i < block.Instructions.Count; i++) + if (loop.Kind != ContainerKind.Loop) + continue; + if (MatchWhileLoop(loop, out var condition, out var loopBody)) { - var loop = block.Instructions[i] as BlockContainer; - if (loop == null || loop.Kind != ContainerKind.Loop) - continue; - // convert a "return" to a "break" so that we can match the high-level - // loop patterns - RemoveRedundantReturn.ReturnToBreak(block, loop, context); - if (MatchWhileLoop(loop, out var condition, out var loopBody)) - { - if (context.Settings.ForStatement) - MatchForLoop(loop, condition, loopBody); - continue; - } - if (context.Settings.DoWhileStatement && MatchDoWhileLoop(loop)) - continue; + if (context.Settings.ForStatement) + MatchForLoop(loop, condition, loopBody); + continue; } + if (context.Settings.DoWhileStatement && MatchDoWhileLoop(loop)) + continue; } } diff --git a/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs index cab4d7d6d..2c48783b7 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs @@ -103,10 +103,6 @@ namespace ICSharpCode.Decompiler.IL // visit the contents of the container Visit(container, continueTarget); - if (container.Kind == ContainerKind.Switch) - { - RemoveRedundantReturn.ReturnToBreak(block, container, context); - } // reduce nesting in switch blocks if (container.Kind == ContainerKind.Switch && CanDuplicateExit(NextInsn(), continueTarget, out var keywordExit1) && From a716828065498cc154407710ea43a132d370ef26 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 3 Jun 2021 18:33:59 +0200 Subject: [PATCH 3/5] Remove redundant code from RemoveRedundantReturn.cs --- .../IL/ControlFlow/RemoveRedundantReturn.cs | 90 +++---------------- 1 file changed, 10 insertions(+), 80 deletions(-) diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs b/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs index 4759b9a62..d6cbe2d1c 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs @@ -18,7 +18,6 @@ #nullable enable -using System.Diagnostics; using System.Linq; using ICSharpCode.Decompiler.IL.Transforms; @@ -59,26 +58,19 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow bool result = false; switch (inst) { - case BlockContainer c: - if (c.Kind != ContainerKind.Normal) + case BlockContainer c when c.Kind == ContainerKind.Normal: + // body of try block, or similar: recurse into last instruction in container + // Note: no need to handle loops/switches here; those already were handled by DetectExitPoints + Block lastBlock = c.Blocks.Last(); + if (lastBlock.Instructions.Last() is Leave { IsLeavingFunction: true, Value: Nop } leave) { - // loop or switch: turn all "return" into "break" - result |= ReturnToLeaveInContainer(c); + leave.TargetContainer = c; + result = true; } - else + else if (ConvertReturnToFallthrough(lastBlock.Instructions.Last())) { - // body of try block, or similar: recurse into last instruction in container - Block lastBlock = c.Blocks.Last(); - if (lastBlock.Instructions.Last() is Leave { IsLeavingFunction: true, Value: Nop } leave) - { - leave.TargetContainer = c; - result = true; - } - else if (ConvertReturnToFallthrough(lastBlock.Instructions.Last())) - { - lastBlock.Instructions.Add(new Leave(c)); - result = true; - } + lastBlock.Instructions.Add(new Leave(c)); + result = true; } break; case TryCatch tryCatch: @@ -119,67 +111,5 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow } return result; } - - /// - /// Transforms - /// loop { ... if (x) return; .. } - /// to - /// loop { ... if (x) break; .. } return; - /// - internal static void ReturnToBreak(Block parentBlock, BlockContainer loopOrSwitch, ILTransformContext context) - { - Debug.Assert(loopOrSwitch.Parent == parentBlock); - // This transform is only possible when the loop/switch doesn't already use "break;" - if (loopOrSwitch.LeaveCount != 0) - return; - // loopOrSwitch with LeaveCount==0 has unreachable exit point and thus must be last in block. - Debug.Assert(parentBlock.Instructions.Last() == loopOrSwitch); - var nearestFunction = parentBlock.Ancestors.OfType().First(); - if (nearestFunction.Body is BlockContainer functionContainer) - { - context.Step("pull return out of loop/switch: " + loopOrSwitch.EntryPoint.Label, loopOrSwitch); - if (ReturnToLeaveInContainer(loopOrSwitch)) - { - // insert a return after the loop - parentBlock.Instructions.Add(new Leave(functionContainer)); - } - } - } - - private static bool ReturnToLeaveInContainer(BlockContainer c) - { - bool result = false; - foreach (var block in c.Blocks) - { - result |= ReturnToLeave(block, c); - } - return result; - } - - private static bool ReturnToLeave(ILInstruction inst, BlockContainer c) - { - if (inst is Leave { IsLeavingFunction: true, Value: Nop } leave) - { - leave.TargetContainer = c; - return true; - } - else if (inst is BlockContainer nested && nested.Kind != ContainerKind.Normal) - { - return false; - } - else if (inst is ILFunction) - { - return false; - } - else - { - bool b = false; - foreach (var child in inst.Children) - { - b |= ReturnToLeave(child, c); - } - return b; - } - } } } From 2419c2641ac878af5880ef740d309205a57372bd Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 3 Jun 2021 19:02:06 +0200 Subject: [PATCH 4/5] StatementBuilder.TransformToForeach: allow both `break;` and `return;` within the using body --- .../CSharp/StatementBuilder.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs b/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs index 70561838c..0094db1a1 100644 --- a/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs +++ b/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs @@ -632,7 +632,7 @@ namespace ICSharpCode.Decompiler.CSharp var enumeratorVar = inst.Variable; // If there's another BlockContainer nested in this container and it only has one child block, unwrap it. // If there's an extra leave inside the block, extract it into optionalReturnAfterLoop. - var loopContainer = UnwrapNestedContainerIfPossible(container, out var optionalReturnAfterLoop); + var loopContainer = UnwrapNestedContainerIfPossible(container, out var optionalLeaveAfterLoop); // Detect whether we're dealing with a while loop with multiple embedded statements. if (loopContainer.Kind != ContainerKind.While) return null; @@ -771,22 +771,22 @@ namespace ICSharpCode.Decompiler.CSharp // If there was an optional return statement, return it as well. // If there were labels or any other statements in the whileLoopBlock, move them after the foreach // loop. - if (optionalReturnAfterLoop != null || whileLoopBlock.Statements.Count > 1) + if (optionalLeaveAfterLoop != null || whileLoopBlock.Statements.Count > 1) { var block = new BlockStatement { Statements = { foreachStmt } }; - if (optionalReturnAfterLoop != null) + if (optionalLeaveAfterLoop != null) { - block.Statements.Add(optionalReturnAfterLoop.AcceptVisitor(this)); + block.Statements.Add(optionalLeaveAfterLoop.AcceptVisitor(this)); } if (whileLoopBlock.Statements.Count > 1) { block.Statements.AddRange(whileLoopBlock.Statements .Skip(1) - .SkipWhile(s => s.Annotations.Any(a => a == optionalReturnAfterLoop)) + .SkipWhile(s => s.Annotations.Any(a => a == optionalLeaveAfterLoop)) .Select(SyntaxExtensions.Detach)); } return block; @@ -860,10 +860,10 @@ namespace ICSharpCode.Decompiler.CSharp /// if the value has no side-effects. /// Otherwise returns the unmodified container. /// - /// If the leave is a return and has no side-effects, we can move the return out of the using-block and put it after the loop, otherwise returns null. - BlockContainer UnwrapNestedContainerIfPossible(BlockContainer container, out Leave optionalReturnInst) + /// If the leave is a return/break and has no side-effects, we can move the return out of the using-block and put it after the loop, otherwise returns null. + BlockContainer UnwrapNestedContainerIfPossible(BlockContainer container, out Leave optionalLeaveInst) { - optionalReturnInst = null; + optionalLeaveInst = null; // Check block structure: if (container.Blocks.Count != 1) return container; @@ -875,11 +875,11 @@ namespace ICSharpCode.Decompiler.CSharp // If the leave has no value, just unwrap the BlockContainer. if (leave.MatchLeave(container)) return nestedContainer; - // If the leave is a return, we can move the return out of the using-block and put it after the loop + // If the leave is a return/break, we can move it out of the using-block and put it after the loop // (but only if the value doesn't have side-effects) - if (leave.IsLeavingFunction && SemanticHelper.IsPure(leave.Value.Flags)) + if (SemanticHelper.IsPure(leave.Value.Flags)) { - optionalReturnInst = leave; + optionalLeaveInst = leave; return nestedContainer; } return container; From 109b6d073a999eb0d493f3aa94d32ca1f13c9125 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 3 Jun 2021 18:41:44 +0200 Subject: [PATCH 5/5] Allow detecting exit points across multiple levels of containers. --- .../TestCases/Pretty/Loops.cs | 21 +++ .../IL/ControlFlow/ExitPoints.cs | 171 ++++++++++-------- 2 files changed, 115 insertions(+), 77 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs index 28fe3d0c0..19e61af25 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs @@ -1009,5 +1009,26 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } Console.WriteLine("end"); } + + public void ForEachInSwitch(int i, IEnumerable args) + { + switch (i) + { + + case 1: + Console.WriteLine("one"); + break; + case 2: + { + foreach (string arg in args) + { + Console.WriteLine(arg); + } + break; + } + default: + throw new NotImplementedException(); + } + } } } diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs index d8d570273..a2026e786 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs @@ -16,8 +16,11 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. +#nullable enable + using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Threading; using ICSharpCode.Decompiler.IL.Transforms; @@ -59,10 +62,10 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow /// internal static ILInstruction GetExit(ILInstruction inst) { - SlotInfo slot = inst.SlotInfo; + SlotInfo? slot = inst.SlotInfo; if (slot == Block.InstructionSlot) { - Block block = (Block)inst.Parent; + Block block = (Block)inst.Parent!; return block.Instructions.ElementAtOrDefault(inst.ChildIndex + 1) ?? ExitNotYetDetermined; } else if (slot == TryInstruction.TryBlockSlot @@ -72,7 +75,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow || slot == UsingInstruction.BodySlot || slot == LockInstruction.BodySlot) { - return GetExit(inst.Parent); + return GetExit(inst.Parent!); } return NoExit; } @@ -100,30 +103,53 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow } } - CancellationToken cancellationToken; - BlockContainer currentContainer; + class ContainerContext + { + public readonly BlockContainer Container; - /// - /// The instruction that will be executed next after leaving the currentContainer. - /// ExitNotYetDetermined means the container is last in its parent block, and thus does not - /// yet have any leave instructions. This means we can move any exit instruction of - /// our choice our of the container and replace it with a leave instruction. - /// - ILInstruction currentExit; + /// + /// The instruction that will be executed next after leaving the Container. + /// ExitNotYetDetermined means the container is last in its parent block, and thus does not + /// yet have any leave instructions. This means we can move any exit instruction of + /// our choice our of the container and replace it with a leave instruction. + /// + public readonly ILInstruction CurrentExit; - /// - /// If currentExit==ExitNotYetDetermined, holds the list of potential exit instructions. - /// After the currentContainer was visited completely, one of these will be selected as exit instruction. - /// - List potentialExits; + /// + /// If currentExit==ExitNotYetDetermined, holds the list of potential exit instructions. + /// After the currentContainer was visited completely, one of these will be selected as exit instruction. + /// + public readonly List? PotentialExits = null; + + public ContainerContext(BlockContainer container, ILInstruction currentExit) + { + this.Container = container; + this.CurrentExit = currentExit; + this.PotentialExits = (currentExit == ExitNotYetDetermined ? new List() : null); + } + + public void HandleExit(ILInstruction inst) + { + if (this.CurrentExit == ExitNotYetDetermined && this.Container.LeaveCount == 0) + { + this.PotentialExits!.Add(inst); + } + else if (CompatibleExitInstruction(inst, this.CurrentExit)) + { + inst.ReplaceWith(new Leave(this.Container).WithILRange(inst)); + } + } + } + CancellationToken cancellationToken; readonly List blocksPotentiallyMadeUnreachable = new List(); + readonly Stack containerStack = new Stack(); public void Run(ILFunction function, ILTransformContext context) { cancellationToken = context.CancellationToken; - currentExit = NoExit; blocksPotentiallyMadeUnreachable.Clear(); + containerStack.Clear(); function.AcceptVisitor(this); // It's possible that there are unreachable code blocks which we only // detect as such during exit point detection. @@ -136,6 +162,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow } } blocksPotentiallyMadeUnreachable.Clear(); + containerStack.Clear(); } static bool IsInfiniteLoop(Block block) @@ -153,30 +180,26 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow protected internal override void VisitBlockContainer(BlockContainer container) { - var oldExit = currentExit; - var oldContainer = currentContainer; - var oldPotentialExits = potentialExits; var thisExit = GetExit(container); - currentExit = thisExit; - currentContainer = container; - potentialExits = (thisExit == ExitNotYetDetermined ? new List() : null); + var stackEntry = new ContainerContext(container, thisExit); + containerStack.Push(stackEntry); base.VisitBlockContainer(container); - if (thisExit == ExitNotYetDetermined && potentialExits.Count > 0) + if (stackEntry.PotentialExits?.Any(i => i.IsConnected) ?? false) { // This transform determined an exit point. - currentExit = ChooseExit(potentialExits); - foreach (var exit in potentialExits) + var newExit = ChooseExit(stackEntry.PotentialExits.Where(i => i.IsConnected)); + Debug.Assert(!newExit.MatchLeave(container)); + foreach (var exit in stackEntry.PotentialExits) { - if (CompatibleExitInstruction(currentExit, exit)) + if (exit.IsConnected && CompatibleExitInstruction(newExit, exit)) { - exit.ReplaceWith(new Leave(currentContainer).WithILRange(exit)); + exit.ReplaceWith(new Leave(container).WithILRange(exit)); } } - Debug.Assert(!currentExit.MatchLeave(currentContainer)); ILInstruction inst = container; // traverse up to the block (we'll always find one because GetExit // only returns ExitNotYetDetermined if there's a block) - while (inst.Parent.OpCode != OpCode.Block) + while (inst.Parent!.OpCode != OpCode.Block) inst = inst.Parent; Block block = (Block)inst.Parent; if (block.HasFlag(InstructionFlags.EndPointUnreachable)) @@ -185,33 +208,32 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow // we still have an unreachable endpoint. // The appended currentExit instruction would not be reachable! // This happens in test case ExceptionHandling.ThrowInFinally() - if (currentExit is Branch b) + if (newExit is Branch b) { blocksPotentiallyMadeUnreachable.Add(b.TargetBlock); } } else { - block.Instructions.Add(currentExit); + block.Instructions.Add(newExit); } } - else + if (containerStack.Pop() != stackEntry) { - Debug.Assert(thisExit == currentExit); + Debug.Fail("containerStack got imbalanced"); } - currentExit = oldExit; - currentContainer = oldContainer; - potentialExits = oldPotentialExits; } - static ILInstruction ChooseExit(List potentialExits) + static ILInstruction ChooseExit(IEnumerable potentialExits) { - ILInstruction first = potentialExits[0]; + using var enumerator = potentialExits.GetEnumerator(); + enumerator.MoveNext(); + ILInstruction first = enumerator.Current; if (first is Leave { IsLeavingFunction: true }) { - for (int i = 1; i < potentialExits.Count; i++) + while (enumerator.MoveNext()) { - var exit = potentialExits[i]; + var exit = enumerator.Current; if (!(exit is Leave { IsLeavingFunction: true })) return exit; } @@ -229,43 +251,13 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow } } - void HandleExit(ILInstruction inst) - { - if (currentExit == ExitNotYetDetermined && CanIntroduceAsExit(inst)) - { - potentialExits.Add(inst); - } - else if (CompatibleExitInstruction(inst, currentExit)) - { - inst.ReplaceWith(new Leave(currentContainer).WithILRange(inst)); - } - } - - private bool CanIntroduceAsExit(ILInstruction inst) - { - if (currentContainer.LeaveCount > 0) - { - // if we're re-running on a block container that already has an exit, - // we can't introduce any additional exits - return false; - } - if (inst is Leave { IsLeavingFunction: true }) - { - // Only convert 'return;' to an exit in a context where we can turn it into 'break;'. - // In other contexts we risk turning it into 'goto'. - return currentContainer.Kind != ContainerKind.Normal; - } - else - { - return true; - } - } - protected internal override void VisitBranch(Branch inst) { - if (!inst.TargetBlock.IsDescendantOf(currentContainer)) + foreach (var entry in containerStack) { - HandleExit(inst); + if (inst.TargetBlock.IsDescendantOf(entry.Container)) + break; + entry.HandleExit(inst); } } @@ -274,7 +266,32 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow base.VisitLeave(inst); if (!inst.Value.MatchNop()) return; - HandleExit(inst); + foreach (var entry in containerStack) + { + if (inst.TargetContainer == entry.Container) + break; + if (inst.IsLeavingFunction || inst.TargetContainer.Kind != ContainerKind.Normal) + { + if (entry.Container.Kind == ContainerKind.Normal) + { + // Don't transform a `return`/`break` into a leave for try-block containers (or similar). + // It's possible that those can be turned into fallthrough later, but + // it might not work out and then we would be left with a `goto`. + // But continue searching the container stack, it might be possible to + // turn the `return` into a `break` instead. + } + else + { + // return; could turn to break; + entry.HandleExit(inst); + break; // but only for the innermost loop/switch + } + } + else + { + entry.HandleExit(inst); + } + } } } }