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 main Ruby tests with MMTk enabled in CI #78

Closed
wants to merge 41 commits into from

Conversation

eightbitraptor
Copy link
Collaborator

Replaces #77 - I've opened a new PR from the Shopify Ruby fork to enable @eileencodes to take over maintenance.


This commit creates a github actions workflow that builds Ruby with the latest commit of the mmtk-ruby binding and runs make test-all with MMTk enabled.

Several changes were necessary to make this CI pipeline pass:

Fix a bug in the setup-directories task so the directory hierarchy can be created succesfully.

C99 prohibits empty compile units, so some code had to be moved around in mmtk_support.c to introduce some superfluous #include directives. These should either be optimised out or remain unused so have little impact on the final binary.

Correct the dependancy lists in common.mk, by building with the appropiate flags defined in tool/update-deps and subsequently running make update-deps

Pre-fixing all externally visible MMTk related symbols and globals with rb_

Skipping tests related to GC compaction or other functionality specific to Ruby's GC.

Updating YJIT bindings

This commit creates a github actions workflow that builds Ruby with the
latest commit of the mmtk-ruby binding and runs `make test-all` with
MMTk enabled.

Several changes were necessary to make this CI pipeline pass:

- C99 prohibits empty compile units, so some code had to be moved around
  in mmtk_support.c to introduce some superfluous #include directives.
  These should either be optimised out or remain unused so have little
  impact on the final binary.

- Correct the dependancy lists in common.mk, by building with the
  appropiate flags defined in `tool/update-deps` and subsequently
  running `make update-deps`

- Pre-fixing all externally visible MMTk related symbols and globals with `rb_`

- Skipping tests related to GC compaction or other functionality
  specific to Ruby's GC.

- Updating YJIT bindings
The native extension is missing a symbol
eightbitraptor and others added 13 commits July 19, 2024 13:48
fixes segv in open_ssl/test_ssl.rb test related to ruby assertion that
capacity was sized correctly.
If we use the `excludes-dir` functionality instead of making a method to
skip tests we:

1) get a smaller diff
2) can just skip entire files without needing to include a module
3) don't run into the missing symbol problem caused when checking enabled?
in certain tests

Locally tests can be run like this to skip the problematic tests/files:

```
make -j12 test-all RUBY_TESTOPTS="--excludes-dir=test/.excludes-mmtk" RUN_OPTS="--mmtk --mmtk-plan=StickyImmix" TESTS=test/objspace/test_objspace.rb
```
these need to be in a directory
* The filename was incorrect
* We don't need to skip all of them, only 6 are segfaulting
Without this, nogc won't run at all and is completely broken. It's
required to set a max heap for NoGC.
These tests are testing an implementation detail of Ruby's GC.
The crash reports don't include `+MMTK` in the version string. They
don't for prism or yjit either, but instead of working around it and
"fixing" it in the test, just skip them.
GC compaction isn't supported by mmtk
These are just testing the bug report matches the expected output, not
really worth fixing for this
Object allocation tracing isn't supported in MMTk
Auto compaction isn't implemented in mmtk, we can skip these
ObjectSpace isn't implemented for mmtk
Otherwise we run out of space and it panics trying to do a GC.

Note: I don't think mmtk should try to GC if we run out of space, it
should panic and say ran out of space. It's confusing that it tries to
GC at all.
In MMTK the str_embed_capa is much larger (64) than CRuby (which is 16)
so increase the string size to test mmtk
The other builds have a `working-directory` key, but this one did not.
@wks
Copy link

wks commented Jul 26, 2024

The following patch fixes a compilation error in debug build (if -DUSE_RUBY_DEBUG_LOG=1)

commit fb6d11f0991f2c6c25620e49c6f912076d4dbb96 (HEAD -> mvh-tests-ci)
Author: Kunshan Wang <[email protected]>
Date:   Fri Jul 26 13:35:15 2024 +0800

    Fix compilation error in debug build
    
    A variable name in a RUBY_DEBUG_LOG macro is not updated.

diff --git a/mmtk_support.c b/mmtk_support.c
index 74cbc0836f..c6c8e5523d 100644
--- a/mmtk_support.c
+++ b/mmtk_support.c
@@ -289,7 +289,7 @@ set_variables_from_options(MMTk_Builder *mmtk_builder)
     RUBY_DEBUG_LOG("mmtk_plan_uses_bump_pointer = %d\n", rb_mmtk_plan_uses_bump_pointer);
 
     rb_mmtk_plan_implicitly_pinning = mmtk_builder_is_mark_sweep(mmtk_builder);
-    RUBY_DEBUG_LOG("mmtk_plan_implicitly_pinning = %d\n", mmtk_plan_implicitly_pinning);
+    RUBY_DEBUG_LOG("mmtk_plan_implicitly_pinning = %d\n", rb_mmtk_plan_implicitly_pinning);
 
     // We sometimes for disabling or enabling barriers to measure the impact of barriers.
     const char* barrier_env_var = getenv("RB_MMTK_FORCE_BARRIER");

@wks
Copy link

wks commented Jul 26, 2024

The GitHub CI reports the MMTk test failed when running with the Immix plan. According to the error message (https://github.com/mmtk/ruby/actions/runs/10082854589/job/27878079140?pr=78#step:10:285), a segmentation fault happened when trying to scan a "potential pinning parent" (PPP) object in order to pin its children. The segmentation fault was caused by accessing a very low address 0x0000000000000028. It looks like some invalid object references got added to the list of potential pinning parents for some reason.

The log said it happens while running test/prism/snippets_test.rb, but I can't reproduce this locally by running this test case alone. I think an error happened while running other tests, corrupting some data structure in the mmtk-ruby binding (including the C part or the Rust part), but only manifests in a GC triggered during snippets_test.rb. So the test case snippets_test.rb itself should be innocent.

* Remove diffs that change nothing
* Fix skipped nogc test
* We don't need --mmtk flag if we set the plan, mmtk is turned on
automatically
@eileencodes
Copy link
Collaborator

Replaced by #80 which squashes the commits and cleans up some changes.

@eileencodes
Copy link
Collaborator

Closing this now that I merged #80. We'll work on the remaining crashes in separate PRs (some are already fixed).

@eileencodes eileencodes closed this Aug 1, 2024
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.

3 participants