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.

2 files changed, 70 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)