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

Run Valgrind on CI and fix memory leaks #2309

Merged
merged 5 commits into from
Mar 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/ruby.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ group :profilers do
gem 'stackprof'
gem 'memory_profiler'
gem 'benchmark-ips'
gem "ruby_memcheck", platform: :ruby
end

# Test gems
Expand Down
7 changes: 7 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,17 @@ 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)
timeout
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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -193,6 +199,7 @@ DEPENDENCIES
rubocop
rubocop-on-rbs
rubocop-rubycw
ruby_memcheck
stackprof
steep
tempfile
Expand Down
12 changes: 11 additions & 1 deletion Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,24 @@ 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|
path =~ %r{test/stdlib/}
end
end

Rake::TestTask.new(test: :compile, &test_config)

unless Gem.win_platform?
require "ruby_memcheck"

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
Expand Down
14 changes: 14 additions & 0 deletions ext/rbs_extension/parserstate.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -387,11 +388,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);
}
7 changes: 7 additions & 0 deletions test/rbs/test/runtime_test_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down