-
Notifications
You must be signed in to change notification settings - Fork 764
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: use Delve's DAP implementation #23
Comments
DAP-in-delve work is tracked under go-delve/delve#1515 |
OverviewThis overview is being updated as new options and details surface in the comments below. Before
In response to `launch` or `attach` request, the DA launches `dlv --headless` command (that builds, executes or attaches to the debugee) or connects to an already running delve server.
After (Option 1)
In response to `launch` or `attach` request, delve-adapter will build, test, execute or attach to the debugee.
After (Option 2)
Open Question: will this option make inline mode possible? |
I have an experimental branch where a new debug adapter is added alongside the existing one, and registered under a different type - As @polinasok's message above details, it registers a new debugger with a new entry in The prototype works. When debugging Go code, we can select between the two different debug adaptors by adjusting the |
[...] Another alternative is to retain a thin debug adapter even when Delve's DAP functionality is complete. This debug adapter will take care of launching Delve and then will just pass-through all DAP commands back and forth. The obvious cost of this alternative is having to maintain an additional debug adapter. That said, this adapter is expected to be very minimal. The potential benefits of this approach:
|
The debug node client supports multiple ways of running a debug adapter. https://github.com/microsoft/vscode-mock-debug/blob/master/src/extension.ts#L57 Can we utilize that and by default run the thin wrapper in inlined mode? |
Yes, though it's still not 100% clear to me what the tradeoff is. It seems like this allows us to debug the adapter and the extension in the same process, which is nice. This seems like a fairly new option in vscode, and I imagine the current Go debug adapter could also use this since it's written in TS/JS. [adding more details] Re-reading the documentation carefully again, when we use the |
@eliben When you register the debug configuration provider, there is a method called resolveDebugConfiguration that you can use to intercept the configuration before sending it to the adapter. This is where you can set the @polinasok I also agree with @eliben's suggestion that we should have a thin layer of DAP in VSCode unless we want to move some of the existing logic that we have like path inference into Delve. |
@quoctruong I was just going to ask you to chime in on the option of the thin adaptor, which we discussed as potentially necessary because of your path inference work. Could you please add more details/links as to what logic will end up in the thin adaptor for this? For inspiration, python has a DAP adaptor to launch PTVSD, handling the rest of communication directly over DAP. The comment says
|
I updated the overview comment to include the 2nd option with the thin adapter, so we have both at a glance. |
Agreed that we want to avoid pushing anything vscode-specific into Delve.
The current debug adaptor also gets all these extra arguments and just ignores them. That configuration passes through all the layers, so I think the idea is to just capture all attributes in one place and to let each layer handle those that of interest to it. Since the arguments are a free-form map and not a struct, we do not have any of those vscode-specific fields coded anywhere. Are you concerned that they are polluting the request that delve receives even if it never reads those fields? Or do you think we should avoid having delve handle launch requests all-together and go back to specifying the program on the delve command line?
Do you know of a specific example where the current approach might cause issues? I have not looked into other IDEs very closely yet. My general understanding is that launch.json-like configuration is of part of the contract, even if DAP spec does not specify it in detail. Some fields are editor-specific. Others are adapter-specific. So the adapter (and in our case delve) has to make it clear, which argument fields it relies on and vscode or any other IDE have to supply them in launch/attach requests. And how they source them from the user or internally (via launch.json or something else), will be up to each client.
Your concern about the dlv installation is very valid. I believe by default you get a "no debug adaptor" message, which is not very useful because the user would not know that delve is the adaptor and delve is missing and needs to be installed. However, if debugServer option is used, it takes precedence over the executable in package.json, which is the very last fallback. So whatever extension code launches delve as an adapter and sets debugServer, can in theory do the same kind of error-checking and messaging that the current adapter does.
I tried to highlight in my overview comment that debugServer will be set in the extension code (goDebugConfiguration.ts). Looks like it was not clear that that we will not need any input from the user via launch.json (unless they want to debug an external adapter). Reworded a bit. Please let me know if it is still unclear. |
Thanks for updating the overview comment with all the options side by side; this is very helpful. |
Regarding "thin adapter" vs. "no adapter", it's also worth pointing out that if we want to implement the This may help eventually resolve issues like #124 |
@eliben @polinasok Thanks for keeping the overview up to date. I don't think directly communicating with DAP is an option. As @quoctruong pointed out in offline discussion, we already have vscode specific stuff. In LSP we handled it using a middleware and that gives us great flexibility (accessing information available in the extension host, translating vscode specific file paths, or dealing with bugs in the language server, etc). For DAP, I guess the thin adapter is the only option that provides such flexibility. I wonder if we can still aim at supporting both modes - to run it as an inlined implementation, and to run it as a separate process. Thanks for the pointer to |
More on
The following related issues have been filed against vscode-go: microsoft/vscode-go#843 (=#124), microsoft/vscode-go#219, microsoft/vscode-go#186, microsoft/vscode-go#2204. However, because of delve's split frontend/backend architecture, this is not straightforward, which is likely why the current architecture does not yet support The complication is that in delve's case, the frontend is a separate client from the server backend and is not launched when the backend connects to the vscode client. Perhaps, the --accept-multiclient feature can be adapted for this, but that would require reworking |
I tried taking the existing debug adapter and making it "inline mode", executing in-process with the rest of the extension. This turned out to be pretty simple - here's a commit from my branch where it was implemented. I can verify that I can debug the extension and the DA in the same vscode session right now. As far as I could figure out from digging in vscode's source, even in inline mode the We should probably move this discussion to a separate issue, though, since it's orthogonal to DAP. |
@eliben Agree that we discuss it in a separate issue #232 For the DAP layer implementation, can we start merging the prototype so more users can play with it? BTW from @polinasok's comment above - it looks like the dap mode assumes only a single client. Isn't it problematic? I saw multiple users using the multiclient mode. @quoctruong can you also take a look if @eliben's prototype and the current delve DAP meets the requirements for the cloud debugging use cases (remote, and attach)? |
@hyangah @quoctruong As far as I know, vscode-go does not take advantage of delve's --accept-multiclient flag when starting delve. So must apply to the case when the debugger is started remotely. This requirement has not come up with our discussions with @quoctruong & his team before, but if this is critical please let us know. |
If I understand correctly, the new path inference mapping (#45) uses delve's RPCServer methods (ListSources and ListPackagesBuildInfo). Equivalent requests are not available under DAP, are they? It appears that we would need to upgrade the DAP protocol to support this via an adaptor in vscode-go. If delve supports the mapping, it could likely get the local side of the mapping via the launch/attach arguments. Those are the only implementation specific fields under DAP, so we are free to put whatever we want there without global changes to the protocol. |
@hyangah Unlike LSP case, I do not think the debug adaptor has direct access to extension host information. Having direct access to that state could be quite powerful. But I think at least the current version gets everything from the DAP requests. |
We must also be careful about handling noDebug case right. This logic does not belong in delve - the user should not need to install a debugger to skip debugging. This is currently done in the debug adapter, but one can also argue that an IDE could have language support for editing and running code without any debugging support, so this could potentially belong in the extension code. Do we know if there is precedent in other languages for this? Related issue: #336 |
@polinasok Isn't I have no good idea about the automated path inferring yet - I wonder if there is a way to teach vscode or the debug adapter to send a partial file path instead of the full path. Trim the local |
Change https://golang.org/cl/240417 mentions this issue: |
Updates #23 This DA serves as a DAP proxy between the editor and a DAP-capable Delve. Right now all it does is launch Delve (in DAP mode) when LaunchRequest is received from the editor, and then relays all messages between the editor and Delve back and forth. package.json section is copied from the current DA as-is. Some of its options will be removed and cleaned up in the subsequent PRs to make the diffs more obvious (see #271). Change-Id: I3493c5ee1d9e80071a17ea32c04a15eb021c8006 GitHub-Last-Rev: ddd68dc GitHub-Pull-Request: #267 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/240417 Reviewed-by: Polina Sokolova <polina@google.com> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Change https://golang.org/cl/246777 mentions this issue: |
Eventually we want to inline the debug adapter, but until it gets more stable and becomes the default adapter, let's launch it as a separate process to isolate failures. This is handled by the definition in package.json. In order to avoid accidental installation of the process-wide uncaughtException handler when we switch back to the inline mode, move the handler out of goDlvDebug.ts. Also, fixes the default configuration provider for delve dap debug adapter. It should use 'godlvdap' as the type. Fixes #469 Updates #23 Change-Id: I4df2fff51c703995fd557fe5595a367d7048bd7b Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/246777 Reviewed-by: Polina Sokolova <polina@google.com>
Change https://golang.org/cl/246959 mentions this issue: |
…onfig Eventually we want to inline the debug adapter, but until it gets more stable and becomes the default adapter, let's launch it as a separate process to isolate failures. This is handled by the definition in package.json. In order to avoid accidental installation of the process-wide uncaughtException handler when we switch back to the inline mode, move the handler out of goDlvDebug.ts. Also, fixes the default configuration provider for delve dap debug adapter. It should use 'godlvdap' as the type. Fixes #469 Updates #23 Change-Id: I4df2fff51c703995fd557fe5595a367d7048bd7b Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/246777 Reviewed-by: Polina Sokolova <polina@google.com> (cherry picked from commit f9c0454) Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/246959 Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> TryBot-Result: kokoro <noreply+kokoro@google.com> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Change https://golang.org/cl/248659 mentions this issue: |
With the resolveDebugConfigurationWithSubstitutedVariables, we can read envFile and combine it into the env property from the extension host. Delve DAP will handle only the env arg and not have any special handling for envFile. So, processing it from the extension side makes more sense for long term. This move allows us to centralize the .env file read support. For backwards compatibility, I left the logic in the old DA but removed it from the new delve DAP DA. Fixes #452 Updates #23 Change-Id: I641eb2e62051985ba3486901483ad796256aba2c Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/248659 Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> TryBot-Result: kokoro <noreply+kokoro@google.com> Reviewed-by: Polina Sokolova <polina@google.com>
Updates:
Based on our experiments, I want to propose some adjustments in the plan: We want to eliminate the node.js-based thin adapter, and the extension will start a local Why?
Disadvantages:
How?
Note: We also found
In this new setup, VSCode will warn users properly if the dlv binary is not found. And, the extension will try to ensure the dlv binary exists and supply the right executable path through VSCode's DebugAdapterDescriptorFactory. /cc @polinasok @eliben @quoctruong @suzmue @aarzilli @derekparker |
@hyangah could you please comment as to why you think we need to have a DA connection over stdin/out? Since delve is designed to talk over a socket, that would require some major changes to delve and/or implementation of an extra layer. Currently vscode offers us the following debug adapter options:
If we do not know of anything that must be in the thin (TS or Go) adapter, then I propose that we, at least initially, consider option 2 above and run dlv-dap as a server, communicating with the editor over a socket and handling the full sequence of the debug session requests without any intermediaries. We can use our version of The current dlv-dap server terminates itself with every disconnect request, so we would need to redo this for every debug session. But as we upgrade dlv-dap with more features, we can let the server survive beyond a single client connection and accept more connections. This ties nicely into supporting In the single-use server case, the server will terminate itself on disconnect. We should use random ports to avoid any conflicts (see discussion in microsoft/debug-adapter-protocol#126). In the multi-use server case, we should be able to terminate in I have not prototyped this yet, so this is all theoretical. Plus my TypeScript familiarity is very superficial, so take my next suggestions with a grain of salt. I will revise them as I learn more. But for now I was thinking we could use the same spawn mechanism to launch the dlv-dap server process as we currently do in Also, here is a pointer to a Java server set-up although in this case the server is both LSP and DAP server and starts up as soon as the project is open: |
I have confirmed with @weinand that to implement a single-use local version of:
we can:
If we make dlv-dap multi-use (able to accept multiple sequential client connections, each launching/attach to new process), we can change the factory to create it only for the first debug session, then just keep reconnecting to it for every new debug session trigger, and then clean it up in In the remote case, the user would launch dlv-dap on the command line on whatever machine of the process, then specify the following:
I also have an idea to add support for just "Connect to server", by adding an option to dlv-dap to specify processId or program on the command-line and then making the launch/attach request a no-op or a sanity check. See go-delve/delve#2328 for more details. |
Change https://golang.org/cl/288954 mentions this issue: |
Connect to the dlv dap server directly from vscode, bypassing the thin adapter. This is accomplished using the vscode.DebugAdapterDescriptorFactory for launch configurations of type "godlvdap". Updates #23 Updates #978 Change-Id: I877a1c1b0cf0c40a2ba0602ed1e90a27d8f0159e Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/288954 Trust: Suzy Mueller <suzmue@golang.org> Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Suzy Mueller <suzmue@golang.org> TryBot-Result: kokoro <noreply+kokoro@google.com> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
dlv-dap is available from the stable extension for try. Many details had changed this year. Since we passed the initial brainstorming and design stage, and most discussion happens outside this specific issue, I will close this. There are still a few missing features and caveats, and we still have many tasks planned for polishing. |
Delve supports debug adapter protocol natively (
dlv dap
).https://github.com/go-delve/delve/tree/master/service/dap
Implement the debug feature using it and deprecate the dlv wrapper (
src/debug/goDebug.ts
).@polinasok @quoctruong @ramya-rao-a
The text was updated successfully, but these errors were encountered: