From 4f00d98b327e3aa23564aa765488d15bc60c9e79 Mon Sep 17 00:00:00 2001 From: Jean byroot Boussier Date: Tue, 4 Jun 2024 22:21:58 +0200 Subject: [PATCH] [3.3 backport] Do not emit shape transition warnings when YJIT is compiling (#10911) Do not emit shape transition warnings when YJIT is compiling [Bug #20522] If `Warning.warn` is redefined in Ruby, emitting a warning would invoke Ruby code, which can't safely be done when YJIT is compiling. Co-authored-by: Jean Boussier Co-authored-by: Takashi Kokubun --- bootstraptest/test_yjit.rb | 43 ++++++++++++++++++++++++++++++++++ shape.c | 18 +++++++++++--- shape.h | 1 + yjit/bindgen/src/main.rs | 2 +- yjit/src/codegen.rs | 2 +- yjit/src/cruby_bindings.inc.rs | 6 ++++- 6 files changed, 66 insertions(+), 6 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 2865c0ec09..734c0428a7 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -4465,3 +4465,46 @@ assert_normal_exit %q{ test_body(array) end } + +# compiling code shouldn't emit warnings as it may call into more Ruby code +assert_equal 'ok', <<~'RUBY' unless rjit_enabled? # Not yet working on RJIT + # [Bug #20522] + $VERBOSE = true + Warning[:performance] = true + + module StrictWarnings + def warn(msg, category: nil, **) + raise warn + end + end + Warning.singleton_class.prepend(StrictWarnings) + + class A + def compiled_method(is_private) + @some_ivar = is_private + end + end + + shape_max_variations = 8 + if defined?(RubyVM::Shape::SHAPE_MAX_VARIATIONS) && RubyVM::Shape::SHAPE_MAX_VARIATIONS != shape_max_variations + raise "Expected SHAPE_MAX_VARIATIONS to be 8, got: #{RubyVM::Shape::SHAPE_MAX_VARIATIONS}" + end + + 100.times do |i| + klass = Class.new(A) + (shape_max_variations - 1).times do |j| + obj = klass.new + obj.instance_variable_set("@base_#{i}", 42) + obj.instance_variable_set("@ivar_#{j}", 42) + end + obj = klass.new + obj.instance_variable_set("@base_#{i}", 42) + begin + obj.compiled_method(true) + rescue + # expected + end + end + + :ok +RUBY diff --git a/shape.c b/shape.c index 8d8314db33..d4e2c0f2bb 100644 --- a/shape.c +++ b/shape.c @@ -695,8 +695,8 @@ rb_shape_get_next_iv_shape(rb_shape_t* shape, ID id) return get_next_shape_internal(shape, id, SHAPE_IVAR, &dont_care, true); } -rb_shape_t * -rb_shape_get_next(rb_shape_t *shape, VALUE obj, ID id) +static inline rb_shape_t * +shape_get_next(rb_shape_t *shape, VALUE obj, ID id, bool emit_warnings) { RUBY_ASSERT(!is_instance_id(id) || RTEST(rb_sym2str(ID2SYM(id)))); if (UNLIKELY(shape->type == SHAPE_OBJ_TOO_COMPLEX)) { @@ -729,7 +729,7 @@ rb_shape_get_next(rb_shape_t *shape, VALUE obj, ID id) if (variation_created) { RCLASS_EXT(klass)->variation_count++; - if (rb_warning_category_enabled_p(RB_WARN_CATEGORY_PERFORMANCE)) { + if (emit_warnings && rb_warning_category_enabled_p(RB_WARN_CATEGORY_PERFORMANCE)) { if (RCLASS_EXT(klass)->variation_count >= SHAPE_MAX_VARIATIONS) { rb_category_warn( RB_WARN_CATEGORY_PERFORMANCE, @@ -746,6 +746,18 @@ rb_shape_get_next(rb_shape_t *shape, VALUE obj, ID id) return new_shape; } +rb_shape_t * +rb_shape_get_next(rb_shape_t *shape, VALUE obj, ID id) +{ + return shape_get_next(shape, obj, id, true); +} + +rb_shape_t * +rb_shape_get_next_no_warnings(rb_shape_t *shape, VALUE obj, ID id) +{ + return shape_get_next(shape, obj, id, false); +} + // Same as rb_shape_get_iv_index, but uses a provided valid shape id and index // to return a result faster if branches of the shape tree are closely related. bool diff --git a/shape.h b/shape.h index 4fc46b4525..5fee17ebd9 100644 --- a/shape.h +++ b/shape.h @@ -163,6 +163,7 @@ int rb_shape_frozen_shape_p(rb_shape_t* shape); rb_shape_t* rb_shape_transition_shape_frozen(VALUE obj); bool rb_shape_transition_shape_remove_ivar(VALUE obj, ID id, rb_shape_t *shape, VALUE * removed); rb_shape_t* rb_shape_get_next(rb_shape_t* shape, VALUE obj, ID id); +rb_shape_t* rb_shape_get_next_no_warnings(rb_shape_t* shape, VALUE obj, ID id); rb_shape_t * rb_shape_rebuild_shape(rb_shape_t * initial_shape, rb_shape_t * dest_shape); diff --git a/yjit/bindgen/src/main.rs b/yjit/bindgen/src/main.rs index 83bee3b4d6..391f15e20e 100644 --- a/yjit/bindgen/src/main.rs +++ b/yjit/bindgen/src/main.rs @@ -99,7 +99,7 @@ fn main() { .allowlist_function("rb_shape_get_shape_by_id") .allowlist_function("rb_shape_id_offset") .allowlist_function("rb_shape_get_iv_index") - .allowlist_function("rb_shape_get_next") + .allowlist_function("rb_shape_get_next_no_warnings") .allowlist_function("rb_shape_id") .allowlist_function("rb_shape_obj_too_complex") .allowlist_var("SHAPE_ID_NUM_BITS") diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index ca987eaf78..c672a40d1e 100644 --- a/yjit/src/codegen.rs +++ b/yjit/src/codegen.rs @@ -2484,7 +2484,7 @@ fn gen_setinstancevariable( // The current shape doesn't contain this iv, we need to transition to another shape. let new_shape = if !shape_too_complex && receiver_t_object && ivar_index.is_none() { let current_shape = comptime_receiver.shape_of(); - let next_shape = unsafe { rb_shape_get_next(current_shape, comptime_receiver, ivar_name) }; + let next_shape = unsafe { rb_shape_get_next_no_warnings(current_shape, comptime_receiver, ivar_name) }; let next_shape_id = unsafe { rb_shape_id(next_shape) }; // If the VM ran out of shapes, or this class generated too many leaf, diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs index 7a54b177f1..cf33f2cdb6 100644 --- a/yjit/src/cruby_bindings.inc.rs +++ b/yjit/src/cruby_bindings.inc.rs @@ -981,7 +981,11 @@ extern "C" { pub fn rb_shape_get_shape_id(obj: VALUE) -> shape_id_t; pub fn rb_shape_get_iv_index(shape: *mut rb_shape_t, id: ID, value: *mut attr_index_t) -> bool; pub fn rb_shape_obj_too_complex(obj: VALUE) -> bool; - pub fn rb_shape_get_next(shape: *mut rb_shape_t, obj: VALUE, id: ID) -> *mut rb_shape_t; + pub fn rb_shape_get_next_no_warnings( + shape: *mut rb_shape_t, + obj: VALUE, + id: ID, + ) -> *mut rb_shape_t; pub fn rb_shape_id(shape: *mut rb_shape_t) -> shape_id_t; pub fn rb_gvar_get(arg1: ID) -> VALUE; pub fn rb_gvar_set(arg1: ID, arg2: VALUE) -> VALUE;