Add ruby 2.5 support

[deivid-rodriguez]

Problem description

Byebug doesn't yet fully support ruby 2.5.

Expected behavior

Full tests pass and byebug works.

Actual behavior

As of today, there's two failures in the test suite against ruby-head, and @MSP-Greg reports that standalone byebug fails to start against it.

Steps to reproduce the problem

Install a build of trunk's ruby and run byebug against it. Using https://github.com/deivid-rodriguez/ruby-bisect against

byebug/script/ci.sh

Lines 1 to 6 in a37248a

	#!/usr/bin/env bash
	gem update --system --no-document
	gem install bundler --no-document
	bundle install --jobs 3 --retry 3
	bundle exec rake clobber compile test sign_hooks overcommit

could be useful to find out when and how the problem was introduced.

[MSP-Greg]

Thanks for links,etc. Re bisect, build & install for ruby takes a bit of time. I'll search thru your travis logs and see if I can narrow it down to a smaller range. A quick look showed that it passed with a trunk build as of 25-May, and I'm sure I can narrow further.

If you have a look at my MinGW test results page (which isn't a log of all my tests), for every date listed, I've got the full build / install saved. I may also have additional builds not listed.

Bottom line, I can narrow it down to the point where we/you will know where the issue is, or I can run a bisect on a smaller set...

EDIT BELOW

I've also got ruby_2_3 and ruby_2_4 stable branch builds, as they tend to update those in batches. I'll check against those also.

If you know without checking, are there any gems that byebug uses that could effect this issue? Do you know if they check/test against trunk/head?

[deivid-rodriguez]

No, byebug has no dependencies.

[MSP-Greg]

Sorry, noticed that afterward.

FYI, ruby has had an mswin (Visual C) build in their CI testing for a while, but they only recently added full testing. MinGW builds, which is what the vast majority of windows users have for Ruby, are not done by them. Currently, I've got a few patches that are needed, etc. IOW, someday, I may create a bisect script, but it will take a bit of dicing & chopping...

But, as mentioned, I've got a lot of saved builds. At present, I'm between:

PASSES  ruby 2.5.0dev (2017-06-14 trunk 59096) [x64-mingw32]
FAILS   ruby 2.5.0dev (2017-06-24 trunk 59168) [x64-mingw32]

I've got a script I'm running, testing on runner_test.rb. I haven't yet tested from command line.

I've got four more builds between those two. Once I narrow it down, can you bisect on *nix further?

[deivid-rodriguez]

Absolutely, thanks for this!

[MSP-Greg]

Well, I ran a few more builds, as this issue happened when I was also seeing varying failures, so I didn't save the builds. Won't do that again. Save.every.build.

59158 Fails
59153 Passes

Below are summaries/links of the commits, maybe the descriptions/patches may save you the time of the bisect.

59158 a64801c rename th->state to th->tag_state.

59157 a90c696 rb_catch_protect() accepts enum ruby_tag_type *.

59156 769ef81 thread.c: suppress warning

59155 2108e55 use "enum ruby_tag_type" and TAG_NONE.

59154 1d248f0 use NULL instead of 0.

I wrote some code to dump a few months of ruby commit info to a spreadsheet, as I got tired of cross referencing the SVN with commit SHA. Markdown links are a spreadsheet formula.

I'll do the PR to add trunk tomorrow...

[deivid-rodriguez]

@MSP-Greg Thanks a lot, you're really reducing the amount of work needed to understand & fix this.

In my opinion, the most plausible canditate is

59155 2108e55 use "enum ruby_tag_type" and TAG_NONE.

since we are using the "old way" here:

byebug/ext/byebug/byebug.c

Lines 746 to 747 in b491521

	rb_load_protect(file, 0, &state);
	if (0 != state)

[MSP-Greg]

@deivid-rodriguez

You're welcome.

In my opinion, the most plausible candidate is

Well, not being a c type, I'm certainly not going to jump into this code, or issues with int, enum, etc.

[deivid-rodriguez]

Yeah, not asking you to. It was more of a written remainder for myself or anyone else wanting to give this a try 😃.

[deivid-rodriguez]

Confirmed via https://github.com/deivid-rodriguez/ruby-bisect that

• 59155 2108e55 use "enum ruby_tag_type" and TAG_NONE

is indeed the culprit.

[deivid-rodriguez]

Marking this issue with the "help wanted" label. Ruby 2.5 release approaches and I still can't find time to work on this... 😞

[deivid-rodriguez]

I digged into this a bit more. The previous culprit was a bug in ruby, fixed by:

• 60007 6f968ca load.c: fix rb_load_protect condition

So next step is to bisect byebug against ruby between r60007 (green) and current trunk (red again).

[MSP-Greg]

@deivid-rodriguez

Well, I seem to be getting different results using Windows MinGW trunk. Below is a summary:

2017-06-14_59096   464 runs, 527 assertions, 0 failures, 0 errors, 3 skips    62 sec
2017-06-23_59153   464 runs, 527 assertions, 0 failures, 0 errors, 3 skips    30 sec

2017-06-23_59158   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips    35 sec
2017-06-23_59162   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips    38 sec
2017-06-27_59184   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips    29 sec
2017-07-01_59246   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips    20 sec
2017-07-31_59454   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips
2017-08-15_59601   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips
2017-09-03_59733   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips
2017-09-09_59792   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips   109 sec
2017-09-12_59858   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips   166 sec
2017-09-14_59870   464 runs, 527 assertions, 2 failures, 0 errors, 3 skips    60 sec

2017-09-14_59897    BAD Stopped Byebug::ThreadTest
2017-09-16_59930    BAD Stopped Byebug::ThreadTest
2017-09-25_60014    BAD Stopped Byebug::ThreadTest
2017-10-01_60083    BAD Stopped Byebug::ThreadTest
2017-12-09_61095_2  BAD Stopped Byebug::ThreadTest

The 2 failures were:

Byebug::RunnerAgainstValidProgramTest#test_run_with_stop_flag_stops_at_the_first_line [byebug/test/support/matchers.rb:27]:
Expected []
 to include ["=> 1: sleep 0"]
 in order.


Byebug::RunnerAgainstValidProgramTest#test_run_stops_at_the_first_line_by_default [byebug/test/support/matchers.rb:27]:
Expected []
 to include ["=> 1: sleep 0"]
 in order.

Below is some commit info between the 2 failure group and the 'Stopped' group:

59897  6617c412  2017-09-14 11:16  lib/webrick/log.rb: sanitize any type of logs
59896  7df1e45b  2017-09-14 10:53  ripper: add states of scanner
59895  a61ae940  2017-09-14 10:49  parse.y: [DOC] fix call-seq [ci skip]
59894  b324a649  2017-09-14 10:45  ext/coverage/coverage.c: use long instead   of int for coverage site id
59893  fff6809b  2017-09-14 09:21  fix the case High Sierra's mincore(2) may   return -128 [Bug #13895]
59892  ea49381e  2017-09-14 08:04  compile.c: iseq_pop_newarray
59891  db9f36f3  2017-09-14 07:45  ext/coverage/coverage.c (method_coverage):   `id` was used uninitialized
59890  e43f3044  2017-09-14 06:07  Measure branch and method coverage for   `make test-all`
59889  3c8c17d3  2017-09-14 05:27  Introduce NODE_UNLESS for branch coverage
59888  78cf4607  2017-09-14 05:12  Add method coverage
59887  a5641cdf  2017-09-14 05:04  added workaround for APFS file format.
59886  3155da02  2017-09-14 04:42  Fix the lineno of case statement that has   no expression
59885  1f7abf72  2017-09-14 04:32  Add branch coverage for case-when statement
59878  c4a64b73  2017-09-14 03:57  Removed needless operator.
59877  16ab236b  2017-09-14 03:36  Add branch coverage for while and until   statements
59876  ce570370  2017-09-14 03:25  Add branch coverage for if statement
59875  c171ca1e  2017-09-14 02:53  ext/coverage/coverage.c: Fix the condition   for non-experimental mode
59874  68394b27  2017-09-14 02:36  [EXPERIMENTAL] Extend the API of   `Coverage.start` and `result`
59873  4d5c414f  2017-09-14 02:01  Update gemspec for gem released versions.
59872  9ea2102e  2017-09-14 01:55  remove an unused variable (sometimes it   fails test).
59871  d1b290d5  2017-09-14 01:55  Add a new instruction `trace2` for hooking   with custom data
59870  a4eedafb  2017-09-13 17:07  test_rubyoptions.rb: keep paths if   necessary

Also, bundler was acting up for the second time recently, and I bypassed it for compiling & running tests. IOW, all I used it for was the initial gem installing/loading, which is cached on Appveyor. I did the testing locally, as I've got several months of builds stored...

HTH, Greg

[deivid-rodriguez]

Thanks Greg!

Sorry I forgot to mention that coverage tracking seems to have breaking changes as well in ruby 2.5, so we need to make sure we ignore that in these tests. We need to pass NOCOV=1 to the test suite so that coverage tracking is ignored.

I run the second bisection and the new culprit is: https://bugs.ruby-lang.org/issues/14104. In particular, https://bugs.ruby-lang.org/issues/14104#Compatibility-issues, I guess.

[MSP-Greg]

Thanks, NOCOV=1 certainly helped.

60767  0965709f  2017-11-14 15:37  * 2017-11-15
60766  50702d16  2017-11-14 15:37  parse.y: zero codepoints
60765  19ae98d5  2017-11-14 13:25  rewrite only if changed.
60764  b000b1d9  2017-11-14 13:18  fix prefix.
60763  665ba24b  2017-11-14 12:58  remove `trace` instruction. [Feature   #14104]
60762  fe3decb2  2017-11-14 04:42  process.c: removed preserving_errno

60762 passes, 60767 fails, and, as you've mentioned, the issue is probably 60763.

FYI, test times seem to be jumping around quite a bit. Seems that one of the test files is leaving artifacts that slow successive tests? As shown below:

2017-09-25_60014    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips  132 sec
2017-10-01_60083    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   25 sec
2017-10-15_60184    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips  144 sec
2017-11-01_60599    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   42 sec
2017-11-07_60677    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips  112 sec
2017-11-09_60723    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   27 sec
2017-11-11_60742    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   89 sec
2017-11-13_60753    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   64 sec
2017-11-14_60760    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   33 sec
2017-11-14_60762    464 runs, 527 assertions, 0 failures, 0 errors, 3 skips   26 sec

2017-11-15_60767    BAD Stopped Byebug::ThreadTest
2017-12-09_61095_2  BAD Stopped Byebug::ThreadTest

[deivid-rodriguez]

Looks like the breaking change was made backwards-compatible, just for us. ❤️ ❤️ ❤️

[MSP-Greg]

I was just reading the issue at ruby-lang.org. Thanks to @ko1 for the fix, and @deivid-rodriguez for testing on Appveyor w/trunk. The current ruby-loco build has includes ko1's commit in case you want to force a test. Current Travis build is from yesterday, don't know if they update on Saturday, but they normally update within the next hour (17:00 UTC +/-)

[ko1]

@yui-knk will send PR for ruby 2.5.

With the following patch, it works on Ruby 2.5. But this patch is strongly connected with 2.5 behavior. So that @yui-knk will send better patch.

diff --git a/lib/byebug/breakpoint.rb b/lib/byebug/breakpoint.rb
index 5a17d5e..11e8899 100644
--- a/lib/byebug/breakpoint.rb
+++ b/lib/byebug/breakpoint.rb
@@ -39,6 +39,12 @@ module Byebug
       Byebug.breakpoints.reject! { |b| b.id == id }
     end
 
+    def self.possible_trace_points iseq, h = {}
+      iseq.trace_points.each{|line, event| h[line] = true}
+      iseq.each_child{|child| possible_trace_points child, h}
+      h
+    end
+
     #
     # Returns an array of line numbers in file named +filename+ where
     # breakpoints could be set. The list will contain an entry for each
@@ -49,17 +55,9 @@ module Byebug
     #
     def self.potential_lines(filename)
       name = "#{Time.new.to_i}_#{rand(2**31)}"
-      lines = {}
       iseq = RubyVM::InstructionSequence.compile(File.read(filename), name)
-
-      iseq.disasm.each_line do |line|
-        res = /^\d+ (?<insn>\w+)\s+.+\(\s*(?<lineno>\d+)\)$/.match(line)
-        next unless res && res[:insn] == 'trace'
-
-        lines[res[:lineno].to_i] = true
-      end
-
-      lines.keys
+      lines = possible_trace_points iseq
+      lines.keys.sort
     end
 
     #

diff --git a/test/commands/break_test.rb b/test/commands/break_test.rb
index 88f926f..6e33298 100644
--- a/test/commands/break_test.rb
+++ b/test/commands/break_test.rb
@@ -41,37 +41,37 @@ module Byebug
     def test_break_with_instance_method_stops_at_correct_place
       enter "break #{example_class}#b", 'cont'
 
-      debug_code(program) { assert_location example_path, 12 }
+      debug_code(program) { assert_location example_path, 13 }
     end
 
     def test_break_with_namespaced_instance_method_stops_at_correct_place
       enter "break Byebug::#{example_class}#b", 'cont'
 
-      debug_code(program) { assert_location example_path, 12 }
+      debug_code(program) { assert_location example_path, 13 }
     end
 
     def test_break_with_class_method_stops_at_correct_place
       enter "break #{example_class}.a", 'cont'
 
-      debug_code(program) { assert_location example_path, 6 }
+      debug_code(program) { assert_location example_path, 7 }
     end
 
     def test_break_with_namespaced_class_method_stops_at_correct_place
       enter "break Byebug::#{example_class}.a", 'cont'
 
-      debug_code(program) { assert_location example_path, 6 }
+      debug_code(program) { assert_location example_path, 7 }
     end
 
     def test_break_with_module_method_stops_at_correct_place
       enter "break #{example_module}.c", 'cont'
 
-      debug_code(program) { assert_location(example_path, 18) }
+      debug_code(program) { assert_location(example_path, 19) }
     end
 
     def test_break_with_namespaced_module_method_stops_at_correct_place
       enter "break Byebug::#{example_module}.c", 'cont'
 
-      debug_code(program) { assert_location example_path, 18 }
+      debug_code(program) { assert_location example_path, 19 }
     end
 
     def test_break_with_a_method_does_not_stop_at_blocks_in_the_method
diff --git a/test/commands/debug_test.rb b/test/commands/debug_test.rb
index 646283f..717fb65 100644
--- a/test/commands/debug_test.rb
+++ b/test/commands/debug_test.rb
@@ -33,7 +33,7 @@ module Byebug
     def test_subdebugger_stops_at_correct_point_when_invoked_from_breakpoint
       enter "break #{example_class}.a", "debug #{example_class}.a"
 
-      debug_code(program) { assert_equal 6, frame.line }
+      debug_code(program) { assert_equal 7, frame.line }
     end
 
     def test_subdebugger_goes_back_to_previous_debugger_after_continue

Please ping me freely :)
@yui-knk tells me this issue.

Thanks,
Koichi

[deivid-rodriguez]

Thanks so much @ko1, I only found out very recently and I was going to comment in the bugs.ruby-lang.org issue, but then I got two weeks of a lot of work and I couldn't find time.

Waiting for @yui-knk's patch then!