Skip to content

A few cleaning up for the mmtk GC module #36

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

Merged
merged 5 commits into from
May 30, 2025

Conversation

wks
Copy link
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
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
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
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
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