Ensure that StLoc.Value.ResultType == StLoc.Variable.StackType

This introduce a new explicit conversion (StartGCTracking) when converting unmanaged pointers to managed references.

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

@@ -60,7 +60,7 @@
     <Compile Include="DataFlowTest.cs" />
     <Compile Include="TestCases\Correctness\LINQRaytracer.cs" />
     <Compile Include="TestCases\Correctness\MiniJSON.cs" />
-    <Compile Include="TestCases\ILPretty\Issue959.cs" />
+    <None Include="TestCases\ILPretty\Issue959.cs" />
     <None Include="TestCases\ILPretty\FSharpLoops_Debug.cs" />
     <None Include="TestCases\ILPretty\FSharpLoops_Release.cs" />
     <Compile Include="TestCases\Pretty\DelegateConstruction.cs" />

ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs

@@ -59,6 +59,9 @@ namespace ICSharpCode.Decompiler.CSharp
 	///        the C# type of the resulting expression shall be Nullable{T}, where T is an integer type (as above).
 	///        The C# value shall be null iff the IL-level value evaluates to null, and otherwise the values shall correspond
 	///        as with non-lifted integer operations.
+	///      * If the IL instruction evaluates to a managed reference (Ref) created by starting tracking of an unmanaged reference,
+	///        the C# instruction may evaluate to any integral/enum/pointer type that when converted to pointer type
+	///        is equivalent to the managed reference.
 	///      * Otherwise, the C# type of the resulting expression shall match the IL stack type,
 	///        and the evaluated values shall be the same.
 	/// </remarks>
@@ -143,6 +146,8 @@ namespace ICSharpCode.Decompiler.CSharp
 					} else {
 						Debug.Assert(underlying.GetStackType() == liftable.UnderlyingResultType);
 					}
+				} else if (inst.ResultType == StackType.Ref) {
+					Debug.Assert(cexpr.Type.GetStackType() == StackType.Ref || cexpr.Type.GetStackType().IsIntegerType());
 				} else {
 					Debug.Assert(cexpr.Type.GetStackType() == inst.ResultType);
 				}
@@ -1220,6 +1225,11 @@ namespace ICSharpCode.Decompiler.CSharp
 			}
 			
 			switch (inst.Kind) {
+				case ConversionKind.StartGCTracking:
+					// A "start gc tracking" conversion is inserted in the ILAst whenever
+					// some instruction expects a managed pointer, but we pass an unmanaged pointer.
+					// We'll leave the C#-level conversion (from T* to ref T) to the consumer that expects the managed pointer.
+					return arg;
 				case ConversionKind.StopGCTracking:
 					if (inputType.Kind == TypeKind.ByReference) {
 						// cast to corresponding pointer type:

ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj

@@ -278,6 +278,7 @@
     <Compile Include="DotNetCore\UniversalAssemblyResolver.cs" />
     <Compile Include="DotNetCore\UnresolvedAssemblyNameReference.cs" />
     <Compile Include="IL\ILAstWritingOptions.cs" />
+    <Compile Include="IL\Instructions\StLoc.cs" />
     <Compile Include="IL\SequencePoint.cs" />
     <Compile Include="IL\Instructions\CallIndirect.cs" />
     <Compile Include="IL\Instructions\DefaultValue.cs" />

ICSharpCode.Decompiler/IL/ControlFlow/AsyncAwaitDecompiler.cs

@@ -903,7 +903,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				}
 			}
 			// if there's any remaining loads (there shouldn't be), replace them with the constant 1
-			foreach (LdLoc load in doFinallyBodies.LoadInstructions) {
+			foreach (LdLoc load in doFinallyBodies.LoadInstructions.ToArray()) {
 				load.ReplaceWith(new LdcI4(1) { ILRange = load.ILRange });
 			}
 			context.StepEndGroup(keepIfEmpty: true);
@@ -914,7 +914,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			if (body == null)
 				return null;
 			Block entryPoint = body.EntryPoint;
-			while (entryPoint.Instructions[0].MatchBranch(out var targetBlock) && targetBlock.IncomingEdgeCount == 1) {
+			while (entryPoint.Instructions[0].MatchBranch(out var targetBlock) && targetBlock.IncomingEdgeCount == 1 && targetBlock.Parent == body) {
 				entryPoint = targetBlock;
 			}
 			return entryPoint;

ICSharpCode.Decompiler/IL/ControlFlow/DetectPinnedRegions.cs

@@ -313,8 +313,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 
 		static bool IsNullOrZero(ILInstruction inst)
 		{
-			var conv = inst as Conv;
-			if (conv != null) {
+			while (inst is Conv conv) {
 				inst = conv.Argument;
 			}
 			return inst.MatchLdcI4(0) || inst.MatchLdNull();

ICSharpCode.Decompiler/IL/ILReader.cs

@@ -1015,21 +1015,29 @@ namespace ICSharpCode.Decompiler.IL
 		{
 			ILInstruction inst = Pop();
 			if (expectedType != inst.ResultType) {
-				if (expectedType == StackType.I && inst.ResultType == StackType.I4) {
+				if (inst is InvalidExpression) {
+					((InvalidExpression)inst).ExpectedResultType = expectedType;
+				} else if (expectedType == StackType.I && inst.ResultType == StackType.I4) {
 					// IL allows implicit I4->I conversions
 					inst = new Conv(inst, PrimitiveType.I, false, Sign.None);
 				} else if (expectedType == StackType.I4 && inst.ResultType == StackType.I) {
 					// C++/CLI also sometimes implicitly converts in the other direction:
 					inst = new Conv(inst, PrimitiveType.I4, false, Sign.None);
-				} else if (expectedType == StackType.Ref && inst.ResultType == StackType.I) {
-					// implicitly start GC tracking
 				} else if (expectedType == StackType.I && inst.ResultType == StackType.Ref) {
 					// Implicitly stop GC tracking; this occurs when passing the result of 'ldloca' or 'ldsflda'
 					// to a method expecting a native pointer.
-				} else if (inst is InvalidExpression) {
-					((InvalidExpression)inst).ExpectedResultType = expectedType;
+					inst = new Conv(inst, PrimitiveType.I, false, Sign.None);
+				} else if (expectedType == StackType.Ref) {
+					// implicitly start GC tracking
+					if (!inst.ResultType.IsIntegerType()) {
+						// We also handle the invalid to-ref cases here because the else case
+						// below uses expectedType.ToKnownTypeCode(), which doesn't work for Ref.
+						Warn($"Expected {expectedType}, but got {inst.ResultType}");
+					}
+					inst = new Conv(inst, PrimitiveType.Ref, false, Sign.None);
 				} else {
 					Warn($"Expected {expectedType}, but got {inst.ResultType}");
+					inst = new Conv(inst, expectedType.ToKnownTypeCode().ToPrimitiveType(), false, Sign.None);
 				}
 			}
 			return inst;

ICSharpCode.Decompiler/IL/ILTypeExtensions.cs

@@ -41,6 +41,7 @@ namespace ICSharpCode.Decompiler.IL
 				case MetadataType.IntPtr:
 				case MetadataType.UIntPtr:
 				case MetadataType.Pointer:
+				case MetadataType.FunctionPointer:
 					return StackType.I;
 				case MetadataType.Single:
 				case MetadataType.Double:
@@ -82,10 +83,10 @@ namespace ICSharpCode.Decompiler.IL
 		}
 		
 		/// <summary>
-		/// Gets whether the type is a small integer type.
-		/// Small integer types are:
-		/// * bool, sbyte, byte, char, short, ushort
-		/// * any enums that have a small integer type as underlying type
+		/// Gets the size in bytes of the primitive type.
+		/// 
+		/// Returns 0 for non-primitive types.
+		/// Returns <c>NativeIntSize</c> for native int/references.
 		/// </summary>
 		public static int GetSize(this PrimitiveType type)
 		{
@@ -106,6 +107,7 @@ namespace ICSharpCode.Decompiler.IL
 					return 8;
 				case PrimitiveType.I:
 				case PrimitiveType.U:
+				case PrimitiveType.Ref:
 					return TypeUtils.NativeIntSize;
 				default:
 					return 0;

ICSharpCode.Decompiler/IL/Instructions.cs

@@ -2264,12 +2264,6 @@ namespace ICSharpCode.Decompiler.IL
 			base.Disconnected();
 		}
 		
-		internal override void CheckInvariant(ILPhase phase)
-		{
-			base.CheckInvariant(phase);
-			Debug.Assert(phase <= ILPhase.InILReader || this.IsDescendantOf(variable.Function));
-			Debug.Assert(phase <= ILPhase.InILReader || variable.Function.Variables[variable.IndexInFunction] == variable);
-		}
 		public static readonly SlotInfo ValueSlot = new SlotInfo("Value", canInlineInto: true);
 		ILInstruction value;
 		public ILInstruction Value {
@@ -2317,8 +2311,6 @@ namespace ICSharpCode.Decompiler.IL
 			clone.Value = this.value.Clone();
 			return clone;
 		}
-		/// <summary>If true, this stloc represents a stack type adjustment. This field is only used in ILReader and BlockBuilder, and should be ignored by ILAst transforms.</summary>
-		internal bool IsStackAdjustment;
 		public override StackType ResultType { get { return variable.StackType; } }
 		protected override InstructionFlags ComputeFlags()
 		{

ICSharpCode.Decompiler/IL/Instructions.tt

@@ -53,7 +53,7 @@
 			MatchCondition("Patterns.ListMatch.DoMatch(this.Blocks, o.Blocks, ref match)")),
 		new OpCode("Block", "A block of IL instructions.",
 			CustomConstructor, CustomVariableName("block"),
-			MatchCondition("this.Type == o.Type"),
+			MatchCondition("this.Kind == o.Kind"),
 			MatchCondition("Patterns.ListMatch.DoMatch(this.Instructions, o.Instructions, ref match)"),
 			MatchCondition("this.FinalInstruction.PerformMatch(o.FinalInstruction, ref match)")),
 		new OpCode("PinnedRegion", "A region where a pinned variable is used (initial representation of future fixed statement).",
@@ -152,10 +152,7 @@
 			CustomClassName("LdLoca"), NoArguments, ResultType("Ref"), HasVariableOperand("Address")),
 		new OpCode("stloc", "Stores a value into a local variable. (IL: starg/stloc)" + Environment.NewLine
 				+ "Evaluates to the value that was stored (for byte/short variables: evaluates to the truncated value, sign/zero extended back to I4 based on variable.Type.GetSign())",
-			CustomClassName("StLoc"), HasVariableOperand("Store"), CustomArguments("value"),
-			AdditionalMember(
-				"/// <summary>If true, this stloc represents a stack type adjustment. This field is only used in ILReader and BlockBuilder, and should be ignored by ILAst transforms.</summary>" + Environment.NewLine
-				+ "internal bool IsStackAdjustment;"),
+			CustomClassName("StLoc"), HasVariableOperand("Store", generateCheckInvariant: false), CustomArguments("value"),
 			ResultType("variable.StackType")),
 		new OpCode("addressof", "Stores the value into an anonymous temporary variable, and returns the address of that variable.",
 			CustomClassName("AddressOf"), CustomArguments("value"), ResultType("Ref")),

ICSharpCode.Decompiler/IL/Instructions/Conv.cs

@@ -75,7 +75,11 @@ namespace ICSharpCode.Decompiler.IL
 		/// <summary>
 		/// Used to convert managed references/objects to unmanaged pointers.
 		/// </summary>
-		StopGCTracking
+		StopGCTracking,
+		/// <summary>
+		/// Used to convert unmanaged pointers to managed references.
+		/// </summary>
+		StartGCTracking
 	}
 	
 	partial class Conv : UnaryInstruction, ILiftableInstruction
@@ -245,6 +249,15 @@ namespace ICSharpCode.Decompiler.IL
 						default:
 							return ConversionKind.Invalid;
 					}
+				case PrimitiveType.Ref:
+					switch (inputType) {
+						case StackType.I4:
+						case StackType.I:
+						case StackType.I8:
+							return ConversionKind.StartGCTracking;
+						default:
+							return ConversionKind.Invalid;
+					}
 				default:
 					return ConversionKind.Invalid;
 			}

ICSharpCode.Decompiler/IL/Instructions/StLoc.cs

@@ -0,0 +1,39 @@
+// Copyright (c) 2014 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.Diagnostics;
+
+namespace ICSharpCode.Decompiler.IL
+{
+	partial class StLoc
+	{
+		/// <summary>
+		/// If true, this stloc represents a stack type adjustment.
+		/// This field is only used in ILReader and BlockBuilder, and should be ignored by ILAst transforms.
+		/// </summary>
+		internal bool IsStackAdjustment;
+
+		internal override void CheckInvariant(ILPhase phase)
+		{
+			base.CheckInvariant(phase);
+			Debug.Assert(phase <= ILPhase.InILReader || this.IsDescendantOf(variable.Function));
+			Debug.Assert(phase <= ILPhase.InILReader || variable.Function.Variables[variable.IndexInFunction] == variable);
+			Debug.Assert(value.ResultType == variable.StackType);
+		}
+	}
+}

ICSharpCode.Decompiler/IL/PrimitiveType.cs

@@ -35,5 +35,6 @@ namespace ICSharpCode.Decompiler.IL
 		U8 = MetadataType.UInt64,
 		I = MetadataType.IntPtr,
 		U = MetadataType.UIntPtr,
+		Ref = MetadataType.ByReference
 	}
 }

ICSharpCode.Decompiler/IL/Transforms/CopyPropagation.cs

@@ -68,6 +68,9 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 
 		static bool CanPerformCopyPropagation(ILVariable target, ILInstruction value)
 		{
+			Debug.Assert(target.StackType == value.ResultType);
+			if (target.Type.IsSmallIntegerType())
+				return false;
 			switch (value.OpCode) {
 				case OpCode.LdLoca:
 //				case OpCode.LdElema:

ICSharpCode.Decompiler/IL/Transforms/ILInlining.cs

@@ -110,6 +110,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				return false;
 			if (v.LoadCount > 1 || v.LoadCount + v.AddressCount != 1)
 				return false;
+			// TODO: inlining of small integer types might be semantically incorrect,
+			// but we can't avoid it this easily without breaking lots of tests.
+			//if (v.Type.IsSmallIntegerType())
+			//	return false; // stloc might perform implicit truncation
 			return InlineOne(stloc, aggressive, context);
 		}