Browse Source

Fix detection of switch statements with cases containing a single break;

Remedy incorrect assumption that the default case was special.
pull/1258/head
Chicken-Bones 8 years ago
parent
commit
1a021635cc
  1. 21
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs
  2. 48
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il
  3. 32
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il
  4. 31
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il
  5. 50
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il
  6. 118
      ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs

21
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs

@ -669,7 +669,25 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -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 @@ -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) {

48
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.il

@ -2468,6 +2468,54 @@ @@ -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
{

32
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.il

@ -1944,6 +1944,38 @@ @@ -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
{

31
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.opt.roslyn.il

@ -2078,6 +2078,37 @@ @@ -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
{

50
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.roslyn.il

@ -2750,6 +2750,56 @@ @@ -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
{

118
ICSharpCode.Decompiler/IL/ControlFlow/SwitchDetection.cs

@ -206,7 +206,15 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow @@ -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();
}
/// <summary>
@ -225,40 +233,46 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow @@ -225,40 +233,46 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
/// <summary>
/// Determines if the analysed switch can be constructed without any gotos
/// </summary>
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>();
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;
}
/// <summary>
@ -293,65 +307,19 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow @@ -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;
/// <summary>
/// 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
/// </summary>
internal static IEnumerable<ControlFlowNode> GetDominatorTreeExits(ControlFlowNode dominator) =>
dominator.DominatorTreeChildren.Concat(new[] {dominator})
.SelectMany(n => n.Successors)
.Where(n => !dominator.Dominates(n));
/// <summary>
/// Lists all potential targets for break; statements from a domination tree,
/// assuming the domination tree must be exited via either break; or continue;
/// </summary>
internal static IEnumerable<ControlFlowNode> GetBreakTargets(ControlFlowNode dominator) =>
GetDominatorTreeExits(dominator).Where(n => !IsContinue((Block)n.UserData));
/// <summary>
/// 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
/// </summary>
/// <returns></returns>
internal static bool FindBreakTarget(List<ControlFlowNode> 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<ControlFlowNode> GetBreakTargets(ControlFlowNode dominator) =>
TreeTraversal.PreOrder(dominator, n => n.DominatorTreeChildren.Where(c => !IsContinue(c)))
.SelectMany(n => n.Successors.Where(c => !dominator.Dominates(c) && !IsContinue(c)));
}
}

Loading…
Cancel
Save