Skip to content
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

Feature: Interpreter Debug Hooks #73

Merged
merged 14 commits into from
Nov 11, 2021
Merged

Conversation

tigh-latte
Copy link
Contributor

@tigh-latte tigh-latte commented Oct 13, 2021

Currently there is no observability into the execution of the interpreter, be it during or after.

To try to address this, I've added the notion of a Debugger which by default is no-op, but can be configured to expose a series of hooks throughout a thread's runtime. Attaching onto these hooks provides a copy of the executing thread's runtime state, at the point of the function call.

Also, as a direct result of this, ParsedOpcode is likely going to see more usage in an exported context, so I've cleaned up that struct a bit to make it more user friendly.

Usage example:

func onOpcode(state *interpreter.ThreadState) {
    // log state
}

func myCoolFunc() {

    . . .

    engine := interpreter.NewEngine()

    . . .

    debugger := debug.NewDebugger()
    debugger.AttachAfterExecuteOpcode(onOpcode)

    engine.Execute(
        interpreter.WithScripts(ls, uls),
        interpreter.WithDebugger(debugger),
    )
}

@tigh-latte tigh-latte added the work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge label Oct 13, 2021
@mergify mergify bot added the enhancement New feature or request label Oct 13, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2021

Codecov Report

Merging #73 (63d8d39) into master (d573f48) will decrease coverage by 1.83%.
The diff coverage is 78.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   85.89%   84.05%   -1.84%     
==========================================
  Files          30       34       +4     
  Lines        3034     3293     +259     
==========================================
+ Hits         2606     2768     +162     
- Misses        285      380      +95     
- Partials      143      145       +2     
Impacted Files Coverage Δ
bscript/interpreter/debug/options.go 0.00% <0.00%> (ø)
bscript/interpreter/state.go 2.85% <2.85%> (ø)
bscript/interpreter/options.go 51.85% <40.00%> (-14.82%) ⬇️
bscript/interpreter/opcodeparser.go 79.68% <87.27%> (-1.70%) ⬇️
bscript/interpreter/debug/debugger.go 87.75% <87.75%> (ø)
bscript/interpreter/stack.go 93.29% <88.88%> (-0.59%) ⬇️
bscript/interpreter/thread.go 93.75% <98.33%> (+0.24%) ⬆️
bscript/interpreter/debug.go 100.00% <100.00%> (ø)
bscript/interpreter/engine.go 100.00% <100.00%> (ø)
bscript/interpreter/operations.go 96.16% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d573f48...63d8d39. Read the comment docs.

@tigh-latte tigh-latte removed the work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge label Oct 14, 2021

// ThreadState a snapshot of a threads state during execution.
type ThreadState struct {
DStack [][]byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's DStack? is this the main stack and AStack is the alt stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah dstack is the main data stack and astack is the alt stack, do you think these should be named better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm used to main stack and alt stack but it's fine

Op opcode
// ParsedOpcode is a parsed opcode.
type ParsedOpcode struct {
op opcode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason why you made this private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes because the data type is both private, and is used as an interally to map opcodes with their byte value, their name, and their function. I worried exposing it in a non-readonly way would allow users to mess with the state of the interpreter mid execution, either via value reassignment or calling opcode functions.


// WithDebugger enable execution debugging with the provided configured debugger.
// It is important to note that when this setting is applied, it enables thread
// state cloning, at every configured debug step.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "thread state cloning"?

Copy link
Contributor Author

@tigh-latte tigh-latte Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each attachment point calls thead.state(), which takes a snapshot of the current threads state and clones it into a ThreadState struct. This is done to ensure that if the user modifies the state in these debug functions, it doesn't impact the state of the interpreter.
I thought it was necessary to call this out because, if it was happening both before and after each opcode execution, it could end up being an expensive enough operation.

This is just a word of caution for those looking to enable these features on production performance critical systems.

@jadwahab
Copy link
Member

jadwahab commented Oct 15, 2021

good stuff mate, it looks pretty good froma high level.. what would be the easiest way for me to test it out?

also have you seen this debugger (it's really out of date and has some issues but serves as a nice PoC)? we've done some similar shit internally - ask me about it next time and i'll tell you more about it

@tigh-latte tigh-latte added the work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge label Oct 18, 2021
@tigh-latte
Copy link
Contributor Author

Placing work-in-progress label back on this as I've had a couple more ideas on where this could go.

@jadwahab right now you'd just need to write some code and run it, but I'll get working on an example.go file that you could run, which executes a script step by step.

@tigh-latte tigh-latte marked this pull request as draft October 19, 2021 11:18
@mergify mergify bot closed this Nov 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 9, 2021

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

@mergify mergify bot added the stale label Nov 9, 2021
@tigh-latte tigh-latte reopened this Nov 10, 2021
@tigh-latte tigh-latte marked this pull request as ready for review November 10, 2021 16:56
@tigh-latte tigh-latte removed stale work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge labels Nov 10, 2021
@mergify mergify bot merged commit 4f458d2 into master Nov 11, 2021
@mergify mergify bot deleted the enhancement/interpreter-hooks branch November 11, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants