YJIT: Fix String#setbyte crashing for converted arguments #10575
Conversation
The added test doesn't raise an exception, does it? In the original PR #10080, the intention was to leave the un-popped frame during the C call and let JIT code resurrect the correct frame using the CFP register. But I guess A potentially more efficient way to fix this might be to just pop a frame after a lazy frame push. I used that trick in #10165 (see |
|
Does this mean we need to be more careful about lazy push frame usage?
It would be nice to have a more efficient solution. How would this work? Would only the C code have to care about this? |
Yes. YJIT ends up looking it at in a branch stub 😅
Yes. Basically we need to pop the lazy frame after the function finishes making a call so there is no lingering frame whether the callee raises or not. I'll try poking at that later today. |
Previously, passing objects that respond to #to_int to `String#setbyte` resulted in a crash when compiled by YJIT. This was due to the lazily pushed frame from rb_yjit_lazy_push_frame() lingering and not being popped by an exception as expected. The fix is to ensure that `ec->cfp` is restored to before the lazy frame push in case the method call for conversion succeeds. Right now, this is only for conversion to integers. Found running `ruby/spec`.
|
Okay, restoring the CFP for the non-rising path seems to work (at least locally). Moving forward, we just need to be careful when lazily pushing frames in the C side. |
object.c
Outdated
| @@ -3236,15 +3236,22 @@ ALWAYS_INLINE(static VALUE rb_to_integer_with_id_exception(VALUE val, const char | |||
| static inline VALUE | |||
| rb_to_integer_with_id_exception(VALUE val, const char *method, ID mid, int raise) | |||
| { | |||
| // when !raise, we need to pop the lazily pushed frame. | |||
There was a problem hiding this comment.
It seems to be popped on L3254 when raise too. But is the case unreachable because try_to_int raises?
There was a problem hiding this comment.
I edited the comment to clarify that it's not really related to the parameter, but whether an exception is raised (we need to pop if the function returns normally)
We just need to make sure `ec->cfp` is always preserved and this can convert without rising when `raise` is true.
Previously, passing objects that respond to #to_int to
String#setbyteresulted in a crash when compiled by YJIT. This was due to the lazily
pushed frame from rb_yjit_lazy_push_frame() lingering and not being
popped by an exception as expected.
The fix is to ensure that
ec->cfpis restored to before the lazy framepush in case the method call for conversion succeeds. Right now, this is
only for conversion to integers.
Found running
ruby/spec.