More clearly document block semantics, in particular regarding phase-1 and phase-2 execution.

Distinguish between phase-1 and phase-2 evaluation stack access in InstructionFlags.

ICSharpCode.Decompiler/IL/ILVariable.cs

@@ -30,7 +30,13 @@ namespace ICSharpCode.Decompiler.IL
 {
 	public enum VariableKind
 	{
+		/// <summary>
+		/// A local variable.
+		/// </summary>
 		Local,
+		/// <summary>
+		/// A parameter.
+		/// </summary>
 		Parameter,
 		/// <summary>
 		/// The 'this' parameter
@@ -46,6 +52,10 @@ namespace ICSharpCode.Decompiler.IL
 	{
 		public readonly VariableKind Kind;
 		public readonly IType Type;
+		
+		/// <summary>
+		/// The index of the local variable or parameter (depending on Kind)
+		/// </summary>
 		public readonly int Index;
 		
 		public string Name { get; set; }
@@ -53,16 +63,25 @@ namespace ICSharpCode.Decompiler.IL
 		/// <summary>
 		/// Number of ldloc instructions referencing this variable.
 		/// </summary>
+		/// <remarks>
+		/// This variable is automatically updated when adding/removing ldloc instructions from the ILAst.
+		/// </remarks>
 		public int LoadCount;
 		
 		/// <summary>
 		/// Number of stloc instructions referencing this variable.
 		/// </summary>
+		/// <remarks>
+		/// This variable is automatically updated when adding/removing stloc instructions from the ILAst.
+		/// </remarks>
 		public int StoreCount;
 		
 		/// <summary>
 		/// Number of ldloca instructions referencing this variable.
 		/// </summary>
+		/// <remarks>
+		/// This variable is automatically updated when adding/removing ldloca instructions from the ILAst.
+		/// </remarks>
 		public int AddressCount;
 		
 		public ILVariable(VariableKind kind, IType type, int index)

ICSharpCode.Decompiler/IL/InstructionFlags.cs

@@ -29,18 +29,25 @@ namespace ICSharpCode.Decompiler.IL
 	{
 		None = 0,
 		/// <summary>
-		/// The instruction may pop from the evaluation stack.
+		/// Phase-1 execution of this instruction may pop from the evaluation stack.
 		/// </summary>
 		MayPop   = 0x01,
+		/// <summary>
+		/// Phase-1 execution of this instruction may peek at the top-most element of the evaluation stack.
+		/// If MayPop is also set, this flag refers to the top-most element after an arbitrary number of pops.
+		/// </summary>
 		MayPeek  = 0x02,
 		/// <summary>
-		/// The instruction may throw an exception.
+		/// Phase-2 execution of this instruction may read the evaluation stack.
+		/// This is not set for instructions that access the stack only in phase-1.
 		/// </summary>
-		MayThrow = 0x04,
+		MayReadEvaluationStack = 0x04,
 		/// <summary>
-		/// The instruction may exit with a branch or return.
+		/// Phase-2 execution of this instruction may modify the evaluation stack.
+		/// This is not set for instructions that access the stack only in phase-1.
 		/// </summary>
-		MayBranch = 0x08,
+		MayWriteEvaluationStack = 0x08,
+		
 		/// <summary>
 		/// The instruction may read from local variables.
 		/// </summary>
@@ -48,15 +55,28 @@ namespace ICSharpCode.Decompiler.IL
 		/// <summary>
 		/// The instruction may write to local variables.
 		/// </summary>
+		/// <remarks>
+		/// This flag is not set for indirect write to local variables through pointers.
+		/// Ensure you also check the SideEffect flag.
+		/// </remarks>
 		MayWriteLocals = 0x20,
 		/// <summary>
 		/// The instruction may have side effects, such as accessing heap memory,
 		/// performing system calls, writing to local variables through pointers, etc.
 		/// </summary>
 		SideEffect = 0x40,
+		
+		/// <summary>
+		/// The instruction may throw an exception.
+		/// </summary>
+		MayThrow = 0x100,
+		/// <summary>
+		/// The instruction may exit with a branch or return.
+		/// </summary>
+		MayBranch = 0x200,
 		/// <summary>
 		/// The instruction performs unconditional control flow, so that its endpoint is unreachable.
 		/// </summary>
-		EndPointUnreachable = 0x80,
+		EndPointUnreachable = 0x400,
 	}
 }

ICSharpCode.Decompiler/IL/Instructions.cs

@@ -70,9 +70,9 @@ namespace ICSharpCode.Decompiler.IL
 		EndFinally,
 		/// <summary>If statement / conditional expression. <c>if (condition) trueExpr else falseExpr</c></summary>
 		IfInstruction,
-		/// <summary>Try-catch statement</summary>
+		/// <summary>Try-catch statement.</summary>
 		TryCatch,
-		/// <summary>Catch handler within a try-catch statement</summary>
+		/// <summary>Catch handler within a try-catch statement.</summary>
 		TryCatchHandler,
 		/// <summary>Try-finally statement</summary>
 		TryFinally,
@@ -606,7 +606,7 @@ namespace ICSharpCode.Decompiler.IL
 		}
 	}
 
-	/// <summary>Try-catch statement</summary>
+	/// <summary>Try-catch statement.</summary>
 	public sealed partial class TryCatch : TryInstruction
 	{
 
@@ -616,7 +616,7 @@ namespace ICSharpCode.Decompiler.IL
 		}
 	}
 
-	/// <summary>Catch handler within a try-catch statement</summary>
+	/// <summary>Catch handler within a try-catch statement.</summary>
 	public sealed partial class TryCatchHandler : ILInstruction
 	{
 		public TryCatchHandler(ILInstruction filter, ILInstruction body, ILVariable variable) : base(OpCode.TryCatchHandler)
@@ -656,11 +656,6 @@ namespace ICSharpCode.Decompiler.IL
 		readonly ILVariable variable;
 		/// <summary>Returns the variable operand.</summary>
 		public ILVariable Variable { get { return variable; } }
-		public override StackType ResultType { get { return StackType.Void; } }
-		protected override InstructionFlags ComputeFlags()
-		{
-			return filter.Flags | body.Flags;
-		}
 		public override T AcceptVisitor<T>(ILVisitor<T> visitor)
 		{
 			return visitor.VisitTryCatchHandler(this);

ICSharpCode.Decompiler/IL/Instructions.tt

@@ -71,13 +71,13 @@
 				new ChildInfo("trueInst"),
 				new ChildInfo("falseInst"),
 			}), CustomConstructor, CustomComputeFlags, CustomWriteTo),
-		new OpCode("try.catch", "Try-catch statement",
+		new OpCode("try.catch", "Try-catch statement.",
 			BaseClass("TryInstruction"), CustomConstructor, CustomComputeFlags, CustomWriteTo),
-		new OpCode("try.catch.handler", "Catch handler within a try-catch statement",
+		new OpCode("try.catch.handler", "Catch handler within a try-catch statement.",
 			CustomChildren(new [] {
 				new ChildInfo("filter") { IsArgument = true },
 				new ChildInfo("body"),
-			}), HasVariableOperand, CustomWriteTo, ResultType("Void")),
+			}), HasVariableOperand, CustomWriteTo, CustomComputeFlags),
 		new OpCode("try.finally", "Try-finally statement",
 			BaseClass("TryInstruction"), CustomConstructor, CustomWriteTo, CustomComputeFlags),
 		new OpCode("try.fault", "Try-fault statement",
@@ -200,13 +200,13 @@ namespace ICSharpCode.Decompiler.IL
 	public enum OpCode
 	{
 <#	foreach (OpCode opCode in opCodes) { #>
-		/// <summary><#=opCode.Description#></summary>
+		/// <summary><#=opCode.Description.Replace("\n", "\n\t\t/// ")#></summary>
 		<#=opCode.Name#>,
 <#  } #>
 	}
 
 <#	foreach (OpCode opCode in baseClasses.Concat(opCodes)) { #>
-	/// <summary><#=opCode.Description#></summary>
+	/// <summary><#=opCode.Description.Replace("\n", "\n\t/// ")#></summary>
 	<#=opCode.ClassModifiers#> partial class <#=opCode.Name#> : <#=string.Join(", ", new[]{opCode.BaseClass}.Concat(opCode.Interfaces))#>
 	{
 <#	if (opCode.GenerateConstructor) { #>

ICSharpCode.Decompiler/IL/Instructions/Block.cs

@@ -25,6 +25,45 @@ using System.Threading.Tasks;
 
 namespace ICSharpCode.Decompiler.IL
 {
+	/// <summary>
+	/// A block consists of a list of IL instructions.
+	/// <para>
+	/// Phase-1 execution of a block is a no-op: any peek/pop instructions within the block are ignored at this stage.
+	/// </para>
+	/// <para>
+	/// Phase-2 execution will execute the instructions in order, pseudo-code:
+	/// </para>
+	/// <code>
+	///   foreach (var inst in Instructions) {
+	///     inst.Phase1();
+	///     var result = inst.Phase2();
+	///     if (result != void) evalStack.Push(result);
+	///   }
+	/// </code>
+	/// <para>
+	/// If the execution reaches the end of the block, one element is popped from the evaluation stack and
+	/// is used as the return value of the block.
+	/// This means it is impossible for a block to return void: any block with ResultType void must
+	/// perform an unconditional branch!
+	/// </para>
+	/// TODO: that seems like a bad idea for the purposes of goto-removal, I think we'll want
+	/// separate classes for void-blocks and inline-blocks.
+	/// Or maybe let blocks evaluate to the return value of their last instruction, and use a final pop()
+	/// instruction for inline-block semantics.
+	/// TODO: actually I think it's a good idea to implement a small ILAst interpreter
+	///    public virtual ILInstruction Phase1(InterpreterState state);
+	///    public virtual InterpreterResult ILInstruction Phase2(InterpreterState state);
+	/// It's probably the easiest solution for specifying clear semantics, and
+	/// we might be able to use the interpreter logic for symbolic execution.
+	/// In fact, I think we'll need at least Phase1() in order to implement TransformStackIntoVariablesState()
+	/// without being incorrect about the pop-order in nested blocks.
+	/// </summary>
+	/// <remarks>
+	/// Fun fact: the empty block acts like a phase-2 pop instruction,
+	/// which is a slightly different behavior than the normal phase-1 <see cref="Pop"/> instruction!
+	/// However, this is just of theoretical interest; we currently don't plan to use inline blocks that
+	/// pop elements that they didn't push themselves.
+	/// </remarks>
 	partial class Block : ILInstruction
 	{
 		public readonly InstructionCollection<ILInstruction> Instructions;
@@ -42,6 +81,13 @@ namespace ICSharpCode.Decompiler.IL
 			}
 		}
 		
+		internal override void CheckInvariant()
+		{
+			base.CheckInvariant();
+			// if the end-point isn't unreachable, there's an implicit pop at the end of the block
+			Debug.Assert(this.HasFlag(InstructionFlags.EndPointUnreachable) || this.ResultType != StackType.Void);
+		}
+		
 		/// <summary>
 		/// Gets the name of this block.
 		/// </summary>
@@ -80,18 +126,40 @@ namespace ICSharpCode.Decompiler.IL
 		protected override InstructionFlags ComputeFlags()
 		{
 			var flags = InstructionFlags.None;
-			foreach (var inst in Instructions)
+			foreach (var inst in Instructions) {
 				flags |= inst.Flags;
+				if (inst.ResultType != StackType.Void) {
+					// implicit push
+					flags |= InstructionFlags.MayWriteEvaluationStack;
+				}
+			}
+			// implicit pop at end of block
+			if ((flags & InstructionFlags.EndPointUnreachable) == 0)
+				flags |= InstructionFlags.MayWriteEvaluationStack;
+			return Phase1Boundary(flags);
+		}
+		
+		/// <summary>
+		/// Adjust flags for a phase-1 boundary:
+		/// The MayPop and MayPeek flags are removed and converted into
+		/// MayReadEvaluationStack and/or MayWriteEvaluationStack flags.
+		/// </summary>
+		internal static InstructionFlags Phase1Boundary(InstructionFlags flags)
+		{
+			// Convert phase-1 flags to phase-2 flags
+			if ((flags & InstructionFlags.MayPop) != 0)
+				flags |= InstructionFlags.MayWriteEvaluationStack;
+			if ((flags & (InstructionFlags.MayPeek | InstructionFlags.MayPop)) != 0)
+				flags |= InstructionFlags.MayReadEvaluationStack;
+			// an inline block has no phase-1 effects
+			flags &= ~(InstructionFlags.MayPeek | InstructionFlags.MayPop);
 			return flags;
-			
 		}
 		
 		internal override ILInstruction Inline(InstructionFlags flagsBefore, Stack<ILInstruction> instructionStack, out bool finished)
 		{
-			if (Instructions.Count > 0)
-				Instructions[0] = Instructions[0].Inline(flagsBefore, instructionStack, out finished);
-			else
-				finished = true;
+			// an inline block has no phase-1 effects, so we're immediately done with inlining
+			finished = true;
 			return this;
 		}
 	}

ICSharpCode.Decompiler/IL/Instructions/BlockContainer.cs

@@ -31,6 +31,10 @@ namespace ICSharpCode.Decompiler.IL
 	/// and only branches within this container may reference the blocks in this container.
 	/// That means that viewed from the outside, the block container has a single entry point (but possibly multiple exit points),
 	/// and the same holds for every block within the container.
+	/// 
+	/// If a block within the container falls through to its end point, control flow is transferred to the end point
+	/// of the whole block container. The return value of the block is ignored in this case, the container always
+	/// returns void.
 	/// </summary>
 	partial class BlockContainer : ILInstruction
 	{
@@ -73,13 +77,24 @@ namespace ICSharpCode.Decompiler.IL
 			}
 		}
 
+		internal override void CheckInvariant()
+		{
+			base.CheckInvariant();
+			Debug.Assert(Blocks.Count >= 1);
+		}
+		
 		protected override InstructionFlags ComputeFlags()
 		{
-			var flags = InstructionFlags.None;
+			InstructionFlags flagsInAnyBlock = InstructionFlags.None;
+			InstructionFlags flagsInAllBlocks = ~InstructionFlags.None;
 			foreach (var block in Blocks) {
-				flags |= block.Flags;
+				flagsInAnyBlock |= block.Flags;
+				flagsInAllBlocks &= block.Flags;
 			}
-			return flags;
+			// Return EndPointUnreachable only if no block has a reachable endpoint.
+			// The other flags are combined from all blocks.
+			return (flagsInAnyBlock & ~InstructionFlags.EndPointUnreachable)
+				| (flagsInAllBlocks & InstructionFlags.EndPointUnreachable);
 		}
 		
 		internal override ILInstruction Inline(InstructionFlags flagsBefore, Stack<ILInstruction> instructionStack, out bool finished)

ICSharpCode.Decompiler/IL/Instructions/ILFunction.cs

@@ -57,6 +57,7 @@ namespace ICSharpCode.Decompiler.IL
 		protected override InstructionFlags ComputeFlags()
 		{
 			// Creating a lambda may throw OutOfMemoryException
+			// We intentionally don't propagate any flags from the lambda body!
 			return InstructionFlags.MayThrow;
 		}
 		

ICSharpCode.Decompiler/IL/Instructions/TryInstruction.cs

@@ -18,6 +18,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 
 namespace ICSharpCode.Decompiler.IL
 {
@@ -39,11 +40,20 @@ namespace ICSharpCode.Decompiler.IL
 		
 		internal override ILInstruction Inline(InstructionFlags flagsBefore, Stack<ILInstruction> instructionStack, out bool finished)
 		{
+			// Cannot inline into try instructions because moving code into the try block would change semantics
 			finished = false;
 			return this;
 		}
 	}
 	
+	/// <summary>
+	/// Try-catch statement.
+	/// </summary>
+	/// <remarks>
+	/// The evaluation stack does not need to be empty when entering or leaving a try-catch-block.
+	/// All try or catch blocks with reachable endpoint must produce compatible stacks.
+	/// The return value of the try or catch blocks is ignored, the TryCatch always returns void.
+	/// </remarks>
 	partial class TryCatch : TryInstruction
 	{
 		public readonly InstructionCollection<TryCatchHandler> Handlers;
@@ -93,12 +103,40 @@ namespace ICSharpCode.Decompiler.IL
 		}
 	}
 	
+	/// <summary>
+	/// Catch handler within a try-catch statement.
+	/// 
+	/// When an exception occurs in the try block of the parent try.catch statement, the runtime searches
+	/// the nearest enclosing TryCatchHandler with a matching variable type and
+	/// assigns the exception object to the <see cref="Variable"/>.
+	/// Then, the evaluation stack is cleared and the <see cref="Filter"/> is executed
+	/// (first phase-1 execution, which should be a no-op given the empty stack, then phase-2 execution).
+	/// If the filter evaluates to 0, the exception is not caught and the runtime looks for the next catch handler.
+	/// If the filter evaluates to 1, the stack is unwound, the exception caught and assigned to the <see cref="Variable"/>,
+	/// the evaluation stack is cleared again, and the <see cref="Body"/> is executed (again, phase-1 + phase-2).
+	/// </summary>
 	partial class TryCatchHandler
 	{
 		internal override ILInstruction Inline(InstructionFlags flagsBefore, Stack<ILInstruction> instructionStack, out bool finished)
 		{
-			finished = false;
-			return this;
+			// should never happen as TryCatchHandler only appears within TryCatch instructions
+			throw new InvalidOperationException();
+		}
+		
+		internal override void CheckInvariant()
+		{
+			base.CheckInvariant();
+			Debug.Assert(Parent is TryCatch);
+			Debug.Assert(filter.ResultType == StackType.I4);
+		}
+		
+		public override StackType ResultType {
+			get { return StackType.Void; }
+		}
+		
+		protected override InstructionFlags ComputeFlags()
+		{
+			return Block.Phase1Boundary(filter.Flags | body.Flags);
 		}
 		
 		public override void WriteTo(ITextOutput output)

ICSharpCode.Decompiler/IL/SemanticHelper.cs

@@ -27,13 +27,19 @@ namespace ICSharpCode.Decompiler.IL
 		/// </summary>
 		internal static bool MayReorder(InstructionFlags inst1, InstructionFlags inst2)
 		{
-			// If both instructions perform a non-read-only action, we cannot reorder them
-			if ((inst1 & inst2 & ~(InstructionFlags.MayPeek | InstructionFlags.MayReadLocals)) != 0)
+			// If both instructions perform an impure action, we cannot reorder them
+			const InstructionFlags pureFlags =
+				InstructionFlags.MayPeek
+				| InstructionFlags.MayReadEvaluationStack
+				| InstructionFlags.MayReadLocals;
+			if ((inst1 & inst2 & ~pureFlags) != 0)
 				return false;
 			// We cannot reorder if inst2 might pop what inst1 peeks at
 			if (ConflictingPair(inst1, inst2, InstructionFlags.MayPeek, InstructionFlags.MayPop))
 				return false;
-			if (ConflictingPair(inst1, inst2, InstructionFlags.MayReadLocals, InstructionFlags.MayWriteLocals))
+			if (ConflictingPair(inst1, inst2, InstructionFlags.MayReadEvaluationStack, InstructionFlags.MayWriteEvaluationStack))
+				return false;
+			if (ConflictingPair(inst1, inst2, InstructionFlags.MayReadLocals, InstructionFlags.MayWriteLocals | InstructionFlags.SideEffect))
 				return false;
 			return true;
 		}
@@ -43,6 +49,6 @@ namespace ICSharpCode.Decompiler.IL
 			// if one instruction has the read flag and the other the write flag, that's a conflict
 			return (inst1 & readFlag) != 0 && (inst2 & writeFlag) != 0
 				|| (inst2 & readFlag) != 0 && (inst1 & writeFlag) != 0;
-        }
+		}
 	}
 }

ICSharpCode.Decompiler/IL/TransformingVisitor.cs

@@ -99,7 +99,9 @@ namespace ICSharpCode.Decompiler.IL
 			}
 			FlushInstructionStack(stack, output);
 			if (!(block.Parent is BlockContainer)) {
-				if (output.Count == 1)
+				// We can't unpack an instruction from a block if there's the potential that this changes
+				// the pop-order in the phase 1 evaluation of the parent block.
+				if (output.Count == 1 && !output[0].HasFlag(InstructionFlags.MayPeek | InstructionFlags.MayPop))
 					return output[0];
 			}
 			block.Instructions.ReplaceList(output);