Skip to content

Better way to update local scopes when method bodies are edited #687

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

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

vitek-karas
Copy link
Contributor

This change will make sure that all local scopes are resolved (they refer to instructions directly and don't rely on instruction offset alone) then fixup local scopes by simply updating instruction references.

Added better tests to validate that the behavior is correct across PDB write/read and some variations of the resolved/unresolved scopes behavior.

This change will make sure that all local scopes are resolved (they refer to instructions directly and don't rely on instruction offset alone) then fixup local scopes by simply updating instruction references.

Added better tests to validate that the behavior is correct across PDB write/read and some variations of the resolved/unresolved scopes behavior.
@vitek-karas
Copy link
Contributor Author

This should hopefully fix #685 - but I can't validate as there's no repro available

@vitek-karas
Copy link
Contributor Author

I validated that this also fixes the issues mentioned in dotnet/linker#1267 (validated on latest dotnet/runtime master build of CoreLib with enabled substitutions on x64).

@vitek-karas vitek-karas marked this pull request as ready for review August 25, 2020 19:08
@jbevain
Copy link
Owner

jbevain commented Aug 25, 2020

@SimonCropp any chance you could give this a try?

@SimonCropp
Copy link
Contributor

sorry for the delay. will try to get to this tomorrow

@SimonCropp
Copy link
Contributor

this fixed the previous issue. but cause another one

Severity	Code	Description	Project	File	Line	Suppression State
Error		Fody: An unhandled exception occurred:
Exception:
Failed to execute weaver C:\Users\simon\.nuget\packages\costura.fody\4.1.0\build\..\weaver\Costura.Fody.dll
Type:
System.Exception
StackTrace:
   at InnerWeaver.ExecuteWeavers() in C:\projects\fody\FodyIsolated\InnerWeaver.cs:line 213
   at InnerWeaver.Execute() in C:\projects\fody\FodyIsolated\InnerWeaver.cs:line 110
Source:
FodyIsolated
TargetSite:
Void ExecuteWeavers()
Object reference not set to an instance of an object.
Type:
System.NullReferenceException
StackTrace:
   at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScope(ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, InstructionOffsetCache& cache) in C:\Code\Fody\cecil\Mono.Cecil.Cil\MethodBody.cs:line 369
   at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScopes(Instruction removedInstruction, Instruction existingInstruction) in C:\Code\Fody\cecil\Mono.Cecil.Cil\MethodBody.cs:line 340
   at Mono.Cecil.Cil.InstructionCollection.OnInsert(Instruction item, Int32 index) in C:\Code\Fody\cecil\Mono.Cecil.Cil\MethodBody.cs:line 261
   at Mono.Collections.Generic.Collection`1.Insert(Int32 index, T item) in C:\Code\Fody\cecil\Mono.Collections.Generic\Collection.cs:line 149
   at Extensions.InsertBefore(Collection`1 instructions, Int32 index, Instruction[] newInstructions)
   at ModuleWeaver.AddToDictionary(FieldDefinition field, String key, String name)
   at ModuleWeaver.BuildUpNameDictionary(Boolean createTemporaryAssemblies, List`1 preloadOrder)
   at ModuleWeaver.Execute()
   at InnerWeaver.ExecuteWeavers() in C:\projects\fody\FodyIsolated\InnerWeaver.cs:line 184
Source:
Mono.Cecil
TargetSite:
Void UpdateLocalScope(Mono.Cecil.Cil.ScopeDebugInformation, Mono.Cecil.Cil.Instruction, Mono.Cecil.Cil.Instruction, InstructionOffsetCache ByRef)
	CosturaSample		1	

@SimonCropp
Copy link
Contributor

seems scope.Start is null

		void UpdateLocalScope (ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, ref InstructionOffsetCache cache)
		{
			if (!scope.Start.IsResolved)
				scope.Start = ResolveInstructionOffset (scope.Start, ref cache);

@SimonCropp
Copy link
Contributor

sorry. i debuged into it. scope is null

@SimonCropp
Copy link
Contributor

adding a null check made all the fody smoke tests build and pass

void UpdateLocalScope (ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, ref InstructionOffsetCache cache)
{
	if (scope == null)
		return;
	if (!scope.Start.IsResolved)
		scope.Start = ResolveInstructionOffset (scope.Start, ref cache);

@vitek-karas
Copy link
Contributor Author

Thanks a lot @SimonCropp - I applied the fix and added a test for this case.

@jbevain
Copy link
Owner

jbevain commented Sep 15, 2020

Thanks for the investigation and for the fix @vitek-karas & @SimonCropp!

vitek-karas added a commit to vitek-karas/cecil that referenced this pull request Jun 21, 2022
When eidting IL with ILProcessor various pieces of debug information have references to the origin IL instructions. These references can be either resolved (point to Instruction instance), in which case the editting mostly works, or unresolved (store IL offset only) in which case they need to be resolved before the editting can occur (after the edit the original IL offsets are invalid and unresolvable).

This is effectively a continuation of jbevain#687 which implemented this for local scopes. This change extends this to async method stepping info and state machine scopes.

The change refactors the code to make it easier to reuse the same logic between the various debug infos being processed.

Updated the existing tests from jbevain#687 to include async and state machine debug info (completely made up) and validate that it gets updated correctly.
vitek-karas added a commit to vitek-karas/cecil that referenced this pull request Jun 21, 2022
When eidting IL with ILProcessor various pieces of debug information have references to the origin IL instructions. These references can be either resolved (point to Instruction instance), in which case the editting mostly works, or unresolved (store IL offset only) in which case they need to be resolved before the editting can occur (after the edit the original IL offsets are invalid and unresolvable).

This is effectively a continuation of jbevain#687 which implemented this for local scopes. This change extends this to async method stepping info and state machine scopes.

The change refactors the code to make it easier to reuse the same logic between the various debug infos being processed.

Updated the existing tests from jbevain#687 to include async and state machine debug info (completely made up) and validate that it gets updated correctly.
marek-safar pushed a commit to dotnet/cecil that referenced this pull request Jun 23, 2022
* ILProcessor should also update custom debug info

When eidting IL with ILProcessor various pieces of debug information have references to the origin IL instructions. These references can be either resolved (point to Instruction instance), in which case the editting mostly works, or unresolved (store IL offset only) in which case they need to be resolved before the editting can occur (after the edit the original IL offsets are invalid and unresolvable).

This is effectively a continuation of jbevain#687 which implemented this for local scopes. This change extends this to async method stepping info and state machine scopes.

The change refactors the code to make it easier to reuse the same logic between the various debug infos being processed.

Updated the existing tests from jbevain#687 to include async and state machine debug info (completely made up) and validate that it gets updated correctly.

* PR Feedback

Renamed some parameters/locals to better match the existing code style.

* PR Feedback
vitek-karas added a commit to vitek-karas/cecil that referenced this pull request Jun 29, 2022
* ILProcessor should also update custom debug info

When eidting IL with ILProcessor various pieces of debug information have references to the origin IL instructions. These references can be either resolved (point to Instruction instance), in which case the editting mostly works, or unresolved (store IL offset only) in which case they need to be resolved before the editting can occur (after the edit the original IL offsets are invalid and unresolvable).

This is effectively a continuation of jbevain#687 which implemented this for local scopes. This change extends this to async method stepping info and state machine scopes.

The change refactors the code to make it easier to reuse the same logic between the various debug infos being processed.

Updated the existing tests from jbevain#687 to include async and state machine debug info (completely made up) and validate that it gets updated correctly.

* PR Feedback

Renamed some parameters/locals to better match the existing code style.

* PR Feedback
jbevain pushed a commit that referenced this pull request Sep 29, 2022
* ILProcessor should also update custom debug info (#34)

* ILProcessor should also update custom debug info

When eidting IL with ILProcessor various pieces of debug information have references to the origin IL instructions. These references can be either resolved (point to Instruction instance), in which case the editting mostly works, or unresolved (store IL offset only) in which case they need to be resolved before the editting can occur (after the edit the original IL offsets are invalid and unresolvable).

This is effectively a continuation of #687 which implemented this for local scopes. This change extends this to async method stepping info and state machine scopes.

The change refactors the code to make it easier to reuse the same logic between the various debug infos being processed.

Updated the existing tests from #687 to include async and state machine debug info (completely made up) and validate that it gets updated correctly.

* PR Feedback

Renamed some parameters/locals to better match the existing code style.

* PR Feedback

* Fix test on Linux

Native PDB is not supported on Linux and the test infra falls back to portable PDB automatically. Since the two PDB implementations read the custom debug info from a different place the test constructing the input needs to adapt to this difference as well.
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

Successfully merging this pull request may close these issues.

3 participants