Skip to content

Commit

Permalink
Fix #2379: Keep return statements around in original form for Condi…
Browse files Browse the repository at this point in the history
…tionDetection, 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.
  • Loading branch information
dgrunwald committed Jun 2, 2021
1 parent 3c6961a commit 6757295
Show file tree
Hide file tree
Showing 12 changed files with 265 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,28 @@ public void ThrowInFinally()
}
}

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
Expand Down Expand Up @@ -476,4 +498,4 @@ public void XXX2()
}
#endif
}
}
}
27 changes: 13 additions & 14 deletions ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,17 +198,15 @@ public void NestedSwitchIf()
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
Expand Down Expand Up @@ -348,12 +346,13 @@ public void EarlyExitInTry()

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
Expand Down
3 changes: 2 additions & 1 deletion ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public static List<IILTransform> GetILTransforms()
}
},
// re-run DetectExitPoints after loop detection
new DetectExitPoints(canIntroduceExitForReturn: true),
new DetectExitPoints(canIntroduceExitForReturn: false),
new BlockILTransform { // per-block transforms
PostOrderTransforms = {
new ConditionDetection(),
Expand Down Expand Up @@ -159,6 +159,7 @@ public static List<IILTransform> GetILTransforms()
new TransformDisplayClassUsage(),
new HighLevelLoopTransform(),
new ReduceNestingTransform(),
new RemoveRedundantReturn(),
new IntroduceDynamicTypeOnLocals(),
new IntroduceNativeIntTypeOnLocals(),
new AssignVariableNames(),
Expand Down
1 change: 1 addition & 0 deletions ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
<Compile Include="CSharp\Syntax\VariableDesignation.cs" />
<Compile Include="Humanizer\Vocabularies.cs" />
<Compile Include="Humanizer\Vocabulary.cs" />
<Compile Include="IL\ControlFlow\RemoveRedundantReturn.cs" />
<Compile Include="IL\Transforms\DeconstructionTransform.cs" />
<Compile Include="IL\Transforms\FixLoneIsInst.cs" />
<Compile Include="IL\Instructions\DeconstructInstruction.cs" />
Expand Down
2 changes: 0 additions & 2 deletions ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -366,7 +365,6 @@ internal static void InvertIf(Block block, IfInstruction ifInst, ILTransformCont
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);

Expand Down
5 changes: 3 additions & 2 deletions ICSharpCode.Decompiler/IL/ControlFlow/ExitPoints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -76,7 +75,9 @@ internal static ILInstruction GetExit(ILInstruction inst)
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);
}
Expand Down
185 changes: 185 additions & 0 deletions ICSharpCode.Decompiler/IL/ControlFlow/RemoveRedundantReturn.cs
Original file line number Diff line number Diff line change
@@ -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
{
/// <summary>
/// Similar to <see cref="DetectExitPoints"/>, but acts only on <c>leave</c> instructions
/// leaving the whole function (<c>return</c>/<c>yield break</c>) that can be made implicit
/// without using goto.
/// </summary>
class RemoveRedundantReturn : IILTransform
{
public void Run(ILFunction function, ILTransformContext context)
{
foreach (var lambda in function.Descendants.OfType<ILFunction>())
{
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;
}

/// <summary>
/// Transforms
/// loop { ... if (x) return; .. }
/// to
/// loop { ... if (x) break; .. } return;
/// </summary>
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<ILFunction>().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;
}
}
}
}
16 changes: 9 additions & 7 deletions ICSharpCode.Decompiler/IL/Instructions/InstructionCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -73,7 +75,7 @@ public Enumerator GetEnumerator()
public struct Enumerator : IEnumerator<T>
{
#if DEBUG
ILInstruction parentInstruction;
ILInstruction? parentInstruction;
#endif
readonly List<T> list;
int pos;
Expand Down Expand Up @@ -140,7 +142,7 @@ System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
/// Runs in O(1) if the item can be found using the Parent/ChildIndex properties.
/// Otherwise, runs in O(N).
/// </remarks>
public int IndexOf(T item)
public int IndexOf(T? item)
{
if (item == null)
{
Expand All @@ -163,7 +165,7 @@ public int IndexOf(T item)
/// This method searches the list.
/// Usually it's more efficient to test item.Parent instead!
/// </remarks>
public bool Contains(T item)
public bool Contains(T? item)
{
return IndexOf(item) >= 0;
}
Expand Down Expand Up @@ -395,7 +397,7 @@ public T First()
return list[0];
}

public T FirstOrDefault()
public T? FirstOrDefault()
{
return list.Count > 0 ? list[0] : null;
}
Expand All @@ -405,17 +407,17 @@ public T Last()
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];
Expand Down
Loading

0 comments on commit 6757295

Please sign in to comment.