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

Fix missing strbuf #92

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

eileencodes
Copy link
Collaborator

@eileencodes eileencodes commented Sep 11, 2024

In CI (and locally) we were seeing a failed assertion for the test_gem_remote_fetcher test. After much debugging it was clear that for this test the strbuf was missing on shared so when orig moved, shared didn't follow`. This caused the offset to be very off.

Assertion Failed: string.c:1805:str_new_frozen_buffer:ofs >= 0
ruby 3.4.0dev (2024-08-09T19:42:38Z reproduction-for-s.. a5b29b5277) +MMTk(Immix) [arm64-darwin23]

With a GC_ASSERT we tracked down a missing rb_mmtk_str_set_strbuf in rb_str_tmp_frozen_no_embed_acquire.

Fixes: #85

In CI (and locally) we were seeing a failed assertion for the
`test_gem_remote_fetcher` test. After much debugging it was clear that
for this test the strbuf was missing on `shared` so when `orig` moved,
`shared` didn't follow`. This caused the offset to be very off.

```
Assertion Failed: string.c:1805:str_new_frozen_buffer:ofs >= 0
ruby 3.4.0dev (2024-08-09T19:42:38Z reproduction-for-s.. a5b29b5277) +MMTk(Immix) [arm64-darwin23]
```

With a `GC_ASSERT` we tracked down a missing `rb_mmtk_str_set_strbuf` in
`rb_str_tmp_frozen_no_embed_acquire`.

Fixes: #85
@wks wks merged commit 68d95d7 into dev/mmtk-overrides-default Sep 12, 2024
207 of 220 checks passed
@wks
Copy link

wks commented Sep 12, 2024

I merged this PR because it does fix one missing rb_mmtk_str_set_strbuf operation. However, as I described in #85 (comment), there are STR_NOFREE strings that will not have strbuf. This PR is OK because in that case the strbuf will just be NULL, and will be copied, too. I'll track the STR_NOFREE strings problem in a new issue: #93

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.

Ruby assertion causing panic with shared strings
2 participants