YJIT: Fix false object collection when setting ivar

[XrXr]

Previously, setinstancevariable could generate code that calls
rb_ensure_iv_list_size() without first updating cfp->sp. This means
in the event that a GC start from within said routine the top few
objects would not be marked, causing them to be falsly collected.

Call jit_prepare_routine_call() first.

[Bug #19601]

[k0kubun]

Not for this PR, but with this amount of jit_prepare_routine_call, we might be calling jit_save_pc multiple times for a single instruction. I guess we should have a PC offset-like concept in JITState and skip jit_save_pc when it's been already written.

[XrXr]

Uh oh, the regression test is so good that it crashes RJIT even after my fix attempt at f889a96. Will need to debug that further, or skip it for now for RJIT?

[k0kubun]

No worries. Please skip it for RJIT for now.

[XrXr]

Not for this PR, but with this amount of jit_prepare_routine_call, we might be calling jit_save_pc multiple times for a single instruction. I guess we should have a PC offset-like concept in JITState and skip jit_save_pc when it's been already written.

Yeah. Kind of related to that, given how often we've had this class of bugs, I think ccall should do jit_prepare_routine_call by default. Should be a much safer default, since calling without prepping requires carefully checking the called routine. It's not trivial to change the default right now because the assembler doesn't have enough info to schedule the cfp updates correctly, but maybe in the future. When it can do that, it can elide duplicate PC updates, too.

[k0kubun]

If it doesn't cause a significant performance drawback, that'd be nice.

I think ccall should do jit_prepare_routine_call by default.

asm.ccall currently doesn't take JITState, which is required for jit_prepare_routine_call. Instead of adding a JITState parameter to it, another approach could be to just assert that sp_offset is 0 and local types have been cleared, which is now possible thanks to Assembler owning Context. It could still leave a stale PC, but it would still catch a missing jit_prepare_routine_call in most cases.

[maximecb]

asm.ccall currently doesn't take JITState, which is required for jit_prepare_routine_call. Instead of adding a JITState parameter to it, another approach could be to just assert that sp_offset is 0 and local types have been cleared, which is now possible thanks to Assembler owning Context. It could still leave a stale PC, but it would still catch a missing jit_prepare_routine_call in most cases.

Seems like a decent solution.

Another option could be to remove the ccall method from assembler, and make it a method in JITState instead.