From f7f1cb4497c90380685d4ae40645ed7c4ac0ae88 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 25 May 2022 10:41:53 +0100 Subject: [PATCH 1/4] Support per-project rdbgrc(.rb) files Each project may have its own project-specific debugger configuration, like different paths to skip. So the debugger should also read rdbgrc files at the project root, similar to what irb does with irbrc. --- lib/debug/session.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/debug/session.rb b/lib/debug/session.rb index db29bd1e6..7e2318482 100644 --- a/lib/debug/session.rb +++ b/lib/debug/session.rb @@ -2024,11 +2024,13 @@ class << self end def self.load_rc - [[File.expand_path('~/.rdbgrc'), true], - [File.expand_path('~/.rdbgrc.rb'), true], - # ['./.rdbgrc', true], # disable because of security concern - [CONFIG[:init_script], false], - ].each{|(path, rc)| + [ + [File.expand_path('~/.rdbgrc'), true], + [File.expand_path('~/.rdbgrc.rb'), true], + [File.join(Dir.pwd, '.rdbgrc'), true], + [File.join(Dir.pwd, '.rdbgrc.rb'), true], + [CONFIG[:init_script], false], + ].each{|(path, rc)| next unless path next if rc && CONFIG[:no_rc] # ignore rc From 14423d7db091d064f0183f3712e0d7c92977f0b3 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 25 May 2022 10:58:40 +0100 Subject: [PATCH 2/4] Expand paths before matching them for skipping Frame paths are usually presented unexpanded: ``` ~/.rbenv/versions/3.1.0/lib/ruby/3.1.0/irb/workspace.rb ``` But important Ruby/Rubygem paths are all stored expanded: ``` RbConfig::CONFIG["rubylibdir"] #=> "/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/3.1.0" Gem.path #=> ["/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0", "/Users/st0012/.gem/ruby/3.1.0"] ``` So expanding frame paths before matching is necessary. And on the otherhand, users may also provide unexpanded skip_path. Therefore we should expand it too. --- lib/debug/config.rb | 2 +- lib/debug/thread_client.rb | 4 ++++ test/console/config_test.rb | 11 +++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/debug/config.rb b/lib/debug/config.rb index 8b176ae09..6253cf8bb 100644 --- a/lib/debug/config.rb +++ b/lib/debug/config.rb @@ -235,7 +235,7 @@ def self.parse_config_value name, valstr if /\A\/(.+)\/\z/ =~ e Regexp.compile $1 else - e + File.expand_path(e) end } else diff --git a/lib/debug/thread_client.rb b/lib/debug/thread_client.rb index e9475c460..bfecebd18 100644 --- a/lib/debug/thread_client.rb +++ b/lib/debug/thread_client.rb @@ -19,6 +19,10 @@ module SkipPathHelper def skip_path?(path) CONFIG.skip? || !path || skip_internal_path?(path) || + skip_config_skip_path?(path) + end + + def skip_config_skip_path?(path) (skip_paths = CONFIG[:skip_path]) && skip_paths.any?{|skip_path| path.match?(skip_path)} end diff --git a/test/console/config_test.rb b/test/console/config_test.rb index ea08f2cd6..a284d8642 100644 --- a/test/console/config_test.rb +++ b/test/console/config_test.rb @@ -274,6 +274,17 @@ def test_skip_path_skip_catch_breakpoint type 'c' end end + + def test_skip_path_expands_the_path + debug_code do + type "config set skip_path ~/test.rb" + type "config skip_path" + # we can't do direct compare using the expanded absolute paths here + # because GH Action doesn't allow modifying HOME path and the result will be different between there and local + assert_no_line_text /~\// + type "q!" + end + end end class ConfigKeepAllocSiteTest < ConsoleTestCase From de63c2da431144fc65790519e8b05e7422935111 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 25 May 2022 12:23:35 +0100 Subject: [PATCH 3/4] Use static value to filter frames FrameInfo#location_str uses #pretty_path underneath, which's value can change when use_short_path is enabled. This makes frame filtering unreliable and we should avoid it by using a more static attribute instead. --- lib/debug/frame_info.rb | 5 +++++ lib/debug/thread_client.rb | 14 +++++--------- test/console/backtrace_test.rb | 21 +++++++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/lib/debug/frame_info.rb b/lib/debug/frame_info.rb index 632a83bd1..3f78dcb4f 100644 --- a/lib/debug/frame_info.rb +++ b/lib/debug/frame_info.rb @@ -115,6 +115,11 @@ def return_str end end + def real_location + # realpath can sometimes be nil so we can't use it here + "#{path}:#{location.lineno}" + end + def location_str "#{pretty_path}:#{location.lineno}" end diff --git a/lib/debug/thread_client.rb b/lib/debug/thread_client.rb index bfecebd18..45b722723 100644 --- a/lib/debug/thread_client.rb +++ b/lib/debug/thread_client.rb @@ -656,15 +656,11 @@ def show_frames max = nil, pattern = nil if @target_frames && (max ||= @target_frames.size) > 0 frames = [] @target_frames.each_with_index{|f, i| - next if pattern && !(f.name.match?(pattern) || f.location_str.match?(pattern)) - next if CONFIG[:skip_path] && CONFIG[:skip_path].any?{|pat| - case pat - when String - f.location_str.start_with?(pat) - when Regexp - f.location_str.match?(pat) - end - } + # we need to use FrameInfo#real_location because #location_str is for display + # and it may change based on configs (e.g. use_short_path) + next if pattern && !(f.name.match?(pattern) || f.real_location.match?(pattern)) + # avoid using skip_path? because we still want to display internal frames + next if skip_config_skip_path?(f.real_location) frames << [i, f] } diff --git a/test/console/backtrace_test.rb b/test/console/backtrace_test.rb index 9d6697cda..cb1247ab7 100644 --- a/test/console/backtrace_test.rb +++ b/test/console/backtrace_test.rb @@ -114,6 +114,27 @@ def test_backtrace_takes_both_number_and_pattern type 'q!' end end + + def test_frame_filtering_works_with_unexpanded_path_and_expanded_skip_path + foo_file = 'class Foo; def bar; debugger; end; end' + program = <<~RUBY + 1| load "~/foo.rb" + 2| Foo.new.bar + RUBY + + debug_code(program) do + type "file = File.open('#{pty_home_dir}/foo.rb', 'w+') { |f| f.write('#{foo_file}') }" + type 'c' + type 'bt' + assert_line_text(/Foo#bar/) + assert_line_text(/~\/foo\.rb/) + type "DEBUGGER__::CONFIG[:skip_path] = '#{pty_home_dir}/foo.rb'" + type 'bt' + assert_no_line_text(/Foo#bar/) # ~/foo.rb should match foo.rb's absolute path and be skipped + assert_no_line_text(/~\/foo\.rb/) + type 'c' + end + end end class BlockTraceTest < ConsoleTestCase From 5fdf8663714891a1768faedf43feb21c694d2192 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 3 Jul 2022 14:59:26 +0100 Subject: [PATCH 4/4] Improve HOME setup for PTY process --- test/support/console_test_case.rb | 26 ++++++++++++++++++++++++++ test/support/test_case.rb | 16 ---------------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/test/support/console_test_case.rb b/test/support/console_test_case.rb index 390e1f092..79d774129 100644 --- a/test/support/console_test_case.rb +++ b/test/support/console_test_case.rb @@ -9,6 +9,32 @@ class ConsoleTestCase < TestCase warn "Tests on local and remote. You can disable remote tests with RUBY_DEBUG_TEST_NO_REMOTE=1." end + class << self + attr_reader :pty_home_dir + + def startup + @pty_home_dir = + if ENV["CI"] + # CIs usually doesn't allow overriding the HOME path + # we also don't need to worry about adding or being affected by ~/.rdbgrc on CI + # so we can just use the original home page there + Dir.home + else + Dir.mktmpdir + end + end + + def shutdown + unless ENV["CI"] + FileUtils.remove_entry @pty_home_dir + end + end + end + + def pty_home_dir + self.class.pty_home_dir + end + def create_message fail_msg, test_info debugger_msg = <<~DEBUGGER_MSG.chomp -------------------- diff --git a/test/support/test_case.rb b/test/support/test_case.rb index da7038337..83aa70657 100644 --- a/test/support/test_case.rb +++ b/test/support/test_case.rb @@ -22,18 +22,6 @@ class TestCase < Test::Unit::TestCase include AssertionHelpers - class << self - attr_reader :pty_home_dir - - def startup - @pty_home_dir = Dir.mktmpdir - end - - def shutdown - FileUtils.remove_entry @pty_home_dir - end - end - def setup @temp_file = nil end @@ -42,10 +30,6 @@ def teardown remove_temp_file end - def pty_home_dir - self.class.pty_home_dir - end - def temp_file_path @temp_file.path end