https://git.ruby-lang.org/ruby.git/commit/?id=b74d6563a6

From b74d6563a665f225f182c4921db68852bbb7e1f1 Mon Sep 17 00:00:00 2001
From: Alan Wu <XrXr@u...>
Date: Mon, 18 Oct 2021 17:01:40 -0400
Subject: Extract yjit_force_iv_index and make it work when object is frozen

In an effort to simplify the logic YJIT generates for accessing instance
variable, YJIT ensures that a given name-to-index mapping exists at
compile time. In the case that the mapping doesn't exist, it was created
by using rb_ivar_set() with Qundef on the sample object we see at
compile time. This hack isn't fine if the sample object happens to be
frozen, in which case YJIT would raise a FrozenError unexpectedly.

To deal with this, make a new function that only reserves the mapping
but doesn't touch the object. This is rb_obj_ensure_iv_index_mapping().
This new function superceeds the functionality of rb_iv_index_tbl_lookup()
so it was removed.

Reported by and includes a test case from John Hawthorn <john@h...>

Fixes: GH-282
---
 bootstraptest/test_yjit.rb | 15 +++++++++++++++
 internal/variable.h        |  1 +
 variable.c                 | 33 ++++++++++++++++++++++++++++++---
 vm_insnhelper.c            |  6 ------
 yjit_codegen.c             | 28 +++++-----------------------
 5 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb
index a190d8ffb1..765580a030 100644
--- a/bootstraptest/test_yjit.rb
+++ b/bootstraptest/test_yjit.rb
@@ -2160,3 +2160,18 @@ assert_equal '[2]', %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_yjit.rb#L2160
 
   5.times.map { default_expression(value: 2) }.uniq
 }
+
+# attr_reader on frozen object
+assert_equal 'false', %q{
+  class Foo
+    attr_reader :exception
+
+    def failed?
+      !exception.nil?
+    end
+  end
+
+  foo = Foo.new.freeze
+  foo.failed?
+  foo.failed?
+}
diff --git a/internal/variable.h b/internal/variable.h
index c527bdbd1f..4b67bef907 100644
--- a/internal/variable.h
+++ b/internal/variable.h
@@ -35,6 +35,7 @@ void rb_gvar_ractor_local(const char *name); https://github.com/ruby/ruby/blob/trunk/internal/variable.h#L35
 static inline bool ROBJ_TRANSIENT_P(VALUE obj);
 static inline void ROBJ_TRANSIENT_SET(VALUE obj);
 static inline void ROBJ_TRANSIENT_UNSET(VALUE obj);
+uint32_t rb_obj_ensure_iv_index_mapping(VALUE obj, ID id);
 
 RUBY_SYMBOL_EXPORT_BEGIN
 /* variable.c (export) */
diff --git a/variable.c b/variable.c
index 5712ccf0ba..6cfc3f68f2 100644
--- a/variable.c
+++ b/variable.c
@@ -1446,12 +1446,13 @@ rb_init_iv_list(VALUE obj) https://github.com/ruby/ruby/blob/trunk/variable.c#L1446
     init_iv_list(obj, len, newsize, index_tbl);
 }
 
-static VALUE
-obj_ivar_set(VALUE obj, ID id, VALUE val)
+// Retreive or create the id-to-index mapping for a given object and an
+// instance variable name.
+static struct ivar_update
+obj_ensure_iv_index_mapping(VALUE obj, ID id)
 {
     VALUE klass = rb_obj_class(obj);
     struct ivar_update ivup;
-    uint32_t len;
     ivup.iv_extended = 0;
     ivup.u.iv_index_tbl = iv_index_tbl_make(obj, klass);
 
@@ -1461,6 +1462,32 @@ obj_ivar_set(VALUE obj, ID id, VALUE val) https://github.com/ruby/ruby/blob/trunk/variable.c#L1462
     }
     RB_VM_LOCK_LEAVE();
 
+    return ivup;
+}
+
+// Return the instance variable index for a given name and T_OBJECT object. The
+// mapping between name and index lives on `rb_obj_class(obj)` and is created
+// if not already present.
+//
+// @note May raise when there are too many instance variables.
+// @note YJIT uses this function at compile time to simplify the work needed to
+//       access the variable at runtime.
+uint32_t
+rb_obj_ensure_iv_index_mapping(VALUE obj, ID id)
+{
+    RUBY_ASSERT(RB_TYPE_P(obj, T_OBJECT));
+    // This uint32_t cast shouldn't lose information as it's checked in
+    // iv_index_tbl_extend(). The index is stored as an uint32_t in
+    // struct rb_iv_index_tbl_entry.
+    return (uint32_t)obj_ensure_iv_index_mapping(obj, id).index;
+}
+
+static VALUE
+obj_ivar_set(VALUE obj, ID id, VALUE val)
+{
+    uint32_t len;
+    struct ivar_update ivup = obj_ensure_iv_index_mapping(obj, id);
+
     len = ROBJECT_NUMIV(obj);
     if (len <= ivup.index) {
         uint32_t newsize = iv_index_tbl_newsize(&ivup);
diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index 3b99f7c350..5d83d3dfd5 100644
--- a/vm_insnhelper.c
+++ b/vm_insnhelper.c
@@ -1105,12 +1105,6 @@ iv_index_tbl_lookup(struct st_table *iv_index_tbl, ID id, struct rb_iv_index_tbl https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1105
     return found ? true : false;
 }
 
-bool
-rb_iv_index_tbl_lookup(struct st_table *iv_index_tbl, ID id, struct rb_iv_index_tbl_entry **ent)
-{
-    return iv_index_tbl_lookup(iv_index_tbl, id, ent);
-}
-
 ALWAYS_INLINE(static void fill_ivar_cache(const rb_iseq_t *iseq, IVC ic, const struct rb_callcache *cc, int is_attr, struct rb_iv_index_tbl_entry *ent));
 
 static inline void
diff --git a/yjit_codegen.c b/yjit_codegen.c
index bca5f203ec..136d7d4dea 100644
--- a/yjit_codegen.c
+++ b/yjit_codegen.c
@@ -1478,33 +1478,12 @@ jit_chain_guard(enum jcc_kinds jcc, jitstate_t *jit, const ctx_t *ctx, uint8_t d https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L1478
     }
 }
 
-bool rb_iv_index_tbl_lookup(struct st_table *iv_index_tbl, ID id, struct rb_iv_index_tbl_entry **ent); // vm_insnhelper.c
-
 enum {
     GETIVAR_MAX_DEPTH = 10,       // up to 5 different classes, and embedded or not for each
     OPT_AREF_MAX_CHAIN_DEPTH = 2, // hashes and arrays
     SEND_MAX_DEPTH = 5,           // up to 5 different classes
 };
 
-static uint32_t
-yjit_force_iv_index(VALUE comptime_receiver, VALUE klass, ID name)
-{
-    ID id = name;
-    struct rb_iv_index_tbl_entry *ent;
-    struct st_table *iv_index_tbl = ROBJECT_IV_INDEX_TBL(comptime_receiver);
-
-    // Make sure there is a mapping for this ivar in the index table
-    if (!iv_index_tbl || !rb_iv_index_tbl_lookup(iv_index_tbl, id, &ent)) {
-        rb_ivar_set(comptime_receiver, id, Qundef);
-        iv_index_tbl = ROBJECT_IV_INDEX_TBL(comptime_receiver);
-        RUBY_ASSERT(iv_index_tbl);
-        // Redo the lookup
-        RUBY_ASSERT_ALWAYS(rb_iv_index_tbl_lookup(iv_index_tbl, id, &ent));
-    }
-
-    return ent->index;
-}
-
 VALUE rb_vm_set_ivar_idx(VALUE obj, uint32_t idx, VALUE val);
 
 // Codegen for setting an instance variable.
@@ -1523,7 +1502,7 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, VALUE recv, VALUE klass, ID ivar_name) https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L1502
     x86opnd_t val_opnd = ctx_stack_pop(ctx, 1);
     x86opnd_t recv_opnd = ctx_stack_pop(ctx, 1);
 
-    uint32_t ivar_index = yjit_force_iv_index(recv, klass, ivar_name);
+    uint32_t ivar_index = rb_obj_ensure_iv_index_mapping(recv, ivar_name);
 
     // Call rb_vm_set_ivar_idx with the receiver, the index of the ivar, and the value
     mov(cb, C_ARG_REGS[0], recv_opnd);
@@ -1596,7 +1575,10 @@ gen_get_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L1575
     jit_chain_guard(JCC_JNE, jit, &starting_context, max_chain_depth, side_exit);
     */
 
-    uint32_t ivar_index = yjit_force_iv_index(comptime_receiver, CLASS_OF(comptime_receiver), ivar_name);
+    // FIXME: Mapping the index could fail when there is too many ivar names. If we're
+    // compiling for a branch stub that can cause the exception to be thrown from the
+    // wrong PC.
+    uint32_t ivar_index = rb_obj_ensure_iv_index_mapping(comptime_receiver, ivar_name);
 
     // Pop receiver if it's on the temp stack
     if (!reg0_opnd.is_self) {
-- 
cgit v1.2.1


--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/