Auto update local scopes in debug info when manipulating instructions (#677)

* Auto update local scopes in debug info when manipulating instructions

Before this change Cecil would not update local scopes when manipulating mehod bodies. This can lead to corrupted debug info - which when written to a PDB results in corrupted PDB.

Cecil can store local scopes in two ways:
* Using IL offset numbers - this is how pretty much all of the symbol readers populate the OMs.
* Using references to instructions.

If the local scopes use offset values this change will update the local scopes for all insert and remove operations on the method body. The behaviors for insert is basically "insert after" in that the new instruction is added to the scopes of the previous instruction.

If the local scopes are using instructions directly the change only removes any references to instructions which are being removed from the method body (and replaces them with the instruction offset value).

To be able to tell the difference between these cases the instruction field has been made internal in the InstructionOffset structure.

* Add `IsResolved` to the `InstructionOffset` to make the code more readable.

Note: Still have to keep the `instruction` field visible to handle the case of removing a resolved instruction offset reliably (since most resolved instructions will have the numerical offset 0).

* Add internal property on `InstructionOffset` to be able to keep all its fields private.

* Improve comments in the code.

3 files changed, 168 insertions, 15 deletions

diff --git a/Mono.Cecil.Cil/MethodBody.cs b/Mono.Cecil.Cil/MethodBody.cs
index ba3024d..df96849 100644
--- a/Mono.Cecil.Cil/MethodBody.cs
+++ b/Mono.Cecil.Cil/MethodBody.cs
@@ -197,25 +197,31 @@ namespace Mono.Cecil.Cil {
 
 		protected override void OnInsert (Instruction item, int index)
 		{
-			if (size == 0)
-				return;
+			int startOffset = 0;
+			if (size != 0) {
+				var current = items [index];
+				if (current == null) {
+					var last = items [index - 1];
+					last.next = item;
+					item.previous = last;
+					return;
+				}
 
-			var current = items [index];
-			if (current == null) {
-				var last = items [index - 1];
-				last.next = item;
-				item.previous = last;
-				return;
-			}
+				startOffset = current.Offset;
 
-			var previous = current.previous;
-			if (previous != null) {
-				previous.next = item;
-				item.previous = previous;
+				var previous = current.previous;
+				if (previous != null) {
+					previous.next = item;
+					item.previous = previous;
+				}
+
+				current.previous = item;
+				item.next = current;
 			}
 
-			current.previous = item;
-			item.next = current;
+			var scope = GetLocalScope ();
+			if (scope != null)
+				UpdateLocalScope (scope, startOffset, item.GetSize (), instructionRemoved: null);
 		}
 
 		protected override void OnSet (Instruction item, int index)
@@ -227,6 +233,12 @@ namespace Mono.Cecil.Cil {
 
 			current.previous = null;
 			current.next = null;
+
+			var scope = GetLocalScope ();
+			if (scope != null) {
+				var sizeOfCurrent = current.GetSize ();
+				UpdateLocalScope (scope, current.Offset + sizeOfCurrent, item.GetSize () - sizeOfCurrent, current);
+			}
 		}
 
 		protected override void OnRemove (Instruction item, int index)
@@ -241,6 +253,12 @@ namespace Mono.Cecil.Cil {
 
 			RemoveSequencePoint (item);
 
+			var scope = GetLocalScope ();
+			if (scope != null) {
+				var size = item.GetSize ();
+				UpdateLocalScope (scope, item.Offset + size, -size, item);
+			}
+
 			item.previous = null;
 			item.next = null;
 		}
@@ -259,5 +277,38 @@ namespace Mono.Cecil.Cil {
 				}
 			}
 		}
+
+		ScopeDebugInformation GetLocalScope ()
+		{
+			var debug_info = method.debug_info;
+			if (debug_info == null)
+				return null;
+
+			return debug_info.Scope;
+		}
+
+		static void UpdateLocalScope (ScopeDebugInformation scope, int startFromOffset, int offset, Instruction instructionRemoved)
+		{
+			// For both start and enf offsets on the scope:
+			// * If the offset is resolved (points to instruction by reference)  only update it if the instruction it points to is being removed.
+			//   For non-removed instructions it remains correct regardless of any updates to the instructions.
+			// * If the offset is not resolved (stores the instruction offset number itself)
+			//   update the number accordingly to keep it pointing to the correct instruction (by offset).
+
+			if ((!scope.Start.IsResolved && scope.Start.Offset >= startFromOffset) || 
+				(instructionRemoved != null && scope.Start.ResolvedInstruction == instructionRemoved))
+				scope.Start = new InstructionOffset (scope.Start.Offset + offset);
+
+			// For end offset only update it if it's not the special sentinel value "EndOfMethod"; that should remain as-is.
+			if (!scope.End.IsEndOfMethod && 
+				((!scope.End.IsResolved && scope.End.Offset >= startFromOffset) ||
+				 (instructionRemoved != null && scope.End.ResolvedInstruction == instructionRemoved)))
+				scope.End = new InstructionOffset (scope.End.Offset + offset);
+
+			if (scope.HasScopes) {
+				foreach (var subScope in scope.Scopes)
+					UpdateLocalScope (subScope, startFromOffset, offset, instructionRemoved);
+			}
+		}
 	}
 }
diff --git a/Mono.Cecil.Cil/Symbols.cs b/Mono.Cecil.Cil/Symbols.cs
index 9c095af..59a2bec 100644
--- a/Mono.Cecil.Cil/Symbols.cs
+++ b/Mono.Cecil.Cil/Symbols.cs
@@ -207,6 +207,10 @@ namespace Mono.Cecil.Cil {
 			get { return instruction == null && !offset.HasValue; }
 		}
 
+		internal bool IsResolved => instruction != null;
+
+		internal Instruction ResolvedInstruction => instruction;
+
 		public InstructionOffset (Instruction instruction)
 		{
 			if (instruction == null)
diff --git a/Test/Mono.Cecil.Tests/ILProcessorTests.cs b/Test/Mono.Cecil.Tests/ILProcessorTests.cs
index 1f22905..17b4fb0 100644
--- a/Test/Mono.Cecil.Tests/ILProcessorTests.cs
+++ b/Test/Mono.Cecil.Tests/ILProcessorTests.cs
@@ -67,6 +67,23 @@ namespace Mono.Cecil.Tests {
 		}
 
 		[Test]
+		public void InsertAfterWithLocalScopes ()
+		{
+			var method = CreateTestMethodWithLocalScopes ();
+			var il = method.GetILProcessor ();
+
+			il.InsertAfter (
+				0,
+				il.Create (OpCodes.Nop));
+
+			AssertOpCodeSequence (new [] { OpCodes.Ldloc_0, OpCodes.Nop, OpCodes.Ldloc_1, OpCodes.Ldloc_2 }, method);
+			var wholeBodyScope = VerifyWholeBodyScope (method);
+			AssertLocalScope (wholeBodyScope.Scopes [0], 0, 2);
+			AssertLocalScope (wholeBodyScope.Scopes [1], 2, 3);
+			AssertLocalScope (wholeBodyScope.Scopes [2], 3, null);
+		}
+
+		[Test]
 		public void ReplaceUsingIndex ()
 		{
 			var method = CreateTestMethod (OpCodes.Ldloc_0, OpCodes.Ldloc_2, OpCodes.Ldloc_3);
@@ -78,6 +95,24 @@ namespace Mono.Cecil.Tests {
 		}
 
 		[Test]
+		public void ReplaceWithLocalScopes ()
+		{
+			var method = CreateTestMethodWithLocalScopes ();
+			var il = method.GetILProcessor ();
+
+			// Replace with larger instruction
+			var instruction = il.Create (OpCodes.Ldstr, "test");
+			instruction.Offset = method.Instructions [1].Offset;
+			il.Replace (1, instruction);
+
+			AssertOpCodeSequence (new [] { OpCodes.Ldloc_0, OpCodes.Ldstr, OpCodes.Ldloc_2 }, method);
+			var wholeBodyScope = VerifyWholeBodyScope (method);
+			AssertLocalScope (wholeBodyScope.Scopes [0], 0, 1);
+			AssertLocalScope (wholeBodyScope.Scopes [1], 1, 6); // size of the new instruction is 5 bytes
+			AssertLocalScope (wholeBodyScope.Scopes [2], 6, null);
+		}
+
+		[Test]
 		public void Clear ()
 		{
 			var method = CreateTestMethod (OpCodes.Ldloc_0, OpCodes.Ldloc_2, OpCodes.Ldloc_3);
@@ -108,7 +143,70 @@ namespace Mono.Cecil.Tests {
 			foreach (var opcode in opcodes)
 				il.Emit (opcode);
 
+			var instructions = method.Body.Instructions;
+			int size = 0;
+			for (int i = 0; i < instructions.Count; i++) {
+				var instruction = instructions [i];
+				instruction.Offset = size;
+				size += instruction.GetSize ();
+			}
+
 			return method.Body;
 		}
+
+		static ScopeDebugInformation VerifyWholeBodyScope (MethodBody body)
+		{
+			var debug_info = body.Method.DebugInformation;
+			Assert.IsNotNull (debug_info);
+			AssertLocalScope (debug_info.Scope, 0, null);
+			return debug_info.Scope;
+		}
+
+		static void AssertLocalScope (ScopeDebugInformation scope, int startOffset, int? endOffset)
+		{
+			Assert.IsNotNull (scope);
+			Assert.AreEqual (startOffset, scope.Start.Offset);
+			if (endOffset.HasValue)
+				Assert.AreEqual (endOffset.Value, scope.End.Offset);
+			else
+				Assert.IsTrue (scope.End.IsEndOfMethod);
+		}
+
+		static MethodBody CreateTestMethodWithLocalScopes ()
+		{
+			var methodBody = CreateTestMethod (OpCodes.Ldloc_0, OpCodes.Ldloc_1, OpCodes.Ldloc_2);
+			var method = methodBody.Method;
+			var debug_info = method.DebugInformation;
+
+			var wholeBodyScope = new ScopeDebugInformation () {
+				Start = new InstructionOffset (0),
+				End = new InstructionOffset ()
+			};
+			int size = 0;
+			var instruction = methodBody.Instructions [0];
+			var innerScopeBegining = new ScopeDebugInformation () { 
+				Start = new InstructionOffset (size), 
+				End = new InstructionOffset (size + instruction.GetSize ())
+			};
+			size += instruction.GetSize ();
+			wholeBodyScope.Scopes.Add (innerScopeBegining);
+
+			instruction = methodBody.Instructions [1];
+			var innerScopeMiddle = new ScopeDebugInformation () {
+				Start = new InstructionOffset (size),
+				End = new InstructionOffset (size + instruction.GetSize ())
+			};
+			size += instruction.GetSize ();
+			wholeBodyScope.Scopes.Add (innerScopeMiddle);
+
+			var innerScopeEnd = new ScopeDebugInformation () {
+				Start = new InstructionOffset (size),
+				End = new InstructionOffset ()
+			};
+			wholeBodyScope.Scopes.Add (innerScopeEnd);
+
+			debug_info.Scope = wholeBodyScope;
+			return methodBody;
+		}
 	}
 }