ILProcessor should also update custom debug info #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@agocke @sbomer could you please take a look (I can't request official review on this repo) This is a fix for the problem reported in dotnet/runtime#70720 (comment). I verified that it fixes that problem. The plan is to merge it to mono/cecil (and update linker with it - hopefully "soon", maybe even for P6?) |
agocke
reviewed
Jun 22, 2022
|
|
||
| UpdateLocalScope (debug_info.Scope, removedInstruction, existingInstruction, ref cache); | ||
| if (method.debug_info != null) |
There was a problem hiding this comment.
How is this check different from the one 30 lines above?
There was a problem hiding this comment.
My bad - the one above should not be there anymore (I added this one and wanted to remove the one above, but forgot).
Mono.Cecil.Cil/MethodBody.cs
Outdated
|
|
||
| var customDebugInfo = method.custom_infos ?? method.debug_info?.custom_infos; | ||
| if (customDebugInfo != null) { | ||
| foreach (var custom_debug_info in customDebugInfo) { |
There was a problem hiding this comment.
Am I correct in that this is basically the only interesting change -- also walk the state machine and async method infos, instead of just the local scope?
There was a problem hiding this comment.
Yes - that's the gist of it.
The rest is mainly refactoring to reuse the same code which was used only for the local scopes.
Renamed some parameters/locals to better match the existing code style.
marek-safar
reviewed
Jun 22, 2022
Mono.Cecil.Cil/MethodBody.cs
Outdated
| @@ -306,12 +306,8 @@ void RemoveSequencePoint (Instruction instruction) | |||
| } | |||
| } | |||
|
|
|||
| void UpdateDebugInformation (Instruction removedInstruction, Instruction existingInstruction) | |||
| void UpdateDebugInformation (Instruction removed_instruction, Instruction existing_instruction) | |||
There was a problem hiding this comment.
Why to change the parameter names, seems the file uses normal C# parameters casing style?
vitek-karas
added a commit
to vitek-karas/linker
that referenced
this pull request
Jun 23, 2022
marek-safar
pushed a commit
to dotnet/linker
that referenced
this pull request
Jun 23, 2022
marek-safar
pushed a commit
that referenced
this pull request
Oct 6, 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 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 * 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.
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
Commit migrated from dotnet/linker@5128bae
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When editing 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.