add GoDebug #1390

mattn · 2017-07-27T10:02:38Z

Hi, I added debugger. Do you have interesting this?

mattn · 2017-07-27T10:21:27Z

Maybe, there are not enough commands, features, behaviors.

arp242 · 2017-07-27T10:55:57Z

A previous PR: #1198

mattn · 2017-07-28T10:35:21Z

status update

fatih · 2017-08-02T07:37:57Z

@mattn so happy that you working on this. I'm on road to NYC, but hopefully gonna take a look more in detail.

I think this is possible now that vim-8 has term support. Do you know which patch version should be cap it? I think we should add something like this:

  " we use XXXtermXXX() which was introduce with 8.0.XXXXX, be sure we have
  " it: http://ftp.vim.org/vim/patches/8.0/8.0.XXX
  if !exists("*XXXtermXXX")
    call go#util#EchoError("GoDebugStart requires 'XXXtermXXX'. Update your Vim/Neovim version.")
    return
  endif

or better if you have the patch version that would be better. I think people should use an updated Vim version otherwise we have to deal with problems that are already fixed on HEAD in vim-dev.

mattn · 2017-08-02T09:20:25Z

Ultimately, I did not use vim8 terminal feature for this debugger. Just used job_xxx. So this should work on current vim8. And one more point is possible to port to neovim easily.

arp242 · 2017-09-07T08:41:57Z

I haven't had time to look at this before, but I just tested it and this is great stuff! It already works very well.

I made a PR on your fork with a bunch of minor changes: mattn#1; that was easier than adding comments :-) See the commit messages for details on that. I also did some work on improving and expanding the documentation a bit but didn't commit that yet; I'll finish that up later.

Some further comments (Vim 8.0.1066, Go 1.9):

The string v := "hello" is displayed as v: [5], and expanding it gives me
the byte values:
```
  v[0]: 104
  v[1]: 101
  v[2]: 108
  v[3]: 108
  v[4]: 111
```
Is this intentional?
Small thing, but you can't close expanded variables. I would expect a second
<CR> to close it. I tried adding this in s:expand_var() but had some
trouble grokking that function at a glance.

Both the the struct:

  s := struct {
  	a, b string
  	u    int64
  }{"aasd", "zxc", 42}

and:

  type named struct {
  	a, b string
  	u    int64
  }
  n := named{"zxc", "qwe", 42}

Give an error on expanding:

  expression "s" (struct { main.a string; main.b string; main.u int64 }) does not support indexing
  expression "n" (main.named) does not support indexing

Expanding slices seems to work fine.

arp242 · 2017-09-10T04:07:52Z

Did some more testing, and found a few more small problems:

String types now show up as v: "" with this example (breaking on the fmt line):
```
v := "hello"
fmt.Println("one", v)
```

Using vim dbg/debug.go gives the error:

2017/09/10 04:49:57 debugger.go:97: launching process with args: [/home/martin/go/src/a/debug ]
Can not debug non-main package

I have to use :cd dbg and then :GoDebugStart works.

:GoDebugRestart doesn't seem to work. I added a breakpoint and the program is paused. I changed a variable declaration line (e.g. from i := 42 to i := 43), and use :GoDebugRestart. I would expect the debugger to restart the program with the new source, but i still shows us as 42 instead of the expected 43. I need to use :GoDebugStop and :GoDebugStart to get the new version.

Also, the debugger doesn't restart with the new source if I restart the program with F5 after it has finished. I'm not sure if it should though!

Pressing F5 after the program has exited messes up the window layout:

:messages has:

API server listening at: 127.0.0.1:8181
Process 19284 has exited with status 0
Process 19284 has exited with status 0
Process 19284 has exited with status 0

The current directory now has a debug binary in it, presumably from the debug.go filename. This is because it's currently using dlv debug .... Maybe it would be better to :GoInstall the binary and then use dlv exec? Or run it from a temp directory (like go run)?
It's not a huge deal, but personally I'm not a huge fan of commands that produce unexpected side-effects like this on the filesystem.
Come to think of it, maybe we want to have a way for the user to add flags to dlv? Especially --build-flags would be useful for some people I think. We can also do this later maybe, if someone asks for it ("YAGNI")

arp242 · 2017-09-11T09:16:56Z

autoload/go/debug.vim

@@ -381,6 +381,7 @@ function! go#debug#Start(...) abort
  try
    echohl SpecialKey | echomsg 'Starting GoDebug...' | echohl None
    let s:state['message'] = []
+    exe 'lcd' fnamemodify(bufname(''), ':p:h')


Won't this cause the current working directory to change? :pwd gives me /home/martin/go/src/a when starting vim dbg/debug.go, but it's /home/martin/go/src/a/debug after running :GoDebugStart.

Also, I get an error using :GoDebugStart with the latest changes?

can't load package: package ['--']: cannot find package "['--']" in any of: /usr/lib/go/src/['--'] (from $GOROOT) /home/martin/go/src/['--'] (from $GOPATH) /home/martin/work/src/['--'] exit status 1

Maybe it's worth adding some tests for this? I never really looked in to testing Vimscript stuff so I don't know how hard that is.

fixed for ["--"].

lcd change current directory for current window.

arp242 · 2017-09-11T10:35:11Z

What are your plans for supporting Neovim btw @mattn? It currently errors out with Vim(let):E117: Unknown function: job_start.

mattn · 2017-09-11T10:42:10Z

Go debugger uses JSON-RPC to communicate with dlv debugger. neovim support TCP connection?

arp242 · 2017-09-11T11:06:18Z

I don't know; never looked in to Neovim's implementation in detail.

Personally I'm fine with not adding support for that in this PR if it's hard, but then we should add a clear error message for that similar to the Vim 7.4 error (if has('nvim') [...]).

fatih · 2017-09-11T11:07:51Z

We should not add NeoVim now. First, let finish making the UX and usability right. And then fix bugs and co. once it's in master. Neovim can wait as Vim-go aims to be a Vim first class plugin rather than supporting both nvim and vim.

As it's not yet supported; otherwise it will give a confusing error. fatih#1390 (comment)

arp242 · 2017-11-28T18:30:36Z

~~This PR currently depends on this delve PR, but after that's merged~~ this PR should be ready to be merged @mattn @fatih @bhcleek. Testing and/or feedback appreciated!

There are some areas that might need some further improving – there always are – but overall it works well and I don't expect any further major changes to the :GoDebug commands.

It's currently a bit slow for larger packages because dlv builds the binary with -a, skipping the cache. This is so that it can add the -gcflags "-N -l" compile flags (the cached packages may not have that). Luckily this will be fixed for us in Go 1.10 which has the ability to keep different caches for different build flags.

Aside from the :GoDebug* commands this also adds a g:go_debug setting to debug vim-go itself. See this commit. Other than this, all changes are limited to :GoDebug* commands.

See :help go-debug for documentation.

fatih · 2018-01-04T07:13:14Z

I want to test it before we merge it. Please don't merge it until then. I appreciate all the hard work btw, I think this is going to be one of the biggest addition in 2018 :)

I think this command makes more sense, as it "prints the result from an expression". It's also the command that the dlv commandline uses (`p` or `print`).

I'm not sure when this is useful? Remove for now.

- Delve needs `stepOut`, not `stepout`. This took me an insane amount of time to track down :-/ - Add `debugger-commands` to `g:go_debug`; useful to track down the above. - Remove stepin, as Delve doesn't seem to support that. The current implementation doesn't do anything, and it's not listed as one of the constants in `service/api/types.go` (around like 265).

Much better UX, I think. Especially since starting dlv can take a bit; this will reduce the time people will need spend fiddling their thumbs.

Need to remove the BufWipe events, as that would cause problems with them stopping the newly created async job. Not sure if that was a good idea in the first place to be honest, why shouldn't I be allowed to remove one of the windows from the layout?

Otherwise hovering over a string and lots of other stuff will show errors.

fatih · 2018-02-26T18:18:40Z

README.md

@@ -28,7 +29,6 @@ This plugin adds Go language support for Vim, with the following main features:
  errors, or make sure errors are checked with `:GoErrCheck`.
 * Advanced source analysis tools utilizing `guru`, such as `:GoImplements`,
  `:GoCallees`, and `:GoReferrers`.
-* Precise type-safe renaming of identifiers with `:GoRename`.


Is this removed accidentally?

If I remember correctly, at some point it was decided to keep the top 10 or 15 points from the help file here instead of all. So I removed the bottom one after I added the debug feature.

Can we remove the automatic gopath detection line and keep this? I think the GOPATH one can change as it's less relevant nowadays.

Sure, sounds good

fatih · 2018-02-26T18:22:36Z

doc/vim-go.txt

+DEBUGGER COMMANDS~
+
+Only |:GoDebugStart| and |:GoDebugBreakpoint| are available by default; the
+rest of the commands become available after starting debug mode.


commands and mappings... Worth noting that we also create default mappings

fatih · 2018-02-26T18:24:20Z

doc/vim-go.txt

+in with |:GoDebugStep| (<F11>).
+
+Struct values are displayed as `{...}`, array/slices as `[4]`. Use <CR> on the
+variable name to expand the values.


This is great, but I didn't know I had to go to the GODEBUG_VARIABLES window. Worth adding this little piece. Actually we should document more each window. We just mentioned them briefly above.

Okay, I rephrased it a bit and added some more info.

fatih · 2018-02-26T18:29:33Z

doc/vim-go.txt

+variable name to expand the values.
+
+When you're done use |:GoDebugStop| to close the debugging windows and halt
+the `dlv` process, or |:GoDebugRestart| to recompile the code.


How can I clear previously set breakpoints? Even though I've stopped or restarted the server, the breakpoints are still intact. Is this something we can provide to the user? Seems like there is no way to remove all breakpoints right now and you have to manually remove them one by one.

Oh it seems like restarting Vim does remove all breakpoints

We could add a :GoDebugReset command maybe?

Yeap definitely, I think this is much needed otherwise one has to close and restart Vim (which is not something we want to do). Also another thing that I've just seen is, when you change or remove the lines, the breakpoint lines will be invalid. Not %100 related to this, but worth adding as well.

when you change or remove the lines, the breakpoint lines will be invalid. Not %100 related to this, but worth adding as well.

How do other debuggers handle this? Move the breakpoint, or just clear it?

In VSCode it moves when you add newlines. So it's dynamic to the content of the line. But not sure if it's worth adding that kind of complexity. But one thing I appreciate is the way to disable/enable breakpoints. This allows to temporary run the whole stack without getting blocked. And they also have something to reset all the breakpoints. So I think we need the following commands:

:GoDebugToggleBreakpoint (current :GoDebugBreakpoint, we can keep this) :GoDebugDisableBreakpoints :GoDebugEnableBreakpoints :GoDebugRemoveBreakpoints

Also I think :GoDebugReset would be confusing with :GoDebugRestart. So don't let us use :GoDebugReset (Instead let's use :GoDebugRemoveBreakpoints), instead let's have the above dedicated commands. We can add :GoDisableBreakpoints and :GoEnableBreakpoints later if it's to much work. Wdyt?

(Here is the Debug menu from VSCode)

I'm not so sure of adding four new commands, I find it more annoying to use as e.g. :GoDebug<Tab> now has many more commands that are pretty much the same.

I think a better way is to use e.g.:

:GoDebugBreakpoint 42 " Toggle breakpoint for a line (add/remove) :GoDebugBreakpoint toggle " Toggle all :GoDebugBreakpoint disable " Disable all :GoDebugBreakpoint enable " Enable all :GoDebugBreakpoint remove " Remove all

That being said, I think we should make an issue for this after merging this. It definitely should be done one way or the other, but I don't think it's a critical feature to add right now.

Same applies to the other issue with the wrong line. It's definitely something that needs to be improved, but not necessarily in this PR. It's already a large PR, and tracking all comments on GitHub is kind of hard.

Agree with that. Let's talk about the UI changes later (command and co).

fatih · 2018-02-26T18:32:14Z

Thanks a lot for the hard work. There are couple of things that I think needs clarification, otherwise it looks good.

codecov-io · 2018-02-26T21:00:01Z

Codecov Report

Merging #1390 into master will decrease coverage by 2.5%.
The diff coverage is 0.18%.

@@            Coverage Diff            @@
##           master   #1390      +/-   ##
=========================================
- Coverage    22.2%   19.7%   -2.51%     
=========================================
  Files          53      57       +4     
  Lines        4197    4735     +538     
=========================================
+ Hits          932     933       +1     
- Misses       3265    3802     +537

Flag	Coverage Δ
#nvim	`15.1% <0.18%> (-1.92%)`	⬇️
#vim74	`17.31% <0.18%> (-2.2%)`	⬇️
#vim80	`18.62% <0.18%> (-2.37%)`	⬇️

Impacted Files	Coverage Δ
plugin/go.vim	`29.26% <ø> (ø)`	⬆️
autoload/go/debug.vim	`0% <0%> (ø)`
syntax/godebugvariables.vim	`0% <0%> (ø)`
syntax/godebugstacktrace.vim	`0% <0%> (ø)`
syntax/godebugoutput.vim	`0% <0%> (ø)`
ftplugin/go/commands.vim	`0% <0%> (ø)`	⬆️
autoload/go/util.vim	`60.91% <33.33%> (-0.12%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39beabe...9290447. Read the comment docs.

fatih · 2018-02-27T16:22:24Z

@mattn @Carpetsmoker lgtm. This is ready to be merged. let's merge this and then open new PR's upon feedback and things we see later (such as the breakpoint improvements). Thanks again for all the hard work, much appreciated.

mattn · 2018-02-28T00:17:08Z

Great. Thank you!

See fatih/vim-go#1390 Change-Id: I5d79ce162d466351ca54e435183e4e2e3151ad2c GitHub-Last-Rev: 4ec9a16 GitHub-Pull-Request: golang#21

See fatih/vim-go#1390 Change-Id: I59a45a207b68d08c53d4f16ff4240a28d145c1a1 Reviewed-on: https://go-review.googlesource.com/c/blog/+/169537 Reviewed-by: Austin Clements <austin@google.com>

See fatih/vim-go#1390 Change-Id: I59a45a207b68d08c53d4f16ff4240a28d145c1a1 Reviewed-on: https://go-review.googlesource.com/c/blog/+/169537 Reviewed-by: Austin Clements <austin@google.com> X-Blog-Commit: 6bedf551f6d7d21cf7f656a0748ca65bf387b884

mattn force-pushed the godebug branch from 9c52224 to ba8de17 Compare August 3, 2017 01:44

arp242 mentioned this pull request Sep 7, 2017

Some small improvements for GoDebug mattn/vim-go#1

Merged

arp242 reviewed Sep 11, 2017

View reviewed changes

mattn force-pushed the godebug branch from 4f9af24 to b6c1aa8 Compare September 11, 2017 16:07

arp242 mentioned this pull request Sep 12, 2017

Show error for Neovim mattn/vim-go#5

Merged

arp242 added the wip label Sep 21, 2017

mattn force-pushed the godebug branch from 951b3be to cbd9260 Compare September 23, 2017 16:32

mattn pushed a commit to mattn/vim-go that referenced this pull request Sep 23, 2017

Show error for Neovim

be37a1a

As it's not yet supported; otherwise it will give a confusing error. fatih#1390 (comment)

arp242 force-pushed the godebug branch from cbd9260 to dd8393b Compare November 23, 2017 15:30

arp242 added a commit to mattn/vim-go that referenced this pull request Nov 23, 2017

Show error for Neovim

b796978

As it's not yet supported; otherwise it will give a confusing error. fatih#1390 (comment)

arp242 removed the wip label Nov 28, 2017

mattn added 6 commits February 26, 2018 14:23

add GoDebug

d3cbf23

set rpcid

2ee2826

abort immediately

95b636a

implement stacktrace, balloon text

19cbb06

highlight

308c51c

output window

5fb78eb

arp242 added 12 commits February 26, 2018 14:24

Don't error out if localVars or functionArgs aren't defined

5eaad29

Fix some more instances where it would jump windows

ceec858

Rename :GoDebugEval to :GoDebugPrint

d370336

I think this command makes more sense, as it "prints the result from an expression". It's also the command that the dlv commandline uses (`p` or `print`).

Remove :GoDebugCommand

a7c734b

I'm not sure when this is useful? Remove for now.

Make :GoDebugBreakpoint accept a line number

9845226

Make it possible to define breakpoints before calling :GoDebugStart

c89bb7e

Much better UX, I think. Especially since starting dlv can take a bit; this will reduce the time people will need spend fiddling their thumbs.

Use --output instead of tmp dir

2bc92ef

Make :GoDebugRestart work

db94226

Need to remove the BufWipe events, as that would cause problems with them stopping the newly created async job. Not sure if that was a good idea in the first place to be honest, why shouldn't I be allowed to remove one of the windows from the layout?

Show string vars better in window

1020b57

silent! the BalloonExpr

e709712

Otherwise hovering over a string and lots of other stuff will show errors.

Some small fixes to the documentation.

3a51a15

arp242 force-pushed the godebug branch from e31b861 to 3a51a15 Compare February 26, 2018 14:25

fatih reviewed Feb 26, 2018

View reviewed changes

Small documentation tweaks

3176f6d

arp242 force-pushed the godebug branch from e358aea to 3176f6d Compare February 26, 2018 21:06

Fix GoDebugBreakpoint colours on cterm

9290447

arp242 merged commit 52c6da2 into fatih:master Feb 27, 2018

arp242 mentioned this pull request Feb 27, 2018

Add commands for disabling, enabling, and removing all breakpoints #1689

Open

mattn added a commit to mattn/blog that referenced this pull request Mar 27, 2019

content: vim-go have debugger.

b88eaf2

See fatih/vim-go#1390 Change-Id: I5d79ce162d466351ca54e435183e4e2e3151ad2c GitHub-Last-Rev: 4ec9a16 GitHub-Pull-Request: golang#21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add GoDebug #1390

add GoDebug #1390

mattn commented Jul 27, 2017 •

edited

Loading

mattn commented Jul 27, 2017

arp242 commented Jul 27, 2017 •

edited

Loading

mattn commented Jul 28, 2017

fatih commented Aug 2, 2017

mattn commented Aug 2, 2017 •

edited

Loading

arp242 commented Sep 7, 2017 •

edited

Loading

arp242 commented Sep 10, 2017 •

edited

Loading

arp242 Sep 11, 2017

mattn Sep 11, 2017

arp242 commented Sep 11, 2017

mattn commented Sep 11, 2017

arp242 commented Sep 11, 2017 •

edited

Loading

fatih commented Sep 11, 2017

arp242 commented Nov 28, 2017 •

edited

Loading

fatih commented Jan 4, 2018

fatih Feb 26, 2018

arp242 Feb 26, 2018

fatih Feb 26, 2018

arp242 Feb 26, 2018

fatih Feb 26, 2018

fatih Feb 26, 2018

arp242 Feb 26, 2018

fatih Feb 26, 2018

fatih Feb 26, 2018

arp242 Feb 26, 2018

fatih Feb 26, 2018

arp242 Feb 26, 2018

fatih Feb 27, 2018 •

edited

Loading

arp242 Feb 27, 2018

fatih Feb 27, 2018

fatih commented Feb 26, 2018

codecov-io commented Feb 26, 2018 •

edited

Loading

fatih commented Feb 27, 2018

mattn commented Feb 28, 2018

add GoDebug #1390

add GoDebug #1390

Conversation

mattn commented Jul 27, 2017 • edited Loading

mattn commented Jul 27, 2017

arp242 commented Jul 27, 2017 • edited Loading

mattn commented Jul 28, 2017

fatih commented Aug 2, 2017

mattn commented Aug 2, 2017 • edited Loading

arp242 commented Sep 7, 2017 • edited Loading

arp242 commented Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arp242 commented Sep 11, 2017

mattn commented Sep 11, 2017

arp242 commented Sep 11, 2017 • edited Loading

fatih commented Sep 11, 2017

arp242 commented Nov 28, 2017 • edited Loading

fatih commented Jan 4, 2018

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

fatih Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatih commented Feb 26, 2018

codecov-io commented Feb 26, 2018 • edited Loading

Codecov Report

fatih commented Feb 27, 2018

mattn commented Feb 28, 2018

mattn commented Jul 27, 2017 •

edited

Loading

arp242 commented Jul 27, 2017 •

edited

Loading

mattn commented Aug 2, 2017 •

edited

Loading

arp242 commented Sep 7, 2017 •

edited

Loading

arp242 commented Sep 10, 2017 •

edited

Loading

arp242 commented Sep 11, 2017 •

edited

Loading

arp242 commented Nov 28, 2017 •

edited

Loading

fatih Feb 27, 2018 •

edited

Loading

codecov-io commented Feb 26, 2018 •

edited

Loading