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

Flush mutator buffers in destroy_mutator #1045

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

k-sareen
Copy link
Collaborator

No description provided.

@k-sareen k-sareen requested a review from wks December 12, 2023 18:25
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think a deeper problem is that the semantics of flush_mutator is unclear, and it should be removed from the API for the VM binding.

See: #1047

And this PR LGTM because it makes the semantics of destroy_mutator clear. We should remove flush_mutator and fix the OpenJDK binding in another PR.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Dec 13, 2023
@qinsoon
Copy link
Member

qinsoon commented Dec 13, 2023

The Ruby tests failed, but it could be unrelated with the PR: https://github.com/mmtk/mmtk-core/actions/runs/7192554303/job/19589222684. @wks Can you check this before merging?

@wks
Copy link
Collaborator

wks commented Dec 13, 2023

The Ruby tests failed, but it could be unrelated with the PR: https://github.com/mmtk/mmtk-core/actions/runs/7192554303/job/19589222684. @wks Can you check this before merging?

Very strange. It is a segmentation fault without any further message.

Actually I am about to make an update to the MMTk-ruby binding. I'll re-run that after updating the Ruby binding, first.

@qinsoon
Copy link
Member

qinsoon commented Dec 13, 2023

The Ruby tests failed, but it could be unrelated with the PR: https://github.com/mmtk/mmtk-core/actions/runs/7192554303/job/19589222684. @wks Can you check this before merging?

Very strange. It is a segmentation fault without any further message.

Actually I am about to make an update to the MMTk-ruby binding. I'll re-run that after updating the Ruby binding, first.

There was an error message in the first attemp:

  ERROR: An MMTk GC thread panicked.  This is a bug.
  panicked at 'Object 0x200fff78718 does not have HAS_MOVED_GIVTBL flag or original givtbl', src/abi.rs:111:17
  Backtrace is disabled.
  run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@wks
Copy link
Collaborator

wks commented Dec 13, 2023

The Ruby tests failed, but it could be unrelated with the PR: https://github.com/mmtk/mmtk-core/actions/runs/7192554303/job/19589222684. @wks Can you check this before merging?

Very strange. It is a segmentation fault without any further message.
Actually I am about to make an update to the MMTk-ruby binding. I'll re-run that after updating the Ruby binding, first.

There was an error message in the first attemp:

  ERROR: An MMTk GC thread panicked.  This is a bug.
  panicked at 'Object 0x200fff78718 does not have HAS_MOVED_GIVTBL flag or original givtbl', src/abi.rs:111:17
  Backtrace is disabled.
  run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is reproducible locally. I think there is a bug.

Another bug is that the CI script in mmtk-core does not look at Cargo.toml in mmtk-ruby for the chosen revision of repo ruby, so it automatically checked out the latest revision of the ruby repo which I am just about to run CI scripts against (which failed, too). I have made a force push to the ruby repo to roll back to the previous head. I'll fix the CI script for mmtk-core later.

@wks
Copy link
Collaborator

wks commented Dec 13, 2023

OK. Now all tests passed. @k-sareen You can hit the merge button if you don't have further changes.

@k-sareen
Copy link
Collaborator Author

You should also set RUST_BACKTRACE=1 in the CI if you haven't already.

@k-sareen k-sareen added this pull request to the merge queue Dec 13, 2023
Merged via the queue into mmtk:master with commit e55f872 Dec 13, 2023
26 checks passed
@k-sareen k-sareen deleted the fix/mutator-flush-destroy branch December 13, 2023 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants