Improve debugger's variable population mechanism #1620

JustinGrote · 2021-11-05T00:36:20Z

Fixes PowerShell/vscode-powershell#3667
Fixes PowerShell/vscode-powershell#3673

JustinGrote · 2021-11-05T01:26:06Z

Thoughts: Do we need to do any kind of additional verification to make sure the variable "scope" matches the stacktrace? Or is this lazy iterator method considered pretty consistent?

SeeminglyScience · 2021-11-05T01:47:18Z

Thoughts: Do we need to do any kind of additional verification to make sure the variable "scope" matches the stacktrace? Or is this lazy iterator method considered pretty consistent?

Might be worth looking into whether CallStackFrame.GetFrameVariables() can be used. I'm guessing there is a reason it was avoided (though maybe that was for PSv3 support). @rkeithhill do you happen to remember?

JustinGrote · 2021-11-05T13:40:22Z

@SeeminglyScience could be because the stack frames are being returned deserialized here, maybe some more formatting can go into the Get-PSCallStack request? @daviwil any recollection?

https://github.com/JustinGrote/PowerShellEditorServices/blob/c93f3edd02c0714c7c24664da800083aecd1599c/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs#L810-L815

JustinGrote · 2021-11-05T13:52:33Z

Another Q: Any reason why this is generic'd to PSObject and not CallStackFrame?
https://github.com/JustinGrote/PowerShellEditorServices/blob/c93f3edd02c0714c7c24664da800083aecd1599c/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs#L817

JustinGrote · 2021-11-05T14:32:42Z

Seems to work fine when generic'd to CallStackFrame, methods still work fine, I can probably cut a lot of code out here.

JustinGrote · 2021-11-05T14:35:26Z

Oh, the reason may be when debugging remote runspaces, need to test that.

JustinGrote · 2021-11-05T14:54:28Z

Yep, that's the reason. Shouldn't the generic ExecutePSCommandAsync throw an exception rather than silently filter objects if they don't match the generic profile?

JustinGrote · 2021-11-05T14:55:38Z

I'll adjust the pscommand here to return the variable info as well so the subsequent queries are unnecessary.

SeeminglyScience · 2021-11-05T16:19:06Z

Yeah I always forget about that. Serialization is the bane of my existence -_-

Yep, that's the reason. Shouldn't the generic ExecutePSCommandAsync throw an exception rather than silently filter objects if they don't match the generic profile?

I always thought it should as well, every time I saw that OfType call. At least a Debug.Assert or something since you don't really want a crash and it's really a design time error. Ultimately it doesn't seem to hurt much but a debug assert would be nice for safety.

andyleejordan · 2021-11-05T16:39:01Z

Oh, the reason may be when debugging remote runspaces, need to test that.

I know Rob spent a good chunk of time making sure this worked, though sadly there wasn't really a way to put tests around it.

andyleejordan · 2021-11-05T16:39:47Z

Yep, that's the reason. Shouldn't the generic ExecutePSCommandAsync throw an exception rather than silently filter objects if they don't match the generic profile?

If it's not already in a big old comment explaining so, would you mind adding @JustinGrote?

SeeminglyScience · 2021-11-05T16:40:06Z

I know Rob spent a good chunk of time making sure this worked, though sadly there wasn't really a way to put tests around it.

Oh yeah I'm sure how it's set up works fine for remote runspaces. It's just my GetFrameVariables idea that explodes 😁

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

JustinGrote · 2021-11-06T21:43:46Z

This now uses serialization only for the remote runspace pathway. I'm not very happy about all the "isOnRemoteMachine" ternaries, this should probably instead be abstracted out to a CallStackProvider class or something to make it easier to test.

SeeminglyScience

Awesome work, thanks for doing this! ❤️ Got a few requests and questions

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/VariableDetails.cs

…ences not matching up

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

JustinGrote · 2021-11-11T00:42:56Z

@andschwa should be good to merge if the CI tests pass

andyleejordan

We're having to go back to the drawing board on this after testing. We accidentally dropped all the local variables 😅

JustinGrote · 2021-11-11T18:53:00Z

Per discussion looks like GetFrameVariables() doesn't work like it should, it's not returning local vars.

andyleejordan · 2021-11-11T21:26:31Z

Per discussion looks like GetFrameVariables() doesn't work like it should, it's not returning local vars.

😭

JustinGrote · 2021-11-11T23:23:40Z

OK, new plan:

Global/Script/Local will reflect just what the gv version of each of those is at the current breakpoint, we're not going to try go up the scope stack and associate those to stackframes because they aren't always 1:1 (modules, etc.)
Auto will contain variables in the current getFrameVariables as well as some whitelisted pull-forward items such as $input,$_, and $args

We will be able to vastly simplify things this way, and it will be more obvious and presentable to users as it would be what they would expect (e.g. these are the variables that can be seen at script scope, not the "script scope" variables, so it would include globals)

Initially the auto pull forward vars will be hardcoded but I want to make this a setting.

JustinGrote · 2021-11-12T06:00:40Z

OK this is much better now. Auto now includes only $args and $PSItem (as the original code was meant to it seems) but it comes directly from the callstack frame, so if you have a deep callstack, it will show $args and $PSItem at the level of that callstack. Local/Script/Global basically reflect what is shown in get-variable, with script and local respectively de-duped as they are processed (this is also original behavior).

The only difference in behavior is that local does not change with the stack layer, because stack layers and scopes are not 1:1 pairings. Instead, a future PR should add "Scope x" variable headings for the top stack frame only as with the current implementation it is not possible to accurately correlate scope layers to stack frames (may be possible in the future)

Also updated so "Auto" shows both $_ and $PSItem even though they represent the same thing. @SeeminglyScience and I discussed and while it would be nice to only present $PSItem to encourage people to use it, that may confuse people who still use or learn $_ and are ignorant that they are the same.

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/StackFrameDetails.cs

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

PaulHigin · 2021-11-16T18:02:32Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+                results = await _executionService.ExecutePSCommandAsync<PSObject>(psCommand, CancellationToken.None)
+                    .ConfigureAwait(false);
+            }
+            catch (CmdletInvocationException ex)


Please add a comment as to why this exception is caught and eaten here, except for 'psargumentoutofrangeexception' types.

PaulHigin · 2021-11-16T18:18:01Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+            // If we're attached to a remote runspace, we need to serialize the callstack prior to transport
+            // because the default depth is too shallow
+            bool isOnRemoteMachine = _psesHost.CurrentRunspace.IsOnRemoteMachine;
+            string returnSerializedIfOnRemoteMachine = isOnRemoteMachine


This is a bit confusing for future maintainers. Please add comments that clearly show the Get-PSCallStack script that is run for both local and remote (serialized results) cases.

Suggested change

string returnSerializedIfOnRemoteMachine = isOnRemoteMachine

// The next command always calls `Get-PSCallStack`. On a local machine, we just return

// its results. On a remote machine we serialize it first and then later deserialize it.

string returnSerializedIfOnRemoteMachine = isOnRemoteMachine

Suggested comment to resolve Paul's request.

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

PaulHigin · 2021-11-16T18:24:56Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs


            // This glorious hack ensures that Get-PSCallStack returns a list of CallStackFrame
            // objects (or "deserialized" CallStackFrames) when attached to a runspace in another
            // process.  Without the intermediate variable Get-PSCallStack inexplicably returns
            // an array of strings containing the formatted output of the CallStackFrame list.
-            var callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";
-            psCommand.AddScript($"{callStackVarName} = Get-PSCallStack; {callStackVarName}");
+            string callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";


This should be reviewed for possible injection opportunities, and ensure that arbitrary script content cannot be injected from external sources. ExecutePSCommandAsync appears to really be 'ExecuteScriptAsync' since it takes a general script string instead of command plus arguments.

While true, the only change here is from var to string. We can do this later.

andyleejordan · 2021-11-16T22:29:47Z

Ok the only thing I'd like to know if we can still keep working (even admitting that it's funky) is to have the Auto variable groupset update per stack frame. In the (admittedly broken in several ways) current version of the code, if you hit a breakpoint and click through the frames in the callstack, the current frame's local and auto variables update in the VS Code debugger. I'll play with this and see if I can get it to work.

JustinGrote · 2021-11-16T22:39:56Z

Auto variable does update per stack frame for the auto variables, or at least it did in my testing.

Local and Global do not because, as discussed, there is no actual correlary between stack frames and scopes, and the previous behavior that enabled this (upcycling through the scopes via -scope 0, -scope 1, etc.) was inaccurate especially if done in a module scope, and presenting inaccurate variables are considered worse than not presenting them at all.

Rather than try to edit Local and Global, it would be better to have a "Stack" or "CurrentFrame" group list all variables in GetFrameVariables.

andyleejordan · 2021-11-16T23:25:01Z

Auto variable does update per stack frame for the auto variables, or at least it did in my testing.

Auto is not in my testing.

Local and Global do not because, as discussed, there is no actual correlary between stack frames and scopes

Yup, yup, no I get that there isn't a precise correlation and that it doesn't hold up across the board. It's annoying because there's at least some correlation in that the top stack frame is scope 0, and its parent scope is (most likely? always? at least a vast majority of the time?) scope 1...but yeah ok, you're right with "presenting inaccurate variables are considered worse than not presenting them at all." I think I agree, I just wish there was a better solution.

Rather than try to edit Local and Global, it would be better to have a "Stack" or "CurrentFrame" group list all variables in GetFrameVariables.

Well and this won't work well at all because we found GetFrameVariables is broken for our needs.

Ugh, ok, this is an improvement and a bug fix. Let me see if the Auto variables can be updated per frame, as you said it was working for you.

andyleejordan · 2021-11-16T23:30:09Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+            }
+            catch (CmdletInvocationException ex)
+            {
+                if (!ex.ErrorRecord.CategoryInfo.Reason.Equals("PSArgumentOutOfRangeException"))


Suggested change

if (!ex.ErrorRecord.CategoryInfo.Reason.Equals("PSArgumentOutOfRangeException"))

// It's possible to be asked to run `Get-Variable -Scope N` where N is a number that

// exceeds the available scopes. In this case, the command throws this exception,

// but there's nothing we can do about it and we shouldn't crash the debugger, so we

// just return no results instead. All other exceptions should be thrown again.

if (!ex.ErrorRecord.CategoryInfo.Reason.Equals("PSArgumentOutOfRangeException"))

Suggested comment to resolve Paul's request.

andyleejordan · 2021-11-16T23:31:02Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs


            // This glorious hack ensures that Get-PSCallStack returns a list of CallStackFrame
            // objects (or "deserialized" CallStackFrames) when attached to a runspace in another
            // process.  Without the intermediate variable Get-PSCallStack inexplicably returns
            // an array of strings containing the formatted output of the CallStackFrame list.
-            var callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";
-            psCommand.AddScript($"{callStackVarName} = Get-PSCallStack; {callStackVarName}");
+            string callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";


While true, the only change here is from var to string. We can do this later.

andyleejordan · 2021-11-16T23:35:30Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+            // If we're attached to a remote runspace, we need to serialize the callstack prior to transport
+            // because the default depth is too shallow
+            bool isOnRemoteMachine = _psesHost.CurrentRunspace.IsOnRemoteMachine;
+            string returnSerializedIfOnRemoteMachine = isOnRemoteMachine


Suggested change

string returnSerializedIfOnRemoteMachine = isOnRemoteMachine

// The next command always calls `Get-PSCallStack`. On a local machine, we just return

// its results. On a remote machine we serialize it first and then later deserialize it.

string returnSerializedIfOnRemoteMachine = isOnRemoteMachine

Suggested comment to resolve Paul's request.

JustinGrote · 2021-11-17T00:23:12Z

@andschwa example of the auto variables updating thru the stack during a Pester Test on PoshNmap. Remember we only currently capture $args and $_/$PSItem to Auto, we can either expand this to include all "current" stack variables that aren't some common noisy ones, or put them in another variable pane named "CurrentFrame"

Capture.mp4

andyleejordan · 2021-11-17T02:03:13Z

@JustinGrote I'm not seeing any auto variables now. I've built your branch (and double checked it's up-to-date except rebased on master, and that my extension is using the right build through the symlink). I'm in a Debug configuration. How are you testing? Also FYI I'm on macOS with PowerShell 7.2.

JustinGrote · 2021-11-19T00:32:01Z

Closing in favor of #1630

JustinGrote mentioned this pull request Nov 5, 2021

Debugging with active breakpoint crashes Powershell session PowerShell/vscode-powershell#3667

Closed

5 tasks

JustinGrote marked this pull request as ready for review November 5, 2021 01:05

JustinGrote requested a review from andyleejordan as a code owner November 5, 2021 01:05

JustinGrote requested a review from SeeminglyScience November 5, 2021 01:05

andyleejordan reviewed Nov 5, 2021

View reviewed changes

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs Outdated Show resolved Hide resolved

JustinGrote marked this pull request as draft November 5, 2021 17:23

JustinGrote changed the title ~~Ignore scoping issues with fetching local variables~~ Fix: Rework Variable Handler to not rely on scope iteration and use serializer for remote runspaces Nov 6, 2021

JustinGrote marked this pull request as ready for review November 6, 2021 21:56

JustinGrote requested a review from andyleejordan November 6, 2021 21:56

SeeminglyScience requested changes Nov 9, 2021

View reviewed changes

JustinGrote and others added 7 commits November 9, 2021 13:34

Ignore scoping issues with fetching local variables

8685daf

New Serializer for PS Variables to support remoting. BROKEN: ID refer…

d34bf1b

…ences not matching up

Match on both properties and noteproperties

df4d194

Make cmdletinvocationexception less specific

e9013f3

Don't serialize local codepaths

491bff8

Make serializationDepth constant

43bbaec

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

callStackVarName explicit typing

1bf4c79

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

andyleejordan changed the title ~~Fix: Rework Variable Handler to not rely on scope iteration and use serializer for remote runspaces~~ Improve debugger's variable population mechanism Nov 11, 2021

andyleejordan requested changes Nov 11, 2021

View reviewed changes

JustinGrote marked this pull request as draft November 11, 2021 18:52

JustinGrote added 5 commits November 11, 2021 17:02

First Pass: Simplify Local and Stack Variable Collection

d8dfbbc

Fix fetching script when I wanted to fetch local

8253a9a

Cleanup unnecessary autoVariable assignments

56044e2

Expose both _ and PSItem

16b294c

Fix auto variable names

5ed6fe8

JustinGrote marked this pull request as ready for review November 12, 2021 05:47

JustinGrote requested review from andyleejordan and SeeminglyScience November 12, 2021 05:48

andyleejordan reviewed Nov 13, 2021

View reviewed changes

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/StackFrameDetails.cs Show resolved Hide resolved

andyleejordan reviewed Nov 13, 2021

View reviewed changes

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs Show resolved Hide resolved

PaulHigin reviewed Nov 16, 2021

View reviewed changes

andyleejordan approved these changes Nov 16, 2021

View reviewed changes

andyleejordan mentioned this pull request Nov 19, 2021

Improve debugger's variable population mechanism #1630

Merged

JustinGrote closed this Nov 19, 2021

JustinGrote deleted the justingrote/IgnoreScopeIssue branch November 19, 2021 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve debugger's variable population mechanism #1620

Improve debugger's variable population mechanism #1620

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021

JustinGrote commented Nov 5, 2021

JustinGrote commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

andyleejordan commented Nov 5, 2021

andyleejordan commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

JustinGrote commented Nov 6, 2021

SeeminglyScience left a comment

JustinGrote commented Nov 11, 2021

andyleejordan left a comment

JustinGrote commented Nov 11, 2021

andyleejordan commented Nov 11, 2021

JustinGrote commented Nov 11, 2021 •

edited

Loading

JustinGrote commented Nov 12, 2021

PaulHigin Nov 16, 2021

PaulHigin Nov 16, 2021

andyleejordan Nov 16, 2021

PaulHigin Nov 16, 2021

andyleejordan Nov 16, 2021

andyleejordan commented Nov 16, 2021

JustinGrote commented Nov 16, 2021 •

edited

Loading

andyleejordan commented Nov 16, 2021

andyleejordan Nov 16, 2021

andyleejordan Nov 16, 2021

andyleejordan Nov 16, 2021

JustinGrote commented Nov 17, 2021

andyleejordan commented Nov 17, 2021 •

edited

Loading

JustinGrote commented Nov 19, 2021

-                if (!ex.ErrorRecord.CategoryInfo.Reason.Equals("PSArgumentOutOfRangeException"))
+                // It's possible to be asked to run `Get-Variable -Scope N` where N is a number that
+                // exceeds the available scopes. In this case, the command throws this exception,
+                // but there's nothing we can do about it and we shouldn't crash the debugger, so we
+                // just return no results instead. All other exceptions should be thrown again.
+                if (!ex.ErrorRecord.CategoryInfo.Reason.Equals("PSArgumentOutOfRangeException"))

Improve debugger's variable population mechanism #1620

Improve debugger's variable population mechanism #1620

Conversation

JustinGrote commented Nov 5, 2021 • edited Loading

JustinGrote commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

JustinGrote commented Nov 5, 2021 • edited Loading

JustinGrote commented Nov 5, 2021 • edited Loading

JustinGrote commented Nov 5, 2021 • edited Loading

JustinGrote commented Nov 5, 2021

JustinGrote commented Nov 5, 2021

JustinGrote commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

andyleejordan commented Nov 5, 2021

andyleejordan commented Nov 5, 2021

SeeminglyScience commented Nov 5, 2021

JustinGrote commented Nov 6, 2021

SeeminglyScience left a comment

Choose a reason for hiding this comment

JustinGrote commented Nov 11, 2021

andyleejordan left a comment

Choose a reason for hiding this comment

JustinGrote commented Nov 11, 2021

andyleejordan commented Nov 11, 2021

JustinGrote commented Nov 11, 2021 • edited Loading

JustinGrote commented Nov 12, 2021

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

andyleejordan commented Nov 16, 2021

JustinGrote commented Nov 16, 2021 • edited Loading

andyleejordan commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinGrote commented Nov 17, 2021

andyleejordan commented Nov 17, 2021 • edited Loading

JustinGrote commented Nov 19, 2021

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 5, 2021 •

edited

Loading

JustinGrote commented Nov 11, 2021 •

edited

Loading

JustinGrote commented Nov 16, 2021 •

edited

Loading

andyleejordan commented Nov 17, 2021 •

edited

Loading