Is Cecil expected to keep branch targets up to date when inserting things like try blocks?

[Zastai]

I have a build task that, for some methods, will essentially wrap the existing method body in two nested try blocks (it does other things too, but the impact there is smaller).

Sometimes, usually related to the leave instruction, this results in invalid IL because branches that I did not touch directly are generated back with the exact target offset they had when reading in the assembly, which is no longer valid due to the nodes inserted at the start of the method, often making them point in the middle of an instruction (luckily I suppose, because then at least I get an InvalidProgramException, which is better than bad jumps).

I was wondering whether that's considered a bug in Cecil, whether there are additional flags/methods I should use to enable this to be handled, or whether I should essentially find all branches in the method body and apply deltas to their offsets myself?

(My latest incident was with a method ending in a string-based switch, where most of the breaks had been compiled to leaves (in Release mode at least). All of those broke when adding the try blocks.)

If Cecil is supposed to manage this for me, I'll try to make a minimal test case and file an issue; otherwise, any general tips on how best to manage this would be appreciated.

[jbevain]

Cecil usually managed this, what it does not manage out of the box is when short form overflow, like you're inserting stuff between a br.s and its target.

That's usually fixed by calling helper methods from the .Rocks assembly:

method.Body.SimplifyMacros(); // turn short forms into long forms

// modify IL

method.Body.OptimizeMacros(); // turn long forms into short forms if they fit

If that's not the issue you're seeing, please submit a repro. Thank you!

[Zastai]

Ok, will have a look at those and try and make a repro case if it doesn't help. I don't think I added enough code for overflow to be likely, but I suppose it's possible.

There's also a related-but-separate issue — where adding a new sequence point to a method's debug info (to mark added instructions as "hidden") results in a bad pdb ("Invalid compressed integer" exception from System.Reflection.Metadata portable PDB reading) — that I'll need to try and make a repro case for.

[jbevain]

There's also a related-but-separate issue — where adding a new sequence point to a method's debug info (to mark added instructions as "hidden") results in a bad pdb ("Invalid compressed integer" exception from System.Reflection.Metadata portable PDB reading) — that I'll need to try and make a repro case for.

Very interested in a repro. Thank you!

[Zastai]

Using SimplifyMacros() seems to have done the trick (at least for the cases I had; will still need to do a full regression test). Thanks!

The PDB issue seems to be related to that fact that the instructions I add to the method body all report offset 0, because that has not been calculated yet.
This also means that if I add a sequence point for one of them, and then use GetSequencePoint for that same insn, I get the one for the start of the method instead.

It looks like the offsets never get updated before emitting the debug info, and the "invalid compressed integer" should probably be interpreted as "there is already a sequence point for this offset" (but that's hard to tell for certain).
If I use GetSequencePoint on my instruction and remove the result before adding a new one, then the PDB processes fine, but there will be only 1 "hidden" sequence point for my generated code (to be expected), which does seem to confirm the duplicate-sp-offset theory.

Is there a way to tell a method body to (re)compute the offsets of its instructions?

I use the pdb2xml utility from https://github.com/dotnet/symreader-converter.git as an easy way to validate the debug info; but ILSpy also reports the same error, with a traceback:

System.BadImageFormatException: Invalid compressed integer.
   at System.Reflection.Throw.InvalidCompressedInteger()
   at System.Reflection.Metadata.SequencePointCollection.Enumerator.MoveNext()
   at ICSharpCode.Decompiler.PdbProvider.PortableDebugInfoProvider.GetSequencePoints(MethodDefinitionHandle method)
   at ICSharpCode.Decompiler.Disassembler.MethodBodyDisassembler.Disassemble(PEFile module, MethodDefinitionHandle handle) in /_/ICSharpCode.Decompiler/Disassembler/MethodBodyDisassembler.cs:line 127
   at ICSharpCode.ILSpy.CSharpILMixedLanguage.MixedMethodBodyDisassembler.Disassemble(PEFile module, MethodDefinitionHandle handle)
   at ICSharpCode.Decompiler.Disassembler.ReflectionDisassembler.DisassembleMethodBlock(PEFile module, MethodDefinitionHandle handle, GenericContext genericContext) in /_/ICSharpCode.Decompiler/Disassembler/ReflectionDisassembler.cs:line 375
   at ICSharpCode.Decompiler.Disassembler.ReflectionDisassembler.DisassembleMethod(PEFile module, MethodDefinitionHandle handle) in /_/ICSharpCode.Decompiler/Disassembler/ReflectionDisassembler.cs:line 152
   at ICSharpCode.ILSpy.TextView.DecompilerTextView.DecompileNodes(DecompilationContext context, ITextOutput textOutput)
   at ICSharpCode.ILSpy.TextView.DecompilerTextView.<>c__DisplayClass51_0.<DecompileAsync>b__0()

[Zastai]

I tried debugging System.Reflection.Metadata.SequencePointCollection.Enumerator.MoveNext() via pdb2xml but it's so highly optimized (with lots of aggressive inlining of unsafe methods) that I can't really see what exactly is going on.
It does look like the invalid compressed integer error happens in one of two cases:

• there is no data left in the current block
• the integer value is not correctly compressed
  So the "duplicate offset" can't be it, I think.

I'll file an issue once I have time to create a proper minimal repro.