-
-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debugger improvements 3 #852
Conversation
They are now relevant (and pass), after the improvements to call stack. Fixed a mistake in previously skipped StepsOverSetAccessor test
DebugInformation now includes public representation of internal Engine callstack, rather than just list of Callable names
The refactor of Call and Construct really simplifies things a lot. I've removed DebugHandler's internal management of the call stack, and use Engine's instead. This does mean I've added an internal property to |
Nothing bothers me with this PR. That's a good think I guess. |
Great to hear that the call stack work reflected also to debug handler positively, looking good! If you find ways to improve the current call stack handling please feel free to do so. |
The only issue I've seen so far with the call stack handling is that it doesn't quite seem to collaborate with try/catch - the stack isn't popped appropriately. I.e., here, thrower isn't popped when catching, so after leaving Other than that, well into proper scope handling in the debugger. A few observations:
|
If you can formulate a test case showing the problem it would be a good issue in the backlog to take care of 👍🏻
I don't have strong opinions in the overall debugger experience, but there's a PR unfortunately left behind #354 that has the Chrome protocol that should be resurrect by any means if possible. So not an answer but a remark.
Maybe 😄 GlobalEnvironment is populated by global functions and
Not sure if this is what you're after, but if it's an arrow function then jint/Jint/Runtime/Environments/FunctionEnvironmentRecord.cs Lines 29 to 44 in b271537
I wouldn't keep that at the top of priorities, The debugger looks great! |
Added two cases in #853
I think the DebugAgent part of that PR should be very helpful. The changes to the debugger and engine itself may have been either implemented since, or the engine has probably changed too much for a merge.
Yeah - for now, I'll just assign them to Global scope.
Will have a look later. 😃
Only a "bit" evil? But then, their very evilness is what might make proper display of eval and with scope useful in some cases (forensics, e.g.). But yeah, I mainly looked at them for completion's sake.
Thanks 😃 It's still very finicky (doesn't reset the engine if adding a new script etc.), but it's useful enough for testing outside of the project I'm using Jint for (where the debugger is more evolved, but tied to other app internals). |
Slight refactor of DebugInformation
Not really sure why the latest commit fails:
I'm thinking I'll refactor the DebugInformation from Jint to align more with devtools protocols (and optimize somewhat for cases where most of the DebugInformation isn't needed - e.g. make call stack and scope chain lazily generate from the engine rather than building them upfront). Then maybe at some point have a look at whether #354 can be used and extended to get devtools support (that would be a separate PR, obviously).
Not quite 🙂 Chromium's debugger (and by extension devtools protocol) has 10 scope types (global, local, with, closure, catch, block, script, eval, module, wasm-expression-stack). Currently,
Other than that, + redesign of DebugInformation, and tests (which I've put off, because most of them would be using |
Lazily populated scope chains New DebugInformation more closely aligned with devtools protocol Currently this causes regression (and failing tests) due to lack of access to the ExecutionContext through JintCallStack
In order to allow generation of scope chains for all call frames in the call stack (for devtools), we need access to the current lexical environment all the way down the call stack. Fixed tests checking in Block scope for const/let at the top level of functions - they're now collapsed to Local scope, mirroring the Chromium approach.
One step towards preparing the debug handling for devtools protocol (and make it more useful in general): Providing scope chains for all call stack frames, not just the current frame. To that end, when creating the The generation of the Other than that, the scope chains are now closer to Chromium, collapsing top-level |
Scope Chain tests More CallStack tests Return point stepping test Simplified function naming tests for CallStack (using TestHelpers)
I think this is now in a state where it's ready for review. All functionality within scope (no pun intended) of this PR are done now, and test coverage is decent (although I may think up more). In the latest commit, had to circumvent some optimization of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some nits and remarks.
Jint/Runtime/Debugger/CallFrame.cs
Outdated
_scopeChain = new Lazy<DebugScopes>(() => new DebugScopes(Environment)); | ||
} | ||
|
||
private JsValue GetThis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks a lot like Engine.GetThisEnvironment
, could these share logic somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be OK to move GetThisEnvironment
to LexicalEnvironment
(since CallFrame
needs it for different execution contexts), or keep it in Engine
but add an optional LexicalEnvironment
parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to move. Only name is important as it's referred to in the spec algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to ExecutionContext
. I've changed calls to call (_engine).ExecutionContext.GetThisEnvironment
minimizing method calls - but let me know if you prefer a stub Engine.GetThisEnvironment
calling ExecutionContext.GetThisEnvironment
instead (guessing it can be inlined anyway).
That should be all in terms of addressing the remarks - although there's plenty of new stuff to remark on now. 😆
_debugCallStack.Push(name); | ||
} | ||
Location location = statement.Location; | ||
BreakPoint breakpoint = _engine.BreakPoints.FirstOrDefault(breakPoint => BpTest(location, breakPoint)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR but wonder why Engine.BreakPoints
is a public list, probably would require better abstraction and offer BpTest
as internal service instead of these context-capturing LINQ queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must admit, I'm wondering why BreakPoints
is even a property on Engine
, which never uses it. 🙂 Other than for ease of access in API. But I might try to move it closer to the DebugHandler
for this PR - now that it's breaking all other debugging APIs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all debugger related properties, events etc. into DebugHandler
, and turned that into a public property. The only "issue" with that is that the sender
of Step/Break events should conventionally be DebugHandler
then, but that's rather useless (but then, sender
sometimes is...) - since it only has the two public events and the BreakPoints
collection. What's the verdict on keeping the sender
as Engine
? Or should the two events and their invocations remain in Engine
?
Jint/Runtime/Debugger/DebugScope.cs
Outdated
/// </summary> | ||
public class DebugScope : IReadOnlyDictionary<string, JsValue> | ||
{ | ||
private IReadOnlyDictionary<string, JsValue> variables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably have this as Dictionary<string, JsValue>
as construction is internal, concrete type as field gives best access performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a "bit" unfinished, since we may not want a dictionary in the first place. It's somewhat left over, and probably overkill - the only real reason for its creation is to leave out shadowed bindings in outer scopes. But that might as easily be done with just a List of the names, with value being retrieved on-demand. Which would also be better for the most common use case - iteration of all of the bindings (for display in Scopes
panel). Especially since we may want to order them in the future. For example, currently Global
scope lists all intrinsic global properties first, with user variables - which are arguably more "important" - coming last. And of course, as it is now, any order of the return bindings is not guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking further on the API of DebugScope...
I may drop the flattening of FunctionEnvironmentRecord
and its child DeclarativeEnvironmentRecord
into "Local/Closure" scope, and let it be up to the caller to flatten them if wanted. Although V8 does this flattening, and it's nice for clarity, I suspect it's mainly a result of internal V8 optimization rather than a "user friendliness choice". Basically like how Jint includes Script
top-level block scoped bindings in Global
. And how V8 also removes closure bindings that aren't needed.
The argument for not flattening them is that it adds some amount of unneeded complexity to the iteration of bindings - and inefficiency to random access value retrieval - in each scope:
Since each binding name may be resolved in a different record, at the very least the DebugScope
would need to iterate over each of its records, then their associated binding names, just for enumeration. And random access retrieval of a binding's value would require a HasBinding
call to each record (the most efficient way I can think of, most often just involving a Dictionary lookup - rather than needing to look up in the ordered list of binding names in DebugScope
).
So, probably better to have 1 environment record per scope. There may also be cases where the caller would want more granular scopes.
The only issue when the caller wants to flatten the scopes, is that because DebugScopes
leaves out empty scopes, you cannot distinguish top-level block scoped variables (which should be flattened) from an empty top-level with block scoped variables in a BlockStatement
inside. But that could be solved with a boolean TopLevel
property on the DebugScope
.
Might also leave the addition of this
to the Local scope to the caller - since some callers may not actually want it included (devtools doesn't) - and it's available from CallFrame.This
(where devtools wants it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dictionary access replaced with Bindings and BindingNames lists on DebugScope Lazy binding value evaluation for scopes Removed this from scope bindings - can be found with CallFrame.This Function top-level block and function scopes are no longer combined - can be combined by caller using new IsTopLevel property on top-level block scopes.
Removed LINQ evaluation of breakpoints. Renamed BreakPoint properties and parameters to reflect the terminology of the rest of Jint (and Esprima)
{ | ||
Condition = condition; | ||
} | ||
|
||
public string Source { get; } | ||
public int Line { get; } | ||
public int Char { get; } | ||
public int Column { get; } | ||
public string Condition { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required for this PR, but I think we should implement equality at some point, would also ensure uniqueness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, would also be needed, if FindMatch
is turned into a dictionary lookup - which it could be, if we require that Line/Column are accurate.
|
||
namespace Jint.Runtime.Debugger | ||
{ | ||
public class BreakPointCollection : ICollection<BreakPoint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah now I see that it's a separte datatype, so AddBreakPoint
not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the reason it "needs" to be at least a readable collection is that currently the only way to remove a breakpoint again is passing a reference to it - which e.g. in my case - means nasty LINQ SingleOrDefault
shenanigans, rather than keeping my own list of the added breakpoints. Until BreakPoint
has been looked through (equality etc.), I didn't see much idea in adding e.g. a Remove(int line, int column, string source = null)
method. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looking good to go, @sebastienros could you also please check this?
@sebastienros your friendly scheduled reminder here, can we merge? 😉 |
Looks better than before, right? |
I don't think we have that, or correct me, a list of sources and information about it. Like all the scripts an engine would be loaded with, such that we could also render where the line of code is coming from (filename for instance). I might have implemented it already or at least partially, sounds familiar ;) Just saying it could help a debugger open the sources from a running engine. |
I believe @Jither has something cool in the works regarding the Chrome debug protocol 😎 |
Thanks @Jither for the hard work and hopefully you have time to further improve with ideas you've listed! |
I hope so 😉 Thanks for approving, again 😃
You've already implemented it, I think, in both Esprima.NET and Jint - Breakpoints already use The same property works fine for communication with devtools - since devtools allows any string as
Already done 😉 - although it needs a lot of work still. And it's based on a fork that's already slightly further ahead than this PR (and which I just managed to mess up by being overeager with force push, so reconstructing now). |
@sebastienros - if interested, I've now reconstructed the branch that adds further changes to debugging for devtools protocol (and debugging in general). The very-much-work-in-progress (but working in terms of a proof of concept) repo for the devtools protocol endpoint is here: https://github.com/Jither/Jint.DevToolsProtocol |
ETA: Summary of changes/fixes:
DebugInformation
CallStack
simplifiesDebugHandler
Engine
toDebugHandler
.Step/Break
.return
statements with literal argument would not callDebugHandler.OnStep
.This obviously includes (entirely) breaking changes to the Debugging API.
Original post:
Just getting back to have another look at the debugger.
One fix I made pretty soon after the last debugger pull request was merged, was adding a guard against Pause reentrancy. It's explained in the test below, but the gist is that Step/Break event handlers may want to do evaluation through the engine (e.g. for conditional breakpoints, or evaluating getters). Doing so would cause reentrance, and since interactive debuggers will be on a separate thread, that would cause a deadlock.
The other part for now is reinstating all the skipped tests for Debugger handling of accessors. They all "automagically" pass after the call stack improvements. 😃 (after fixing a typo in one)
I've marked the pull request as a draft (but I'm fine with marking it ready if wanted), since I do intend to also have a look at proper handling of scopes. Changes in the runtime there means that consts will "disappear" from the current
Locals
/Globals
when they're initialized - might be time to get proper division into global/local/block/catch/etc. Also going to look through the updated code for best way to add a debugger event after the last statement but before a call returns.I've tagged this "debugger-improvements-3", in spite of there being no "2", since there were actually two debugger-improvements pull requests in the past.