Debugger

[mostafa]

Description

As you probably have read the issue https://github.com/dop251/goja/issues/294, this PR adds support for a debugger and exposes some APIs. The PR adds a bare minimum debugger that can control execution and wait for the commands from any frontend, like goja_debugger as a CLI frontend, and possibly DAP and other possible frontends.

Currently these commands, interfaces and utility functions are implemented:

// Compile script in debug mode
func CompileASTDebug(*js_ast.Program, bool) (*Program, error)

// Attach/detach a debugger
func (*Runtime) AttachDebugger() *Debugger
func (*Debugger) Detach()

// Resume running the script until the next debugger statement or breakpoint
func (*Debugger) Continue() ActivationReason

// Breakpoints (add, remove, list)
func (*Debugger) SetBreakpoint(string, int) error
func (*Debugger) ClearBreakpoint(string, int) error
func (*Debugger) Breakpoints() (map[string][]int, error)

func (*Debugger) StepIn() error
func (*Debugger) Next() error
func (*Debugger) Continue() error
func (*Debugger) Exec(string) (Value, error)
func (*Debugger) Print(string) (string, error)
func (*Debugger) List() ([]string, error)

// Return current values of the current script
func (*Debugger) Line() int
func (*Debugger) Filename() string
func (*Debugger) PC() int

Tests

Currently only three tests are written and clearly more tests are needed. These tests actually show failures in different parts, from failing to report correct line number to local variable retrieval issue in exec and print commands. Other tests will be added. The ultimate "E2E" test is the CLI frontend: goja_debugger.

Discussion

1. For the local variable resolution, I found that dbg.vm.stack[dbg.vm.sb+dbg.vm.args+n] returns the correct value on the stack, but it isn't mapped to anything, which is understandable in terms of optimizations. How can we figure out the n from the varName?
2. It seems that having sourcemap reference in the code helps greatly with pinpointing lines in the source code, so I added sourcemap generation on-the-fly using esbuild (suggested by @nwidger) in goja_debugger project. Why does vm.prg.src.Position(vm.prg.sourceOffset(vm.pc)).Line sometime report the incorrect line, mostly the next line instead of the current line?

Contributors

• @mostafa (original POC and further developments)
• @MStoykov (new design of API and further developments)
• @nwidger (lots of great feedback on the original issue)

[mstoykov]

@nwidger , currently one of the big questions is whether we can fix the print and exec to work in a useful manner. That definitely depends on @dop251 being happy with even more internal changes and hopefully helping at least guidance on what will be needed to be done, if not with actually doing it ;). Without fixing those problems I see very little reason to continue fixing something that can actually tell me less stuff than a console.log(a) ;).

The current problem with print and exec as I see it:

• due to how js name resolution works and goja trying to do some optimizations it is not easy/possible currently to know if a is a variable in the local scope, an argument to the list, a global, defined in the calling function to a lambda etc ... All of those things basically change where the variable is stored and effectively how to retrieve it. While goja compiles code it knows this stuff and if there is a simple variable that will never leave the scope it just gets put somewhere on the stack (for example). Here for example the solution IMO will be for goja to be able to emit some debug information, possibly by just recompiling the source code with some flag, so that we don't add additional work for a normal compilation if the debugger will never be attached. That debug information can then be used to know that a on line 15, character 12 is defined as a stack variable in this place, but at character 15 it's redefined and is now in this stack (for example if there are multiple scopes).
• exec, IMO, will be "a bit" harder even conceptually as it's basically "execute me this code as if it was written in exactly the place I'm currently". "The place" the debugger is currently can be midline for example and as the above example have a difference as it will need to resolve a from a different place depending on whether it's a bit later or not. This also likely has even more implications than I can think of. I would expect things such as v8 to just move from very optimized code to very not optimized code at this point that just expects that you can run eval with all variables everywhere. This can possibly be done in goja as well but it will mean that something like the debug information above will need to be used to move variables between the two stacks(and more) as that will definitely change if we compile the code in such a way.

Given the above two and the complexity I envision, I really do want @dop251 to confirm or deny if there is any interest on their side to help or even merge something that will make print and exec actually used within a local scope.

For the record I did try to:

1. run exec without making a new function and looked at what ended up on the stack, but this still did not help in any way as the new program has no idea what a is so it just tries to load it as global(or something like that).
2. I was thinking of trying to recompile the original code with the added one, but I very quickly realized that this will likely move stuff around and it will be really hard to do any surgery such that I can go back and forth between the two programs while saving the current state.

[nwidger]

@mostafa I wonder if it might not be a good idea to have Debugger.Breakpoints return a struct type for each breakpoint. I could totally see wanting to, say, add the ability to temporarily disable a breakpoint without actually deleting it, or to include a count indicating how many times a breakpoint has been hit. The current signature of the method would make future extensions like that difficult without breaking the API, which I assume isn't something we'd want to do.

I still also think that if it's not too difficult a Debugger method to get the current call stack would probably be good to include in an MVP like this. At least when I've used gdb in the past, bt was the first command I'd run.

[nwidger]

@MStoykov I agree that getting some feedback/guidance from @dop251 sooner rather than later is probably a good idea. At this point finding out whether he has any interest in merging a debugger API in the first place, and also how willing he'd be for more internal changes to make the debugger interface more useful would probably be a good idea. I don't have any specific feedback regarding the current issues with print or exec (it sounds like you have a far deeper knowledge of the goja internals in this area than me), but your idea of asking the compiler to optionally include extra debugging information or perform fewer optimizations certainly sounds reasonable to me.

On a separate topic, has any thought been given to activating the debugger when an exception/panic occurs? I believe in some JS debuggers this is an option that can be toggled on/off. At least for JS exceptions, you may not want the debugger to activate for every exception, but I'd assume the debugger should probably activate if a Go panic is otherwise going to make the debugger itself explode. For example, I this most people would want the debugger to activate when it hits line 5 of this script:

function test(val) {
    return val + 1;
}
console.log(test(0));
console.log(tes(1));
console.log(test(2));

But instead a just get a nasty panic causing the debugger to terminate:

debug[10]> l
  1     function test(val) {
  2       return val + 1;
  3     }
  4     console.log(test(0));
> 5     console.log(tes(1));
  6     console.log(test(2));
  7     //# sourceMappingURL=data:application/json;base64,ewogICJ2ZXJzaW9uIjogMywKICAic291cmNlcyI6IFsic2NyaXB0cy9kZWxtZS5qcyJdLAogICJzb3VyY2VzQ29udGVudCI6IFsiZnVuY3Rpb24gdGVzdCh2YWwpIHtcbiAgICByZXR1cm4gdmFsICsgMTtcbn1cbmNvbnNvbGUubG9nKHRlc3QoMCkpO1xuY29uc29sZS5sb2codGVzKDEpKTtcbmNvbnNvbGUubG9nKHRlc3QoMikpO1xuIl0sCiAgIm1hcHBpbmdzIjogIkFBQUEsY0FBYyxLQUFLO0FBQ2YsU0FBTyxNQUFNO0FBQUE7QUFFakIsUUFBUSxJQUFJLEtBQUs7QUFDakIsUUFBUSxJQUFJLElBQUk7QUFDaEIsUUFBUSxJQUFJLEtBQUs7IiwKICAibmFtZXMiOiBbXQp9Cg==

debug[10]> n
panic: ReferenceError: tes is not defined

goroutine 34 [running]:
github.com/dop251/goja.(*Runtime).throwReferenceError(...)
	/home/niels/projects/goja/runtime.go:409
github.com/dop251/goja.valueUnresolved.throw(...)
	/home/niels/projects/goja/value.go:903
github.com/dop251/goja.(*vm).toCallee(0xc000296480, 0xadfb58, 0xc0002230e0, 0x10)
	/home/niels/projects/goja/vm.go:633 +0x2cd
github.com/dop251/goja.call.exec(0x1, 0xc000296480)
	/home/niels/projects/goja/vm.go:2379 +0x7a
github.com/dop251/goja.(*Debugger).Next(0xc0002c9050, 0x1, 0xa207bd)
	/home/niels/projects/goja/debugger.go:123 +0xc6
main.repl(0xc0001ead12, 0x1, 0xa496e0)
	/home/niels/projects/goja_debugger/repl.go:86 +0xf65
main.main.func3(0x1, 0xa20929, 0x2)
	/home/niels/projects/goja_debugger/main.go:97 +0x2be
created by main.main
	/home/niels/projects/goja_debugger/main.go:86 +0x3e5

Perhaps a new ActivationReason like panic/exception for when this occurs would make sense? I understand that this is all a work in progress, so I'm just curious if this is on the roadmap at all. :)

[mostafa]

@nwidger

1. While experimenting with DAP, I found that some things are left for the frontend to implement, like breakpoint counting or maybe enable/disable of breakpoints, but I'll try to have a stable API with tests before merging anything.
2. I think implementing a command to see the current call stack is feasible.
3. I am not sure about jumping into debugger if an error/panic/exception happens because the program should have debugMode enabled on the runtime, otherwise it won't even bother. Yet I see that the current frontend panics on such issues. I'll see if I can fix that.

[dop251]

Sorry, I haven't got the chance to review the code yet, but I would merge it assuming good maintainability and minimum performance impact when debugging is not enabled.

As for resolving variables, I think there is an easier approach: variable names are kept at runtime when (and only when) there is a possibility of a dynamic lookup (i.e. lookup by name that is not known at compile time). This can happen when at least one nested scope has a direct eval(). In this case scope.dynLookup is set to true. If you change the compiler so that in debug mode it always sets this flag, it should do the trick. Thinking further ahead, in order to be able to run code in the current scope, the compiler should assume each scope contains a direct eval, so scope.dynamic should always be set to true, rather than scope.dynLookup.

While there definitely should be a debug mode for compiling, there is a problem that a Program could be compiled externally (using goja.Compile() or goja.MustCompile()). The source code may not be available (e.g. it was compiled from a dynamically generated AST). This should be handled somehow.

The use of sourcemaps looks strange to me, there should be no need for that. If you could give me some examples where is skips lines I could take a look.

[mstoykov]

As for resolving variables, I think there is an easier approach: variable names are kept at runtime when (and only when) there is a possibility of a dynamic lookup (i.e. lookup by name that is not known at compile time). This can happen when at least one nested scope has a direct eval(). In this case scope.dynLookup is set to true. If you change the compiler so that in debug mode it always sets this flag, it should do the trick. Thinking further ahead, in order to be able to run code in the current scope, the compiler should assume each scope contains a direct eval, so scope.dynamic should always be set to true, rather than scope.dynLookup.

I did try just making compiler.isDynamic() return true, but apparently, I either tested something for which this wasn't enough or tested print not exec, as print currently tries to find stuff on the stack(somehow).

But one of our tests now works :tada: , so this seems like it will work at least for some of the cases, and it probably works even better if I do what you suggested instead of changing the function ;). Thanks @dop251

Unfortunately, this has a (small) downside that you need to enable debug mode in the very begging and compile your source code with debug mode, which might mean that it will be less useful, but definitely a lot better than not working at all :).

I will try to get this change made and fix exec and print accordingly, before doing any other API changes like that will probably necessitate some.

[mostafa]

@dop251 @MStoykov
I found one of the nasty bugs I used to run into, which reported incorrect lines while testing step-in command: While inside a function, the vm.pc and hence the dbg.vm.prg.src.Position(dbg.vm.prg.sourceOffset(dbg.vm.pc)).Line is reset. Inside a function the vm.pc equals 0 and thus the line number is reported based on that, yet I assumed that the function declarations are part of the vm.prg.code, yet they behave differently. I'll add more tests and try to see if I can find a way to fix this issue in debugger. Actually I can detect if we're inside a function, but the question remains: how should we calculate the line number and reflect it in the rest of the functions, so we don't jump lines.

[nwidger]

@mostafa @MStoykov Would it make sense to enable debugging with something like an optional WithDebugMode option to Compile/CompileAST/MustCompile instead of adding a whole new CompileASTDebug function? This would be similar to what was done for the WithSourceMapLoader option in the parser package.

[mstoykov]

Current problems from my "integration" with k6:

1. next/stepin either don't work very well or not at all. They sometimes just don't do anything and then they jump ... or don't :shrug: .
2. k6 has a lot of cases where you will call into k6/go and then it will call a js function like in group("name", func() { //code here }) where group is implemented in go and it does some stuff and then calls the provided function. As far as I can test (in the limited time I tried) this completely breaks the line detection and I can't really do anything useful within the group if I just stepin/next to any code from outside to inside a group. It clearly executes the instructions at some point as I can see side-effects of it, but I can't actually execute in the context unless I put a breakpoint/debugger statement inside and continue to it. This might be a problem with how k6 runs this functions and how the debugger works.
3. As a whole the feedback on what just got executed is ... bad/nonexistent which leads to you not really being certain what stepin will did and because of 1 sometimes I can't really be certain if it just did nothing or did it do something that didn't change anything visible.
4. There is no step command but it will probably suffer even more heavily from the above
5. I do think that between 3 and 4 it's very likely that the current Next and Stepin implementation will need to be redone ;)
6. List is another function that works strangely. In the case of k6 (and possibly other users needing debugger) we do have transpilers in a lot of the cases( babel ) which can generate sourcemaps and k6 now has PoC support to actually use them. This still does mean that List will return the actual code goja execute, but other parts(Line) will return according to the original code that was transformed, which is arguably the correct behaviour. This gets really confusing and it seems to mostly work, except when it doesn/t Which is why k6 just gets the original code itself. Also of note is that even without source maps the stepin/next work funky, so that isn't because of that :(.
7. Again in the case of k6, babel may rename identifiers, for example transpiling import http from "k6/http" will not lead to having an http identifier but _http(it obviously changes it everywhere) which also is super confusing and is likely just a very small of such changes. So if you don't know about those things it seems like exec doesn't work. I expect this might be even more prevalent in other transpilers like the typescript compiler or a minimizer of any sort. This can probably be fixed through just sourcemaps ?

A lot of the above seem to me like things that proper use of sourcemaps is already fixing somewhat and the debugger(and probably also goja) needs to get a bit better support for them. And as I want to actually get sourcemap support in k6 and 80% of the work is done, I will likely try to see if I can fix some of them or at least diagnose them more accurately.

For the rest I think this is due to the hackish nature of some of the things that were done in order to get something working and likely with a bit more understanding of goja it will be possible to stabilize them.

Things that I definitely think should be dropped from this PoC are:

1. List - it practically should be provided by the debugger/application as is the case in k6 PoC
2. Print - it was an early try at getting variables somehow but it in practice is the same as exec for that use case so I see no reason to have Print (especially as it doesn't work ;) ).

I also think that likely Next and StepIn have some (subtle) bug that makes them behave as said above.

Thank you @mostafa for starting the work on this.

Also thanks to @nwidger for the suggestions and review and of course thanks to @dop251 for goja as a whole and the help so far to get this working. :tada:

[mstoykov]

@mostafa @MStoykov Would it make sense to enable debugging with something like an optional WithDebugMode option to Compile/CompileAST/MustCompile instead of adding a whole new CompileASTDebug function? This would be similar to what was done for the WithSourceMapLoader option in the parser package.

It makes sense, but it will be a breaking change and as I mention above there are still things to change so the API is definitely not final.

I guess we can add a variadic parameter on the current ones without breaking changes :thinking:

[nwidger]

@MStoykov Yes sorry I should been a little clearer, I meant changing the signatures to look something like:

type CompileOption func(*compileOptions)
func WithDebugMode() CompileOption

func Compile(name, src string, strict bool, options ...CompileOption) (*Program, error)
func CompileAST(prg *js_ast.Program, strict bool, options ...CompileOption) (*Program, error)
func MustCompile(name, src string, strict bool, options ...CompileOption) *Program

which I believe for the majority of people would not be a breaking change.

[mostafa]

Thanks @MStoykov for taking time and fixing lots of my erroneous Go code. You know I am a Python developer by nature. :wink:

Thanks @nwidger for your great feedback, comments and suggestions along the way. I tried your proposed changes as parser options and it didn't turn well. Probably @MStoykov and I could have a look after we find and fix deeper issues we currently have.

@dop251 What @MStoykov described as issues in k6 are actually mostly implementation issues in the debugger or goja_debugger application, mostly because of lack of understanding of how Goja works internally. I tried multiple ways to step through lines using delve, but it still needs your attention. As @MStoykov also mentioned, the API needs more work to become stable, but the current PR just works, until you have a look and guide us in the correct direction and point us any issues you see in the code, especially in Next and StepIn. Thanks in advance! :slightly_smiling_face: