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

Add commands to start and use the debugger #449

Merged
merged 9 commits into from
Nov 21, 2022

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Nov 18, 2022

This PR adds the following IRB commands that start the debugger and seamlessly run the command at the same time:

  • break
  • delete
  • next
  • step
  • continue
  • finish
  • backtrace
  • info
  • catch

Given that we're trying to make debug.gem a superset of IRB functionality #446 (comment), there should be few issues in switching to debug.gem.

@k0kubun k0kubun requested a review from st0012 November 18, 2022 19:09
@st0012
Copy link
Member

st0012 commented Nov 18, 2022

I like the idea but IMO we need to have more discussion before introducing these commands. Currently I have these questions:

  • What subset of functionalities make sense in the context of irb? Just off the top of my head:
    • Stepping commands
    • Frame control commands
    • Breakpoint commands
  • When adding a command, do we want to completely replicate it here, or just part of it? For example:
    • In debug, I usually use command aliases like n (next) and c (continue). So typing full commands in irb is not convenient to me.
      • But on the other hand, if we make n and c commands in irb, it could annoy many users as they could easily be variable names.
    • Commands like break or catch support many options, like break Foo#bar if: a == b do: caller. Do we want to support them as well?
  • How can we help users discover them? debug has help command to help users learn all the commands' usages. But irb doesn't have it and we also don't have good documentation atm. This is already a problem, but it'll be more obvious after we introducing these commands.
    • Side note: I want to rename the current help command to something like doc and let help command prints command usages.

Implementation-wise, I think we also need to figure how to write tests for them because the scenarios to consider will grow very fast. debug has a test framework that allows us to test the debugger in a separate process so we can verify its functionalities end-to-end (example).

I also found that the break command isn't working:

From: test.rb @ line 8 :

     3:     10
     4:   end
     5: end
     6:
     7: a = 1
 =>  8: binding.irb
     9:
    10: b = 2
    11: c = 3

irb(main):001:0> break Foo#bar
Loaded debug-1.6.3
[3, 11] in test.rb
     3|     10
     4|   end
     5| end
     6|
     7| a = 1
=>   8| binding.irb
     9|
    10| b = 2
    11| c = 3
=>#0    <main> at test.rb:8
(rdbg:irb) break Foo
Unknown breakpoint format: Foo


### Breakpoint

* `b[reak]`
  * Show all breakpoints.
* `b[reak] <line>`
  * Set breakpoint on `<line>` at the current frame's file.
* `b[reak] <file>:<line>` or `<file> <line>`
  * Set breakpoint on `<file>:<line>`.
* `b[reak] <class>#<name>`
   * Set breakpoint on the method `<class>#<name>`.
* `b[reak] <expr>.<name>`
   * Set breakpoint on the method `<expr>.<name>`.
* `b[reak] ... if: <expr>`
  * break if `<expr>` is true at specified location.
* `b[reak] ... pre: <command>`
  * break and run `<command>` before stopping.
* `b[reak] ... do: <command>`
  * break and run `<command>`, and continue.
* `b[reak] ... path: <path>`
  * break if the path matches to `<path>`. `<path>` can be a regexp with `/regexp/`.
* `b[reak] if: <expr>`
  * break if: `<expr>` is true at any lines.
  * Note that this feature is super slow.


# and the program exits

Given that we now have the debug command to quickly enter debugging session and the effort for these commands are not small, I think we can prioritise other improvements before Ruby 3.2. WDYT?


Not directly related, but besides these commands a feature we can learn from debug is its scriptable breakpoints. For example:

binding.irb(do: "ls") # run ls and continue the program
binding.irb(pre: "ls") # run ls and stop the program
binding.irb(pre: "ls ;; foo.bar?") # run ls, call foo.bar?, and stop the program

@k0kubun
Copy link
Member Author

k0kubun commented Nov 18, 2022

What subset of functionalities make sense in the context of irb? Just off the top of my head:

Same. It shouldn't be everything; you should use debug.gem directly if you want all. We need to hit a reasonable balance between convenience and maintainability. 20% of the features could cover 80% of the use cases, or something close enough to it.

My ambition is always about making it a Pry alternative that is available by default. In this case it's about a pry-byebug alternative. So, looking at https://github.com/deivid-rodriguez/pry-byebug, I'd say break (and delete on our side, which I've forgotten. I'll add that later)/step/next/finish/continue should be the first set of features we should support, which is what this PR does, and we could also have backtrace/up/down/frame.

When adding a command, do we want to completely replicate it here, or just part of it?

I personally prefer being a bit conservative about a single-char shorthand at least in the first release because n and c are much more likely to conflict with local variable names. I'd like to support it only through command aliases for now.

As to supporting complicated features, as long as we can delegate that to debug.gem with a reasonable implementation cost, I think there's no reason to reject it. At least for the scope that could work by writing a simple transform_args, it should be fine.

How can we help users discover them? debug has help command to help users learn all the commands' usages. But irb doesn't have it and we also don't have good documentation atm. This is already a problem, but it'll be more obvious after we introducing these commands.

This should probably be discussed in a separate issue. Personally, the way I discover debug.gem's commands and pry-byebug's commands was to look at their README. Nothing stops us from writing a table like https://github.com/ruby/debug#debug-command-on-the-debug-console at the README on https://github.com/ruby/irb, and it seems like enough discoverability, at least for me.

Implementation-wise, I think we also need to figure how to write tests for them because the scenarios to consider will grow very fast. debug has a test framework that allows us to test the debugger in a separate process so we can verify its functionalities end-to-end (example).

Nice 👍 Agreed.

I also found that the break command isn't working:

This was kind of half-intended; I didn't think I supported everything yet, but at the same time I knew this would work perfectly as soon as I write a small transform_args. I should be able to fix it as soon as I work on it.

Given that we now have the debug command to quickly enter debugging session and the effort for these commands are not small, I think we can prioritise other improvements before Ruby 3.2. WDYT?

So you're proposing to push it for another year (the whole point of this is to NOT write them in Gemfile; when you work on random OSS projects, you can only use what's installed by default) because we don't have isolated tests for it? If that's the only concern, I'll write a starter test code for each command. Again, with the presence of transform_args, delegating the implementation to debug.gem to fix any issues that could be fixed by that is a really trivial concern.

As soon as I started using debug, it turned out to be really frustrating that the features I added in this PR didn't exist. I kept using next before starting debug. Because debug.gem copied some IRB coloring stuff and tried to look similar to it, it's pretty hard to recognize whether it's IRB or debug.gem. On one hand, it's useful; obviously that's what this PR is taking advantage of. But this UI similarity instinctively trigger you to immediately use debug.gem's commands on binding.irb. IMO, I don't want to see Ruby 3.2 only with debug command because of this experience.

@st0012
Copy link
Member

st0012 commented Nov 18, 2022

because we don't have isolated tests for it?

Not just that. My concern is that with other supporting work (e.g. command documentation), issues like #445, and we're pretty short on time (1 month away from 3.2 release), I'm not sure if we can ship these commands in good shape and test them well.

But I guess we can give a try because it does look like most of the problems may be on argument handling.

However, I do feel strongly that we should make sure command documentation can be shipped along with this. So I just opened #450

@k0kubun
Copy link
Member Author

k0kubun commented Nov 19, 2022

My concern is that with other supporting work (e.g. command documentation), issues like #445

Just to be clear, I'm not asking you to implement those non-debug commands. I do understand that reviewing PRs is also a time-consuming task, but I hope it doesn't take too much or slip documentation/autocompletion improvements from the last 1-month development.

But I guess we can give a try because it does look like most of the problems may be on argument handling.

Thanks 👍 I'll give it a try, and give it up if it turns out to be too hard to write tests or fix the argument handling.

However, I do feel strongly that we should make sure command documentation can be shipped along with this. So I just opened #450

👍

lib/irb/cmd/debug.rb Outdated Show resolved Hide resolved
@st0012
Copy link
Member

st0012 commented Nov 19, 2022

After using the next command, the debug seems to take over the session completely:

From: test.rb @ line 3 :

    1: class Foo
    2:   def bar
 => 3:     binding.irb
    4:     b = 2
    5:     c = 3
    6:     d = 4
    7:   end
    8: end

irb(#<Foo:0x0000000106fec0e8>):001:0> next
Loaded debug-1.6.3
[1, 10] in test.rb
     1| class Foo
     2|   def bar
=>   3|     binding.irb
     4|     b = 2
     5|     c = 3
     6|     d = 4
     7|   end
     8| end
     9|
    10| a = 1
=>#0    Foo#bar at test.rb:3
  #1    <main> at test.rb:12
(rdbg:irb) next
[1, 10] in test.rb
     1| class Foo
     2|   def bar
     3|     binding.irb
=>   4|     b = 2
     5|     c = 3
     6|     d = 4
     7|   end
     8| end
     9|
    10| a = 1
=>#0    Foo#bar at test.rb:4
  #1    <main> at test.rb:12
(rdbg) next    # command
[1, 10] in test.rb
     1| class Foo
     2|   def bar
     3|     binding.irb
     4|     b = 2
=>   5|     c = 3
     6|     d = 4
     7|   end
     8| end
     9|
    10| a = 1
=>#0    Foo#bar at test.rb:5
  #1    <main> at test.rb:12
(rdbg) ls Object -g to    # outline command
eval error: (rdbg)/test.rb:1: syntax error, unexpected local variable or method, expecting `do' or '{' or '('
Object -g to
          ^~
(rdbg)

@st0012
Copy link
Member

st0012 commented Nov 19, 2022

^ Sorry that I probably shouldn't post issues command-by-command. It's pretty late here and I didn't think it though 😅

I'll test other commands and post what I found together.

@k0kubun
Copy link
Member Author

k0kubun commented Nov 19, 2022

It's pretty late here and I didn't think it though

Thanks for your time! Have a good night.

After using the next command, the debug seems to take over the session completely

Yes, it's the intended behavior (again, at least for Ruby 3.2. It could be changed once we have UI_*, but I think it's okay to support it after Ruby 3.2). For Ruby 3.2, it's meant to be a thin wrapper of debug command; it's only as useful as debug, and it's as inconvenient as debug command. If there's a problem in it, it's a problem for debug as well, and I think the current behavior of debug is sufficiently useful.

@k0kubun
Copy link
Member Author

k0kubun commented Nov 19, 2022

Let me get this straight. So far we've discussed two ways to introduce debugger commands to IRB:

  1. Change the session to the debugger like debug command. Then run the command on it that was originally passed to IRB.
  2. Implement a UI_* class in IRB and use that to start the debugger session. You could pass rich commands to the debugger instead of a String. Unlike debug command, the debugger session doesn't take over and you can keep talking to the debugger while staying in an IRB session.

And I'm proposing to implement only option 1 in Ruby 3.2, and potentially improving it to option 2 in Ruby 3.3 or later. I already spent time experimenting with option 2 and then left this comment #425 (comment). I gave up option 2 for Ruby 3.2. However, after that effort and playing with your debug implementation, I noticed we could implement option 1 with a reasonable cost. So here we're.

@k0kubun k0kubun force-pushed the seamless-debug-mode branch from e0563e4 to ca62e66 Compare November 19, 2022 06:20
@k0kubun
Copy link
Member Author

k0kubun commented Nov 19, 2022

I addressed all your concerns. This is working really well, even more than I initially expected. I don't see any reason to avoid shipping this in Ruby 3.2 at this point. Please have another look :)

@st0012
Copy link
Member

st0012 commented Nov 19, 2022

Thanks for all the updated and explanation 🙏

Change the session to the debugger like debug command. Then run the command on it that was originally passed to IRB.

I think I get it now: these commands are to save typing debug, <cmd> to just <cmd>. And then the user would stay in the debug session?

(When I saw "Seamlessly integrate", my immediate expectation was pry-byebug experience and I stuck with it 😅)

If my current understanding is correct, I think we can also add:

  • backtrace (for this one, can we add bt too? It's a less-likely conflicting command)
  • info - It lists variables and their values, which irb can support in the future too
  • catch - This adds exception breakpoints

@k0kubun
Copy link
Member Author

k0kubun commented Nov 19, 2022

I think I get it now: these commands are to save typing debug, to just . And then the user would stay in the debug session?
(When I saw "Seamlessly integrate", my immediate expectation was pry-byebug experience and I stuck with it sweat_smile)

Yeah, sorry about the confusion.

If my current understanding is correct, I think we can also add:

done c347b4e

@k0kubun k0kubun changed the title Seamlessly integrate a few debug commands Add commands to start and use the debugger Nov 19, 2022
@k0kubun
Copy link
Member Author

k0kubun commented Nov 20, 2022

@st0012 I also fixed a conflict after merging #453. Do you have any final feedback?

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I think the implementation looks good 👍

But I'm concerned about testing the commands with yamatanooroti. It and its dependency vterm are both not under ruby's control, so if they have problems going forward, we won't be able to fix them.

I think it's fine to keep and maintain the current tests but ideally we shouldn't add new tests with them, especially when these tests seem to be less-demanding on terminal setup. WDYT?

Also, can you help me setup vterm on Mac (with libvterm installed)? I got these errors when installing it:

current directory: /Users/hung-wulo/.gem/ruby/3.1.2/gems/vterm-0.0.5/ext/vterm
/opt/rubies/3.1.2/bin/ruby -I /opt/rubies/3.1.2/lib/ruby/3.1.0 -r ./siteconf20221120-15782-673e37.rb extconf.rb
checking for vterm.h... no
checking for vterm_new() in -lvterm... no
creating Makefile

current directory: /Users/hung-wulo/.gem/ruby/3.1.2/gems/vterm-0.0.5/ext/vterm
make DESTDIR\= clean

current directory: /Users/hung-wulo/.gem/ruby/3.1.2/gems/vterm-0.0.5/ext/vterm
make DESTDIR\=
compiling vterm.c
vterm.c:10:10: fatal error: 'vterm.h' file not found
#include "vterm.h"
         ^~~~~~~~~
1 error generated.
make: *** [vterm.o] Error 1

make failed, exit code 2

@k0kubun
Copy link
Member Author

k0kubun commented Nov 21, 2022

I think the implementation looks good 👍

Thanks! Now that Koichi has merged ruby/debug@87b1fab as well, I'll merge this PR.

Since this PR is already too big, I think your last point, adding a new console test framework to IRB, should be addressed in a separate PR. In fact, #456 is bigger than this 😅 Let's merge the test cases we already have, and then replace the framework.

I think it's fine to keep and maintain the current tests but ideally we shouldn't add new tests with them, especially when these tests seem to be less-demanding on terminal setup. WDYT?

Yup, agreed. I gave a shot at it #456. It's currently not working, but you're an expert in that framework and I think you already know the contexts to make it work. Could you have a look at it from there once you come back from a vacation? Please feel free to just abandon the PR and open another one if that's easier.

Also, can you help me setup vterm on Mac (with libvterm installed)? I got these errors when installing it:

I use Mac only when I write YJIT, so I actually don't have the answer. But I can still ask: with "libvterm installed", do you mean you have a libvterm installation that has vterm headers, not just the library to be linked? You can check if vterm.h exists under the installation prefix, and try installing another package with headers if you don't find it. Also, if you haven't done that yet, you could also try doing the same thing as what CI does. It's for Ubuntu, but it's just building it from a .tar.gz file.

@k0kubun k0kubun merged commit 976100c into ruby:master Nov 21, 2022
@k0kubun k0kubun deleted the seamless-debug-mode branch November 21, 2022 08:46
matzbot pushed a commit to ruby/ruby that referenced this pull request Nov 21, 2022
(ruby/irb#449)

* Seamlessly integrate a few debug commands

* Improve the break command support

* Utilize skip_src option if available

* Add step and delete commands

* Write end-to-end tests for each debugger command

* Add documentation

* Add backtrace, info, catch commands

ruby/irb@976100c1c2
st0012 added a commit to st0012/irb that referenced this pull request Dec 1, 2022
As discussed in ruby#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.
st0012 added a commit to st0012/irb that referenced this pull request Dec 1, 2022
As discussed in ruby#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.
st0012 added a commit to st0012/irb that referenced this pull request Dec 1, 2022
As discussed in ruby#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.
st0012 added a commit to st0012/irb that referenced this pull request Dec 1, 2022
As discussed in ruby#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.
st0012 added a commit to st0012/irb that referenced this pull request Dec 1, 2022
As discussed in ruby#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.
k0kubun pushed a commit that referenced this pull request Dec 2, 2022
* Add debug command tests that don't require yamatanooroti

* Remove debug command related yamatanooroti tests

As discussed in #449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.

* Test against latest debug gem
matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 2, 2022
(ruby/irb#464)

* Add debug command tests that don't require yamatanooroti

* Remove debug command related yamatanooroti tests

As discussed in ruby/irb#449 (review),
we should avoid adding new tests that need yamatanooroti because it's
not maintained by the Ruby org. And since debug commands are now tested
in `test/irb/test_debug_cmd.rb`, we don't need these tests anymore.

* Test against latest debug gem

ruby/irb@78a8aa8834
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants