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 staying in invalidated code after proc calls #6711

Merged
merged 1 commit into from Nov 11, 2022

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Nov 10, 2022

Previously, there is no instruction boundary patch point after the call to a non-leaf C function we generate for
OPTIMIZED_METHOD_TYPE_CALL. This meant that if code GC is triggered while inside the C function, we would keep running invalidated code when we return from the C function. This had the effect of running stale branch stubs, jumping to bad code, etc.

Use jit_prepare_routine_call() to make sure we exit from the invalidated region as soon as possible after the C call in case of invalidation.

Previously, there is no instruction boundary patch point after
the call to a non-leaf C function we generate for
OPTIMIZED_METHOD_TYPE_CALL. This meant that if code GC is triggered
while inside the C function, we would keep running invalidated code when
we return from the C function. This had the effect of running
stale branch stubs, jumping to bad code, etc.

Use jit_prepare_routine_call() to make sure we exit from the invalidated
region as soon as possible after the C call in case of invalidation.
@matzbot matzbot requested a review from a team November 10, 2022 22:36
@k0kubun
Copy link
Member

k0kubun commented Nov 10, 2022

This had the effect of running stale branch stubs, jumping to bad code, etc.

Do branch stubs owned by an on-stack ISEQ get freed? I thought that'd be marked as pages_in_use. "jumping to bad code" makes sense though, because it seems to happen when a jump to another iseq by gen_send_iseq is used and that ISEQ is not on stack and gets freed.

@XrXr
Copy link
Member Author

XrXr commented Nov 10, 2022

Do branch stubs owned by an on-stack ISEQ get freed?

The code for the stubs don't get freed for the reason you mentioned. The way I saw it cause a crash is when the code for the stub staddles two pages, and when it's run after code GC the branch size "grows" because the second page for code is different after code GC (we changed freed_pages).

As for the struct Branch that is passed to branch_stub_hit, that isn't freed either, but I think that might be a memory leak. When we invalidate all the code, we should free their associated metadata. I think it's kept alive by this ref count increment that don't have a corresponding decrement:

ruby/yjit/src/core.rs

Lines 1893 to 1896 in 4c55409

// Get a raw pointer to the branch while keeping the reference count alive
// Here clone increments the strong count by 1
// This means the branch stub owns its own reference to the branch
let branch_ptr: *const RefCell<Branch> = BranchRef::into_raw(branchref.clone());

In this case though, them not being freed helped with debugging because I was able to stamp each branch with a GC generation number 😅

@jimmyhmiller
Copy link
Contributor

Good catch. I need to study this a bit closer to understand exactly how all of this works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants