Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NotSupportedException in symbol write in 0.11.4 #781

Closed
SteveGilham opened this issue Jul 16, 2021 · 1 comment
Closed

NotSupportedException in symbol write in 0.11.4 #781

SteveGilham opened this issue Jul 16, 2021 · 1 comment

Comments

@SteveGilham
Copy link
Contributor

This continues on from issue #697, with the same old Mono C# compiler generated assembly, and is something which I should have caught back then as well (mea culpa).

After making the modification to the instruction offset resolution that was generating the NRE in PR #698, the upshot is a method with an OK top-level scope, but a nested scope which has the default ("end of method") InstructionOffset value at its start. This then faults when being written.

A simple test to demonstrate the behaviour is as follows

		[Test]
		public void InsertBeforeIssue697bis ()
		{
			var parameters = new ReaderParameters { SymbolReaderProvider = new MdbReaderProvider () };
			using (var module = GetResourceModule ("Issue697.dll", parameters)) {
				var pathGetterDef = module.GetTypes ()
					.SelectMany (t => t.Methods)
					.First (m => m.Name.Equals ("get_Defer"));

				var body = pathGetterDef.Body;
				var worker = body.GetILProcessor ();
				var initialBody = body.Instructions.ToList ();
				var head = initialBody.First ();
				var opcode = worker.Create (OpCodes.Ldc_I4_1);
				worker.InsertBefore (head, opcode);

				Assert.That (pathGetterDef.DebugInformation.Scope.Start.IsEndOfMethod, Is.False);
				foreach (var subscope in pathGetterDef.DebugInformation.Scope.Scopes)
					Assert.That (subscope.Start.IsEndOfMethod, Is.False); // <== should fail here

				// big test -- if we can write w/o crashing when the previous asserts are removed
				var output = Path.GetTempFileName ();
				var outputdll = output + ".dll";

				var writer = new WriterParameters () {
					SymbolWriterProvider = new MdbWriterProvider (),
					WriteSymbols = true
				};
				using (var sink = File.Open (outputdll, FileMode.Create, FileAccess.ReadWrite)) {
					module.Write (sink, writer);
				}

				Assert.Pass ();
			}
		}

The underlying cause is that at the start of the operation. the inner scope has a start offset of 18 and a null instruction (related to the trailing nulls at the root of the previous issue), whereas the non-null instructions sum in size to 14. Having thwarted the NRE exit, by making the previous change, we now return from the instruction resolution with an "off the end" value which is used to update the start of the scope.

That value then causes a NotSupportedException (which should probably be an InvalidDataException ) as the symbol data are being written.

A quick and very dirty fix can be made in MethodBody.UpdateLocalScope by pruning such dangling scopes

			if (scope.HasScopes) {
				foreach (var subScope in scope.Scopes)
					UpdateLocalScope (subScope, removedInstruction, existingInstruction, ref cache);
				var dead = scope.Scopes.Where (s => s.Start.IsEndOfMethod).ToList (); // added
				dead.ForEach (d => scope.Scopes.Remove (d)); // added
			}

but I feel sure that there must be a better way to tackle this, around the point where the odd inner scope is being constructed.

@SteveGilham
Copy link
Contributor Author

After a little thought, this seems to be the better fix

                    // Allow for trailing null values in the case of
                    // instructions.Size < instructions.Capacity
                    if (item == null)
-                       break;
+                       return new InstructionOffset (items [i - 1]);

                    cache.Instruction = item;

jbevain pushed a commit that referenced this issue Aug 12, 2021
* Pose the problem

* Quick and v dirty fix

* A better and more targeted fix (to fix the previous fix)
@jbevain jbevain closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants