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

Debug adapter protocol support #574

Merged
merged 262 commits into from
Feb 14, 2022
Merged

Debug adapter protocol support #574

merged 262 commits into from
Feb 14, 2022

Conversation

dsseng
Copy link
Contributor

@dsseng dsseng commented Aug 12, 2021

Fixes #505.
/cc @archseer

Current state: an initial implementation of DAP with some types, editor connection. To test DAP client:

  1. Create a debuggee program:
package main

import "fmt"

func main() {
	a := "Hello"
	b := "World"
	fmt.Println(a, b)
	for {
	}
}
#include <stdio.h>

void main() {
	char *a = "Hello";
	char *b = "World";
	printf("%s %s\n", a, b);
	while (1) {
	}
}
  1. go build -gcflags '-N -l' main.go to build binary with readable variables or use :dbg source main.go for Go.
    2.1. gcc main.c -o main -O0 -g for C program
    3. Start up Delve (chosen as a monolithic DAP+debugger in one binary) on port 7777: dlv dap -l 127.0.0.1:7777
    3.1. Or lldb: lldb-vscode -p 7777
  2. Don't care about that, tcp_process transport will find free port and start debug adapter itself.
  3. Run DAP example cargo run --example dap-dlv or dap-lldb
  4. Press enter when you want to continue communicating to debugger.

Editor integration works with Go, C/C++ and Rust programs at the moment. Just enter directory containing main.go/main (build output)/target/debug/rustdebug for Rust and work with it. Now you need to specify target manually, see languages.toml
In editor I currently use example attached as a zip.
godebug.zip

TODO:

  • DAP runs over TCP/IP. Not stdio. So, Transport must be able to use TCP sockets and the lifecycle of debug adapters
  • More types, requests, events implemented. I'll do that in some time.
    • attach
    • parse stopped event
    • Pass unused events to application or stream all the events in some way via channel
    • pause, step, restart, eval
    • restart
      • supportsRestartRequest
    • eval
    • conditional breakpoints
    • logpoints
    • conditional logpoints
    • Some internal state tracking
    • breakpoint event
    • progress* events
    • output event's data can be recorded as well
    • Better state handling (replace is_running)
    • BreakpointLocations (not supported by lldb and Delve)
    • Goto (no supported debugger)
    • Completions Request for eval autocompletion (no supported debugger)
      • supportsCompletionsRequest
    • setExceptionBreakpoints
    • SetVariable Request
    • Source Request
    • Check support for features in debugger
      • Breakpoints
      • supportsTerminateRequest
  • Editor UI connection
    • Breakpoints setting
      • Show unverified breakpoints and breakpoints moved by debugger
    • Debugger state management
    • Variable introspection
      • Scrollable window for variables
    • Highlight stack pointer
      • Scroll to pos
    • REPL for eval (after commpletions for eval are done)
    • Attach support
    • Pretty-print errors instead of crashing when something goes wrong with debugger
    • multi-thread support for stack pointers
    • Preview of stack location
    • Go to previous stack frame
    • Column-precision for breakpoints
    • Set breakpoints by mouse
      • Right click opens the prompt for editing condition
      • Middle click Right click + Alt opens the prompt for editing condition
      • those (:top:) ideas, but with keybindings
  • Fix Tried sending event into a closed channel for events
  • Split out interface for adapter-specific launch and attach args
  • Configuration for languages to specify how to start and configure debuggers for them , maybe some debug adapters' quirks.
    • Names for launch configs
    • Templates
    • Pickers
      • Show defaults and allow editing args
    • Non-string arguments
    • Multiple debuggers for language (needs a use case, gdb will be the first one)
  • Consider adding workspace configs for debug targets or parsing existing ones.
  • Test debuggers, add them to editor.
    • Delve's dlv dap
    • lldb with lldb-vscode
    • Node.js
    • gdb integration, which means rr could be supported, test it as well
      • Initial connection to gdbserver with lldb, ⚠️ Is not compatible enough, OpenOCD and rr did not work
    • https://github.com/microsoft/debugpy/
    • Chromium
    • Firefox
  • Docs for everything

Local ``languages.toml` for Node:

[[language]]
name = "javascript"

[language.debugger]
command = "node"
args = [ "/usr/share/code-insiders/resources/app/extensions/ms-vscode.node-debug2/out/src/nodeDebug.js" ]

Adjust path to extension in VSCode (or location where you unpacked the extension manually).

helix-dap/src/client.rs Outdated Show resolved Hide resolved
@dsseng
Copy link
Contributor Author

dsseng commented Aug 12, 2021

I think that requests I have already implemented are sufficient to build a starter-level debug integration. Maybe output and other events now handled by client should be passed to application via a channel for usage like showing state in the status line.

Now the main task is to think about the way DAP client is built into the editor, maybe this PR will need to wait for some stuff like vars or stack trace components.

Helix needs a way to specify implementation-specific arguments (mainly for launch and attach): see dlv dap as an example of specific launch options.

@archseer
Copy link
Member

By the way are you on Matrix? If not, feel free to join the development channel so we can discuss :)

@archseer
Copy link
Member

Yeah I think we'll end up with something similar to the LSP config

https://github.com/emacs-lsp/dap-mode/blob/8ec7e98986ea46e3c36b7ecbdf9a6562c90c7632/dap-go.el#L52-L73

But there will also be a section to add different run types:

https://github.com/emacs-lsp/dap-mode/blob/8ec7e98986ea46e3c36b7ecbdf9a6562c90c7632/dap-go.el#L93-L150

I imagine we'd end up visualizing these in a picker.

Run
---
> Launch
  Attach

@archseer
Copy link
Member

Note that delve requires the most config, here's lldb: https://github.com/emacs-lsp/dap-mode/blob/master/dap-lldb.el

I think delve is a good start though, and it's also the one I'll probably end up using the most (besides lldb).

helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Show resolved Hide resolved
helix-dap/src/transport.rs Outdated Show resolved Hide resolved
helix-dap/src/transport.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
helix-dap/src/client.rs Outdated Show resolved Hide resolved
port_format: &str,
id: usize,
) -> Result<Self> {
let port = Self::get_port().await.unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might have race condition in really rare situation (like lack of ports which might increase the probability):

  1. We acquired ephemeral port from OS
  2. Another process took it
  3. Debug adapter fails to start because port is acquired by other process.

Copy link
Member

@sudormrfbin sudormrfbin left a comment

Choose a reason for hiding this comment

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

Seems like there's an off-by-one error when calculating cursor position returned from the debug server; hitting next always stops one character after the beginning of a variable. I have still not gone through the code completely, testing out the client first. Some kind of feedback on the current state of the debugger would also be nice (probably in the status line).

(We might want to include gitignored files to the path completion, used by the binary template for example since build directories are usually ignored.)

@archseer
Copy link
Member

archseer commented Dec 5, 2021

Yeah DAP uses 0-indexing by default, I think there's some missing conversions -- I changed up breakpoint tracking quite a lot over the last few commits.

pub caps: Option<DebuggerCapabilities>,
// thread_id -> frames
pub stack_frames: HashMap<ThreadId, Vec<StackFrame>>,
pub thread_states: HashMap<ThreadId, String>,
Copy link
Member

Choose a reason for hiding this comment

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

Should thread_states map to an enum ? It seems like both the state and reason for the state change is stored here ("running", reason from StoppedEvent, etc).

Copy link
Member

Choose a reason for hiding this comment

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

It could be an enum if we hand an Other variant for it, the specification allows for custom reasons:

reason: 'started' | 'exited' | string;

helix-term/src/commands/dap.rs Outdated Show resolved Hide resolved
helix-term/src/commands/dap.rs Outdated Show resolved Hide resolved
helix-term/src/commands/dap.rs Outdated Show resolved Hide resolved
Comment on lines +323 to +327
if cx.editor.debugger.is_some() {
cx.editor
.set_error("Debugger is already running".to_string());
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

editor.set_error(...); return; is becoming a common enough pattern across commands so much that I think it's better to have commands return a Result, similar to typable commands and handle setting the error in a single place. Better off as a separate PR though.

Copy link
Member

Choose a reason for hiding this comment

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

I've been thinking about this but I'm not sure. DAP/LSP commands need error handling, but most built in commands don't. I guess it would be nice for consistency?

helix-term/src/commands/dap.rs Outdated Show resolved Hide resolved
})
}

pub fn dap_step_in(cx: &mut Context) {
Copy link
Member

Choose a reason for hiding this comment

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

step_in, step_out, next, etc all seem to share most of the code, differing only in the let request = debugger.step_in() part.

Copy link
Member

Choose a reason for hiding this comment

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

Not much we can do here since the future types are all different :/ only thing I can think of is a macro to generate these

@dsseng
Copy link
Contributor Author

dsseng commented Jan 3, 2022

What work does it require to get this into master? Probably gdb and docs only, but not sure.
Merging master here caused build failure :(

@archseer
Copy link
Member

archseer commented Jan 3, 2022

Yeah I was holding back on merging master to avoid more merge commits and do it all at once just before merging the PR (at this point we have too many commits to cleanly rebase but I'd like to keep a clean history).

It's mostly ready for an initial merge, I rewrote all the UI components and was working on a tree style viewer for the variables but I got distracted with some other helix tickets. I'll release 0.6 tomorrow then merge this so that we can work in smaller follow-up PRs.

@dsseng
Copy link
Contributor Author

dsseng commented Jan 3, 2022

Well, so no work really required on this now? Then it should be easier: merge this PR, but don't close the issue. Move TODO list there and it's the debug roadmap now!

@archseer archseer merged commit df3b883 into helix-editor:master Feb 14, 2022
@archseer
Copy link
Member

Finally merged this! There's a bunch of work left to do, but it's easier if we don't have to maintain a separate branch.

@dsseng
Copy link
Contributor Author

dsseng commented Feb 15, 2022

6 month PR :) Need to integrate gdb and then many other tasks will get unlocked

@colemickens
Copy link

Is there a reason you went with lldb-vscode instead of vscode-lldb? I ask because the former seems newer and only supported on Darwin?

@archseer
Copy link
Member

lldb-vscode works fine for me on Linux, it came together with the lldb package on NixOS.

lldb-vscode is an official part of LLVM, and vscode-lldb seems to consist of two layers of wrappers around LLDB (lldb->typescript->python)

@colemickens
Copy link

It's a bit funny, earlier in the week and earlier in the day, this misnomer repo (github: lanza/lldb-vscode) came up much higher when searching for lldb-vscode than the lldb-vscode in lldb... . Of course after dozens of searches today, it's buried under vague but clear links to the official lldb version.

Anyway, my mistake, lldb-vscode definitely seems easier. Very nice to have rust debugging just work out of box as soon as helix could see lldb! Great feature, thanks @sh7dm !

@lukepighetti
Copy link

Does anyone have any guides to getting lldb-vscode working on macOS? I'm not finding many resources

@AOx0
Copy link

AOx0 commented Nov 2, 2022

Does anyone have any guides to getting lldb-vscode working on macOS? I'm not finding many resources
~ @lukepighetti

As stated in the wiki:

To simply install the default debugger that will work out of the box you need to:

Install LLVM.

Hence, you just need to install llvm with brew.

brew install llvm@14

LLVM comes with lldb-vscode bundled, but for it to work you have to add the bin folder where it is to the $PATH.

In fact, brew does tell you to add it to the $PATH when the installation ends, in my case for the fish shell, but yours may be zsh or bash, so follow the post-installation instructions it gives you:

llvm@14 is keg-only, which means it was not symlinked into /usr/local,
because this is an alternate version of another formula.

If you need to have llvm@14 first in your PATH, run:
  fish_add_path /usr/local/opt/llvm@14/bin
                                          
For compilers to find llvm@14 you may need to set:
  set -gx LDFLAGS "-L/usr/local/opt/llvm@14/lib"
  set -gx CPPFLAGS "-I/usr/local/opt/llvm@14/include"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugging support with DAP connection
8 participants