ICsharpCode.Decompiler integration into VS Debugger

[jacdavis]

I am one of the principal developers on the Visual Studio Debugger and the original developer of several of Visual Studio's core debugging scenarios. The ilspy decompiler is a very impressive piece of technology.

We (Visual Studio) are preparing to release a new feature where the ilspy decompiler is fully integrated into the debugger. This will allow users to produce symbols and source while debugging even in cases where modules only exist in memory (such as dump files that contain module memory) or when a user realizes after starting a debug session that decompilation is required.

We are running into some limitations with the symbol sequence point generation. I have been prototyping fixes for these issues, and I would like to start a conversation around them.

1. Sequence points seem to be getting emitted at locations where the ilstack is not empty. Anytime this happens, the debugger will be incapable of performing evaluations and many times breakpoints will fail to bind. The CLR will consider the current execution location within a can't stop region. Even in release builds, our compilers still often try to empty the il stack across statement boundaries. Note that this is even happening when decompiling debug builds with nop based sequence points in the il.

2. Sequence points are missing at opening and closing braces. from some other issues, I believe this has been discussed but was determined to be low priority. However, with the VS integration, this breaks several scenarios such as stepping in to a method with symbols, and then invoking the decompiler. As an example, since C# compilers emit sequence points at the opening braces, the debugger with symbols (but without source) will stop on the opening brace of a function during a stepin. If, after stepping in, the user performs decompilation to produce source, the debugger will complain that the current instruction pointer has no source info since the sequence points emitted by the decompiler will not contain this location.

I hope you are as excited about this scenario as we are on the VS Debugger. Fully integrated decompilation will make my own day-to-day workflow much easier.

[christophwille]

I had a quick chat with @siegfriedpammer (currently very time-constrained), maybe @dgrunwald can help out.

Siegi gave me a few pointers for the second item in your list: We have issue #1245 and comment #1422 (comment) in this area. Given that our output was to be the PDB (without tooling integration), we did hit some hard limits on what we could do (and thus made PDBgen a lower-pri feature).

[siegfriedpammer]

As for the status of pdbgen, issues #1422 and #1230 (linked in #1422 but still relevant to point out separately) give a good overview of the status of the feature. To re-review those issues & comments on other issues, I'd like to point out:

• decompilation (the source included in the PDB) is a fully done feature
• however, as you already pointed out above (no. 1), the decompiler sometimes generates sequence points were they should not be. This is because we only have limited knowledge of how the debugger interprets the sequence points from our analysis of existing PDBs/source code. If this feature is meant to work properly in all cases, we will need more information and help/guidance from your side.
  Other known problems:
  • Sometimes the decompiler introduces local variables that have no local variable slot in the locals signature. This is because the decompiler engine introduces a stackslot dummy variable for every value that is pushed onto the eval stack. In later steps of the decompiler pipeline we try to inline these stackslots so that only existing local variables are used in the program. Sometimes however we cannot inline these, one reason might be readability or simply because Roslyn (especially in release builds) makes extensive use of the evaluation stack and in some cases even optimizes user-defined local variables away. Currently these variables cannot be added to the locals signature of the method body and thus cannot be accessed by the debugger. In order to support this, the PDB format might need some extensions, similar to how iirc - in layman's terms - some C/C++ compilers/debuggers support references to registers (e.g. variable/expression 'x' is stored in register X - this is how @dgrunwald described the feature at one of our meetings).
  • No sequence points on opening/closing braces: see Add sequence points on opening/closing braces #1245. This will be hard, if not impossible, as the debugger seems to expect the stack to be empty, so if there are no nop instructions, we will not be able to emit sequence points for those. "Stealing" instructions from other statements is - like @dgrunwald pointed out - likely not going to work out-of-the-box.
  • Additionally the stepping experience might not yet be the same as with a compiler-generated PDB.
  • async/await and yield state-machines are only partially implemented.
  • We currently only emit one single top-level local scope for the whole method. Variable names are unique across the whole method, however this might hinder a good debugging experience because the locals pad is filled with a lot of variables that are not in scope at the current IP.
  • We currently only emit one single top-level import scope for the current document/file. (generally not a problem)

In order for the decompiler to produce correct decompilation results, it is very important that we have all transitive compile-time references available when generating the source code or debug info. Otherwise the decompiler will fail to recognize some of the more advanced patterns that rely on well-known types such as IDisposable, IEnumerable, Task, etc. For the .NET Standard/Core scenario it is particularly necessary to provide the correct netstandard.dll as well, because we need to resolve type-forwarders.

In general, the feature ended up on the back burner because most people use ILSpy to look at Release builds, that is what Daniel's comment on #1422 is geared towards - Roslyn simply makes Release builds "un-PDB-able".

[KirillOsenkov]

I'm (also?) seeing that the SequencePointBuilder doesn't seem to be producing sequence points for trivial expression bodied getter properties, such as Prop => field;. As a result when stepping into get_Prop, we can't find sequence points. The IL for that method body is just ldarg.0, ldfld ... and ret.

Is this because the visitor should be visiting expressions or something else is going on? Or is it because I'm decompiling a .dll compiled in Release mode?

[siegfriedpammer]

Is this because the visitor should be visiting expressions or something else is going on?

Yes, this is the exact reason. I have a fix implemented locally, however, I am not done with testing yet... PDBGen tests are almost non-existing and not exactly easy to write, so I will have to perform some more manual testing.

[KirillOsenkov]

feel free to send me a branch, I can help with testing

[siegfriedpammer]

That's great! Please see https://github.com/icsharpcode/ILSpy/tree/pdbgen-fixes

Thank you very much!

[KirillOsenkov]

Hmm, I'm still not seeing any sequence points for the property expression body. I'm not even using DebugInfoGenerator, I'm just using the SequencePointBuilder to get the sequence points for all the methods in the decompiled type. I see your change adds overrides to DebugInfoGenerator, but not SequencePointBuilder.

I'm only getting 4 sequence point sets: for indexer getter and setter, Main itself and the local function.

Here's a test program that I'm using:

class C
{
    int Property => 42;

    static void Main()
    {
        var c = new C();

        _ = c.Property;
        c[2] = c[1];

        Local();

        void Local()
        {
            _ = c.Property;
        }
    }

    C this[int index]
    {
        get
        {
            return null;
        }
        set
        {
        }
    }
}

[KirillOsenkov]

Ah, nice, if I add this to SequencePointBuilder.cs, then it works like a charm:

		public override void VisitPropertyDeclaration(PropertyDeclaration propertyDeclaration)
		{
			if (propertyDeclaration.ExpressionBody != null)
			{
				VisitAsSequencePoint(propertyDeclaration.ExpressionBody);
			}
			else
			{
				base.VisitPropertyDeclaration(propertyDeclaration);
			}
		}

Unrelatedly, NormalizeBlockStatements seems to not be simplifying the indexer getter for some reason. But I suspect it's a separate bug.

[KirillOsenkov]

I see, CalculatedGetterOnlyIndexerPattern will not match if there is a setter. Not sure if it's a bug or by design. Technically we could still simplify the getter and getter independently. But then we'd perhaps need two ExpressionBodies, one for the getter and one for the setter.

[KirillOsenkov]

Sorry, I realize I should have filed a separate issue instead of hijacking this general issue. Did so now in #3031.

[KirillOsenkov]

and the PR in #3032