Fix handling of try-finally blocks in ReachingDefinitionsVisitor.

This was causing variables to get split incorrectly.

ICSharpCode.Decompiler.Tests/DataFlowTest.cs

@@ -0,0 +1,67 @@
+// Copyright (c) 2016 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.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using ICSharpCode.Decompiler.FlowAnalysis;
+using ICSharpCode.Decompiler.IL;
+using ICSharpCode.Decompiler.TypeSystem;
+using NUnit.Framework;
+
+namespace ICSharpCode.Decompiler.Tests
+{
+	[TestFixture]
+	class DataFlowTest
+	{
+		class RDTest : ReachingDefinitionsVisitor
+		{
+			ILVariable v;
+
+			public RDTest(ILFunction f, ILVariable v) : base(f, _ => true, CancellationToken.None)
+			{
+				this.v = v;
+			}
+
+			protected internal override void VisitTryFinally(TryFinally inst)
+			{
+				Assert.IsTrue(IsPotentiallyUninitialized(state, v));
+				base.VisitTryFinally(inst);
+				Assert.IsTrue(state.IsReachable);
+				Assert.AreEqual(1, GetStores(state, v).Count());
+				Assert.IsFalse(IsPotentiallyUninitialized(state, v));
+			}
+		}
+
+		[Test]
+		public void TryFinallyWithAssignmentInFinally()
+		{
+			ILVariable v = new ILVariable(VariableKind.Local, SpecialType.UnknownType, 0);
+			ILFunction f = new ILFunction(null, new TryFinally(
+				new Nop(),
+				new StLoc(v, new LdcI4(0))
+			));
+			f.AddRef();
+			f.Variables.Add(v);
+			f.Body.AcceptVisitor(new RDTest(f, v));
+		}
+	}
+}

ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj

@@ -49,6 +49,7 @@
   </ItemGroup>
 
   <ItemGroup>
+    <Compile Include="DataFlowTest.cs" />
     <Compile Include="Helpers\CodeAssert.cs" />
     <Compile Include="Helpers\SdkUtility.cs" />
     <Compile Include="Helpers\RemoveCompilerAttribute.cs" />

ICSharpCode.Decompiler/FlowAnalysis/DataFlowVisitor.cs

@@ -110,23 +110,18 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 		/// <code>this.isReachable |= incomingState.isReachable;</code>
 		/// </example>
 		void JoinWith(Self incomingState);
-		
+
 		/// <summary>
-		/// The meet operation.
+		/// A special operation to merge the end-state of the finally-block with the end state of
+		/// a branch leaving the try-block.
 		/// 
-		/// If possible, this method sets <c>this</c> to the greatest state that is smaller than (or equal to)
-		/// both input states.
-		/// At a minimum, meeting with an unreachable state must result in an unreachable state.
+		/// If either input state is unreachable, this call must result in an unreachable state.
 		/// </summary>
-		/// <remarks>
-		/// <c>MeetWith()</c> is used when control flow passes out of a try-finally construct: the endpoint of the try-finally
-		/// is reachable only if both the endpoint of the <c>try</c> and the endpoint of the <c>finally</c> blocks are reachable.
-		/// </remarks>
 		/// <example>
-		/// The simple state "<c>bool isReachable</c>", would implement <c>MeetWith</c> as:
-		/// <code>this.isReachable &amp;= incomingState.isReachable;</code>
+		/// The simple state "<c>bool isReachable</c>", would implement <c>TriggerFinally</c> as:
+		/// <code>this.isReachable &amp;= finallyState.isReachable;</code>
 		/// </example>
-		void MeetWith(Self incomingState);
+		void TriggerFinally(Self finallyState);
 		
 		/// <summary>
 		/// Gets whether this is the bottom state.
@@ -400,35 +395,56 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			DebugEndPoint(container);
 			workLists.Remove(container);
 		}
-		
+
+		readonly List<(IBranchOrLeaveInstruction, State)> branchesTriggeringFinally = new List<(IBranchOrLeaveInstruction, State)>();
+
 		protected internal override void VisitBranch(Branch inst)
+		{
+			if (inst.TriggersFinallyBlock) {
+				Debug.Assert(state.LessThanOrEqual(currentStateOnException));
+				branchesTriggeringFinally.Add((inst, state.Clone()));
+			} else {
+				MergeBranchStateIntoTargetBlock(inst, state);
+			}
+			MarkUnreachable();
+		}
+
+		void MergeBranchStateIntoTargetBlock(Branch inst, State branchState)
 		{
 			var targetBlock = inst.TargetBlock;
 			var targetState = GetBlockInputState(targetBlock);
-			if (!state.LessThanOrEqual(targetState)) {
-				targetState.JoinWith(state);
-				
+			if (!branchState.LessThanOrEqual(targetState)) {
+				targetState.JoinWith(branchState);
+
 				BlockContainer container = (BlockContainer)targetBlock.Parent;
 				workLists[container].Add(targetBlock.ChildIndex);
 			}
-			MarkUnreachable();
 		}
 		
 		protected internal override void VisitLeave(Leave inst)
 		{
 			inst.Value.AcceptVisitor(this);
-			State targetState;
-			if (stateOnLeave.TryGetValue(inst.TargetContainer, out targetState)) {
-				targetState.JoinWith(state);
+			if (inst.TriggersFinallyBlock) {
+				Debug.Assert(state.LessThanOrEqual(currentStateOnException));
+				branchesTriggeringFinally.Add((inst, state.Clone()));
+			} else {
+				MergeBranchStateIntoStateOnLeave(inst, state);
+			}
+			MarkUnreachable();
+		}
+
+		void MergeBranchStateIntoStateOnLeave(Leave inst, State branchState)
+		{
+			if (stateOnLeave.TryGetValue(inst.TargetContainer, out State targetState)) {
+				targetState.JoinWith(branchState);
 			} else {
-				stateOnLeave.Add(inst.TargetContainer, state.Clone());
+				stateOnLeave.Add(inst.TargetContainer, branchState.Clone());
 			}
 			// Note: We don't have to put the block container onto the work queue,
 			// because it's an ancestor of the Leave instruction, and hence
 			// we are currently somewhere within the VisitBlockContainer() call.
-			MarkUnreachable();
 		}
-		
+
 		protected internal override void VisitThrow(Throw inst)
 		{
 			inst.Argument.AcceptVisitor(this);
@@ -512,22 +528,57 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 		{
 			throw new NotSupportedException();
 		}
-		
+
 		protected internal override void VisitTryFinally(TryFinally inst)
 		{
 			DebugStartPoint(inst);
+			int branchesTriggeringFinallyOldCount = branchesTriggeringFinally.Count;
 			// At first, handle 'try { .. } finally { .. }' like 'try { .. } catch {} .. if (?) rethrow; }'
 			State onException = HandleTryBlock(inst);
 			State onSuccess = state.Clone();
 			state.JoinWith(onException);
 			inst.FinallyBlock.AcceptVisitor(this);
-			PropagateStateOnException();
-			// Use MeetWith() to ensure points after the try-finally are reachable only if both the
+			//PropagateStateOnException(); // rethrow the exception after the finally block -- should be redundant
+			Debug.Assert(state.LessThanOrEqual(currentStateOnException));
+
+			ProcessBranchesLeavingTryFinally(inst, branchesTriggeringFinallyOldCount);
+
+			// Use TriggerFinally() to ensure points after the try-finally are reachable only if both the
 			// try and the finally endpoints are reachable.
-			state.MeetWith(onSuccess);
+			onSuccess.TriggerFinally(state);
+			state = onSuccess;
 			DebugEndPoint(inst);
 		}
 
+		/// <summary>
+		/// Process branches leaving the try-finally,
+		///  * Calls TriggerFinally() on each branchesTriggeringFinally
+		///  * Removes entries from branchesTriggeringFinally if they won't trigger additional finally blocks.
+		///  * After all finallies are applied, the branch state is merged into the target block.
+		/// </summary>
+		void ProcessBranchesLeavingTryFinally(TryFinally tryFinally, int branchesTriggeringFinallyOldCount)
+		{
+			int outPos = branchesTriggeringFinallyOldCount;
+			for (int i = branchesTriggeringFinallyOldCount; i < branchesTriggeringFinally.Count; ++i) {
+				var (branch, stateOnBranch) = branchesTriggeringFinally[i];
+				Debug.Assert(((ILInstruction)branch).IsDescendantOf(tryFinally));
+				Debug.Assert(tryFinally.IsDescendantOf(branch.TargetContainer));
+				stateOnBranch.TriggerFinally(state);
+				bool triggersAnotherFinally = Branch.GetExecutesFinallyBlock(tryFinally, branch.TargetContainer);
+				if (triggersAnotherFinally) {
+					branchesTriggeringFinally[outPos++] = (branch, stateOnBranch);
+				} else {
+					// Merge state into target block.
+					if (branch is Leave leave) {
+						MergeBranchStateIntoStateOnLeave((Leave)branch, stateOnBranch);
+					} else {
+						MergeBranchStateIntoTargetBlock((Branch)branch, stateOnBranch);
+					}
+				}
+			}
+			branchesTriggeringFinally.RemoveRange(outPos, branchesTriggeringFinally.Count - outPos);
+		}
+
 		protected internal override void VisitTryFault(TryFault inst)
 		{
 			DebugStartPoint(inst);
@@ -537,8 +588,9 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			State onSuccess = state;
 			state = onException;
 			inst.FaultBlock.AcceptVisitor(this);
-			PropagateStateOnException(); // rethrow the exception after the fault block
-			
+			//PropagateStateOnException(); // rethrow the exception after the fault block
+			Debug.Assert(state.LessThanOrEqual(currentStateOnException));
+
 			// try-fault exits normally only if no exception occurred
 			state = onSuccess;
 			DebugEndPoint(inst);
@@ -579,5 +631,10 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			inst.Value.AcceptVisitor(this);
 			DebugEndPoint(inst);
 		}
+
+		protected internal override void VisitILFunction(ILFunction function)
+		{
+			throw new NotImplementedException();
+		}
 	}
 }

ICSharpCode.Decompiler/FlowAnalysis/DefiniteAssignmentVisitor.cs

@@ -37,8 +37,9 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			/// <summary>
 			/// bits[i]: There is a code path from the entry point to this state's position
 			///          that does not write to function.Variables[i].
+			///          (i.e. the variable is not definitely assigned at the state's position)
 			/// 
-			/// Initial state: all bits set
+			/// Initial state: all bits set = nothing is definitely assigned
 			/// Bottom state: all bits clear
 			/// </summary>
 			readonly BitSet bits;
@@ -77,9 +78,20 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 				bits.UnionWith(incomingState.bits);
 			}
 
-			public void MeetWith(State incomingState)
+			public void TriggerFinally(State finallyState)
 			{
-				bits.IntersectWith(incomingState.bits);
+				// If there is no path to the end of the try-block that leaves a variable v
+				// uninitialized, then there is no such path to the end of the whole try-finally either.
+				// (the try-finally cannot complete successfully unless the try block does the same)
+				// ==> any bits that are false in this.state must be false in the output state.
+
+				// Or said otherwise: a variable is definitely assigned after try-finally if it is
+				// definitely assigned in either the try or the finally block.
+				// Given that the bits are the opposite of definite assignment, this gives us:
+				//    !outputBits[i] == !bits[i] || !finallyState.bits[i].
+				// and thus:
+				//    outputBits[i] == bits[i] && finallyState.bits[i].
+				bits.IntersectWith(finallyState.bits);
 			}
 
 			public void ReplaceWithBottom()

ICSharpCode.Decompiler/FlowAnalysis/ReachingDefinitionsVisitor.cs

@@ -91,8 +91,8 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			/// 
 			/// Reaching store bit (bit si, where <c>allStores[si] != null</c>):
 			///     There is a code path from the entry point to this state's position
-			///     that passes through through <c>allStores[i]</c> and does not pass through another
-			///     store to <c>allStores[i].Variable</c>.
+			///     that passes through through <c>allStores[si]</c> and does not pass through another
+			///     store to <c>allStores[si].Variable</c>.
 			/// 
 			/// The indices for a variable's reaching store bits are between <c>firstStoreIndexForVariable[v.IndexInScope]</c>
 			/// to <c>firstStoreIndexForVariable[v.IndexInScope + 1]</c> (both endpoints exclusive!).
@@ -128,17 +128,29 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			public void JoinWith(State incomingState)
 			{
 				// When control flow is joined together, we can simply union our bitsets.
-				// (joined node is reachable iff either input is reachable)
+				// (a store is reachable iff it is reachable through either incoming path)
 				bits.UnionWith(incomingState.bits);
 			}
 			
-			public void MeetWith(State incomingState)
+			public void TriggerFinally(State finallyState)
 			{
-				// At the end of a try-finally construct, we intersect the try-bitset
-				// with the finally-bitset
-				// (the try-finally-endpoint is reachable if both the try-block-endpoint and
-				// the finally-block-endpoint are reachable)
-				bits.IntersectWith(incomingState.bits);
+				// Some cases to consider:
+				//   try { v = 1; } finally { v = 2; }
+				//     => only the store 2 is visible after the try-finally
+				//   v = 1; try { v = 2; } finally { }
+				//     => both stores are visible after the try-finally
+				// In general, we're looking for the post-state of the finally-block
+				// assume the finally-block was entered without throwing an exception.
+				// But we don't have that information (it would require analyzing the finally block twice),
+				// so the next best thing is to approximate it by just keeping the state after the finally
+				// (i.e. doing nothing at all).
+				// However, the DataFlowVisitor requires us to return bottom if the end-state of the
+				// try-block was unreachable, so let's so at least that.
+				// (note that in principle we could just AND the reachable and uninitialized bits,
+				//  but we don't have a good solution for the normal store bits)
+				if (IsReachable) {
+					ReplaceWith(finallyState);
+				}
 			}
 			
 			public bool IsBottom {
@@ -148,7 +160,7 @@ namespace ICSharpCode.Decompiler.FlowAnalysis
 			public void ReplaceWithBottom()
 			{
 				// We need to clear all bits, not just ReachableBit, so that
-				// the bottom state behaves as expected in joins/meets.
+				// the bottom state behaves as expected in joins.
 				bits.ClearAll();
 			}
 			

ICSharpCode.Decompiler/IL/ControlFlow/AsyncAwaitDecompiler.cs

@@ -76,6 +76,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 
 		// These fields are set by AnalyzeStateMachine():
 		int smallestAwaiterVarIndex;
+		HashSet<Leave> moveNextLeaves = new HashSet<Leave>();
 
 		// For each block containing an 'await', stores the awaiter variable, and the field storing the awaiter
 		// across the yield point.
@@ -89,6 +90,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			fieldToParameterMap.Clear();
 			cachedFieldToParameterMap.Clear();
 			awaitBlocks.Clear();
+			moveNextLeaves.Clear();
 			if (!MatchTaskCreationPattern(function))
 				return;
 			try {
@@ -99,7 +101,9 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			}
 
 			InlineBodyOfMoveNext(function);
+			function.CheckInvariant(ILPhase.InAsyncAwait);
 			CleanUpBodyOfMoveNext(function);
+			function.CheckInvariant(ILPhase.InAsyncAwait);
 
 			AnalyzeStateMachine(function);
 			DetectAwaitPattern(function);
@@ -469,6 +473,12 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 					});
 				}
 			}
+			foreach (var leave in function.Descendants.OfType<Leave>()) {
+				if (leave.TargetContainer == moveNextFunction.Body) {
+					leave.TargetContainer = (BlockContainer)function.Body;
+					moveNextLeaves.Add(leave);
+				}
+			}
 			function.Variables.AddRange(function.Descendants.OfType<IInstructionWithVariableOperand>().Select(inst => inst.Variable).Distinct());
 			function.Variables.RemoveDead();
 			function.Variables.AddRange(fieldToParameterMap.Values);
@@ -478,7 +488,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 		{
 			context.Step("FinalizeInlineMoveNext()", function);
 			foreach (var leave in function.Descendants.OfType<Leave>()) {
-				if (leave.TargetContainer == moveNextFunction.Body) {
+				if (moveNextLeaves.Contains(leave)) {
 					leave.ReplaceWith(new InvalidBranch {
 						Message = "leave MoveNext - await not detected correctly",
 						ILRange = leave.ILRange
@@ -508,7 +518,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 
 				foreach (var block in container.Blocks) {
 					context.CancellationToken.ThrowIfCancellationRequested();
-					if (block.Instructions.Last().MatchLeave((BlockContainer)moveNextFunction.Body)) {
+					if (block.Instructions.Last() is Leave leave && moveNextLeaves.Contains(leave)) {
 						// This is likely an 'await' block
 						if (AnalyzeAwaitBlock(block, out var awaiterVar, out var awaiterField, out var state)) {
 							block.Instructions.Add(new Await(new LdLoca(awaiterVar)));

ICSharpCode.Decompiler/IL/Instructions/Branch.cs

@@ -27,7 +27,7 @@ namespace ICSharpCode.Decompiler.IL
 	/// <remarks>
 	/// When jumping to the entrypoint of the current block container, the branch represents a <c>continue</c> statement.
 	/// </remarks>
-	partial class Branch : SimpleInstruction
+	partial class Branch : SimpleInstruction, IBranchOrLeaveInstruction
 	{
 		readonly int targetILOffset;
 		Block targetBlock;
@@ -95,7 +95,25 @@ namespace ICSharpCode.Decompiler.IL
 		public string TargetLabel {
 			get { return targetBlock != null ? targetBlock.Label : CecilExtensions.OffsetToString(TargetILOffset); }
 		}
-		
+
+		/// <summary>
+		/// Gets whether this branch executes at least one finally block before jumping to the target block.
+		/// </summary>
+		public bool TriggersFinallyBlock {
+			get {
+				return GetExecutesFinallyBlock(this, TargetContainer);
+			}
+		}
+
+		internal static bool GetExecutesFinallyBlock(ILInstruction inst, BlockContainer container)
+		{
+			for (; inst != container; inst = inst.Parent) {
+				if (inst.Parent is TryFinally && inst.SlotInfo == TryFinally.TryBlockSlot)
+					return true;
+			}
+			return false;
+		}
+
 		internal override void CheckInvariant(ILPhase phase)
 		{
 			base.CheckInvariant(phase);
@@ -113,4 +131,9 @@ namespace ICSharpCode.Decompiler.IL
 			output.WriteReference(TargetLabel, (object)targetBlock ?? TargetILOffset, isLocal: true);
 		}
 	}
+
+	interface IBranchOrLeaveInstruction
+	{
+		BlockContainer TargetContainer { get; }
+	}
 }

ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs

@@ -37,6 +37,11 @@ namespace ICSharpCode.Decompiler.IL
 		/// The usual invariants are established.
 		/// </summary>
 		Normal,
+		/// <summary>
+		/// Special phase within the async-await decompiler, where a few selected invariants
+		/// are temporarily suspended. (see Leave.CheckInvariant)
+		/// </summary>
+		InAsyncAwait
 	}
 	
 	/// <summary>

ICSharpCode.Decompiler/IL/Instructions/Leave.cs

@@ -30,7 +30,7 @@ namespace ICSharpCode.Decompiler.IL
 	/// Phase-2 execution removes PopCount elements from the evaluation stack
 	/// and jumps to the target block.
 	/// </remarks>
-	partial class Leave : ILInstruction
+	partial class Leave : ILInstruction, IBranchOrLeaveInstruction
 	{
 		BlockContainer targetContainer;
 		
@@ -93,11 +93,20 @@ namespace ICSharpCode.Decompiler.IL
 			get { return targetContainer?.Parent is ILFunction; }
 		}
 
+		/// <summary>
+		/// Gets whether this branch executes at least one finally block before jumping to the end of the target block container.
+		/// </summary>
+		public bool TriggersFinallyBlock {
+			get {
+				return Branch.GetExecutesFinallyBlock(this, TargetContainer);
+			}
+		}
+
 		internal override void CheckInvariant(ILPhase phase)
 		{
 			base.CheckInvariant(phase);
 			Debug.Assert(phase <= ILPhase.InILReader || this.IsDescendantOf(targetContainer));
-			Debug.Assert(phase <= ILPhase.InILReader || value.ResultType == targetContainer.ResultType);
+			Debug.Assert(phase <= ILPhase.InILReader || phase == ILPhase.InAsyncAwait || value.ResultType == targetContainer.ResultType);
 		}
 		
 		public override void WriteTo(ITextOutput output, ILAstWritingOptions options)