Fix #3110: Add support for MCS 2.6.4 pinned region with array variable

* Added additional code to remove the conv instruction present in the initialization part of the pinned region. * Extended the code responsible for removing the unpin stloc to correctly match the inverted condition found in MCS 2.6.4 compiled code. * Enabled already present correctness test to run for MCS 2.6.4. This is a more generalized version of the fix on PR #3110 proposed by @ElektroKill.
icsharpcode · Apr 1, 2024 · 38e7ab4 · 38e7ab4
1 parent 67eade3
commit 38e7ab4
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 14 deletions.
diff --git a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs
@@ -336,10 +336,6 @@ public async Task StackTypes([Values(false, true)] bool force32Bit)
 		[Test]
 		public async Task UnsafeCode([ValueSource(nameof(defaultOptions))] CompilerOptions options)
 		{
-			if (options.HasFlag(CompilerOptions.UseMcs2_6_4))
-			{
-				Assert.Ignore("Decompiler bug with mono!");
-			}
 			await RunCS(options: options);
 		}
 

diff --git a/ICSharpCode.Decompiler/IL/ControlFlow/DetectPinnedRegions.cs b/ICSharpCode.Decompiler/IL/ControlFlow/DetectPinnedRegions.cs
@@ -19,6 +19,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
+using System.Runtime.InteropServices.ComTypes;
 
 using ICSharpCode.Decompiler.IL.Transforms;
 using ICSharpCode.Decompiler.TypeSystem;
@@ -612,18 +613,16 @@ bool CreatePinnedRegion(Block block, StLoc stLoc)
 						innerBlock = (Block)innerBlock.Clone();
 						clonedBlocks[i] = innerBlock;
 					}
-					Branch br = innerBlock.Instructions.LastOrDefault() as Branch;
-					if (br != null && br.TargetBlock.IncomingEdgeCount == 1
-						&& br.TargetContainer == sourceContainer && reachedEdgesPerBlock[br.TargetBlock.ChildIndex] == 0)
+					if (innerBlock.MatchIfAtEndOfBlock(out _, out var trueInst, out var falseInst))
 					{
-						// branch that leaves body.
-						// 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);
-						}
+						HandleBranchLeavingPinnedRegion(trueInst, reachedEdgesPerBlock, sourceContainer, stLoc.Variable);
+						HandleBranchLeavingPinnedRegion(falseInst, reachedEdgesPerBlock, sourceContainer, stLoc.Variable);
+					}
+					else
+					{
+						HandleBranchLeavingPinnedRegion(innerBlock.Instructions.LastOrDefault(), reachedEdgesPerBlock, sourceContainer, stLoc.Variable);
 					}
+
 					// move block into body
 					if (sourceContainer.Blocks[i] == entryBlock)
 					{
@@ -698,6 +697,21 @@ bool CreatePinnedRegion(Block block, StLoc stLoc)
 			return true;
 		}
 
+		static void HandleBranchLeavingPinnedRegion(ILInstruction potentialBranch, int[] reachedEdgesPerBlock, BlockContainer sourceContainer, ILVariable pinnedRegionVar)
+		{
+			if (potentialBranch is Branch branch && branch.TargetBlock.IncomingEdgeCount == 1
+			 	&& branch.TargetContainer == sourceContainer && reachedEdgesPerBlock[branch.TargetBlock.ChildIndex] == 0)
+			{
+				// branch that leaves body.
+				// The target block should have an instruction that resets the pin; delete that instruction:
+				StLoc unpin = branch.TargetBlock.Instructions.First() as StLoc;
+				if (unpin != null && unpin.Variable == pinnedRegionVar && IsNullOrZero(unpin.Value))
+				{
+					branch.TargetBlock.Instructions.RemoveAt(0);
+				}
+			}
+		}
+
 		static bool IsNullOrZero(ILInstruction inst)
 		{
 			while (inst is Conv conv)
@@ -750,6 +764,13 @@ void ProcessPinnedRegion(PinnedRegion pinnedRegion)
 				// fixing a string
 				HandleStringToPointer(pinnedRegion);
 			}
+			else if (pinnedRegion.Init is Conv { Kind: ConversionKind.StopGCTracking, Argument: var convArg })
+			{
+				// If pinnedRegion.Variable was already a pointer type, the input IL has a StopGCTracking conversion.
+				// We can simply remove this conversion, as it is not needed.
+				context.Step("Remove StopGCTracking conversion", pinnedRegion);
+				pinnedRegion.Init = convArg;
+			}
 			// Detect nested pinned regions:
 			BlockContainer body = (BlockContainer)pinnedRegion.Body;
 			foreach (var block in body.Blocks)

diff --git a/ICSharpCode.Decompiler/IL/Transforms/IILTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/IILTransform.cs
@@ -88,12 +88,14 @@ internal ILReader CreateILReader()
 		/// Unlike <c>context.Stepper.Step()</c>, calls to this method are only compiled in debug builds.
 		/// </summary>
 		[Conditional("STEP")]
+		[DebuggerStepThrough]
 		internal void Step(string description, ILInstruction? near)
 		{
 			Stepper.Step(description, near);
 		}
 
 		[Conditional("STEP")]
+		[DebuggerStepThrough]
 		internal void StepStartGroup(string description, ILInstruction? near = null)
 		{
 			Stepper.StartGroup(description, near);

diff --git a/ICSharpCode.Decompiler/IL/Transforms/Stepper.cs b/ICSharpCode.Decompiler/IL/Transforms/Stepper.cs
@@ -95,11 +95,13 @@ public Stepper()
 		/// 
 		/// May throw <see cref="StepLimitReachedException"/> in debug mode.
 		/// </summary>
+		[DebuggerStepThrough]
 		public void Step(string description, ILInstruction? near = null)
 		{
 			StepInternal(description, near);
 		}
 
+		[DebuggerStepThrough]
 		private Node StepInternal(string description, ILInstruction? near)
 		{
 			if (step == StepLimit)
@@ -123,6 +125,7 @@ private Node StepInternal(string description, ILInstruction? near)
 			return stepNode;
 		}
 
+		[DebuggerStepThrough]
 		public void StartGroup(string description, ILInstruction? near = null)
 		{
 			groups.Push(StepInternal(description, near));