[pull] master from ruby:master

.github/workflows/zjit-macos.yml

@@ -98,7 +98,7 @@ jobs:
           rustup install ${{ matrix.rust_version }} --profile minimal
           rustup default ${{ matrix.rust_version }}
 
-      - uses: taiki-e/install-action@9114bf4d891761788c546334fd37538eae1bf8b3 # v2.87.16
+      - uses: taiki-e/install-action@94c31af3204a9f15ab40b35ad084410b905bbc73 # v2.87.17
         with:
           tool: nextest@0.9
         if: ${{ matrix.test_task == 'zjit-check' }}

.github/workflows/zjit-ubuntu.yml

@@ -152,7 +152,7 @@ jobs:
           ruby-version: '3.1'
           bundler: none
 
-      - uses: taiki-e/install-action@9114bf4d891761788c546334fd37538eae1bf8b3 # v2.87.16
+      - uses: taiki-e/install-action@94c31af3204a9f15ab40b35ad084410b905bbc73 # v2.87.17
         with:
           tool: nextest@0.9
         if: ${{ matrix.test_task == 'zjit-check' }}

compile.c

@@ -358,8 +358,6 @@ static void iseq_add_setlocal(rb_iseq_t *iseq, LINK_ANCHOR *const seq, const NOD
 #define IS_INSN_ID(iobj, insn) (INSN_OF(iobj) == BIN(insn))
 #define IS_NEXT_INSN_ID(link, insn) \
     ((link)->next && IS_INSN((link)->next) && IS_INSN_ID((link)->next, insn))
-#define IS_NEXT_NEXT_INSN_ID(link, insn) \
-    ((link)->next && IS_NEXT_INSN_ID((link)->next, insn))
 
 static inline bool
 IS_INDEPENDENT_INSN(LINK_ELEMENT *link)
@@ -378,6 +376,10 @@ IS_INDEPENDENT_INSN(LINK_ELEMENT *link)
         type == BIN(duphash) ||
         type == BIN(getinstancevariable) ||
         type == BIN(getlocal) ||
+        type == BIN(getlocal_WC_0) ||
+        type == BIN(getlocal_WC_1) ||
+        type == BIN(putobject_INT2FIX_0_) ||
+        type == BIN(putobject_INT2FIX_1_) ||
         type == BIN(opt_getconstant_path)
     );
 }
@@ -4326,10 +4328,11 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
     *  putnil  / (or any other independent instruction)
     *  putself / (or any other independent instruction)
     */
-    if (IS_NEXT_NEXT_INSN_ID(&iobj->link, swap)) {
-        LINK_ELEMENT *first = &iobj->link;
-        LINK_ELEMENT *second = first->next;
+    if (IS_NEXT_INSN_ID(&iobj->link, swap)) {
+        LINK_ELEMENT *second = &iobj->link;
+        LINK_ELEMENT *first = second->prev;
         LINK_ELEMENT *swap = second->next;
+
         if (IS_INDEPENDENT_INSN(first) && IS_INDEPENDENT_INSN(second)) {
             ELEM_REMOVE(swap);
             ELEM_SWAP(first, second);

jit.c

@@ -647,6 +647,14 @@ rb_iseq_set_jit_payload(const rb_iseq_t *iseq, void *payload)
     ISEQ_BODY(iseq)->jit_payload = payload;
 }
 
+void
+rb_iseq_clear_jit_payload(const rb_iseq_t *iseq)
+{
+    RUBY_ASSERT_ALWAYS(IMEMO_TYPE_P(iseq, imemo_iseq));
+    RUBY_ASSERT_ALWAYS(ISEQ_BODY(iseq));
+    ISEQ_BODY(iseq)->jit_payload = NULL;
+}
+
 void
 rb_iseq_reset_jit_func(const rb_iseq_t *iseq)
 {

pack.c

@@ -1108,6 +1108,8 @@ pack_unpack_internal(VALUE str, VALUE fmt, VALUE ofs, enum unpack_mode mode)
     long align_base;
     const char *sptr;
     long slen;
+    const char *fptr;
+    long flen;
 #define UNPACK_PUSH(item) do {\
         VALUE item_val = (item);\
         if ((mode) == UNPACK_BLOCK) {\
@@ -1116,6 +1118,10 @@ pack_unpack_internal(VALUE str, VALUE fmt, VALUE ofs, enum unpack_mode mode)
             if (RSTRING_PTR(str) != sptr || RSTRING_LEN(str) != slen) {\
                 rb_raise(rb_eRuntimeError, "string modified");\
             }\
+            /* The block may have also modified fmt and invalidated p */ \
+            if (RSTRING_PTR(fmt) != fptr || RSTRING_LEN(fmt) != flen) {\
+                rb_raise(rb_eRuntimeError, "format string modified");\
+            }\
         }\
         else if ((mode) == UNPACK_ARRAY) {\
             rb_ary_push(ary, item_val);\
@@ -1144,6 +1150,8 @@ pack_unpack_internal(VALUE str, VALUE fmt, VALUE ofs, enum unpack_mode mode)
 
     p = RSTRING_PTR(fmt);
     pend = p + RSTRING_LEN(fmt);
+    fptr = p;
+    flen = RSTRING_LEN(fmt);
 
 #define UNPACK_FETCH(var, type) (memcpy((var), s, sizeof(type)), s += sizeof(type))
 

test/ruby/test_optimization.rb

@@ -840,6 +840,17 @@ def freeze
     RUBY
   end
 
+  def test_peephole_static_swap
+    code = <<~'RUBY'
+      outer = nil
+      m = Module.new
+      outer::M = m
+    RUBY
+    iseq = RubyVM::InstructionSequence.compile(code)
+    insn = iseq.disasm
+    assert_no_match(/ getlocal\s/, insn, '[Bug #22377]') # unoptimized getlocal
+  end
+
   def test_branch_condition_backquote
     bug = '[ruby-core:80740] [Bug #13444] redefined backquote should be called'
     class << self

test/ruby/test_pack.rb

@@ -259,6 +259,13 @@ def test_unpack_with_block_modifying_string
     }
   end
 
+  def test_unpack_with_block_modifying_format
+    fmt = "C" * 1_000_000
+    assert_raise_with_message(RuntimeError, /format string modified/) {
+      ("A" * 4000).unpack(fmt) { fmt.clear }
+    }
+  end
+
   def test_comment
     assert_equal("\0\1", [0,1].pack("  C  #foo \n  C  "))
     assert_equal([0,1], "\0\1".unpack("  C  #foo \n  C  "))

zjit/bindgen/src/main.rs

@@ -308,7 +308,7 @@ fn main() {
         .allowlist_type("rb_iseq_type")
         .allowlist_type("rb_event_flag_t")
         .allowlist_function("rb_object_shape_count")
-        .allowlist_function("rb_iseq_(get|set)_jit_payload")
+        .allowlist_function("rb_iseq_(get|set|clear)_jit_payload")
         .allowlist_function("rb_iseq_pc_at_idx")
         .allowlist_function("rb_iseq_opcode_at_pc")
         .allowlist_function("rb_iseq_bare_opcode_at_pc")

zjit/src/gc.rs

@@ -3,7 +3,7 @@
 use std::ptr::null;
 use std::{ffi::c_void, ops::Range};
 use crate::{cruby::*, state::ZJITState, stats::with_time_stat, virtualmem::CodePtr};
-use crate::payload::{IseqPayload, IseqVersionRef, get_or_create_iseq_payload};
+use crate::payload::{IseqPayload, IseqVersionRef, get_iseq_payload_ptr};
 use crate::stats::Counter::gc_time_ns;
 
 /// GC callback for marking GC objects in the per-ISEQ payload.
@@ -51,14 +51,26 @@ pub extern "C" fn rb_zjit_iseq_free(iseq: IseqPtr) {
         return;
     }
 
-    // TODO(Shopify/ruby#682): Free `IseqPayload`
-    let payload = get_or_create_iseq_payload(iseq);
-    for version in payload.versions.iter_mut() {
-        unsafe { version.as_mut() }.iseq = null();
+    ZJITState::get_invariants().forget_iseq(iseq);
+
+    // If ZJIT has never created a payload for this ISEQ, do nothing.
+    let payload_ptr = get_iseq_payload_ptr(iseq);
+    if payload_ptr.is_null() {
+        return;
     }
 
-    let invariants = ZJITState::get_invariants();
-    invariants.forget_iseq(iseq);
+    // Take ownership of the payload and unset it from the ISEQ.
+    let payload = unsafe { Box::from_raw(payload_ptr) };
+    unsafe { rb_iseq_clear_jit_payload(iseq) };
+
+    // Clear IseqVersion references. Patch points may hold raw pointers to them, so
+    // they have to outlive the ISEQ. They're dropped when the assumption is broken.
+    for &version in payload.versions.iter() {
+        unsafe { (*version.as_ptr()).iseq = null() };
+    }
+
+    // Free the IseqPayload.
+    drop(payload);
 }
 
 /// GC callback for finalizing a CME
@@ -242,3 +254,43 @@ pub extern "C" fn rb_zjit_root_mark() {
         unsafe { &*jit_frame }.mark();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::cruby::test_utils::{eval, with_rubyvm};
+    use crate::options::set_call_threshold;
+
+    /// Regression test that freeing an ISEQ must not leave an `IseqPayload` behind.
+    #[test]
+    fn test_freed_iseqs_do_not_retain_payloads() {
+        with_rubyvm(|| {
+            // A high call threshold to keep any ISEQs out of the JIT
+            let old_call_threshold = unsafe { crate::options::rb_zjit_call_threshold };
+            set_call_threshold(1_000_000);
+
+            eval(r#"
+                def zjit_eval_iseqs(n)
+                  n.times do |i|
+                    eval("def __zjit_churn#{i}(x) = x + 1", TOPLEVEL_BINDING, "zjit_churn#{i}.rb")
+                    send(:"__zjit_churn#{i}", i)
+                    Object.send(:remove_method, :"__zjit_churn#{i}")
+                  end
+                  3.times { GC.start }
+                end
+            "#);
+
+            // Each iteration creates and frees several ISEQs. It should not grow the heap usage.
+            let alloc_bytes_after_eval = |n: usize| {
+                eval(&format!("zjit_eval_iseqs({n})"));
+                crate::stats::zjit_alloc_bytes()
+            };
+            let small = alloc_bytes_after_eval(200);
+            let large = alloc_bytes_after_eval(2000);
+
+            set_call_threshold(old_call_threshold);
+
+            let alloc_growth = large.saturating_sub(small);
+            assert!(alloc_growth < 100_000, "zjit_alloc_bytes grew by {alloc_growth} bytes across freed ISEQs");
+        });
+    }
+}

zjit/src/payload.rs

@@ -122,6 +122,11 @@ pub fn get_or_create_iseq_payload_ptr(iseq: IseqPtr) -> *mut IseqPayload {
     }
 }
 
+/// Get a pointer to the payload object associated with an ISEQ, or null if never allocated.
+pub fn get_iseq_payload_ptr(iseq: IseqPtr) -> *mut IseqPayload {
+    unsafe { rb_iseq_get_jit_payload(iseq) as *mut IseqPayload }
+}
+
 /// Get the payload object associated with an ISEQ. Create one if none exists.
 pub fn get_or_create_iseq_payload(iseq: IseqPtr) -> &'static mut IseqPayload {
     let payload_non_null = get_or_create_iseq_payload_ptr(iseq);