Allow emitting debugging symbols via high level scripting APIs #16489

filipw · 2017-01-13T09:05:46Z

Customer scenario

As a user of scripting APIs you'd like to be able to consume the symbols to step through the script source. This is not a mainstream scenario, but very useful for script runner authors as they can offer debugging experience.

This was discussed with @tmat and @ManishJayaswal and the suggestion was to open a PR.

Bugs this fixes:

Fixes #12046
Fixes #1543

Workarounds, if any

Currently this can be achieved with lots of reflection against the Roslyn APIs and manually emitting the compilation. See example here https://github.com/filipw/dotnet-script/blob/master/src/Dotnet.Script.Core/DebugScriptRunner.cs#L25

However, this is hardly ideal.

Risk

Low. Some new public members on ScriptOptions

Performance impact

Low. The original execution path is unaffected.

Is this a regression from a previous update?

No.

Root cause analysis:

At the moment Scripting APIs completely ignores PDBs and passes null into "pdb symbols" when invoking Compilation.Emit.

How was the bug found?

Lots of community requests for supporting debugging in script runners.

Usage & Notes

    class Program
    {
        static void Main(string[] args)
        {
            var path = "foo.csx";
            var txt = File.ReadAllText(path);
            var script = CSharpScript.Create(txt, ScriptOptions.Default.WithDebugInformation(DebugInformationFormat.Pdb).
                WithFilePath(path));
            script.RunAsync().Wait();

           // or just:
           // CSharpScript.RunAsync(txt, ScriptOptions.Default.WithDebugInformation(DebugInformationFormat.Pdb).WithFilePath(path)).Wait();

            Console.ReadKey();
        }
    }

The new setting on the ScriptOptions allows the user to specify if debug information should be emitted and what DebugInformationFormat to use. If so, then when compilation is emitted, symbols are created and loaded.

~~Additionally, the EmitPdb setting is used to control optimizationLevel on the script compilation.~~ (removed this as it created some regression on the available stacktrace in case of errors)

In order for debugging to work, encoding is mandatory so I also added a possibility to specify encoding via ScriptOptions with the default being UTF8.

The new settings (encoding + emit pdb) have not been added to the ScriptOptions constructor because the defaults (UTF8 + false) seem reasonable to me.

filipw · 2017-01-13T09:07:25Z

The end result:

tmat · 2017-01-15T04:23:38Z

src/Scripting/Core/ScriptCompiler.cs

@@ -3,6 +3,7 @@
 using System;
 using System.Text;
 using System.Threading;
+using Microsoft.CodeAnalysis.Emit;


tmat · 2017-01-15T04:23:57Z

src/Scripting/Core/ScriptOptions.cs


 namespace Microsoft.CodeAnalysis.Scripting
 {
+    using Microsoft.CodeAnalysis.Emit;


using Microsoft.CodeAnalysis.Emit; [](start = 4, length = 34)

Move outside the namespace

tmat · 2017-01-15T04:28:32Z

src/Scripting/Core/ScriptOptions.cs

+        /// <summary>
+        /// <see cref="DebugInformationFormat"/> to be used for debugging symbols if needed
+        /// </summary>
+        public DebugInformationFormat? DebugInformationFormat { get; private set; } = null;


DebugInformationFormat? [](start = 15, length = 23)

Nullable is not needed, we can use default(DebugInformationFormat). I guess we should add DebugInformationFormat.None to the enum -- it should be there per .NET Design Guidelines. #Resolved

tmat · 2017-01-15T04:30:29Z

src/Scripting/Core/ScriptOptions.cs

+        /// <summary>
+        /// <see cref="DebugInformationFormat"/> to be used for debugging symbols if needed
+        /// </summary>
+        public DebugInformationFormat? DebugInformationFormat { get; private set; } = null;


DebugInformationFormat? [](start = 15, length = 23)

Actually, I wonder if we need this option at all. We can detect whether the current platform supports Portable PDBs and only if it doesn't use Windows PDB. #Resolved

That sounds like the best idea, I agree. I'm not sure what would be the determining factor though, is it similar to how it is the case for choosing the logic for assembly loading APIs? => if CoreCLR, use PortablePDB, otherwise use full ("properietary") PDB ? #Resolved

That's a good question. Ideally we would use Windows PDB only when necessary. On CoreCLR and Mono we would always use Portable PDB. On Desktop FX, if we can detect 4.6.3+ we can use portable as well. 4.6.2 and below doesn't support portable. I'm not sure yet what would the best way to determine this be. Perhaps we can start with CoreCLR and Mono and use Windows PDB on Destkop FX and change it later. #Resolved

tmat · 2017-01-15T04:31:04Z

src/Scripting/Core/Hosting/AssemblyLoader/DesktopAssemblyLoaderImpl.cs

@@ -30,6 +30,14 @@ public override Assembly LoadFromStream(Stream peStream, Stream pdbStream)
        {
            byte[] peImage = new byte[peStream.Length];
            peStream.TryReadAll(peImage, 0, peImage.Length);
+
+            if (pdbStream != null) {


{ [](start = 35, length = 1)

Style: { on the next line #Resolved

tmat · 2017-01-15T04:34:49Z

src/Scripting/Core/ScriptOptions.cs

+        /// <summary>
+        /// Specifies the encoding to be used for the code.
+        /// </summary>
+        public Encoding Encoding { get; private set; } = Encoding.UTF8;


Encoding [](start = 24, length = 8)

I'd call it FileEncoding and explained in the comment that this is only relevant for debugging scripts that were read from a file or they will be saved to a file for debugging purposes.

tmat · 2017-01-15T04:35:24Z

src/Scripting/Core/ScriptBuilder.cs

@@ -115,20 +115,28 @@ private static void ThrowIfAnyCompilationErrors(DiagnosticBag diagnostics, Diagn
        /// </summary>
        private Func<object[], Task<T>> Build<T>(
            Compilation compilation,
-            DiagnosticBag diagnostics,
+            DiagnosticBag diagnostics, ScriptOptions scriptOptions,


ScriptOptions scriptOptions, [](start = 38, length = 29)

Style: on a new line. #Resolved

I'd only pass DebugInformationFormat instead of all options to this method because that's the only thing needed here.

In reply to: 96129988 [](ancestors = 96129988)

tmat · 2017-01-15T04:36:59Z

src/Scripting/Core/ScriptBuilder.cs

            CancellationToken cancellationToken)
        {
            var entryPoint = compilation.GetEntryPoint(cancellationToken);

            using (var peStream = new MemoryStream())
+            using (var pdbStream = scriptOptions.EmitDebugInformation ? new MemoryStream() : null)


pdbStream [](start = 23, length = 9)

Let's call this variable pdbStreamOpt as it can be null #Resolved

tmat · 2017-01-15T04:43:14Z

Great start, thanks for taking initiative on this! I have commented on the changes. In addition I'd like to see more tests that validate the presence of debug information. I think we could use StackTrace API to see whether there is debug info or not for the compiled script.

filipw · 2017-01-15T15:16:23Z

Thanks a lot for the review - will apply the changes 👍

filipw · 2017-01-17T13:21:01Z

@tmat

I've applied the code changes requested and simplified the API - as you suggested, instead of passing in DebugInformationFormat into ScriptOptions we now expose just ScriptOptions.Default.WithDebugInformation(). Then we internally infer the PDB format -> PortablePdb for CoreCLR/Mono and "classic" for Windows desktop.

I also added unit tests to verify the presence of PDB (check for line number in the stack trace).

PS. I also noticed this PR, aside from the 2 originally mentioned issues, also fixes #13482

tmat · 2017-01-17T17:57:32Z

src/Scripting/Core/ScriptOptions.cs

+        /// <summary>
+        /// Creates a new <see cref="ScriptOptions"/> with debugging information enabled.
+        /// </summary>
+        public ScriptOptions WithDebugInformation() =>


WithDebugInformation [](start = 29, length = 20)

To follow the With- pattern this should be WithEmitDebugInformation(bool value)

tmat · 2017-01-17T17:58:05Z

src/Scripting/Core/ScriptOptions.cs

@@ -123,6 +134,8 @@ private ScriptOptions(ScriptOptions other)
                   metadataResolver: other.MetadataResolver,
                   sourceResolver: other.SourceResolver)
        {
+            EmitDebugInformation = other.EmitDebugInformation;
+            FileEncoding = other.FileEncoding;


Let's add corresponding parameters to the constructor instead.

tmat · 2017-01-17T18:03:11Z

src/Scripting/Core/ScriptOptions.cs

+        /// Specifies the encoding to be used when debugging scripts loaded from a file, or that will be saved to a file for debugging purposes.
+        /// </summary>
+        public Encoding FileEncoding { get; private set; } = Encoding.UTF8;
+


Having default encoding might cause confusion. Even if the file uses UTF8 encoding there are still two kinds of encoding - with BOM and without. If it differs the script won't be debuggable. I'd rather require the user to explicitly specify encoding than to run into these subtleties with the default value.

ok will change to null (that would be part of ScriptOptions.Default too)

daveaglick · 2017-01-17T18:14:57Z

@filipw Been watching this PR with interest - thanks for doing the work to put it together, and thanks to @tmat for the quick review

tmat · 2017-01-17T18:22:39Z

src/Scripting/CSharpTest/ScriptTests.cs

+            catch (Exception ex)
+            {
+                // line information is only available when PDBs have been emitted
+                Assert.DoesNotContain("at Submission#0.<<Initialize>>d__0.MoveNext() in :line 1", ex.StackTrace);


:line 1 [](start = 88, length = 7)

This is odd, why do we have line number info here?

It would also be good to set the FileName and validate it's included in the stack frame string.

this was intended to be a negative test (does not contain the line number info). I will change it to a specific stack trace without line number, it will be much more explicit

tmat · 2017-01-17T18:24:40Z

src/Scripting/Core/ScriptBuilder.cs

@@ -66,7 +66,7 @@ public int GenerateSubmissionId(out string assemblyName, out string typeName)
        }

        /// <exception cref="CompilationErrorException">Compilation has errors.</exception>
-        internal Func<object[], Task<T>> CreateExecutor<T>(ScriptCompiler compiler, Compilation compilation, CancellationToken cancellationToken)
+        internal Func<object[], Task<T>> CreateExecutor<T>(ScriptCompiler compiler, Compilation compilation, ScriptOptions scriptOptions, CancellationToken cancellationToken)


ScriptOptions scriptOption [](start = 109, length = 26)

Let's just pass bool emitDebugInformation

tmat · 2017-01-17T18:27:51Z

src/Scripting/Core/ScriptOptions.cs

+        /// Creates a new <see cref="ScriptOptions"/> with specified <see cref="FileEncoding"/>.
+        /// </summary>
+        public ScriptOptions WithFileEncoding(Encoding encoding) =>
+            new ScriptOptions(this) { FileEncoding = encoding };


new ScriptOptions(this) [](start = 12, length = 23)

Check if FileEncoding == encoding to avoid creating new instance if the values are equal. See WithMetadataResolver

tmat · 2017-01-17T18:36:30Z

src/Scripting/CSharpTest/ScriptTests.cs

+            {
+                // line information is only available when PDBs have been emitted
+                Assert.DoesNotContain("at Submission#0.<<Initialize>>d__0.MoveNext() in :line 1", ex.StackTrace);
+            }


Add a test where FileEncoding is null. We should test all 4 combinations of FileEncoding is null/non-null and EmitDebugInformation == true/false.

filipw · 2017-01-17T18:52:06Z

@tmat thanks for quick responses, much appreciated

tmat · 2017-01-17T21:12:36Z

src/Scripting/Core/Hosting/CommandLine/CommandLineRunner.cs

-                sourceResolver: sourceResolver);
+                sourceResolver: sourceResolver,
+                emitDebugInformation: false,
+                fileEncoding: null);


Doesn't need to be fixed now but shouldn't the command line runner (csi) actually use this feature, so that we have better exception stack traces?

yes that's what I thought as well - let's add it in a separate PR

filipw · 2017-01-17T21:25:39Z

@tmat can you take a look again? applied the feedback + improved the tests

tmat · 2017-01-17T22:01:10Z

src/Scripting/Core/ScriptOptions.cs

+        /// <summary>
+        /// Creates a new <see cref="ScriptOptions"/> with specified <see cref="FileEncoding"/>.
+        /// </summary>
+        public ScriptOptions WithFileEncoding(Encoding encoding) => 


WithFileEncoding [](start = 29, length = 16)

Just realized passing the encoding in options is not enough. The problem is that encodings are not 1:1 mappings in general. Say you have a script file encoded by encoding E saved on disk. You read it to a string using decoder for E and then create a Script from that string and also pass in encoding E. Then to calculate the checksum for the source file that's stored in the PDB, we need to convert the string back to bytes using encoder for E. The problem is that for some encodings the resulting bytes might not be exactly the same that the file contains, unless the encoding is 1:1 mapping.

Also, the compiler implements somewhat sophisticated encoding detection. So it would be unfortunate if the user needed to reimplement that.

I'd suggest that we add overloads to core Scripting methods that take string code today. We don't need to add overloads to all methods since most of them are just convenience APIs that call the core ones.

I'm thinking these 3:

CSharpScript.Create(Stream code, ...) Script.ContinueWith(Stream code, ...) Script.ContinueWith<T>(Stream code, ...)

Then the implementation of these methods would create SourceText using From(Stream, Encoding) method. Then we would pass SourceText around internally, instead creating it all the way down in the builder.

Could you give it a try?

sure, so the suggestion - if I understand correctly - is to rely on letting encoding be detected by SourceText.From(stream) because internally it would try to do it by looking at BOM etc when loading the file content?

When we add the stream overloads, do you also remove manual encoding setting from options and specify that using the non-stream scripting methods will not allow debugging (since under the hood it will use non-debuggable SourceText)?

No, we keep the FileEncoding property. If the encoding is not passed to SourceText the detection is attempted (using BOM) and if we can't detect we default to UTF8 with no BOM. If there is no BOM the encoding needs to be specified, which is what FileEncoding would be used for. You can look at the implementation of http://source.roslyn.io/#Microsoft.CodeAnalysis/Text/SourceText.cs,3687bd0c8922a81f to see what it's doing. Note the exceptions it may throw. We should pass them on the to the caller of the script APIs that take Stream and document them.

OK sounds good.

One thing I noticed straight away though is that adding the overloaded methods with Stream, will cause the analyzer to complain: Symbol Create<T> violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'

What analyzer is that?

This does not line up with how we've been approaching our APIs. There should be a single overload with optional parameters. That is part of the versioning story.

@Pilchie

There are multiple suppressions of this error already

That's not good. When did these pop up? Never went over any of these during API reviews. If this is happening we need to start flagging them and reviewing.

The rule is overly conservative. We can version these overloads methods just fine.

I filed an issue here: https://github.com/dotnet/roslyn/issues/16564

The rule is overly conservative. We can version these overloads methods just fine.

That may be true. It's also not a decision we can not be making in a PR in a release branch during lock down w/o discussion with the API owners.

filipw · 2017-01-17T23:53:22Z

I pushed some changes - I'm not sure how much tests there should be, given that it all ends up calling into the same underlying stuff, the stream methods are merely shortcuts.

improve tests by using StackTrace API and diagnostic verifier

filipw · 2017-02-07T16:20:03Z

@tmat thanks a lot!
I rebased on top of post-dev15 and applied the requested changes. Let me know if something still stands out

tmat · 2017-02-07T17:30:28Z

It seems that github is not doing the rebase right. The PR now has 300+ commits :(

@jasonmalinowski @tannergooding

filipw · 2017-02-07T17:33:19Z

should we repoint the PR to the post-dev15 branch? at the moment we are targeting a merge to master

jaredpar · 2017-02-07T17:33:49Z

The rebase looks correct. Problem is the PR is still targeting master as the base. It needs to move to post-dev15 as well.

tannergooding · 2017-02-07T17:37:06Z

Modified the target branch to be correct. Back down to a +370 −30 change

tmat · 2017-02-07T22:02:06Z

src/Scripting/CSharp/CSharpScript.cs

@@ -23,7 +26,23 @@ public static class CSharpScript
        /// <typeparam name="T">The return type of the script</typeparam>
        public static Script<T> Create<T>(string code, ScriptOptions options = null, Type globalsType = null, InteractiveAssemblyLoader assemblyLoader = null)
        {
-            return Script.CreateInitialScript<T>(CSharpScriptCompiler.Instance, code, options, globalsType, assemblyLoader);


return [](start = 12, length = 6)

throw if code == null here as well

and add doc comment that this method throws ArgNullEx

In reply to: 99941131 [](ancestors = 99941131)

tmat · 2017-02-07T22:03:20Z

tmat · 2017-02-07T22:04:42Z

Looks good, other than one last minor thing here: #16489 (diff) :)
I'm gonna merge as soon as it's addressed.

filipw · 2017-02-07T23:02:27Z

thanks - should be all good now 🙈

tmat · 2017-02-07T23:33:31Z

@filipw Thanks a lot for making it happen!

filipw · 2017-02-08T07:10:35Z

let's enable this for CSI out of the box 😄

dnfclas added the cla-already-signed label Jan 13, 2017

tmat reviewed Jan 15, 2017

View reviewed changes

tmat reviewed Jan 17, 2017

View reviewed changes

filipw force-pushed the feature/script-pdb branch 2 times, most recently from 6ad17bd to 0b781da Compare January 17, 2017 19:53

filipw changed the title ~~[scripting] Allow emitting debugging symbols via high level scripting APIs~~ Allow emitting debugging symbols via high level scripting APIs Jan 17, 2017

filipw force-pushed the feature/script-pdb branch from 95a3aae to 668042a Compare January 17, 2017 20:54

tmat reviewed Jan 17, 2017

View reviewed changes

filipw added 2 commits February 7, 2017 16:43

check parameters for null

5fbe48b

improve tests by using StackTrace API and diagnostic verifier

applied review feedback

931fdf9

filipw force-pushed the feature/script-pdb branch from 4de580c to 931fdf9 Compare February 7, 2017 16:20

tannergooding changed the base branch from master to post-dev15 February 7, 2017 17:36

tmat reviewed Feb 7, 2017

View reviewed changes

added missing null check in CSharpScript Create

3160a3e

tmat merged commit 110ae70 into dotnet:post-dev15 Feb 7, 2017

filipw deleted the feature/script-pdb branch February 8, 2017 06:33

filipw mentioned this pull request Feb 8, 2017

Remove debugging reflection code dotnet-script/dotnet-script#57

Closed

enricosada mentioned this pull request Feb 21, 2017

Debugging f# on .net core dotnet/vscode-csharp#1257

Closed

This was referenced Mar 27, 2017

Debugging scripts loaded in via #load directive dotnet-script/dotnet-script#74

Closed

Take on Roslyn 3 alpha dependency dotnet-script/dotnet-script#77

Closed

buybackoff mentioned this pull request Jun 29, 2017

Execute selection roslynpad/roslynpad#39

Closed

This was referenced Jul 24, 2017

GH1540: Update to latest Roslyn cake-build/cake#1645

Merged

updated to Roslyn 2.3.1 scriptcs/scriptcs#1246

Merged

tmat mentioned this pull request Jul 23, 2018

Allow emitting portable PDBs for CSI on Windows #19691

Closed

Allow emitting debugging symbols via high level scripting APIs #16489

Allow emitting debugging symbols via high level scripting APIs #16489

Conversation

filipw commented Jan 13, 2017 • edited Loading

filipw commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

filipw Jan 15, 2017 • edited by tmat Loading

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

tmat commented Jan 15, 2017

filipw commented Jan 15, 2017

filipw commented Jan 17, 2017

Choose a reason for hiding this comment

tmat Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveaglick commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Jan 17, 2017

tmat Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

filipw Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

filipw commented Jan 17, 2017

filipw commented Feb 7, 2017

tmat commented Feb 7, 2017

filipw commented Feb 7, 2017

jaredpar commented Feb 7, 2017

tannergooding commented Feb 7, 2017

Choose a reason for hiding this comment

tmat Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

tmat commented Feb 7, 2017

tmat commented Feb 7, 2017 • edited Loading

filipw commented Feb 7, 2017

tmat commented Feb 7, 2017

filipw commented Feb 8, 2017

filipw commented Jan 13, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

filipw Jan 15, 2017 •

edited by tmat

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 15, 2017 •

edited

Loading

tmat Jan 17, 2017 •

edited

Loading

tmat Jan 17, 2017 •

edited

Loading

tmat Jan 17, 2017 •

edited

Loading

tmat Jan 17, 2017 •

edited

Loading

filipw Jan 17, 2017 •

edited

Loading

jaredpar Jan 19, 2017 •

edited

Loading

tmat Feb 7, 2017 •

edited

Loading

tmat commented Feb 7, 2017 •

edited

Loading