diff --git a/.github/workflows/zjit-ubuntu.yml b/.github/workflows/zjit-ubuntu.yml index 0843385bce8346..e85bea1c865ca0 100644 --- a/.github/workflows/zjit-ubuntu.yml +++ b/.github/workflows/zjit-ubuntu.yml @@ -53,6 +53,17 @@ jobs: - run: cargo clippy --all-targets --all-features working-directory: zjit + doc-test: + name: cargo test --doc + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - run: cargo test --doc + working-directory: zjit + make: strategy: fail-fast: false diff --git a/bignum.c b/bignum.c index d1a2001f5db6ee..b72b18ad08ab79 100644 --- a/bignum.c +++ b/bignum.c @@ -41,6 +41,7 @@ #include "internal/sanitizers.h" #include "internal/variable.h" #include "internal/warnings.h" +#include "ruby/atomic.h" #include "ruby/thread.h" #include "ruby/util.h" #include "ruby_assert.h" @@ -4762,7 +4763,7 @@ power_cache_get_power(int base, int power_level, size_t *numdigits_ret) if (MAX_BASE36_POWER_TABLE_ENTRIES <= power_level) rb_bug("too big power number requested: maxpow_in_bdigit_dbl(%d)**(2**%d)", base, power_level); - VALUE power = base36_power_cache[base - 2][power_level]; + VALUE power = rbimpl_atomic_value_load(&base36_power_cache[base - 2][power_level], RBIMPL_ATOMIC_ACQUIRE); if (!power) { size_t numdigits; if (power_level == 0) { @@ -4777,9 +4778,16 @@ power_cache_get_power(int base, int power_level, size_t *numdigits_ret) numdigits *= 2; } rb_obj_hide(power); - base36_power_cache[base - 2][power_level] = power; - base36_numdigits_cache[base - 2][power_level] = numdigits; - rb_vm_register_global_object(power); + base36_numdigits_cache[base - 2][power_level] = numdigits; // benign race + /* Ractors can race this fill */ + VALUE old = rbimpl_atomic_value_cas(&base36_power_cache[base - 2][power_level], 0, power, + RBIMPL_ATOMIC_RELEASE, RBIMPL_ATOMIC_ACQUIRE); + if (old) { + power = old; + } + else { + rb_vm_register_global_object(power); + } } if (numdigits_ret) *numdigits_ret = base36_numdigits_cache[base - 2][power_level]; diff --git a/test/ruby/test_ractor.rb b/test/ruby/test_ractor.rb index c60d973a994115..5ae5f488357110 100644 --- a/test/ruby/test_ractor.rb +++ b/test/ruby/test_ractor.rb @@ -434,6 +434,21 @@ def test_ractor_vm_once_dispatch RUBY end + def test_bignum_to_s + assert_ractor(<<~'RUBY') + 8.times.map do + Ractor.new do + 1_000.times do |i| + v = (2**96 - 1) + i + s = v.to_s + # round-trip through str2big (uses the same cache) + raise "bad to_s: #{s.inspect}" unless Integer(s) == v + end + end + end.each(&:join) + RUBY + end + def assert_make_shareable(obj) refute Ractor.shareable?(obj), "object was already shareable" Ractor.make_shareable(obj) diff --git a/thread_pthread.c b/thread_pthread.c index d6e27950570942..a1a4eb5ebfd537 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1713,7 +1713,7 @@ get_native_thread_id(void) #endif #if defined(HAVE_WORKING_FORK) -void rb_internal_thread_event_hooks_rw_lock_atfork(void); +static void rb_internal_thread_event_hooks_rw_lock_atfork(void); static void thread_sched_atfork(struct rb_thread_sched *sched) @@ -3559,7 +3559,7 @@ struct rb_internal_thread_event_hook { static pthread_rwlock_t rb_internal_thread_event_hooks_rw_lock = PTHREAD_RWLOCK_INITIALIZER; #if defined(HAVE_WORKING_FORK) -void +static void rb_internal_thread_event_hooks_rw_lock_atfork(void) { // After fork(), this rwlock may have been held by a now-dead thread. diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index a2d730dfb7e5b1..6b7325d8147592 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -3225,6 +3225,35 @@ fn test_opt_newarray_send_max_redefined() { "), @"[60, 90]"); } +#[test] +fn test_opt_newarray_send_min() { + eval(" + def test(a,b) = [a,b].min + test(10, 20) + "); + assert_contains_opcode("test", YARVINSN_opt_newarray_send); + assert_snapshot!(assert_compiles("[test(10, 20), test(40, 30)]"), @"[10, 30]"); +} + +#[test] +fn test_opt_newarray_send_min_redefined() { + eval(" + class Array + alias_method :old_min, :min + def min + old_min * 2 + end + end + def test(a,b) = [a,b].min + "); + assert_contains_opcode("test", YARVINSN_opt_newarray_send); + assert_snapshot!(assert_compiles_allowing_exits(" + def test(a,b) = [a,b].min + test(15, 30) + [test(15, 30), test(45, 35)] + "), @"[30, 70]"); +} + #[test] fn test_new_hash_empty() { eval(" @@ -4346,6 +4375,17 @@ fn test_method_call() { "), @"12"); } +#[test] +fn test_polymorphic_iseq_dispatch_same_site() { + assert_snapshot!(inspect(" + class A; def foo = 1; end + class B; def foo = 2; end + def test(obj) = obj.foo + test(A.new); test(A.new) # warm up and specialize the call site for A + [test(A.new), test(B.new)] + "), @"[1, 2]"); +} + #[test] fn test_recursive_fact() { assert_snapshot!(inspect(" diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 861a6d0c26828b..b0b61a7fbb1e26 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -9,7 +9,7 @@ use crate::{ backend::lir::C_ARG_OPNDS, cast::IntoUsize, codegen::max_iseq_versions, cruby::*, invariants::{self, iseq_seen_ep_escape}, json::Json, options::{DumpHIR, InlineDepth, debug, get_option}, payload::get_or_create_iseq_payload, profile::reset_profiles_remaining, state::{self, ZJITState}, }; use std::{ - cell::RefCell, collections::{HashMap, HashSet, VecDeque}, ffi::{c_void, c_uint, c_int, CStr}, fmt::Display, mem::{align_of, size_of}, ptr, slice::Iter, + cell::RefCell, collections::{HashMap, HashSet, VecDeque}, ffi::{c_void, c_uint, c_int, CStr}, fmt::Display, ptr, slice::Iter, sync::atomic::Ordering, }; use crate::hir_type::{Type, types}; @@ -478,69 +478,71 @@ impl std::fmt::LowerHex for Offset { } } -/// Marker trait that allowlists the pointee types [`PtrPrintMap::map_ptr`] accepts. -/// Without it, a multi-level pointer like `*const IseqPtr`, usually the result of -/// casting a reference created by matching on an [`Insn`], would map the address -/// of the temporary holding the pointer instead of the pointer value itself. -pub trait HIRPointee {} -impl HIRPointee for VALUE {} -impl HIRPointee for ID {} -impl HIRPointee for c_void {} -impl HIRPointee for u8 {} -impl HIRPointee for rb_iseq_t {} -impl HIRPointee for rb_callable_method_entry_t {} -impl HIRPointee for iseq_inline_constant_cache {} - -/// Raw pointer types accepted by [`PtrPrintMap::map_ptr`]. Only implemented for -/// raw pointers to [`HIRPointee`] types so that accidentally passing a reference -/// (e.g. `&IseqPtr`), which would map the address of the reference itself, is a -/// compile error. -pub trait ActualPtr: Copy { - fn to_void(self) -> *const c_void; - fn from_void(ptr: *const c_void) -> Self; - /// Alignment of the pointee, used to pick a suitably aligned fake address - fn pointee_align() -> usize; - /// Size of the pointee, used to bump the fake address allocator - fn pointee_size() -> usize; +/// A trait tailored for [`PtrPrintMap`] to disable coercion of `&*const T` into `*const *const T`. +/// This is implemented for `*const/mut T`, but rules for coercing into `impl Trait` don't consider the +/// underlying type, so we avoid the undesirable coercion. (It would be weird for the treatment of a +/// trait to change based on the set of types that implements it since Rust has a nominal type system.) +pub trait OneLevelPtr: Copy { + /// Get the address component of the pointer. + fn addr(self) -> usize; + /// The layout of the pointed-to type. + fn pointee_layout(self) -> std::alloc::Layout; } -impl ActualPtr for *const T { - fn to_void(self) -> *const c_void { self.cast() } - fn from_void(ptr: *const c_void) -> Self { ptr.cast() } - fn pointee_align() -> usize { align_of::() } - fn pointee_size() -> usize { size_of::() } +impl OneLevelPtr for *const T { + fn addr(self) -> usize { + <*const T>::addr(self) + } + + fn pointee_layout(self) -> std::alloc::Layout { + std::alloc::Layout::new::() + } } -impl ActualPtr for *mut T { - fn to_void(self) -> *const c_void { self.cast_const().cast() } - fn from_void(ptr: *const c_void) -> Self { ptr.cast::().cast_mut() } - fn pointee_align() -> usize { align_of::() } - fn pointee_size() -> usize { size_of::() } +impl OneLevelPtr for *mut T { + fn addr(self) -> usize { + <*mut T>::addr(self) + } + + fn pointee_layout(self) -> std::alloc::Layout { + std::alloc::Layout::new::() + } } impl PtrPrintMap { - /// Map a pointer for printing - pub fn map_ptr(&self, ptr: P) -> P { - // When testing, address stability is not a concern so print real address to enable code - // reuse + /// Map a pointer for printing. + /// + /// The type bound on this function rejects `&*const T`, which we commonly get through matching: + /// + /// ```compile_fail + /// let value = 0; + /// let ptr: *const usize = &value; + /// let ref_to_ptr: &*const usize = &ptr; + /// + /// let map = zjit::hir::PtrPrintMap::identity(); + /// // error[E0277]: the trait bound `&*const usize: OneLevelPtr` is not satisfied + /// map.map_ptr(ref_to_ptr); + /// ``` + pub fn map_ptr(&self, ptr: impl OneLevelPtr) -> *const c_void { + let raw = ptr::without_provenance(ptr.addr()); if !self.map_ptrs { - return ptr; + return raw } use std::collections::hash_map::Entry::*; - let raw = ptr.to_void(); let inner = &mut *self.inner.borrow_mut(); match inner.map.entry(raw) { - Occupied(entry) => P::from_void(*entry.get()), + Occupied(entry) => *entry.get(), Vacant(entry) => { // Pick a fake address that is suitably aligned for the pointee and // remember it in the map - let mapped = inner.next_ptr.wrapping_add(inner.next_ptr.align_offset(P::pointee_align())); + let layout = ptr.pointee_layout(); + let mapped = inner.next_ptr.wrapping_add(inner.next_ptr.align_offset(layout.align())); entry.insert(mapped); // Bump for the next pointer - inner.next_ptr = mapped.wrapping_add(P::pointee_size()); - P::from_void(mapped) + inner.next_ptr = mapped.wrapping_add(layout.size()); + mapped } } } @@ -2518,7 +2520,7 @@ impl + PartialEq + std::convert::From> UnionFind /// /// and after `find(A)`: /// - /// ``` + /// ```text /// A -> C /// B ---^ /// ``` @@ -3180,7 +3182,7 @@ impl Function { /// /// This is _the_ function for reading [`Insn`]. Use frequently. Example: /// - /// ```rust + /// ```ignore /// match func.find(insn_id) { /// IfTrue { val, target } if func.is_truthy(val) => { /// let jump = self.new_insn(Insn::Jump(target)); diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 739d382b3a5b1a..641e01d05a328b 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -8040,6 +8040,95 @@ mod hir_opt_tests { "); } + #[test] + fn test_optimize_array_max() { + eval(r##" + def test(a,b) = [a,b].max + "##); + assert_contains_opcode("test", YARVINSN_opt_newarray_send); + assert_snapshot!(hir_string("test"), @" + fn test@:2: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :a@0x1000 + v4:BasicObject = LoadField v2, :b@0x1001 + Jump bb3(v1, v3, v4) + bb2(): + EntryPoint JIT(0) + v7:BasicObject = LoadArg :self@0 + v8:BasicObject = LoadArg :a@1 + v9:BasicObject = LoadArg :b@2 + Jump bb3(v7, v8, v9) + bb3(v11:BasicObject, v12:BasicObject, v13:BasicObject): + PatchPoint BOPRedefined(ARRAY_REDEFINED_OP_FLAG, BOP_MAX) + v20:BasicObject = ArrayMax v12, v13 + CheckInterrupts + Return v20 + "); + } + + #[test] + fn test_optimize_array_min() { + eval(r##" + def test(a,b) = [a,b].min + "##); + assert_contains_opcode("test", YARVINSN_opt_newarray_send); + assert_snapshot!(hir_string("test"), @" + fn test@:2: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :a@0x1000 + v4:BasicObject = LoadField v2, :b@0x1001 + Jump bb3(v1, v3, v4) + bb2(): + EntryPoint JIT(0) + v7:BasicObject = LoadArg :self@0 + v8:BasicObject = LoadArg :a@1 + v9:BasicObject = LoadArg :b@2 + Jump bb3(v7, v8, v9) + bb3(v11:BasicObject, v12:BasicObject, v13:BasicObject): + PatchPoint BOPRedefined(ARRAY_REDEFINED_OP_FLAG, BOP_MIN) + v20:BasicObject = ArrayMin v12, v13 + CheckInterrupts + Return v20 + "); + } + + #[test] + fn test_dont_optimize_array_min_if_redefined() { + eval(r##" + class Array + def min = [] + end + def test = [4,5,6].min + "##); + assert_snapshot!(hir_string("test"), @" + fn test@:5: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v10:ArrayExact[VALUE(0x1000)] = Const Value(VALUE(0x1000)) + v11:ArrayExact = ArrayDup v10 + PatchPoint NoSingletonClass(Array@0x1008) + PatchPoint MethodRedefined(Array@0x1008, min@0x1010, cme:0x1018) + PushInlineFrame v11 (0x1040) + v27:ArrayExact = NewArray + CheckInterrupts + PopInlineFrame + Return v27 + "); + } + #[test] fn test_set_type_from_constant() { eval(" diff --git a/zjit/src/lib.rs b/zjit/src/lib.rs index 1440b6ff6942e6..552b9d2c7e39e3 100644 --- a/zjit/src/lib.rs +++ b/zjit/src/lib.rs @@ -5,6 +5,10 @@ #![allow(clippy::too_many_arguments)] #![allow(clippy::needless_bool)] +// We make things public for doc-test to reach them, +// and don't mind linking to private items in docs. +#![allow(rustdoc::private_intra_doc_links)] + // Add std docs to cargo doc. #[doc(inline)] pub use std; @@ -13,7 +17,7 @@ mod state; mod distribution; mod cruby; mod cruby_methods; -mod hir; +pub mod hir; mod hir_type; mod hir_effect; mod codegen; diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index a118254ac21ad3..3e8a2e1e8873b0 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -320,6 +320,8 @@ impl ProfiledType { /// Profile the class and shape of the given object fn new(obj: VALUE) -> Self { + // Qundef must never escape the VM internals; rb_class_of(Qundef) is undefined + debug_assert_ne!(obj, Qundef, "should not profile Qundef"); if obj.special_const_p() { return Self { class: obj.class_of(), shape: INVALID_SHAPE_ID,