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

YJIT: Fix BorrowMutError on GC.compact #7176

Merged
merged 1 commit into from Jan 30, 2023
Merged

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Jan 23, 2023

Follows up #7151 and #7164.

#7164 downgraded mut borrow of a block to (non-mut) borrow for target.get_blockid(), but target.set_iseq() needed to mut borrow it. So a block shouldn't be borrowed at all when calling target.set_iseq.

We could achieve this by cloning a vector of rc and then dropping the block. This addresses the following error:

thread '<unnamed>' panicked at 'already borrowed: BorrowMutError', src/core.rs:509:16
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/panicking.rs:575:5
     1: core::panicking::panic_fmt
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/panicking.rs:65:14
     2: core::result::unwrap_failed
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/result.rs:1791:5
     3: core::result::Result<T,E>::expect
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/result.rs:1070:23
     4: core::cell::RefCell<T>::borrow_mut
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/cell.rs:958:9
     5: yjit::core::BlockRef::borrow_mut
               at /home/runner/work/ruby/ruby/src/yjit/src/core.rs:509:9
     6: yjit::core::BranchTarget::set_iseq
               at /home/runner/work/ruby/ruby/src/yjit/src/core.rs:379:46
     7: rb_yjit_iseq_update_references
               at /home/runner/work/ruby/ruby/src/yjit/src/core.rs:765:21
     8: rb_iseq_mark_and_update
               at ./../src/iseq.c:363:13
     9: gc_ref_update
               at ./../src/gc.c:10765:17
    10: gc_update_references
               at ./../src/gc.c:10799:13
    11: gc_compact_finish
               at ./../src/gc.c:5536:5
    12: gc_sweep_compact
               at ./../src/gc.c:8672:5
    13: gc_sweep
               at ./../src/gc.c:6196:9
    14: gc_marks
               at ./../src/gc.c:8735:9
    15: gc_start
               at ./../src/gc.c:9566:9
    16: garbage_collect
               at ./../src/gc.c:[94](https://github.com/ruby/ruby/actions/runs/3979122841/jobs/6821344008#step:19:99)47:15
    17: gc_start_internal
               at ./../src/gc.c:[98](https://github.com/ruby/ruby/actions/runs/3979122841/jobs/6821344008#step:19:103)67:5
    18: gc_verify_compaction_references
               at ./../src/gc.c:10[99](https://github.com/ruby/ruby/actions/runs/3979122841/jobs/6821344008#step:19:104)2:5
    19: <unknown>

https://github.com/ruby/ruby/actions/runs/3979122841/jobs/6821344008

@k0kubun k0kubun changed the title YJIT: Fix BorrowMutError YJIT: Fix BorrowMutError on GC.compact Jan 23, 2023
@k0kubun k0kubun marked this pull request as ready for review January 23, 2023 04:21
@matzbot matzbot requested a review from a team January 23, 2023 04:21
@maximecb
Copy link
Contributor

Will merge the fix, but would you also have a regression test that triggers this failure?

@k0kubun
Copy link
Member Author

k0kubun commented Jan 30, 2023

Will merge the fix, but would you also have a regression test that triggers this failure?

worked on that at ba482d0. I confirmed that this fails on #7168 (I need that change to avoid splitting a block on opt_plus)

@k0kubun k0kubun requested a review from maximecb January 30, 2023 18:39
Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

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

Thank you Kokubun! :)

@k0kubun k0kubun merged commit 2e0f3b5 into ruby:master Jan 30, 2023
@k0kubun k0kubun deleted the yjit-fix-borrow branch January 30, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants