From 40b6616eb73204c9aed7574fa63a8141d9109d60 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Tue, 4 Mar 2025 18:19:13 +0000 Subject: [PATCH 1/5] Run valgrind on CI to check memory leaks Many gems with C-extensions have been using valgrind to check memory leaks, such as `prism`, `lrama`, and `json`. The simplest way to run valgrind on CI is through `ruby_memcheck` gem, which is used by both `prism` and `json`. So I followed its readme to add a new job `valgrind` to `ruby.yml`. --- .github/workflows/ruby.yml | 26 ++++++++++++++++++++++++++ Gemfile | 1 + Gemfile.lock | 7 +++++++ Rakefile | 14 +++++++++++++- 4 files changed, 47 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml index dd06f5123..acd9bf63a 100644 --- a/.github/workflows/ruby.yml +++ b/.github/workflows/ruby.yml @@ -80,3 +80,29 @@ jobs: - name: Run test run: | bundle exec rake ${{ matrix.job }} + valgrind: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.4" + bundler-cache: none + - name: Set working directory as safe + run: git config --global --add safe.directory $(pwd) + - name: Set up permission + run: chmod -R o-w /opt/hostedtoolcache/Ruby + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y libdb-dev curl autoconf automake m4 libtool python3 valgrind + - name: Update rubygems & bundler + run: | + ruby -v + gem update --system + - name: bin/setup + run: | + bin/setup + - run: bundle exec rake test:valgrind + env: + RUBY_FREE_AT_EXIT: 1 diff --git a/Gemfile b/Gemfile index ca3cc6f1e..095664ffa 100644 --- a/Gemfile +++ b/Gemfile @@ -37,6 +37,7 @@ group :profilers do gem 'stackprof' gem 'memory_profiler' gem 'benchmark-ips' + gem "ruby_memcheck", platform: :ruby end # Test gems diff --git a/Gemfile.lock b/Gemfile.lock index 6f413c0d0..18e5b26f5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -60,6 +60,7 @@ GEM logger (1.6.6) marcel (1.0.4) memory_profiler (1.1.0) + mini_portile2 (2.8.8) minitest (5.25.4) mutex_m (0.3.0) net-protocol (0.2.2) @@ -67,6 +68,9 @@ GEM net-smtp (0.5.1) net-protocol nkf (0.2.0) + nokogiri (1.18.3) + mini_portile2 (~> 2.8.2) + racc (~> 1.4) ostruct (0.6.1) parallel (1.26.3) parser (3.3.7.1) @@ -126,6 +130,8 @@ GEM lint_roller (~> 1.1) rubocop (~> 1.72.1) ruby-progressbar (1.13.0) + ruby_memcheck (3.0.1) + nokogiri securerandom (0.4.1) stackprof (0.2.27) steep (1.9.4) @@ -193,6 +199,7 @@ DEPENDENCIES rubocop rubocop-on-rbs rubocop-rubycw + ruby_memcheck stackprof steep tempfile diff --git a/Rakefile b/Rakefile index bd7e3d3df..eb989260b 100644 --- a/Rakefile +++ b/Rakefile @@ -3,6 +3,10 @@ require "rake/testtask" require "rbconfig" require 'rake/extensiontask' +on_windows = /mswin|mingw/ =~ RUBY_PLATFORM + +require "ruby_memcheck" if !on_windows + $LOAD_PATH << File.join(__dir__, "test") ruby = ENV["RUBY"] || RbConfig.ruby @@ -11,7 +15,7 @@ bin = File.join(__dir__, "bin") Rake::ExtensionTask.new("rbs_extension") -Rake::TestTask.new(:test => :compile) do |t| +test_config = lambda do |t| t.libs << "test" t.libs << "lib" t.test_files = FileList["test/**/*_test.rb"].reject do |path| @@ -19,6 +23,14 @@ Rake::TestTask.new(:test => :compile) do |t| end end +Rake::TestTask.new(test: :compile, &test_config) + +unless on_windows + namespace :test do + RubyMemcheck::TestTask.new(valgrind: :compile, &test_config) + end +end + multitask :default => [:test, :stdlib_test, :typecheck_test, :rubocop, :validate, :test_doc] task :lexer do From ee323ed41046228786e49ada18797718c092e5a1 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Wed, 5 Mar 2025 16:24:52 -0500 Subject: [PATCH 2/5] Skip tests that would incorrectly fail valgrind --- test/rbs/test/runtime_test_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/rbs/test/runtime_test_test.rb b/test/rbs/test/runtime_test_test.rb index 89dc2e2ba..7a1807fc2 100644 --- a/test/rbs/test/runtime_test_test.rb +++ b/test/rbs/test/runtime_test_test.rb @@ -20,6 +20,10 @@ def test_runtime_test_with_sample_size end def test_runtime_test_error_with_invalid_sample_size + # Skip this test if running under Valgrind because `RUBY_FREE_AT_EXIT` has a bug. + # See: https://bugs.ruby-lang.org/issues/21173 + omit if ENV["RUBY_MEMCHECK_RUNNING"] + string_err_msg = refute_test_success(other_env: {"RBS_TEST_SAMPLE_SIZE" => 'yes'}) assert_match(/E, .+ ERROR -- rbs: Sample size should be a positive integer: `.+`\n/, string_err_msg) @@ -93,6 +97,9 @@ def refute_test_success(other_env: {}, rbs_content: nil, ruby_content: nil) end def test_test_target + # Skip this test if running under Valgrind because `RUBY_FREE_AT_EXIT` has a bug. + # See: https://bugs.ruby-lang.org/issues/21173 + omit if ENV["RUBY_MEMCHECK_RUNNING"] output = refute_test_success(other_env: { "RBS_TEST_TARGET" => nil }) assert_match "test/setup handles the following environment variables:", output end From 9cf3d98c904eec01490a4208bf68f4669a5ba1fa Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 6 Mar 2025 23:30:06 +0000 Subject: [PATCH 3/5] Free typevar tables when freeing parser --- ext/rbs_extension/parserstate.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ext/rbs_extension/parserstate.c b/ext/rbs_extension/parserstate.c index ceb3d3cc5..6d0a23c33 100644 --- a/ext/rbs_extension/parserstate.c +++ b/ext/rbs_extension/parserstate.c @@ -387,11 +387,24 @@ parserstate *alloc_parser(VALUE buffer, lexstate *lexer, int start_pos, int end_ return parser; } +void free_typevar_tables(id_table *table) { + while (table != NULL) { + id_table *next = table->next; + if (table->ids != NULL) { + free(table->ids); + } + free(table); + table = next; + } +} + void free_parser(parserstate *parser) { free(parser->lexstate); if (parser->last_comment) { free_comment(parser->last_comment); } + + free_typevar_tables(parser->vars); rbs_constant_pool_free(&parser->constant_pool); free(parser); } From 686e0cc14aa562da20d79662b3e3a8cb4b3d6af8 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Fri, 7 Mar 2025 15:02:22 +0000 Subject: [PATCH 4/5] Free the parser if alloc_parser raises --- ext/rbs_extension/parserstate.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/rbs_extension/parserstate.c b/ext/rbs_extension/parserstate.c index 6d0a23c33..98e5773fc 100644 --- a/ext/rbs_extension/parserstate.c +++ b/ext/rbs_extension/parserstate.c @@ -363,6 +363,7 @@ parserstate *alloc_parser(VALUE buffer, lexstate *lexer, int start_pos, int end_ if (!NIL_P(variables)) { if (!RB_TYPE_P(variables, T_ARRAY)) { + free_parser(parser); rb_raise(rb_eTypeError, "wrong argument type %"PRIsVALUE" (must be array or nil)", rb_obj_class(variables)); From 9a71cb1d7bbc051e6121b1f2a4cb3bf4b05f3791 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Fri, 7 Mar 2025 18:19:43 +0000 Subject: [PATCH 5/5] Improve Rakefile setup --- Rakefile | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Rakefile b/Rakefile index eb989260b..77e9d6521 100644 --- a/Rakefile +++ b/Rakefile @@ -3,10 +3,6 @@ require "rake/testtask" require "rbconfig" require 'rake/extensiontask' -on_windows = /mswin|mingw/ =~ RUBY_PLATFORM - -require "ruby_memcheck" if !on_windows - $LOAD_PATH << File.join(__dir__, "test") ruby = ENV["RUBY"] || RbConfig.ruby @@ -25,7 +21,9 @@ end Rake::TestTask.new(test: :compile, &test_config) -unless on_windows +unless Gem.win_platform? + require "ruby_memcheck" + namespace :test do RubyMemcheck::TestTask.new(valgrind: :compile, &test_config) end