Debugger improvements 4b #1113

Jither · 2022-03-22T21:36:04Z

And we're back...

I figure we might as well try to find a way to merge this, before I might randomly leave it for another year (although no plans to do so yet, my work assignments are increasing). 🙂 Also, before it gets an (even more) unwieldy amount of changes (although few requirements remain, I think). It's proven its worth and usefulness (to me) on the VSCode DebugAdapter so far, at least.

Much of this is verbatim from the previous pull request: #1018 - so I'll include the change list from that pull request, marking new stuff compared to then with a nice light bulb:

Discussion point 🤔

I'm not sure about the DebugHandler's own methods for evaluation of expressions - it's needed in order to reuse the active execution context, and avoid an evaluated expression trashing the engine's call stack. However, it shares most of its code with Engine's DoInvoke (inside Execute). It might be a good idea to combine the two?

Summary of changes

bold areas = breaking changes (breaking for debugger implementations, that is)

💡 Closer to "standard" step implementation. That is, block statements (i.e. the brackets themselves) are no longer a step, while the loop statements include stepping through the relevant expressions (initialization, condition, for-loop update; left side of for-of and for-in).
💡 More intuitive Step and Break event behavior. Break is now no longer triggered while stepping - neither for breakpoints nor for the debugger statement. In other words, it's only called if StepMode is None. Because the same event handler delegate type is used for both, they can easily be combined in a single handler if there's no need to distinguish between the two conditions.
💡As a result of the above, it now makes most sense that the DebugHandler starts with StepMode.None by default (to make breakpoints "just work"). An option has been added - InitialStepMode - to allow changing that default.
💡 Breakpoint property is set on DebugInformation whenever a breakpoint is reached (and its condition is true) - both in the Step and Break events. This allows still handling (and potentially extending functionality of) breakpoints, even if the breakpoint didn't actually trigger Break (due to the change above). This is used in the Jint.DebugAdapter to add "hit count/hit condition" functionality (where the counter needs increment even when stepping through the breakpoint), and for log points.
💡PauseType is set on DebugInformation to indicate what kind of pause caused Break or Step triggering - Breakpoint, Step, or DebuggerStatement. In future, would be expanded with e.g. Exception (when/if exceptions are centralized enough for them all to call DebugHandler before throwing).
💡 Method for evaluating expressions in current execution context (e.g. while paused) - DebugHandler.Evaluate. Used by conditional breakpoints, and useful for everything from debugger watch expressions to a console REPL. This also fixes issues with execution context for previous conditional breakpoint implementation.
💡Lazy evaluation (of callstack/scopes) in DebugInformation. While most full debuggers would need it, there's no point allocating and building it for many simpler step/break handlers (including in the unit tests)
💡CurrentLocation property on DebugHandler - available even when not stepping/breaking (as long as DebugMode is enabled). This allows e.g. console object implementations to include the script location of the call writing to the console.
💡 Event** (Parsed) on engine for notification of when scripts have been through Esprima. This avoids the need for double parsing, because debugger implementations are likely to need the AST too (for e.g. determining proper locations for breakpoints). - this addition from the previous pull request has been removed for now - it needs a better approach to also support Modules.
Closer to Chromium devtools scopes: Splitting global scope into Global (Object Environment Record) and Script (Declarative Environment Record).
Debug scopes now hold a reference to the BindingObject for OER's - this eases inspection a lot in devtools - because the BindingObject can simply be serialized for the protocol, rather than copying every single property to a new object and serializing that.
Breakpoints:
- Only allows a single breakpoint at a given location (source/line/column) - hence, API is now "Set" instead of "Add".
- Requires the breakpoint to be set at exactly the correct line/column (this allows faster matching, column breakpoints, and more).
- Additional BreakPoint tests
- Breakpoint class unsealed - it's useful for a debugger to be able to add additional data to the class. As such, the data important to the Jint side (other than condition - source, line, column) has moved into a sealed BreakLocation class.
Fixed tests broken by addition of Script scope and new breakpoint semantics.
Fixed inspection of uninitialized block scoped bindings (const/let)
💡Tests for new and changed functionality.

Phew! 😂

BindingObject property on Global and With scopes (mainly for devtools for now, but may be useful for VSCode DebugAdapter)

Only allows a single breakpoint at a given location (source/line/column) - hence, API is now "Set" instead of "Add". Requires the breakpoint to be set at *exactly* the correct line/column (this allows faster matching, column breakpoints, and more). Fixed tests broken by addition of Script scope and new breakpoint semantics. Breakpoints still need work on the API (and probably implementation) side. Additional BreakPoint tests

Test case for failing breakpoint conditions (currently they, and other evaluations during paused state) reset the call stack (and probably other things). Documentation of SourceParsedEventArgs

Renamed DebugHandler OnBreak event to clarify that it only handles debugger statements. Added PauseType (Break/Step/DebuggerStatement) to DebugInformation to allow debuggers to distinguish between breaks and debugger statements.

Removed unused Source property from SourceParsedEventArgs

Removed redundant BreakPoint constructors

Option for initial StepMode - it's now StepMode.None by default to properly handle breakpoints from the start. Previously, it was StepMode.Into Updated tests to reflect new initial StepMode

Error messages for a few TypeErrors

DebugHandler cleanup and more documentation

Currently unused, and a better approach for getting AST for scripts is needed, in order to support Modules etc.

lahma

Looks good and great to see such test coverage, you've really put the effort in! I added some comments, please see if makes sense to address.

I'm not sure about the DebugHandler's own methods for evaluation of expressions - it's needed in order to reuse the active execution context, and avoid an evaluated expression trashing the engine's call stack. However, it shares most of its code with Engine's DoInvoke (inside Execute). It might be a good idea to combine the two?

It's an option, but can also bind the two needlessly. I think you can make the call 🙂

lahma · 2022-03-23T08:15:01Z

Jint/Runtime/Debugger/BreakLocation.cs

+using System;
+using System.Collections.Generic;
+
+namespace Jint.Runtime.Debugger


could use file-scoped namespaces for new files and set #nullable enable to reduce later work

lahma · 2022-03-23T08:15:39Z

Jint/Runtime/Debugger/BreakLocation.cs

+    /// Equals returns true if all properties are equal - or if Source is null on either BreakLocation.
+    /// GetHashCode excludes Source.
+    /// </remarks>
+    public sealed class OptionalSourceBreakLocationEqualityComparer : IEqualityComparer<BreakLocation>


should this be internal, maybe in it's own file (same namespace and nullable rules would apply)

lahma · 2022-03-23T08:16:28Z

Jint/Runtime/Debugger/BreakPoint.cs

-    public sealed class BreakPoint
+    // BreakPoint is not sealed. It's useful to be able to add additional properties on a derived BreakPoint class (e.g. a breakpoint ID
+    // or breakpoint type) but still let it be managed by Jint's breakpoint collection.
+    public class BreakPoint


is there something deriving from this in test project, would make sure nobody (like me) would accidentally seal again, as many changes already could also do the namespace and nullable thing

lahma · 2022-03-23T08:16:55Z

Jint/Runtime/Debugger/DebugEvaluationException.cs

@@ -0,0 +1,18 @@
+using System;
+
+namespace Jint.Runtime.Debugger


could use file-scoped namespaces for new files and set #nullable enable to reduce later work

lahma · 2022-03-23T08:17:09Z

Jint/Runtime/Debugger/DebugEvaluationException.cs

+    /// <summary>
+    /// Thrown when an evaluation executed through the DebugHandler results in any type of error - parsing or runtime.
+    /// </summary>
+    public sealed class DebugEvaluationException : Exception


should this derive from JintException?

lahma · 2022-03-23T08:21:40Z

Jint/Runtime/Interpreter/Statements/JintDoWhileStatement.cs

@@ -50,6 +50,11 @@ protected override Completion ExecuteInternal(EvaluationContext context)
                    }
                }

+                if (context.Engine._isDebugMode)


I think we could promote the value to context.DebugMode and initialize in context constructor to make code cleaner?

Checked, and context.DebugMode is already there - it's just only used once. I've changed all the references to _isDebugMode outside Engine. Only one that's a small issue is:

jint/Jint/Native/Function/ScriptFunctionInstance.cs

Line 144 in 0e315f4

if (_engine._isDebugMode && result.Type != CompletionType.Throw)

I've changed that one to _engine._activeExecutionContext.DebugMode for consistency - now, only Engine and the context constructor read the _isDebugMode field. Could declare context as a local variable, since _activeExecutionContext is also used a few lines earlier - however, I'm not actually sure if the OrdinaryCallEvaluateBody() call might conceivably change the value of it - DebugMode won't care, but anything else that might decide to use that local context variable would.

Looking at the others (all agreed) tonight.

Jither · 2022-03-23T18:40:20Z

I'm not sure about the DebugHandler's own methods for evaluation of expressions - it's needed in order to reuse the active execution context, and avoid an evaluated expression trashing the engine's call stack. However, it shares most of its code with Engine's DoInvoke (inside Execute). It might be a good idea to combine the two?

It's an option, but can also bind the two needlessly. I think you can make the call 🙂

I guess we'll keep them separate - because, yeah, the DebugHandler's evaluate might actually need eventually be closer to 😱 eval() than just an evaluation of a StatementList - although it's "good enough" for now.

#nullable enable and namespace statements on new files Slight improvement of DebugEvaluationException Got rid of DebugHandler references to _engine._isDebugMode (use context) A few new tests

lahma · 2022-03-23T18:55:10Z

@Jither I think we can merge when you feel this is ready 👍🏻

Jither · 2022-03-23T19:53:41Z

@lahma Any time 🙂 - no more changes intended - there's sure to be a debugger-improvements-5 sooner or later, but this is now pretty self-consistent, and supports (in one way or another) almost anything a full-fledged JavaScript debugger needs (Jint.DebugAdapter already shows that, even with its threading issues). The three main things remaining (two "big", but probably not big in terms of code, eventually - the last one small) is:

modules support (engine should preferably notify the DebugHandler whenever a script is loaded, including the first script and any modules).
break on exceptions (requires that Errors get more centralized, and preferably consistent (the "return Completion.Throw vs throwing CLR exceptions" debacle we've discussed before) so that the DebugHandler can get notified at the point of the exception - both for call stack handling, and to allow breaking on script-caught Errors - i.e. first chance exceptions).
support for evaluation on any stack frame (I only realized yesterday that this should already be possible, since every stack frame has an execution context - but the DebugHandler needs to eventually expose an API for it). See https://microsoft.github.io/debug-adapter-protocol/specification - "Evaluate Request" (that page's <a name=""> elements seem to come and go - right now it has a "manual scrolling/search" day...)

Might also eventually consider easing breakpoint management and possibly some other things - Jint.DebugAdapter has some stuff that would be generally useful:

Retrieving a collection of possible breakpoint positions from the AST - that's one of the main reasons the engine needs to notify the DebugHandler when a script is loaded. Would probably make more sense if that was in Jint itself, since only Jint really knows where those potential breakpoint positions are. In other words, https://github.com/Jither/Jint.DebugAdapter/blob/main/Jint.DebugAdapter/Breakpoints/BreakpointCollector.cs only works by way of its knowledge of Jint internals.
Locating the nearest possible breakpoint position (just a binary search through that breakpoint position list).
Maybe a helper class for easing visual debugger implementation - basically this one, eventually: https://github.com/Jither/Jint.DebugAdapter/blob/main/Jint.DebugAdapter/Debugger.cs

But that stuff probably doesn't make sense to consider until Jint.DebugAdapter is more final. 😋

lahma · 2022-03-23T20:50:44Z

Thanks for the rundown, seems quite clear and reasonable future improvements. Once again, great work and thanks for finalizing 4b 😉

Jither added 25 commits March 7, 2022 20:43

Added Parsed event to engine

c000817

Split Global scope into Global (OER) and Script (DER) in debugger

98fffaa

BindingObject property on Global and With scopes (mainly for devtools for now, but may be useful for VSCode DebugAdapter)

Let Scope resolve BindingObject

7af6c24

Slight cleanup

4f571e1

BreakLocation to record

613c410

Sealed classes

99ca6a9

Fixed inspection of uninitialized block scoped bindings

281a614

Member order

b72c854

Silently catch errors in breakpoint conditions

5a13506

Test case for failing breakpoint conditions (currently they, and other evaluations during paused state) reset the call stack (and probably other things). Documentation of SourceParsedEventArgs

Distinguish between breakpoints and debugger statements

2252d61

Renamed DebugHandler OnBreak event to clarify that it only handles debugger statements. Added PauseType (Break/Step/DebuggerStatement) to DebugInformation to allow debuggers to distinguish between breaks and debugger statements.

DebugHandler-specific script evaluation

fd15360

Fix: DebugHandler reentry when evaluating conditional breakpoints

d52430a

Stepping: Skip block statements, include loop expressions

a8c0db3

Removed unused Source property from SourceParsedEventArgs

Include relevant BreakPoint in DebugInformation

4b4d632

Removed redundant BreakPoint constructors

DebugHandler Support for evaluating (preparsed) AST

e9babba

More logical Break/Step events

7ba7a41

Option for initial StepMode - it's now StepMode.None by default to properly handle breakpoints from the start. Previously, it was StepMode.Into Updated tests to reflect new initial StepMode

Cleanup

745594c

Don't react to debugger statement when stepping

3c8961c

Error messages for a few TypeErrors

DebugHandler.CurrentLocation

855729c

DebugHandler cleanup and more documentation

Merge branch 'dev' into debugger-improvements-4b

0cb99b7

Evaluation context guard for DebugHandler Evaluate

8b8ec53

Tests for new DebugHandler functionality

391157c

Removed Engine.Parsed event for now

cc81716

Currently unused, and a better approach for getting AST for scripts is needed, in order to support Modules etc.

Merge branch 'dev' into debugger-improvements-4b

f0f1006

lahma reviewed Mar 23, 2022

View reviewed changes

Jither added 2 commits March 23, 2022 19:50

Changes after review

d76df0d

#nullable enable and namespace statements on new files Slight improvement of DebugEvaluationException Got rid of DebugHandler references to _engine._isDebugMode (use context) A few new tests

Minor variable renaming for test clarity

dfee6dc

lahma approved these changes Mar 23, 2022

View reviewed changes

lahma merged commit 3fa1f83 into sebastienros:main Mar 23, 2022

Jither deleted the debugger-improvements-4b branch March 23, 2022 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debugger improvements 4b #1113

Debugger improvements 4b #1113

Jither commented Mar 22, 2022 •

edited

Loading

lahma left a comment

lahma Mar 23, 2022

lahma Mar 23, 2022

lahma Mar 23, 2022

lahma Mar 23, 2022

lahma Mar 23, 2022

lahma Mar 23, 2022

Jither Mar 23, 2022

Jither commented Mar 23, 2022

lahma commented Mar 23, 2022

Jither commented Mar 23, 2022 •

edited

Loading

lahma commented Mar 23, 2022

		@@ -0,0 +1,18 @@
		using System;

		namespace Jint.Runtime.Debugger

Debugger improvements 4b #1113

Debugger improvements 4b #1113

Conversation

Jither commented Mar 22, 2022 • edited Loading

Discussion point 🤔

Summary of changes

lahma left a comment

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

lahma Mar 23, 2022

Choose a reason for hiding this comment

Jither Mar 23, 2022

Choose a reason for hiding this comment

Jither commented Mar 23, 2022

lahma commented Mar 23, 2022

Jither commented Mar 23, 2022 • edited Loading

lahma commented Mar 23, 2022

Jither commented Mar 22, 2022 •

edited

Loading

Jither commented Mar 23, 2022 •

edited

Loading