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 @@ public interface IDataFlowState<Self> where Self: IDataFlowState<Self>
 		/// <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 @@ protected internal override void VisitBlockContainer(BlockContainer container)
 			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 @@ protected internal override sealed void VisitTryCatchHandler(TryCatchHandler ins
 		{
 			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 @@ protected internal override void VisitTryFault(TryFault inst)
 			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 @@ protected internal override void VisitYieldReturn(YieldReturn inst)
 			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 @@ public struct State : IDataFlowState<State>
 			/// <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 @@ public void JoinWith(State incomingState)
 				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 @@ public struct State : IDataFlowState<State>
 			/// 
 			/// 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 @@ public void ReplaceWith(State newContent)
 			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 @@ public bool IsBottom {
 			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();
 			}