From 5d505d9bcd1af93140f3abaaec7279e337a3aa77 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Tue, 23 Jan 2024 12:35:34 -0500 Subject: [PATCH] YJIT: Fix ruby2_keywords splat+rest and drop bogus checks YJIT didn't guard for ruby2_keywords hash in case of splat calls that land in methods with a rest parameter, creating incorrect results. The compile-time checks didn't correspond to any actual effects of ruby2_keywords, so it was masking this bug and YJIT was needlessly refusing to compile some code. About 16% of fallback reasons in `lobsters` was due to the ISeq check. We already handle the tagging part with exit_if_supplying_kw_and_has_no_kw() and should now have a dynamic guard for all splat cases. Note for backporting: You also need 7f51959ff1. [Bug #20195] --- bootstraptest/test_yjit.rb | 16 ++++++++++++++ yjit.c | 13 ++++++++++++ yjit/bindgen/src/main.rs | 1 + yjit/src/codegen.rs | 38 +++++++++------------------------- yjit/src/cruby.rs | 1 - yjit/src/cruby_bindings.inc.rs | 1 + yjit/src/stats.rs | 2 -- 7 files changed, 41 insertions(+), 31 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 8ae3399dc20058..6832a0d76c513f 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -4443,3 +4443,19 @@ def entry def defined_yield = defined?(yield) [defined_yield, defined_yield {}] } + +# splat with ruby2_keywords into rest parameter +assert_equal '[[{:a=>1}], {}]', %q{ + ruby2_keywords def foo(*args) = args + + def bar(*args, **kw) = [args, kw] + + def pass_bar(*args) = bar(*args) + + def body + args = foo(a: 1) + pass_bar(*args) + end + + body +} diff --git a/yjit.c b/yjit.c index d5071cb952858e..cf6d81092fae33 100644 --- a/yjit.c +++ b/yjit.c @@ -871,6 +871,19 @@ rb_yjit_fix_mod_fix(VALUE recv, VALUE obj) return rb_fix_mod_fix(recv, obj); } +// Return non-zero when `obj` is an array and its last item is a +// `ruby2_keywords` hash. We don't support this kind of splat. +size_t +rb_yjit_ruby2_keywords_splat_p(VALUE obj) +{ + if (!RB_TYPE_P(obj, T_ARRAY)) return 0; + long len = RARRAY_LEN(obj); + if (len == 0) return 0; + VALUE last = RARRAY_AREF(obj, len - 1); + if (!RB_TYPE_P(last, T_HASH)) return 0; + return FL_TEST_RAW(last, RHASH_PASS_AS_KEYWORDS); +} + // Print the Ruby source location of some ISEQ for debugging purposes void rb_yjit_dump_iseq_loc(const rb_iseq_t *iseq, uint32_t insn_idx) diff --git a/yjit/bindgen/src/main.rs b/yjit/bindgen/src/main.rs index e249181d515275..742885de3b53ff 100644 --- a/yjit/bindgen/src/main.rs +++ b/yjit/bindgen/src/main.rs @@ -426,6 +426,7 @@ fn main() { .allowlist_function("rb_yarv_str_eql_internal") .allowlist_function("rb_str_neq_internal") .allowlist_function("rb_yarv_ary_entry_internal") + .allowlist_function("rb_yjit_ruby2_keywords_splat_p") .allowlist_function("rb_yjit_fix_div_fix") .allowlist_function("rb_yjit_fix_mod_fix") .allowlist_function("rb_FL_TEST") diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index 8847affd2be4b5..344150b025623a 100644 --- a/yjit/src/codegen.rs +++ b/yjit/src/codegen.rs @@ -5555,18 +5555,6 @@ fn gen_send_cfunc( return None; } - // In order to handle backwards compatibility between ruby 3 and 2 - // ruby2_keywords was introduced. It is called only on methods - // with splat and changes they way they handle them. - // We are just going to not compile these. - // https://docs.ruby-lang.org/en/3.2/Module.html#method-i-ruby2_keywords - if unsafe { - get_iseq_flags_ruby2_keywords(jit.iseq) && flags & VM_CALL_ARGS_SPLAT != 0 - } { - gen_counter_incr(asm, Counter::send_args_splat_cfunc_ruby2_keywords); - return None; - } - let kw_arg = unsafe { vm_ci_kwarg(ci) }; let kw_arg_num = if kw_arg.is_null() { 0 @@ -6147,7 +6135,6 @@ fn gen_send_iseq( exit_if_has_post(asm, iseq)?; exit_if_has_kwrest(asm, iseq)?; exit_if_kw_splat(asm, flags)?; - exit_if_splat_and_ruby2_keywords(asm, jit, flags)?; exit_if_has_rest_and_captured(asm, iseq_has_rest, captured_opnd)?; exit_if_has_rest_and_supplying_kws(asm, iseq_has_rest, iseq, supplying_kws)?; exit_if_supplying_kw_and_has_no_kw(asm, supplying_kws, iseq)?; @@ -6426,6 +6413,8 @@ fn gen_send_iseq( asm.jbe(Target::side_exit(Counter::guard_send_se_cf_overflow)); if iseq_has_rest && flags & VM_CALL_ARGS_SPLAT != 0 { + let splat_pos = i32::from(block_arg) + kw_arg_num; + // Insert length guard for a call to copy_splat_args_for_rest_callee() // that will come later. We will have made changes to // the stack by spilling or handling __send__ shifting @@ -6439,12 +6428,19 @@ fn gen_send_iseq( if take_count > 0 { asm_comment!(asm, "guard splat_array_length >= {take_count}"); - let splat_array = asm.stack_opnd(i32::from(block_arg) + kw_arg_num); + let splat_array = asm.stack_opnd(splat_pos); let array_len_opnd = get_array_len(asm, splat_array); asm.cmp(array_len_opnd, take_count.into()); asm.jl(Target::side_exit(Counter::guard_send_iseq_has_rest_and_splat_too_few)); } } + + // All splats need to guard for ruby2_keywords hash. Check with a function call when + // splatting into a rest param since the index for the last item in the array is dynamic. + asm_comment!(asm, "guard no ruby2_keywords hash in splat"); + let bad_splat = asm.ccall(rb_yjit_ruby2_keywords_splat_p as _, vec![asm.stack_opnd(splat_pos)]); + asm.cmp(bad_splat, 0.into()); + asm.jnz(Target::side_exit(Counter::guard_send_splatarray_last_ruby_2_keywords)); } match block_arg_type { @@ -7008,20 +7004,6 @@ fn exit_if_kw_splat(asm: &mut Assembler, flags: u32) -> Option<()> { exit_if(asm, flags & VM_CALL_KW_SPLAT != 0, Counter::send_iseq_kw_splat) } -#[must_use] -fn exit_if_splat_and_ruby2_keywords(asm: &mut Assembler, jit: &mut JITState, flags: u32) -> Option<()> { - // In order to handle backwards compatibility between ruby 3 and 2 - // ruby2_keywords was introduced. It is called only on methods - // with splat and changes they way they handle them. - // We are just going to not compile these. - // https://www.rubydoc.info/stdlib/core/Proc:ruby2_keywords - exit_if( - asm, - unsafe { get_iseq_flags_ruby2_keywords(jit.iseq) } && flags & VM_CALL_ARGS_SPLAT != 0, - Counter::send_iseq_ruby2_keywords, - ) -} - #[must_use] fn exit_if_has_rest_and_captured(asm: &mut Assembler, iseq_has_rest: bool, captured_opnd: Option) -> Option<()> { exit_if(asm, iseq_has_rest && captured_opnd.is_some(), Counter::send_iseq_has_rest_and_captured) diff --git a/yjit/src/cruby.rs b/yjit/src/cruby.rs index ac0bdf68856f1d..9f289e93dda4ae 100644 --- a/yjit/src/cruby.rs +++ b/yjit/src/cruby.rs @@ -165,7 +165,6 @@ pub use rb_get_iseq_flags_has_lead as get_iseq_flags_has_lead; pub use rb_get_iseq_flags_has_opt as get_iseq_flags_has_opt; pub use rb_get_iseq_flags_has_kw as get_iseq_flags_has_kw; pub use rb_get_iseq_flags_has_rest as get_iseq_flags_has_rest; -pub use rb_get_iseq_flags_ruby2_keywords as get_iseq_flags_ruby2_keywords; pub use rb_get_iseq_flags_has_post as get_iseq_flags_has_post; pub use rb_get_iseq_flags_has_kwrest as get_iseq_flags_has_kwrest; pub use rb_get_iseq_flags_has_block as get_iseq_flags_has_block; diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs index e6dba2b59df2a5..7b0f5678977897 100644 --- a/yjit/src/cruby_bindings.inc.rs +++ b/yjit/src/cruby_bindings.inc.rs @@ -1140,6 +1140,7 @@ extern "C" { pub fn rb_yjit_rb_ary_subseq_length(ary: VALUE, beg: ::std::os::raw::c_long) -> VALUE; pub fn rb_yjit_fix_div_fix(recv: VALUE, obj: VALUE) -> VALUE; pub fn rb_yjit_fix_mod_fix(recv: VALUE, obj: VALUE) -> VALUE; + pub fn rb_yjit_ruby2_keywords_splat_p(obj: VALUE) -> usize; pub fn rb_yjit_dump_iseq_loc(iseq: *const rb_iseq_t, insn_idx: u32); pub fn rb_FL_TEST(obj: VALUE, flags: VALUE) -> VALUE; pub fn rb_FL_TEST_RAW(obj: VALUE, flags: VALUE) -> VALUE; diff --git a/yjit/src/stats.rs b/yjit/src/stats.rs index d8dd4b93893a85..98e33f23757c94 100644 --- a/yjit/src/stats.rs +++ b/yjit/src/stats.rs @@ -378,10 +378,8 @@ make_counters! { send_args_splat_opt_call, send_args_splat_cfunc_var_args, send_args_splat_cfunc_zuper, - send_args_splat_cfunc_ruby2_keywords, send_iseq_splat_arity_error, send_splat_too_long, - send_iseq_ruby2_keywords, send_send_not_imm, send_send_wrong_args, send_send_null_mid,