Decouple debugger from TUI #286

hinshun · 2022-02-05T05:28:56Z

Features

Clean separation of concerns: codegen.Debugger doesn't know about stdin, stdout or input steering. debug.TUIFrontend only needs to deal with user interactions.
Adds TUI as the first frontend for debugger
Solves each step as its yielded (non-fs types return noop solve requests that solve with no error)
Creates possibility of remote attach / multiple tui attaching / DAP attaching to debugger
Full test suite

Implementation notes

Introduces interface ast.StopNode which are nodes that debugger should potentially halt on. These are: *ast.FuncSignature, *ast.CallStmt and *ast.CallExpr. (ast.StopNode).Subject() returns the node that should be the subject of a breakpoint, since we shouldn't highlight the entire command which may be multi-line.
Breakpoint no longer fs or option::run builtin, but rather keyword that is ignored by checker & codegen. The debugger parses these to create breakpoints.
Scopes now have levels (categorization), to make it easy to access nodes of a specific scope, or to compare apples to apples. For example, with scope levels you can compare two *codegen.State to see if they live inside the same block scope. Or you can quickly access its parent *ast.Module.
shell-on-error removed in favor of the debugger being able to catch *errdefs.SolveError as a regular runtime exception. Users can choose to reverse step from there, exec into the exception state as many times as they like. In the future, when debug attach is available, consider additional TUI commands to exec into the same process, or start multiple processes.
(*RunInfo).ControlDebugger is a new callback that gives programmatic access to the codegen.Debugger interface. See codegen/debugger_tests.go to see how this can be used.
debug.ParseLinespec based on delve linespec: https://github.com/go-delve/delve/blob/master/Documentation/cli/locspec.md

Design

When codegen executes and a debugger is attached, codegen will yield to give a chance for the debugger to decide to halt. Debugger may choose to halt at the start of the program, based on movement mode selected by the user (e.g. next, step, continue, etc), or based on source-defined breakpoints or breakpoints created by the user.
Every time codegen yields to debugger, the debugger also takes a snapshot of the context, scope, node, return register value, etc. Older snapshots are not used unless it is instructed to make reverse movements (rev step, rev next, rev continue, restart). Instead of trying to run codegen backwards, the debugger continues to block on the last yield and simulates reversal by playing back from its snapshots. To the user, it looks and feels like codegen running backwards.
Note this snapshotting, and reverse execution is identical in design to the old implementation.
Control of the debugger is based on one sync.Mutex. It is locked when the debugger is created and only unlocked when waiting for instruction. When a client instructs how to continue, the mutex is locked again. (codegen.Debugger).GetState is blocked until the debugger is available to accept instructions, leading to a clean interface in the TUI (and DAP server later on).
When exceptions are hit (like a diagnostic error), a "snapshot" is created for it so users can step back and forth like any other, or issue other commands to inspect the state.

How `errdefs.SolveError` is handled

BuildKit's errdefs.SolveError are returned after codegen is complete and the solve request is sent off to BuildKit.
When captured, codegen's error handler will yield to the debugger with that error, creating a new snapshot.
Since errdefs.SolveError is a snapshot just like any other yield from codegen, the codegen.Debugger interface only has one Exec method. Internally it distinguishes based on the snapshot's Err containing an errdefs.SolveError to choose between ExecWithFS and ExecWithSolveErr, but clients of the debugger need not worry about the difference.

aaronlehmann · 2022-02-18T05:34:19Z

codegen/gateway_exec.go

+		ssh          []llbutil.SSHOption
+	)
+
+	f, err := os.Create("/tmp/tee")


Is this leftover debugging?

Writing to a fixed path in /tmp is dangerous - for example, a different user can symlink /tmp/tee -> /etc/passwd.

Good catch! It was from debugging.

aaronlehmann · 2022-02-18T05:39:11Z

codegen/debugger_test.go

+}
+
+func cleanup(value string) string {
+	return fmt.Sprintf("%s\n", strings.TrimSpace(dedent.Dedent(value)))


Nit: strings.TrimSpace(dedent.Dedent(value)) + "\n" is simpler

Outside scope of this PR imo, but I'm still thinking about how to share test utility but that would mean testing functions in a non-testing package. Maybe use internal?

It should be fine to create a utility package that's only imported by tests.

aaronlehmann · 2022-02-18T05:40:19Z

codegen/debug/linespec_test.go

+)
+
+func cleanup(value string) string {
+	return fmt.Sprintf("%s\n", strings.TrimSpace(dedent.Dedent(value)))


Nit: strings.TrimSpace(dedent.Dedent(value)) + "\n" is simpler

aaronlehmann · 2022-02-18T16:36:29Z

cmd/hlb/command/run.go

+		} else {
+			g.Go(func() error {
+				pr, pw := io.Pipe()
+				is := steer.NewInputSteerer(info.Stdin, pw)
+				return debug.TUIFrontend(ctx, dbgr, is, pr, info.Stdout, info.Stderr)
+			})
+		}


Would it make sense to have the caller pass this in instead of having Run default to the TUI frontend?

I don't mind.

aaronlehmann · 2022-02-18T16:38:56Z

cmd/hlb/command/run.go

-	ErrOutput         solver.Console
-	Output            io.Writer
-	DefaultPlatform   string // format: osname/osarch
+	Tree            bool


As a future followup, we might want to move this outside of Run as well.

Yeah I don't like Tree tbh, and it will have to go away eventually when we move codegen into a gateway.BuildFunc.

aaronlehmann · 2022-02-18T16:55:49Z

codegen/debugger.go

+				}
+
+				// Breakpoints can be either a call expr or within a func lit.
+				if with.Expr.CallExpr != nil && with.Expr.CallExpr.Breakpoint() {


How about:

if with.Expr.CallExpr == nil || !with.Expr.CallExpr.Breakpoint() { if with.Expr.FuncLit == nil { return } breakpoint := false ... } }

aaronlehmann · 2022-02-18T16:57:07Z

codegen/debugger_test.go

+	d.Terminate()
+
+	select {
+	case <-time.After(100 * time.Millisecond):


Is this long enough to be reliable? I'd make it a few seconds just in case. It shouldn't affect test runtime except in the failure case.

aaronlehmann · 2022-02-18T16:58:03Z

codegen/debug/tui.go

+				diagnostic.DisplayError(s.Ctx, stdout, spans, s.Err, true)
+			} else {
+				for _, span := range diagnostic.Spans(s.Err) {
+					fmt.Fprintf(stdout, "%s\n", span.Pretty(ctx))


nit: fmt.Fprintln(stdout, span.Pretty(ctx))

aaronlehmann · 2022-02-18T17:00:21Z

codegen/debug/tui.go

+			if err != nil {
+				printError(stderr, s, err)
+			}
+			goto prompt


I don't really mind the goto, but another way to do this would be to set a flag when we want to run the part of the loop before prompt. Then most commands would automatically skip that part, except the ones that set the flag.

There is also goto execute, I just think there's unnecessary nesting and this is simpler to read.

aaronlehmann · 2022-02-18T17:00:57Z

codegen/debug/tui.go

+
+	bps := dbgr.Breakpoints()
+	if i > len(bps) {
+		return fmt.Errorf("no breakpoint with id %d\n", i)


Is the newline intentional?

openllb#286 changed FileBuffer to return a newly constructed SourceMap on each call to the SourceMap method. This causes a dramatic increase in the gRPC request size, because It turns out that BuildKit deduplicates source maps based on pointer address: https://github.com/moby/buildkit/blob/d21254e7f74b49a84c3fbe1b13260cb23954cf92/client/llb/sourcemap.go#L57 Reuse the same SourceMap structure when nothing has changed.

hinshun mentioned this pull request Feb 5, 2022

Add support for DAP for IDE debugging #288

Merged

hinshun closed this Feb 5, 2022

hinshun reopened this Feb 5, 2022

hinshun mentioned this pull request Feb 5, 2022

Add DAP support and decouple debugger from TUI #210

Closed

hinshun force-pushed the debug-tui branch 12 times, most recently from a88ab13 to 6904b0a Compare February 12, 2022 05:12

hinshun force-pushed the debug-tui branch 9 times, most recently from a243559 to 907ef49 Compare February 14, 2022 19:13

hinshun mentioned this pull request Feb 14, 2022

Add parsed modules to module lookup registry in context #301

Merged

hinshun force-pushed the debug-tui branch 4 times, most recently from 1970425 to aa2b833 Compare February 14, 2022 22:42

hinshun force-pushed the debug-tui branch 3 times, most recently from 4884444 to 927946b Compare February 17, 2022 18:02

hinshun mentioned this pull request Feb 17, 2022

Refactor RunInfo to prepare for DAP #284

Closed

hinshun force-pushed the debug-tui branch 2 times, most recently from 565cec5 to bfadb54 Compare February 17, 2022 23:16

Refactor RunInfo to prepare for DAP

41c80a9

hinshun force-pushed the debug-tui branch from bfadb54 to a1e21e2 Compare February 18, 2022 00:33

hinshun marked this pull request as ready for review February 18, 2022 03:26

hinshun requested review from aaronlehmann and coryb February 18, 2022 03:26

aaronlehmann reviewed Feb 18, 2022

View reviewed changes

aaronlehmann approved these changes Feb 18, 2022

View reviewed changes

Decouple debugger from TUI

00046e7

hinshun force-pushed the debug-tui branch from 3738298 to 00046e7 Compare February 18, 2022 20:08

hinshun merged commit 686ef0f into openllb:master Feb 18, 2022

hinshun mentioned this pull request Feb 24, 2022

Split off codegen.Debugger into its own package #11

Closed

aaronlehmann mentioned this pull request Jul 21, 2022

Return cached SourceMap in FileBuffer #324

Merged

yun-wang mentioned this pull request Aug 1, 2022

Support extra env vars for the debug shell #327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple debugger from TUI #286

Decouple debugger from TUI #286

hinshun commented Feb 5, 2022 •

edited

Loading

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

aaronlehmann Feb 18, 2022

aaronlehmann Feb 18, 2022

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

aaronlehmann Feb 18, 2022

aaronlehmann Feb 18, 2022

aaronlehmann Feb 18, 2022

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

aaronlehmann Feb 18, 2022

hinshun Feb 18, 2022

Decouple debugger from TUI #286

Decouple debugger from TUI #286

Conversation

hinshun commented Feb 5, 2022 • edited Loading

Features

Implementation notes

Design

How errdefs.SolveError is handled

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hinshun commented Feb 5, 2022 •

edited

Loading

How `errdefs.SolveError` is handled