microsoft / DbgShell

A PowerShell front-end for the Windows debugger engine.
MIT License
674 stars 89 forks source link

Improve FormatBaseCommand.RenderScriptValue #76

Closed Zhentar closed 5 years ago

Zhentar commented 5 years ago

My gaze was turned upon RenderScriptValue once again, after profiling suggested the majority of the time spent on output was compiling the script text in it. So I took another go at it (repeatedly throwing myself at walls is just how I learn best, okay, don't judge me!). And this time I managed to find a version that can use modules to capture context variables, without using any reflection to get at private members - by using the ForEach-Object cmdlet to invoke the ScriptBlock (incidentally, the trouble with script.InvokeWithContext is that the current ExecutionContext causes the errors to go to the parent context error buffer).

Performance is also significantly improved, from this:

> Measure-Command { x ntdll!* | Format-AltTable }

TotalSeconds      : 197.5878559

To this:

> Measure-Command { x ntdll!* | Format-AltTable }

TotalSeconds      : 18.0818597

Caveats:

jazzdelightsme commented 5 years ago

Thank you for looking at this; I've also noticed the formatting stuff is extra slow. That speedup looks extremely attractive! I'll see if I can see what's up with those caveats. #Closed

jazzdelightsme commented 5 years ago

For this one:

I had to add a ToString override to PipeIndexPSVariable to preserve the desired current behavior. Which isn't a big deal, but I don't understand why

The member m_pipeIndexPSVar is an instance of PipeIndexPSVariable, which is not a variable value; it's the whole PowerShell Variable: the name, the value, other settings. Before, this variable was added to the set of other variables in the context to use when invoking the user script.

After your change, now there's just a little snippet of script: $PipeOutputIndex = $args[0], which creates a variable called "PipeOutputIndex", which has the value of m_pipeIndexPSVar... which is itself a PSVariable.

Sort of like the difference between this:

$var = get-variable PWD
$mypwd = $var

versus:

$var = get-variable PWD
$mypwd = $var.Value # <-- note the .Value

So one way to fix this would be to add ".Value" here:

InvokeCommand.InvokeScript(false, sm_pipeIndexScript, null, m_pipeIndexPSVar.Value);

Of course, at that point, we don't really need a PipeIndexPSVariable type anymore, since we are now creating the variable in script (sm_pipeIndexScript).

All that said... I'm still fiddling with this. I had intended $PipeOutputIndex to be available to formatting scripts, and when done this way, it is not (it works for the index column, because of the scope of where the index column ScriptBlock is created, but the scoping of the formatting script from .psfmt files makes it so they can't see the $PipeOutputIndex variable, even if I set it in a -begin scriptblock). But on the other hand, no formatting script is currently taking advantage of that variable... so maybe that's okay.

But what other subtle and perhaps unexpected differences come into play? I'm still trying to figure it out.

Closed

jazzdelightsme commented 5 years ago

For this one:

(Truncate $childItem.Name $nameColumnWidth $true) stopped working.

Can you elaborate? I didn't notice any problem. The spot that I found that looked like that was in the format view definition for namespace items:

$null = $sb.Append( [MS.Dbg.ColorString]::Truncate($childItem.Name, $nameColumnWidth, $true) )

Was it a different spot? I didn't notice any problems when I attached to a target and ran dir. What went wrong for you? #Closed

Zhentar commented 5 years ago

That's the spot - I included a fix for it in my pull request. In master, it's (Truncate $childItem.Name $nameColumnWidth $true), but with my change Truncate stopped resolving to ColorString.Truncate, so that ends up piping "$childItem.Name $nameColumnWidth $true" to Invoke-DbgEng instead of returning a truncated string. I don't understand why Truncate worked as a command before, though; I don't see any definition for it anywhere. #Closed

jazzdelightsme commented 5 years ago

Ah, I see.

What was happening there is that before, the Truncate function was being picked up from the FmtUtils.ps1 script. -PreserveHeaderContext was used for that view definition, so that variables created in the GroupBy header-producing script (-ProduceGroupByHeader), specifically $nameColumnWidth and $summaryColumnWidth, would be available in the per-item script block. But using -PreserveHeaderContext causes that per-item ScriptBlock to get re-bound:

if( preserveHeaderContext )
{
    m_groupByHeaderContext = new PSModuleInfo( false );
}

...

// Let the script also use context created by the custom header script (if any).
if( m_groupByHeaderContext != null )
{
    script = m_groupByHeaderContext.NewBoundScriptBlock( script );
}

And we lost access to the functions from the FmtUtils.ps1 script when that happened. So that's why the Truncate function was not found. As for how to re-wire or re-arrange things to work... I'm not sure off the top of my head. (What you did, replacing the Truncate call, is fine for this specific case, of course; I'd just like to be able to solve the general pattern of "I've got some some code that I'd like to share between view definitions, so I package them up in a function, but they're not really intended to be "public" functions.")

And then for the bit about Truncate resulting in a call to Invoke-DbgEng... yeah, that can be surprising. The idea with that feature is that we have a CommandNotFoundAction event handler, and if a command is not found, we just send it to dbgeng. This lets you just type stuff that you would in windbg, and it executes as you would expect. However, in error cases (typos, or cases like this), it just results in confusion. I've thought about making it configurable so you can turn that behavior on or off.

Finally, it was a pain to see what those errors were, so I took the liberty of pushing a bit of code that adds those errors to $error. (I hope that's alright; I wasn't sure what the best way to collaborate was.)


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

Zhentar commented 5 years ago

I don't mind at all 😃

Not sure why I missed the Truncate in FmtUtils when I searched before, but that helped clear things up quite a bit. After thinking about it further, it seemed like script.Module.Clone() would do the trick, but it was a dead end - While it clones the module, it does not clone the SessionState, so state can bleed between invocations. But Import-Module does seem to do the trick - and while it does add some overhead cost it's still nearly nothing compared to the cost of script compilation. Though I imagine there are some semantic subtleties here that I'm not aware of that could be gotchas. #Closed

jazzdelightsme commented 5 years ago
        if( ${_tmp_ scrambled} )

There is a problem here. Try:

ntna
$stuff = x TestNativeConsoleApp!wmain
ln $stuff[0].Address

Expected result: list of "near symbols"; no errors.

Actual result: There's an error about "The variable '$tmp scrambled' cannot be retrieved because it has not been set." #Closed


Refers to: DbgProvider/Debugger.psfmt:2220 in 82c0894. [](commit_id = 82c089439e416c96930c8487cc5ae35e3cd63f31, deletion_comment = False)

Zhentar commented 5 years ago

Sorry, I got sidetracked for a week there playing with the .NET Core 3 hardware intrinsics 😅 Should be all fixed up now #Closed

jazzdelightsme commented 5 years ago

Thank you!


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