#1918: Fix a bunch of issues with pinned region detection.

Not every pinned region has a clean `P = null` assignment to mark its end.
If a second pinned region starts with the same variable `P`, consider that to mark the end of the previous pinned region for that variable.

Also, fix a bunch of special cases with empty pinned regions.

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

@@ -61,7 +61,10 @@
     <None Include="TestCases\Correctness\StackTests.il" />
     <None Include="TestCases\Correctness\StackTypes.il" />
     <None Include="TestCases\Correctness\Uninit.vb" />
+    <None Include="TestCases\ILPretty\Issue1454.il" />
+    <None Include="TestCases\ILPretty\Issue1681.il" />
     <None Include="TestCases\ILPretty\Issue1922.il" />
+    <None Include="TestCases\ILPretty\Issue1918.il" />
     <None Include="TestCases\ILPretty\WeirdEnums.il" />
     <None Include="TestCases\ILPretty\ConstantBlobs.il" />
     <None Include="TestCases\ILPretty\CS1xSwitch_Debug.il" />
@@ -86,6 +89,8 @@
     <Compile Include="..\ILSpy\DebugInfo\PortableDebugInfoProvider.cs" Link="PortableDebugInfoProvider.cs" />
     <Compile Include="DisassemblerPrettyTestRunner.cs" />
     <Compile Include="TestCases\Correctness\StringConcat.cs" />
+    <Compile Include="TestCases\ILPretty\Issue1681.cs" />
+    <None Include="TestCases\ILPretty\Issue1918.cs" />
     <Compile Include="TestCases\ILPretty\Issue1922.cs" />
     <Compile Include="TestCases\Ugly\NoLocalFunctions.cs" />
     <Compile Include="TestCases\VBPretty\Issue1906.cs" />

ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs

@@ -94,6 +94,12 @@ namespace ICSharpCode.Decompiler.Tests
 			Run();
 		}
 
+		[Test]
+		public void Issue1918()
+		{
+			Run();
+		}
+
 		[Test]
 		public void Issue1922()
 		{

ICSharpCode.Decompiler.Tests/TestCases/ILPretty/Issue1918.cs

@@ -0,0 +1,25 @@
+using System;
+
+namespace ICSharpCode.Decompiler.Tests.TestCases.ILPretty
+{
+	internal class Issue1918
+	{
+		public static Guid[] NullVal;
+
+		public unsafe void ProblemFunction(Guid[] A_0, int A_1)
+		{
+			fixed (Guid* ptr = A_0) {
+				void* ptr2 = ptr;
+				UIntPtr* ptr3 = (UIntPtr*)((byte*)ptr2 - sizeof(UIntPtr));
+				UIntPtr uIntPtr = *ptr3;
+				try {
+					*ptr3 = (UIntPtr)(ulong)A_1;
+				} finally {
+					*ptr3 = uIntPtr;
+				}
+			}
+			fixed (Guid[] ptr = NullVal) {
+			}
+		}
+	}
+}

ICSharpCode.Decompiler.Tests/TestCases/ILPretty/Issue1918.il

@@ -0,0 +1,96 @@
+// Metadata version: v4.0.30319
+.assembly extern mscorlib
+{
+  .publickeytoken = (B7 7A 5C 56 19 34 E0 89 )                         // .z\V.4..
+  .ver 4:0:0:0
+}
+.assembly Issue1918
+{
+  .ver 1:0:0:0
+}
+.module Issue1918.exe
+.imagebase 0x00400000
+.file alignment 0x00000200
+.stackreserve 0x00100000
+.subsystem 0x0003       // WINDOWS_CUI
+.corflags 0x00020003    //  ILONLY 32BITPREFERRED
+
+.class private auto ansi beforefieldinit ICSharpCode.Decompiler.Tests.TestCases.ILPretty.Issue1918
+       extends [mscorlib]System.Object
+{
+
+.method public hidebysig 
+	instance void ProblemFunction (valuetype [mscorlib]System.Guid[] '', int32 ''
+	) cil managed 
+{
+	.maxstack 2
+	.locals init (
+		[0] valuetype [mscorlib]System.Guid[],
+		[1] int32,
+		[2] void*,
+		[3] valuetype [mscorlib]System.Guid[] pinned,
+		[4] native uint*,
+		[5] native uint
+	)
+
+	IL_0000: ldarg.1
+	stloc.0
+
+	IL_0010:
+	ldarg.2
+	stloc.1
+	ldloc.0
+	dup
+	stloc.3
+	brfalse.s IL_0026
+
+	ldloc.3
+	ldlen
+	conv.i4
+	brtrue.s IL_002b
+
+	IL_0026: ldc.i4.0
+	conv.u
+	stloc.2
+	br.s IL_0034
+
+	IL_002b: ldloc.3
+	ldc.i4.0
+	ldelema [mscorlib]System.Guid
+	conv.u
+	stloc.2
+
+	IL_0034: ldloc.2
+	sizeof [mscorlib]System.UIntPtr
+	sub
+	stloc.s 4
+	ldloc.s 4
+	ldind.i
+	stloc.s 5
+	.try
+	{
+		ldloc.s 4
+		ldloc.1
+		conv.i8
+		call native uint [mscorlib]System.UIntPtr::op_Explicit(uint64)
+		stind.i
+		ldarg.1
+		
+		leave.s IL_005c
+	} // end .try
+	finally
+	{
+		ldloc.s 4
+		ldloc.s 5
+		stind.i
+		endfinally
+	} // end handler
+
+	IL_005c: ldsfld valuetype [mscorlib]System.Guid[] ICSharpCode.Decompiler.Tests.TestCases.ILPretty.Issue1918::NullVal
+	stloc.3
+	ret
+}
+
+.field public static valuetype [mscorlib]System.Guid[] NullVal
+
+}

ICSharpCode.Decompiler.Tests/TestCases/Pretty/UnsafeCode.cs

@@ -208,6 +208,16 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 			}
 		}
 
+#if !(LEGACY_CSC && OPT)
+		// legacy csc manages to optimize out the pinned variable altogether in this case;
+		// leaving no pinned region we could detect.
+		public unsafe void FixedArrayNoPointerUse(int[] arr)
+		{
+			fixed (int* ptr = arr) {
+			}
+		}
+#endif
+
 		public unsafe void PutDoubleIntoLongArray1(long[] array, int index, double val)
 		{
 			fixed (long* ptr = array) {

ICSharpCode.Decompiler/IL/ControlFlow/DetectPinnedRegions.cs

@@ -95,17 +95,30 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				for (int j = 0; j < block.Instructions.Count - 1; j++) {
 					var inst = block.Instructions[j];
 					ILVariable v;
-					if (inst.MatchStLoc(out v) && v.Kind == VariableKind.PinnedLocal && block.Instructions[j + 1].OpCode != OpCode.Branch) {
-						// split block after j:
-						context.Step("Split block after pinned local write", inst);
-						var newBlock = new Block();
-						for (int k = j + 1; k < block.Instructions.Count; k++) {
-							newBlock.Instructions.Add(block.Instructions[k]);
+					if (inst.MatchStLoc(out v) && v.Kind == VariableKind.PinnedLocal) {
+						if (block.Instructions[j + 1].OpCode != OpCode.Branch) {
+							// split block after j:
+							context.Step("Split block after pinned local write", inst);
+							var newBlock = new Block();
+							for (int k = j + 1; k < block.Instructions.Count; k++) {
+								newBlock.Instructions.Add(block.Instructions[k]);
+							}
+							newBlock.AddILRange(newBlock.Instructions[0]);
+							block.Instructions.RemoveRange(j + 1, newBlock.Instructions.Count);
+							block.Instructions.Add(new Branch(newBlock));
+							container.Blocks.Insert(i + 1, newBlock);
+						}
+						if (j > 0) {
+							// split block before j:
+							context.Step("Split block before pinned local write", inst);
+							var newBlock = new Block();
+							newBlock.Instructions.Add(block.Instructions[j]);
+							newBlock.Instructions.Add(block.Instructions[j + 1]);
+							Debug.Assert(block.Instructions.Count == j + 2);
+							block.Instructions.RemoveRange(j, 2);
+							block.Instructions.Insert(j, new Branch(newBlock));
+							container.Blocks.Insert(i + 1, newBlock);
 						}
-						newBlock.AddILRange(newBlock.Instructions[0]);
-						block.Instructions.RemoveRange(j + 1, newBlock.Instructions.Count);
-						block.Instructions.Add(new Branch(newBlock));
-						container.Blocks.Insert(i + 1, newBlock);
 					}
 				}
 			}
@@ -347,8 +360,17 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			ILInstruction value;
 			if (block.Instructions.Count != 2)
 				return false;
-			if (!block.Instructions[0].MatchStLoc(p, out value))
+			if (!block.Instructions[0].MatchStLoc(out var p2, out value))
 				return false;
+			if (p != p2) {
+				// If the pointer is unused, the variable P might have been split.
+				if (p.LoadCount == 0 && p.AddressCount == 0 && p2.LoadCount == 0 && p2.AddressCount == 0) {
+					if (!ILVariableEqualityComparer.Instance.Equals(p, p2))
+						return false;
+				} else {
+					return false;
+				}
+			}
 			if (v.Kind == VariableKind.PinnedLocal) {
 				value = value.UnwrapConv(ConversionKind.StopGCTracking);
 			}
@@ -411,30 +433,24 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				// we didn't find a single block to be added to the pinned region
 				return false;
 			}
-			reachedEdgesPerBlock[entryBlock.ChildIndex]++;
-			workList.Enqueue(entryBlock);
+			if (entryBlock.Instructions[0].MatchStLoc(stLoc.Variable, out _)) {
+				// pinned region has empty body
+			} else {
+				reachedEdgesPerBlock[entryBlock.ChildIndex]++;
+				workList.Enqueue(entryBlock);
+			}
 			while (workList.Count > 0) {
 				Block workItem = workList.Dequeue();
-				StLoc workStLoc = workItem.Instructions.SecondToLastOrDefault() as StLoc;
-				int instructionCount;
-				if (workStLoc != null && workStLoc.Variable == stLoc.Variable && IsNullOrZero(workStLoc.Value)) {
-					// found unpin instruction: only consider branches prior to that instruction
-					instructionCount = workStLoc.ChildIndex;
-				} else {
-					instructionCount = workItem.Instructions.Count;
-				}
-				for (int i = 0; i < instructionCount; i++) {
-					foreach (var branch in workItem.Instructions[i].Descendants.OfType<Branch>()) {
-						if (branch.TargetBlock.Parent == sourceContainer) {
-							if (branch.TargetBlock == block) {
-								// pin instruction is within a loop, and can loop around without an unpin instruction
-								// This should never happen for C#-compiled code, but may happen with C++/CLI code.
-								return false;
-							}
-							if (reachedEdgesPerBlock[branch.TargetBlock.ChildIndex]++ == 0) {
-								// detected first edge to that block: add block as work item
-								workList.Enqueue(branch.TargetBlock);
-							}
+				foreach (var branch in workItem.Descendants.OfType<Branch>()) {
+					if (branch.TargetBlock.Parent == sourceContainer) {
+						if (branch.TargetBlock.Instructions[0].MatchStLoc(stLoc.Variable, out _)) {
+							// Found unpin instruction
+							continue;
+						}
+						Debug.Assert(branch.TargetBlock != block);
+						if (reachedEdgesPerBlock[branch.TargetBlock.ChildIndex]++ == 0) {
+							// detected first edge to that block: add block as work item
+							workList.Enqueue(branch.TargetBlock);
 						}
 					}
 				}
@@ -454,12 +470,14 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				if (reachedEdgesPerBlock[i] > 0) {
 					var innerBlock = sourceContainer.Blocks[i];
 					Branch br = innerBlock.Instructions.LastOrDefault() as Branch;
-					if (br != null && br.TargetContainer == sourceContainer && reachedEdgesPerBlock[br.TargetBlock.ChildIndex] == 0) {
+					if (br != null && br.TargetBlock.IncomingEdgeCount == 1 
+						&& br.TargetContainer == sourceContainer && reachedEdgesPerBlock[br.TargetBlock.ChildIndex] == 0)
+					{
 						// branch that leaves body.
-						// Should have an instruction that resets the pin; delete that instruction:
-						StLoc innerStLoc = innerBlock.Instructions.SecondToLastOrDefault() as StLoc;
-						if (innerStLoc != null && innerStLoc.Variable == stLoc.Variable && IsNullOrZero(innerStLoc.Value)) {
-							innerBlock.Instructions.RemoveAt(innerBlock.Instructions.Count - 2);
+						// The target block should have an instruction that resets the pin; delete that instruction:
+						StLoc unpin = br.TargetBlock.Instructions.First() as StLoc;
+						if (unpin != null && unpin.Variable == stLoc.Variable && IsNullOrZero(unpin.Value)) {
+							br.TargetBlock.Instructions.RemoveAt(0);
 						}
 					}
 					
@@ -468,6 +486,14 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 					// we'll delete the dummy block later
 				}
 			}
+			if (body.Blocks.Count == 0) {
+				// empty body, the entryBlock itself doesn't belong into the pinned region
+				Debug.Assert(reachedEdgesPerBlock[entryBlock.ChildIndex] == 0);
+				var bodyBlock = new Block();
+				bodyBlock.SetILRange(stLoc);
+				bodyBlock.Instructions.Add(new Branch(entryBlock));
+				body.Blocks.Add(bodyBlock);
+			}
 
 			var pinnedRegion = new PinnedRegion(stLoc.Variable, stLoc.Value, body).WithILRange(stLoc);
 			stLoc.ReplaceWith(pinnedRegion);
@@ -546,6 +572,8 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 						return; // variable access that is not LdLoc
 				}
 			}
+			if (ldloc == null)
+				return;
 			if (!(ldloc.Parent is GetPinnableReference arrayToPointer))
 				return;
 			if (!(arrayToPointer.Parent is Conv conv && conv.Kind == ConversionKind.StopGCTracking))
@@ -642,20 +670,25 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				return;
 			if (!IsBranchOnNull(body.EntryPoint.Instructions[1], nativeVar, out Block targetBlock))
 				return;
-			if (targetBlock.Parent != body)
-				return;
 			if (!body.EntryPoint.Instructions[2].MatchBranch(out Block adjustOffsetToStringData))
 				return;
 			if (!(adjustOffsetToStringData.Parent == body && adjustOffsetToStringData.IncomingEdgeCount == 1
 					&& IsOffsetToStringDataBlock(adjustOffsetToStringData, nativeVar, targetBlock)))
 				return;
 			context.Step("Handle pinned string (with adjustOffsetToStringData)", pinnedRegion);
-			// remove old entry point
-			body.Blocks.RemoveAt(0);
-			body.Blocks.RemoveAt(adjustOffsetToStringData.ChildIndex);
-			// make targetBlock the new entry point
-			body.Blocks.RemoveAt(targetBlock.ChildIndex);
-			body.Blocks.Insert(0, targetBlock);
+			if (targetBlock.Parent == body) {
+				// remove old entry point
+				body.Blocks.RemoveAt(0);
+				body.Blocks.RemoveAt(adjustOffsetToStringData.ChildIndex);
+				// make targetBlock the new entry point
+				body.Blocks.RemoveAt(targetBlock.ChildIndex);
+				body.Blocks.Insert(0, targetBlock);
+			} else {
+				// pinned region has empty body, immediately jumps to targetBlock which is outside
+				body.Blocks[0].Instructions.Clear();
+				body.Blocks.RemoveRange(1, body.Blocks.Count - 1);
+				body.Blocks[0].Instructions.Add(new Branch(targetBlock));
+			}
 			pinnedRegion.Init = new GetPinnableReference(pinnedRegion.Init, null);
 
 			ILVariable otherVar;