From 9d18661e1de053a9fecae7f4ab4ed41300537cec Mon Sep 17 00:00:00 2001
From: Noah Gibbs <noah.gibbs@shopify.com>
Date: Wed, 1 Jun 2022 15:22:08 +0100
Subject: [PATCH] Revert incorrect string-guard optimisation. (#5969)

Also add jhawthorn's test to for this bug.
Fix String#to_s invalidation test
---
 bootstraptest/test_yjit.rb | 32 +++++++++++++++++++++++++++++++-
 yjit/src/codegen.rs        | 26 --------------------------
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb
index 6d0ba93fc8..503af1ab80 100644
--- a/bootstraptest/test_yjit.rb
+++ b/bootstraptest/test_yjit.rb
@@ -1339,12 +1339,42 @@ assert_equal 'foo123', %q{
 
 # test that invalidation of String#to_s doesn't crash
 assert_equal 'meh', %q{
+  def inval_method
+    "".to_s
+  end
+
+  inval_method
+
   class String
     def to_s
       "meh"
     end
   end
-  "".to_s
+
+  inval_method
+}
+
+# test that overriding to_s on a String subclass isn't over-optimised
+assert_equal 'meh', %q{
+  class MyString < String
+    def to_s
+      "meh"
+    end
+  end
+
+  def test_to_s(obj)
+    obj.to_s
+  end
+
+  OBJ = MyString.new
+
+  # Should return 'meh' both times
+  test_to_s("")
+  test_to_s("")
+
+  # Can return '' if YJIT optimises String#to_s too aggressively
+  test_to_s(OBJ)
+  test_to_s(OBJ)
 }
 
 # test string interpolation with overridden to_s
diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs
index 782d87a144..87639b9d88 100644
--- a/yjit/src/codegen.rs
+++ b/yjit/src/codegen.rs
@@ -3421,32 +3421,6 @@ fn jit_guard_known_klass(
             jit_chain_guard(JCC_JNE, jit, ctx, cb, ocb, max_chain_depth, side_exit);
             ctx.upgrade_opnd_type(insn_opnd, Type::Flonum);
         }
-    } else if unsafe { known_klass == rb_cString } && sample_instance.string_p() {
-        assert!(!val_type.is_imm());
-        if val_type != Type::String {
-            assert!(val_type.is_unknown());
-
-            // Need the check for immediate, because trying to look up the klass field of an immediate will segfault
-            if !val_type.is_heap() {
-                add_comment(cb, "guard not immediate (for string)");
-                assert!(Qfalse.as_i32() < Qnil.as_i32());
-                test(cb, REG0, imm_opnd(RUBY_IMMEDIATE_MASK as i64));
-                jit_chain_guard(JCC_JNZ, jit, ctx, cb, ocb, max_chain_depth, side_exit);
-                cmp(cb, REG0, imm_opnd(Qnil.into()));
-                jit_chain_guard(JCC_JBE, jit, ctx, cb, ocb, max_chain_depth, side_exit);
-            }
-
-            add_comment(cb, "guard object is string");
-            let klass_opnd = mem_opnd(64, REG0, RUBY_OFFSET_RBASIC_KLASS);
-            mov(cb, REG1, uimm_opnd(unsafe { rb_cString }.into()));
-            cmp(cb, klass_opnd, REG1);
-            jit_chain_guard(JCC_JNE, jit, ctx, cb, ocb, max_chain_depth, side_exit);
-
-            // Upgrading here causes an error with invalidation writing past end of block
-            ctx.upgrade_opnd_type(insn_opnd, Type::String);
-        } else {
-            add_comment(cb, "skip guard - known to be a string");
-        }
     } else if unsafe {
         FL_TEST(known_klass, VALUE(RUBY_FL_SINGLETON)) != VALUE(0)
             && sample_instance == rb_attr_get(known_klass, id__attached__ as ID)