From 1a021635ccd6ede669ada056fc12eb7d229e5c87 Mon Sep 17 00:00:00 2001 From: Chicken-Bones Date: Thu, 19 Jul 2018 17:20:35 +1000 Subject: [PATCH] Fix detection of switch statements with cases containing a single break; Remedy incorrect assumption that the default case was special. --- .../TestCases/Pretty/Switch.cs | 21 +++- .../TestCases/Pretty/Switch.il | 48 +++++++ .../TestCases/Pretty/Switch.opt.il | 32 +++++ .../TestCases/Pretty/Switch.opt.roslyn.il | 31 +++++ .../TestCases/Pretty/Switch.roslyn.il | 50 ++++++++ .../IL/ControlFlow/SwitchDetection.cs | 118 +++++++----------- 6 files changed, 223 insertions(+), 77 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs index aaa97af62..5a3c7b1f7 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs @@ -669,7 +669,25 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } #endregion - // Ensure correctness of SwitchDetection.FindBreakTarget + // Ensure correctness of SwitchDetection.UseCSharpSwitch control flow heuristics + public static void SwitchWithBreakCase(int i, bool b) + { + if (b) { + switch (i) { + case 1: + Console.WriteLine(1); + break; + default: + Console.WriteLine("default"); + break; + case 2: + break; + } + Console.WriteLine("b"); + } + Console.WriteLine("end"); + } + public static void SwitchWithReturnAndBreak(int i, bool b) { switch (i) { @@ -709,7 +727,6 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty return 0; } - // Ensure correctness of SwitchDetection.FindBreakTarget (default case has no associated block) public static void SwitchWithReturnAndBreak3(int i) { switch (i) { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il index 7321936f5..8bfc4217b 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il @@ -2468,6 +2468,54 @@ IL_004f: ret } // end of method Switch::SwitchlikeIf2 + .method public hidebysig static void SwitchWithBreakCase(int32 i, + bool b) cil managed + { + // Code size 78 (0x4e) + .maxstack 2 + .locals init (bool V_0, + int32 V_1) + IL_0000: nop + IL_0001: ldarg.1 + IL_0002: ldc.i4.0 + IL_0003: ceq + IL_0005: stloc.0 + IL_0006: ldloc.0 + IL_0007: brtrue.s IL_0042 + + IL_0009: nop + IL_000a: ldarg.0 + IL_000b: stloc.1 + IL_000c: ldloc.1 + IL_000d: ldc.i4.1 + IL_000e: sub + IL_000f: switch ( + IL_001e, + IL_0034) + IL_001c: br.s IL_0027 + + IL_001e: ldc.i4.1 + IL_001f: call void [mscorlib]System.Console::WriteLine(int32) + IL_0024: nop + IL_0025: br.s IL_0036 + + IL_0027: ldstr "default" + IL_002c: call void [mscorlib]System.Console::WriteLine(string) + IL_0031: nop + IL_0032: br.s IL_0036 + + IL_0034: br.s IL_0036 + + IL_0036: ldstr "b" + IL_003b: call void [mscorlib]System.Console::WriteLine(string) + IL_0040: nop + IL_0041: nop + IL_0042: ldstr "end" + IL_0047: call void [mscorlib]System.Console::WriteLine(string) + IL_004c: nop + IL_004d: ret + } // end of method Switch::SwitchWithBreakCase + .method public hidebysig static void SwitchWithReturnAndBreak(int32 i, bool b) cil managed { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il index 41c744b50..39e9f2ba7 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il @@ -1944,6 +1944,38 @@ IL_0024: ret } // end of method Switch::SwitchlikeIf2 + .method public hidebysig static void SwitchWithBreakCase(int32 i, + bool b) cil managed + { + // Code size 62 (0x3e) + .maxstack 2 + .locals init (int32 V_0) + IL_0000: ldarg.1 + IL_0001: brfalse.s IL_0033 + + IL_0003: ldarg.0 + IL_0004: stloc.0 + IL_0005: ldloc.0 + IL_0006: ldc.i4.1 + IL_0007: sub + IL_0008: switch ( + IL_0017, + IL_0029) + IL_0015: br.s IL_001f + + IL_0017: ldc.i4.1 + IL_0018: call void [mscorlib]System.Console::WriteLine(int32) + IL_001d: br.s IL_0029 + + IL_001f: ldstr "default" + IL_0024: call void [mscorlib]System.Console::WriteLine(string) + IL_0029: ldstr "b" + IL_002e: call void [mscorlib]System.Console::WriteLine(string) + IL_0033: ldstr "end" + IL_0038: call void [mscorlib]System.Console::WriteLine(string) + IL_003d: ret + } // end of method Switch::SwitchWithBreakCase + .method public hidebysig static void SwitchWithReturnAndBreak(int32 i, bool b) cil managed { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il index 625a70267..6d379b5d6 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il @@ -2078,6 +2078,37 @@ IL_0024: ret } // end of method Switch::SwitchlikeIf2 + .method public hidebysig static void SwitchWithBreakCase(int32 i, + bool b) cil managed + { + // Code size 52 (0x34) + .maxstack 8 + IL_0000: ldarg.1 + IL_0001: brfalse.s IL_0029 + + IL_0003: ldarg.0 + IL_0004: ldc.i4.1 + IL_0005: beq.s IL_000d + + IL_0007: ldarg.0 + IL_0008: ldc.i4.2 + IL_0009: beq.s IL_001f + + IL_000b: br.s IL_0015 + + IL_000d: ldc.i4.1 + IL_000e: call void [mscorlib]System.Console::WriteLine(int32) + IL_0013: br.s IL_001f + + IL_0015: ldstr "default" + IL_001a: call void [mscorlib]System.Console::WriteLine(string) + IL_001f: ldstr "b" + IL_0024: call void [mscorlib]System.Console::WriteLine(string) + IL_0029: ldstr "end" + IL_002e: call void [mscorlib]System.Console::WriteLine(string) + IL_0033: ret + } // end of method Switch::SwitchWithBreakCase + .method public hidebysig static void SwitchWithReturnAndBreak(int32 i, bool b) cil managed { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il index a64fee342..819f8252b 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il @@ -2750,6 +2750,56 @@ IL_0049: ret } // end of method Switch::SwitchlikeIf2 + .method public hidebysig static void SwitchWithBreakCase(int32 i, + bool b) cil managed + { + // Code size 69 (0x45) + .maxstack 2 + .locals init (bool V_0, + int32 V_1) + IL_0000: nop + IL_0001: ldarg.1 + IL_0002: stloc.0 + IL_0003: ldloc.0 + IL_0004: brfalse.s IL_0039 + + IL_0006: nop + IL_0007: ldarg.0 + IL_0008: stloc.1 + IL_0009: ldloc.1 + IL_000a: ldc.i4.1 + IL_000b: beq.s IL_0015 + + IL_000d: br.s IL_000f + + IL_000f: ldloc.1 + IL_0010: ldc.i4.2 + IL_0011: beq.s IL_002b + + IL_0013: br.s IL_001e + + IL_0015: ldc.i4.1 + IL_0016: call void [mscorlib]System.Console::WriteLine(int32) + IL_001b: nop + IL_001c: br.s IL_002d + + IL_001e: ldstr "default" + IL_0023: call void [mscorlib]System.Console::WriteLine(string) + IL_0028: nop + IL_0029: br.s IL_002d + + IL_002b: br.s IL_002d + + IL_002d: ldstr "b" + IL_0032: call void [mscorlib]System.Console::WriteLine(string) + IL_0037: nop + IL_0038: nop + IL_0039: ldstr "end" + IL_003e: call void [mscorlib]System.Console::WriteLine(string) + IL_0043: nop + IL_0044: ret + } // end of method Switch::SwitchWithBreakCase + .method public hidebysig static void SwitchWithReturnAndBreak(int32 i, bool b) cil managed { diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs b/ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs index c9517dc22..42682e831 100644 --- a/ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs +++ b/ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs @@ -206,7 +206,15 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow // These goto statements may be "goto case x" or "goto default", but these are a hint that the original code was not a switch, // and that the switch statement may be very poor quality. // Thus the rule of thumb is no goto statements if the original code didn't include them - return !SwitchUsesGoto(); + if (SwitchUsesGoto(out var breakBlock)) + return false; + + if (breakBlock == null) + return true; + + // The switch has a single break target and there is one more hint + // The break target cannot be inlined, and should have the highest IL offset of everything targetted by the switch + return breakBlock.ChildIndex >= analysis.Sections.Select(s => s.Value.MatchBranch(out var b) ? b.ChildIndex : -1).Max(); } /// @@ -225,40 +233,46 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow /// /// Determines if the analysed switch can be constructed without any gotos /// - private bool SwitchUsesGoto() + private bool SwitchUsesGoto(out Block breakBlock) { if (controlFlowGraph == null) controlFlowGraph = new ControlFlowGraph(currentContainer, context.CancellationToken); // grab the control flow nodes for blocks targetted by each section var caseNodes = new List(); - ControlFlowNode defaultNode = null; - foreach (var s in analysis.Sections) { - // leave sections not considered - if (s.Value.MatchBranch(out var block) && !IsContinue(block)) { - if (s.Key.Count() > MaxValuesPerSection) - defaultNode = controlFlowGraph.GetNode(block); - else - caseNodes.Add(controlFlowGraph.GetNode(block)); - } + if (!s.Value.MatchBranch(out var block)) + continue; + + var node = controlFlowGraph.GetNode(block); + if (!IsContinue(node)) + caseNodes.Add(node); } - var flowBlocks = analysis.InnerBlocks.Concat(new[] {analysis.RootBlock}).ToHashSet(); - + var flowBlocks = analysis.InnerBlocks.ToHashSet(); + flowBlocks.Add(analysis.RootBlock); AddNullCase(flowBlocks, caseNodes); - // all predecessors of case blocks must be part of the switch logic (to avoid gotos) - foreach (var caseNode in caseNodes) - if (caseNode.Predecessors.Any(n => !flowBlocks.Contains(n.UserData))) - return true; + // cases with predecessors that aren't part of the switch logic + // must either require "goto case" statements, or consist of a single "break;" + var externalCases = caseNodes.Where(c => c.Predecessors.Any(n => !flowBlocks.Contains(n.UserData))).ToList(); + + breakBlock = null; + if (externalCases.Count > 1) + return true; // cannot have more than one break case without gotos + + // check that case nodes flow through a single point + var breakTargets = caseNodes.Except(externalCases).SelectMany(GetBreakTargets).ToHashSet(); + + // if there are multiple break targets, then gotos are required + // if there are none, then the external case (if any) can be the break target + if (breakTargets.Count != 1) + return breakTargets.Count > 1; - if (!FindBreakTarget(caseNodes, defaultNode, out var breakTarget)) - return true; // more than one exit point, requires goto statements + breakBlock = (Block) breakTargets.Single().UserData; - // if the switch has a single break target there is one more hint - // The break target cannot be inlined, and should have a higher IL offset than anything in the switch body - return breakTarget?.UserIndex < caseNodes.Select(c => c.UserIndex).Max(); + // external case must consist of a single "break;" + return externalCases.Count == 1 && breakBlock != externalCases.Single().UserData; } /// @@ -293,65 +307,19 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow flowBlocks.Add(nullableBlock); } - internal static bool IsContinue(Block block) => false; + internal static bool IsContinue(ControlFlowNode node) => false; - /// - /// Enumerates all nodes via which a domination tree can be exited. (Not distinct) - /// That is, all nodes with at least one dominated predecessor which are not dominated themselves - /// - /// Note that while this construction appears less efficient than a recursive solution, - /// a recursive solution would require the use of the Visted flag to avoid loops in the dominator tree - /// - internal static IEnumerable GetDominatorTreeExits(ControlFlowNode dominator) => - dominator.DominatorTreeChildren.Concat(new[] {dominator}) - .SelectMany(n => n.Successors) - .Where(n => !dominator.Dominates(n)); - /// /// Lists all potential targets for break; statements from a domination tree, /// assuming the domination tree must be exited via either break; or continue; - /// - internal static IEnumerable GetBreakTargets(ControlFlowNode dominator) => - GetDominatorTreeExits(dominator).Where(n => !IsContinue((Block)n.UserData)); - - /// - /// Finds the target node of all branches leaving the dominator tree of the cases of a switch block. - /// Returns true if a single target was found, or if no break statements are required (breakTarget = null) - /// If the defaultNode is the breakTarget, then it should not be inlined. /// - /// Note that branches to case labels ("goto case x") are considered break targets, and will fail this method + /// First list all nodes in the dominator tree (excluding continue nodes) + /// Then return the all successors not contained within said tree. /// - /// defaultNode may be null if there is no control block associated with the default case. + /// Note that node will be returned once for every outgoing edge /// - /// - internal static bool FindBreakTarget(List caseNodes, ControlFlowNode defaultNode, out ControlFlowNode breakTarget) - { - breakTarget = null; - var breakTargets = caseNodes.SelectMany(GetBreakTargets).ToHashSet(); - - // no cases require break statements, inlining of default case doesn't matter - if (breakTargets.Count == 0) - return true; - - if (defaultNode != null) { - // default case is break target, don't inline - if (breakTargets.Count == 1 && breakTargets.Single() == defaultNode) { - breakTarget = defaultNode; - return true; - } - - // default case requires inlining, ensure break target matches that of case statements - breakTargets.AddRange(GetBreakTargets(defaultNode)); - } - - // single break target found - if (breakTargets.Count == 1) { - breakTarget = breakTargets.Single(); - return true; - } - - // more than 1 break target found - return false; - } + internal static IEnumerable GetBreakTargets(ControlFlowNode dominator) => + TreeTraversal.PreOrder(dominator, n => n.DominatorTreeChildren.Where(c => !IsContinue(c))) + .SelectMany(n => n.Successors.Where(c => !dominator.Dominates(c) && !IsContinue(c))); } }