Automatically update local variable debug info #676

2020-06-20T16:28:15-05:00

vitek-karas commented

2020-06-20 16:28:15 -05:00

(Migrated from github.com)

When manipulating the local variables collection on a method body, automatically update the debug info for all affected local variables.

When removing a local variable, also remove the debug info for that local variable from the scopes.

When removing or inserting update the indices of local variable debug infos which are afected by the action. Note the local variable debug info either holds just a backpointer to the local variable in which case it doesn't store the index at all (and thus nothing to do), or it stores the index explicitly in which case it needs to be updated.

Added tests for both insert and remove cases.

Fixes #674

/cc @marek-safar

When manipulating the local variables collection on a method body, automatically update the debug info for all affected local variables. When removing a local variable, also remove the debug info for that local variable from the scopes. When removing or inserting update the indices of local variable debug infos which are afected by the action. Note the local variable debug info either holds just a backpointer to the local variable in which case it doesn't store the index at all (and thus nothing to do), or it stores the index explicitly in which case it needs to be updated. Added tests for both insert and remove cases. Fixes #674 /cc @marek-safar

jbevain (Migrated from github.com) reviewed

2020-06-25 17:27:32 -05:00

Mono.Cecil.Cil/MethodBody.cs

				`@ -163,10 +165,47 @@ namespace Mono.Cecil.Cil {`

Because this looks a bit odd, I think we want to comment as to why we're doing this. In that case my understanding is that you want to update the indexes of variables that have not been resolved into a VariableDefinition, while a VariableIndex can represent both the resolved and not resolved case.

jbevain commented

2020-06-25 17:27:43 -05:00

(Migrated from github.com)

Thanks, this looks good!

jbevain commented

2020-06-25 17:32:18 -05:00

(Migrated from github.com)

Looking at this PR and the other one, one thing that would be neat is to add a internal property to InstructionOffset and VariableIndex, like bool IsResolved. This way you can have internal logic for the non resolved case, which you have in both PR, and access the relevant data without having to poke at the internals. That would avoid the .index.index access which does look weird.

Looking at this PR and the other one, one thing that would be neat is to add a internal property to InstructionOffset and VariableIndex, like `bool IsResolved`. This way you can have internal logic for the non resolved case, which you have in both PR, and access the relevant data without having to poke at the internals. That would avoid the `.index.index` access which does look weird.

vitek-karas (Migrated from github.com) reviewed

2020-06-26 09:24:38 -05:00

Mono.Cecil.Cil/MethodBody.cs

				`@ -163,10 +165,47 @@ namespace Mono.Cecil.Cil {`

Hopefully the new version is more readable (and I also added a comment) - and it now also handles removal of resolved variable debug infos.

vitek-karas commented

2020-06-26 09:25:57 -05:00

(Migrated from github.com)

I ended up adding two new internal properties (to both VariableIndex and InstructionOffset):

IsResolved per your suggestion
ResolvedVariable/ResolvedInstruction as the code still needs to be able to access the resolved reference to check for removal of the target.

I ended up adding two new internal properties (to both `VariableIndex` and `InstructionOffset`): * `IsResolved` per your suggestion * `ResolvedVariable`/`ResolvedInstruction` as the code still needs to be able to access the resolved reference to check for removal of the target.

vitek-karas commented

2020-07-21 15:22:55 -05:00

(Migrated from github.com)

@jbevain could you please let me know if the latest state of this PR and the friend in https://github.com/jbevain/cecil/pull/677 is such that it will get merged sometime soon-ish? I'm trying to decide if we need to port it over to mono/cecil and use the fork there to ship the linker for .NET 5, or if we should keep using the cecil from this repo. (Without these fixes we are having trouble using the linker in the dotnet/runtime repo to build the libraries for .NET 5)