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

service,terminal,cmd/dlv: automatically guessing substitute-path config #3781

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aarzilli
Copy link
Member

Add command, API calls and launch.json option to automatically guess
substitute-path configuration.

@aarzilli aarzilli marked this pull request as draft July 15, 2024 14:10
@aarzilli aarzilli force-pushed the substitutepath-guess branch 3 times, most recently from 15b7904 to d41a9b9 Compare July 15, 2024 20:14
@aarzilli
Copy link
Member Author

aarzilli commented Jul 16, 2024

cc @hyangah I've been wanting to remove the legacy mode of vscode-go adapter and I was wondering if something like this would work.

How it works

Client side it runs go list --json all and uses the output to create a mapping from module paths to their directories. Server side it tries to do the same thing by examining debug_info/debug_line. It works for vendor directories, for modules in the pkg/mod directory, for -trimpath and across windows/linux connections.

What the extension would have to do

The extension, before connecting to dlv, would have to run locally dlv substitute-path-guess-helper and copy its output into the configuration that is sent to dlv with the AttachRequest. That's all. If we want to extend the guessing algorithm to cover more situations we can then do it without
involving the extension itself.

Thoughts? Would this work?

@hyangah
Copy link
Contributor

hyangah commented Aug 7, 2024

Thank you so much!

A couple of questions:

  1. dlv substitute-path-guess-helper:

The extension, before connecting to dlv, would have to run locally dlv substitute-path-guess-helper ...

In case of the remote debugging, the debugged target executable is available only on the remote host. Shouldn't dlv substitute-path-guess-helper be able to connect to the headless server running remotely?

  1. In the current snapshot, you added ClientMod2Dir. Does it mean that the DAP client can send this mapping (that's what client can compute locally using go list) instead of dlv substitute-path-guess-helper? I think that will work.

  2. One minor detail is handling of standard libraries. Should the ClientMod2Dir have a special entry for the standard library module? Or should we specify the full package path for stdlib packages? (e.g. "runtime": "/mymachine/goroot/src/runtime")?

@aarzilli
Copy link
Member Author

aarzilli commented Aug 8, 2024

Thank you so much!

A couple of questions:

1. dlv substitute-path-guess-helper:

The extension, before connecting to dlv, would have to run locally dlv substitute-path-guess-helper ...

In case of the remote debugging, the debugged target executable is available only on the remote host. Shouldn't dlv substitute-path-guess-helper be able to connect to the headless server running remotely?

The idea is that dlv substitute-path-guess-helper collects the informations it needs from the local source, which is then sent to the server within the launch configuration. Then the rest of the work is done by the server, which does have access to the binary.

2. In the current snapshot, you added `ClientMod2Dir`. Does it mean that the DAP client can send this mapping (that's what client can compute locally using `go list`) instead of `dlv substitute-path-guess-helper`?  I think that will work.

The idea is that dlv substitute-path-guess-helper can be called to produce the contents of ClientMod2Dir, instead of having to replicate that part of the logic in javascript.

3. One minor detail is handling of standard libraries. Should the `ClientMod2Dir` have a special entry for the standard library module? Or should we specify the full package path for stdlib packages? (e.g. "runtime": "/mymachine/goroot/src/runtime")?

I initially did this and then backed off of it. There is no guarantee that the standard library that was used to build the executable is the same one that's available locally, so we shouldn't blindly map it. We could be smarter about it, but in practice it is rare that the user will step into the standard library (I think) so it's probably fine to leave it unmapped.

@hyangah
Copy link
Contributor

hyangah commented Aug 20, 2024

Thanks. I will give it a try and test it.

3. One minor detail is handling of standard libraries. Should the `ClientMod2Dir` have a special entry for the standard library module? Or should we specify the full package path for stdlib packages? (e.g. "runtime": "/mymachine/goroot/src/runtime")?

I initially did this and then backed off of it. There is no guarantee that the standard library that was used to build the executable is the same one that's available locally, so we shouldn't blindly map it. We could be smarter about it, but in practice it is rare that the user will step into the standard library (I think) so it's probably fine to leave it unmapped.

I'd say there is also no guarantee that the user code used to build the executable is not exactly the same as the one available locally strictly speaking. I hope users of remote debugging to ensure both systems use the identical versions. (Or can we have a warning based on the Go build info?)
In DAP, the path translation occurs not only in the breakpoint setting, but many other message types that can use Source type such as stack trace, output event, ... And VS Code UI makes it really easy to navigate up and down over the stack. (I will test and report the impact of missing translation)

@aarzilli
Copy link
Member Author

I'd say there is also no guarantee that the user code used to build the executable is not exactly the same as the one available locally strictly speaking.

Yes, we have a warning for this. Vscode-go doesn't but that's a UI problem.

(I will test and report the impact of missing translation)

Sure, let us know.

hyangah added a commit to hyangah/go-docker-alpine-remote-debug that referenced this pull request Sep 6, 2024
@hyangah
Copy link
Contributor

hyangah commented Sep 6, 2024

I think we need to handle stdlib locations unfortunately.

  • Often the stack trace includes locations from stdlib packages. Navigating through the stack causes the editor to attempt to open non-existing file paths.
  • When requesting to pause the debugee, the editor subsequently attempts to open the location of the top stack frame. That's often in the runtime... so..

Screenshot 2024-09-06 at 10 41 46 AM

I agree that it's not perfect if local/remote go versions don't match. We can track this issue - how to notify users of this version skew - separately, independent of this PR.

There is one more issue. In the specific example I tested (https://github.com/hyangah/go-docker-alpine-remote-debug), the main package is main, not the package path. So, I had to adjust "ClientMod2Dir" like

           "ClientMod2Dir": {
                    "main": "/usr/local/google/home/hakim/projects/go-docker-alpine-remote-debug",
                    "github.com/ccampo133/go-docker-alpine-remote-debug":"/usr/local/google/home/hakim/projects/go-docker-alpine-remote-debug"
            }

I will send the copy of the binary built in case it's helpful for investigation.

Add command, API calls and launch.json option to automatically guess
substitute-path configuration.
@aarzilli aarzilli marked this pull request as ready for review September 10, 2024 15:28
@aarzilli
Copy link
Member Author

Added the GOROOT mapping and fixed the example. The example did not work because there the main module contains a single package (main).

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.

2 participants