Skip to content

A few cleaning up for the mmtk GC module#36

Merged
peterzhu2118 merged 5 commits into
ruby:mainfrom
wks:fix/env_var_parsing
May 30, 2025
Merged

A few cleaning up for the mmtk GC module#36
peterzhu2118 merged 5 commits into
ruby:mainfrom
wks:fix/env_var_parsing

Conversation

@wks
Copy link
Copy Markdown
Collaborator

@wks wks commented May 28, 2025

This pull request includes several commits that clean up the code, including:

  • Removed an unused constant HAS_MOVED_GFIELDSTBL. The mmtk/mmtk-ruby repo has stopped using that flag a while ago, so we can remove it here, too.
  • Updated the mmtk Rust dependency to the current latest version.
  • Fixed all Clippy warnings (cargo clippy) and formatting issues (cargo fmt --check).
    • I also added a GitHub CI test for checking code styles.
  • Rewrote some of the environment variable parsing code to use more idiomatic Rust style.

@wks wks requested a review from peterzhu2118 May 28, 2025 09:25
@wks wks force-pushed the fix/env_var_parsing branch from 831d2fd to 471ae81 Compare May 28, 2025 12:24
@wks
Copy link
Copy Markdown
Collaborator Author

wks commented May 29, 2025

ruby/ruby@f483bef#diff-db708b4117de648f1041678860fd9c891e3e45773eb1bcde4f4bd83c9558f6ed made changes to the file /gc/mmtk/mmtk.c directly in the ruby/ruby repo. The same change is not done in this repo. Therefore the GC::INTERNAL_CONSTANTS[:RBASIC_SIZE] is missing, causing some tests to fail.

@wks
Copy link
Copy Markdown
Collaborator Author

wks commented May 29, 2025

ruby/gc/mmtk/mmtk.c includes this line:

#include "internal/object.h"

If I compile the mmtk GC module inside the ruby repo, it will work.

make modular-gc install-modular-gc MODULAR_GC=mmtk MMTK_BUILD=debug

But if I copy mmtk.c from the ruby repo to the mmtk repo, goto the mmtk repo and compile it there, it will complain that it cannot include "internal/object.h". See https://github.com/ruby/mmtk/actions/runs/15314883809/job/43086759608?pr=36

> ~/projects/mmtk-github/parallels/ruby-master/ruby/build-debug/install/bin/bundle exec ~/projects/mmtk-github/parallels/ruby-master/ruby/build-debug/install/bin/rake install:debug
cd tmp/x86_64-linux/debug/3.5.0
/usr/bin/make
compiling ../../../../gc/mmtk/mmtk.c
../../../../gc/mmtk/mmtk.c: In function ‘rb_gc_impl_shutdown_call_finalizer’:
../../../../gc/mmtk/mmtk.c:1023:13: error: implicit declaration of function ‘RBASIC_RESET_FLAGS’ [-Wimplicit-function-declaration]
 1023 |             RBASIC_RESET_FLAGS(obj);
      |             ^~~~~~~~~~~~~~~~~~
At top level:
cc1: note: unrecognized command-line option ‘-Wno-self-assign’ may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option ‘-Wno-parentheses-equality’ may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option ‘-Wno-constant-logical-operand’ may have been intended to silence earlier diagnostics
make: *** [Makefile:264: mmtk.o] Error 1
rake aborted!
Command failed with status (2): [/usr/bin/make]
/home/wks/projects/mmtk-github/parallels/ruby-master/ruby/build-debug/install/bin/bundle:25:in 'Kernel#load'
/home/wks/projects/mmtk-github/parallels/ruby-master/ruby/build-debug/install/bin/bundle:25:in '<main>'
Tasks: TOP => install:debug => compile:debug => compile:debug:x86_64-linux => copy:debug:x86_64-linux:3.5.0 => tmp/x86_64-linux/debug/3.5.0/librubygc.mmtk.so
(See full trace by running task with --trace)

@peterzhu2118
Copy link
Copy Markdown
Member

I fixed this issue on the main branch.

wks added 4 commits May 30, 2025 09:33
Remove the unused constant HAS_MOVED_GFIELDSTBL and related methods.

In the mmtk/mmtk-ruby repo, we are now able to find the global field
(IV) table of a moved object during copying GC without using the
HAS_MOVED_GFIELDSTBL bit.  We synchronize some of the code, although we
haven't implemented moving GC in ruby/mmtk, yet.

See: mmtk/mmtk-ruby@13080ac
We also enable `#![warn(unsafe_op_in_unsafe_fn)]` in the whole mmtk_ruby
crate.
Ues more idiomatic rust approaches.
@wks wks force-pushed the fix/env_var_parsing branch from 2cbc2a1 to 3b8c7e2 Compare May 30, 2025 01:41
This will deny lint and code formatting issues.
@wks wks force-pushed the fix/env_var_parsing branch from 3b8c7e2 to 11435ab Compare May 30, 2025 02:40
@wks
Copy link
Copy Markdown
Collaborator Author

wks commented May 30, 2025

All tests passed except Prism::RactorTest#test_parse_file, which failed in a way similar to #23

@peterzhu2118 peterzhu2118 merged commit c38b5ba into ruby:main May 30, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants