diff --git a/.github/workflows/zjit-macos.yml b/.github/workflows/zjit-macos.yml index 376163fcdc11a7..4d3ffcb9ea7bf0 100644 --- a/.github/workflows/zjit-macos.yml +++ b/.github/workflows/zjit-macos.yml @@ -30,6 +30,8 @@ env: # Work around transient crates.io download failures (curl HTTP/2 framing / partial transfer) CARGO_NET_RETRY: 10 CARGO_HTTP_MULTIPLEXING: false + RUST_BACKTRACE: 1 + ZJIT_RB_BUG: 1 jobs: make: @@ -60,8 +62,6 @@ jobs: RUN_OPTS: ${{ matrix.run_opts }} SPECOPTS: ${{ matrix.specopts }} -B../src/spec/zjit.mspec TESTOPTS: ${{ matrix.testopts }} - RUST_BACKTRACE: 1 - ZJIT_RB_BUG: 1 runs-on: macos-26 diff --git a/.github/workflows/zjit-ubuntu.yml b/.github/workflows/zjit-ubuntu.yml index 4fb0bd5100345b..63836e3a45305c 100644 --- a/.github/workflows/zjit-ubuntu.yml +++ b/.github/workflows/zjit-ubuntu.yml @@ -30,6 +30,8 @@ env: # Work around transient crates.io download failures (curl HTTP/2 framing / partial transfer) CARGO_NET_RETRY: 10 CARGO_HTTP_MULTIPLEXING: false + RUST_BACKTRACE: 1 + ZJIT_RB_BUG: 1 jobs: lint: @@ -125,8 +127,6 @@ jobs: TESTOPTS: ${{ matrix.testopts }} RUBY_DEBUG: ci BUNDLE_JOBS: 8 # for yjit-bench - RUST_BACKTRACE: 1 - ZJIT_RB_BUG: 1 runs-on: ${{ matrix.runs-on || 'ubuntu-22.04' }} diff --git a/ext/-test-/io_buffer/io_buffer.c b/ext/-test-/io_buffer/io_buffer.c index 1e61e228df3a38..ef35548bcb1010 100644 --- a/ext/-test-/io_buffer/io_buffer.c +++ b/ext/-test-/io_buffer/io_buffer.c @@ -89,6 +89,30 @@ io_buffer_for_writing_modify_string(VALUE self, VALUE string) return rb_io_buffer_for_writing(string, io_buffer_modify_string, string); } +static VALUE +io_buffer_lock(VALUE self, VALUE buffer) +{ + return rb_io_buffer_lock(buffer); +} + +static VALUE +io_buffer_unlock(VALUE self, VALUE buffer) +{ + return rb_io_buffer_unlock(buffer); +} + +static VALUE +io_buffer_new_locked(VALUE self, VALUE size) +{ + return rb_io_buffer_new_locked(NULL, NUM2SIZET(size), RB_IO_BUFFER_INTERNAL); +} + +static VALUE +io_buffer_free_locked(VALUE self, VALUE buffer) +{ + return rb_io_buffer_free_locked(buffer); +} + void Init_io_buffer(void) { @@ -102,4 +126,8 @@ Init_io_buffer(void) rb_define_singleton_method(mIOBuffer, "for_writing_set_string", io_buffer_for_writing_set_string, 2); rb_define_singleton_method(mIOBuffer, "for_writing_readonly?", io_buffer_for_writing_readonly_p, 1); rb_define_singleton_method(mIOBuffer, "for_writing_modify_string", io_buffer_for_writing_modify_string, 1); + rb_define_singleton_method(mIOBuffer, "lock", io_buffer_lock, 1); + rb_define_singleton_method(mIOBuffer, "unlock", io_buffer_unlock, 1); + rb_define_singleton_method(mIOBuffer, "new_locked", io_buffer_new_locked, 1); + rb_define_singleton_method(mIOBuffer, "free_locked", io_buffer_free_locked, 1); } diff --git a/file.c b/file.c index ddea32d0e54710..64f5965aef53bd 100644 --- a/file.c +++ b/file.c @@ -2734,21 +2734,27 @@ rb_file_ctime(VALUE obj) #if defined(HAVE_STAT_BIRTHTIME) /* * call-seq: - * File.birthtime(entry_path) -> new_time + * File.birthtime(path) -> time * * Returns a new Time object containing the create time - * of the entry at the given +path+: + * of the entry at the given +path+; + * see {File System Timestamps}[rdoc-ref:file/timestamps.md]: * - * path = 't.tmp' - * File.birthtime(path) # Raises Errno::ENOENT: No such file or directory - * File.write(path, 'foo') - * File.birthtime(path) # => 2026-04-14 11:10:43.2891695 -0500 - * File.write(path, 'bar') - * File.birthtime(path) # => 2026-04-14 11:10:43.2891695 -0500 - * File.delete(path) - * File.birthtime(path) # Raises Errno::ENOENT: No such file or directory + * filepath = 't.tmp' + * File.birthtime(filepath) # Raises Errno::ENOENT: No such file or directory + * File.write(filepath, 'foo') + * File.birthtime(filepath) # => 2026-04-14 11:10:43.2891695 -0500 + * File.write(filepath, 'bar') + * File.birthtime(filepath) # => 2026-04-14 11:10:43.2891695 -0500 + * File.delete(filepath) + * File.birthtime(filepath) # Raises Errno::ENOENT: No such file or directory. + * + * dirpath = 'tmp' + * Dir.mkdir(dirpath) + * File.birthtime(dirpath) # => 2026-08-21 13:42:19.389324172 -0500 + * Dir.rmdir(dirpath) + * File.birthtime(dirpath) # Raises Errno::ENOENT: No such file or directory. * - * See {File System Timestamps}[rdoc-ref:file/timestamps.md]. */ VALUE @@ -5364,11 +5370,11 @@ ruby_enc_find_basename(const char *name, long *baselen, long *alllen, rb_encodin /* * call-seq: - * File.basename(path, suffix = '') -> new_string + * File.basename(path, suffix = '') -> string * - * Returns a new string containing all or part of the last entry of the given +path+. - * Entries are delimited by the value of constant File::SEPARATOR - * and, if non-nil, the value of constant File::ALT_SEPARATOR. + * Returns a new string containing all or part of the last component of the given +path+. + * Components are delimited by the value of constant File::SEPARATOR + * and, if non-+nil+, the value of constant File::ALT_SEPARATOR. * * When +suffix+ is the empty string '', * returns all of the last entry: diff --git a/include/ruby/io/buffer.h b/include/ruby/io/buffer.h index e4d98bf0511b12..88df750056915a 100644 --- a/include/ruby/io/buffer.h +++ b/include/ruby/io/buffer.h @@ -48,11 +48,6 @@ enum rb_io_buffer_flags { // A mapped buffer that is also shared. RB_IO_BUFFER_SHARED = 8, - // The buffer is locked and cannot be resized. - // More specifically, it means we can't change the base address or size. - // A buffer is typically locked before a system call that uses the data. - RB_IO_BUFFER_LOCKED = 32, - // The buffer mapping is private and will not impact other processes or the underlying file. RB_IO_BUFFER_PRIVATE = 64, @@ -79,13 +74,22 @@ enum rb_io_buffer_endian { }; VALUE rb_io_buffer_new(void *base, size_t size, enum rb_io_buffer_flags flags); +// Create a buffer with an initial lock count of one. This is typically used +// for temporary wrappers around borrowed memory and paired with +// rb_io_buffer_free_locked. +VALUE rb_io_buffer_new_locked(void *base, size_t size, enum rb_io_buffer_flags flags); VALUE rb_io_buffer_map(VALUE io, size_t size, rb_off_t offset, enum rb_io_buffer_flags flags); +// Acquire and release a reference-counted lock on the backing allocation. +// Every successful lock call must be paired with exactly one unlock call. VALUE rb_io_buffer_lock(VALUE self); VALUE rb_io_buffer_unlock(VALUE self); int rb_io_buffer_try_unlock(VALUE self); VALUE rb_io_buffer_free(VALUE self); +// Release the buffer's only lock and immediately invalidate it. This is for +// temporary wrappers around borrowed memory. Calls rb_bug if the lock count is +// not exactly one. VALUE rb_io_buffer_free_locked(VALUE self); // Access the internal buffer and flags. Validates the pointers. diff --git a/io_buffer.c b/io_buffer.c index ae00daa7e641da..d1b2f047110a0e 100644 --- a/io_buffer.c +++ b/io_buffer.c @@ -46,7 +46,7 @@ enum { RB_IO_BUFFER_INSPECT_HEXDUMP_WIDTH = 16, // This is used to validate the flags given by the user. - RB_IO_BUFFER_FLAGS_MASK = RB_IO_BUFFER_EXTERNAL | RB_IO_BUFFER_INTERNAL | RB_IO_BUFFER_MAPPED | RB_IO_BUFFER_SHARED | RB_IO_BUFFER_LOCKED | RB_IO_BUFFER_PRIVATE | RB_IO_BUFFER_READONLY, + RB_IO_BUFFER_FLAGS_MASK = RB_IO_BUFFER_EXTERNAL | RB_IO_BUFFER_INTERNAL | RB_IO_BUFFER_MAPPED | RB_IO_BUFFER_SHARED | RB_IO_BUFFER_PRIVATE | RB_IO_BUFFER_READONLY, RB_IO_BUFFER_DEBUG = 0, }; @@ -55,6 +55,8 @@ struct rb_io_buffer { void *base; size_t size; enum rb_io_buffer_flags flags; + // Locking and unlocking are performed with the GVL held. + size_t lock_count; #if defined(_WIN32) HANDLE mapping; @@ -189,6 +191,7 @@ io_buffer_zero(struct rb_io_buffer *buffer) { buffer->base = NULL; buffer->size = 0; + buffer->lock_count = 0; #if defined(_WIN32) buffer->mapping = NULL; #endif @@ -222,6 +225,7 @@ io_buffer_initialize(VALUE self, struct rb_io_buffer *buffer, void *base, size_t buffer->base = base; buffer->size = size; buffer->flags = flags; + buffer->lock_count = 0; RB_OBJ_WRITE(self, &buffer->source, source); #if defined(_WIN32) @@ -260,6 +264,7 @@ io_buffer_free(struct rb_io_buffer *buffer) buffer->size = 0; buffer->flags = 0; + buffer->lock_count = 0; buffer->source = Qnil; } @@ -344,6 +349,25 @@ get_io_buffer(VALUE self) return buffer; } +// Return the buffer which owns the lock count. A slice backed by another +// buffer shares that source buffer's lock count. Other external sources, such +// as strings, manage their own lifetime and do not share buffer lock state. +static struct rb_io_buffer * +io_buffer_lock_owner(struct rb_io_buffer *buffer) +{ + if (rb_typeddata_is_kind_of(buffer->source, &rb_io_buffer_type)) { + return get_io_buffer(buffer->source); + } + + return buffer; +} + +static bool +io_buffer_locked(struct rb_io_buffer *buffer) +{ + return io_buffer_lock_owner(buffer)->lock_count > 0; +} + static inline enum rb_io_buffer_flags io_buffer_extract_flags(VALUE argument) { @@ -757,6 +781,16 @@ rb_io_buffer_new(void *base, size_t size, enum rb_io_buffer_flags flags) return instance; } +VALUE +rb_io_buffer_new_locked(void *base, size_t size, enum rb_io_buffer_flags flags) +{ + VALUE instance = rb_io_buffer_new(base, size, flags); + + rb_io_buffer_lock(instance); + + return instance; +} + VALUE rb_io_buffer_map(VALUE io, size_t size, rb_off_t offset, enum rb_io_buffer_flags flags) { @@ -1135,7 +1169,7 @@ rb_io_buffer_to_s(VALUE self) rb_str_cat2(result, " SHARED"); } - if (buffer->flags & RB_IO_BUFFER_LOCKED) { + if (io_buffer_locked(buffer)) { rb_str_cat2(result, " LOCKED"); } @@ -1427,12 +1461,11 @@ rb_io_buffer_shared_p(VALUE self) /* * call-seq: locked? -> true or false * - * If the buffer is _locked_, meaning it is inside #locked block execution. - * Locked buffer can't be resized or freed, and another lock can't be acquired - * on it. + * If the buffer is _locked_, its underlying allocation cannot be resized, + * freed or transferred. Locks are shared with slices and may be nested. * - * Locking is not thread safe, but is a semantic used to ensure buffers don't - * move while being used by a system call. + * Locking is a lifetime mechanism used to ensure buffers don't move while + * being used by a system call or other native operation. * * buffer.locked do * buffer.write(io) # theoretical system call interface @@ -1443,7 +1476,7 @@ rb_io_buffer_locked_p(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - return RBOOL(buffer->flags & RB_IO_BUFFER_LOCKED); + return RBOOL(io_buffer_locked(buffer)); } /* call-seq: private? -> true or false @@ -1499,23 +1532,16 @@ io_buffer_readonly_p(VALUE self) return RBOOL(rb_io_buffer_readonly_p(self)); } -static int -io_buffer_try_lock(struct rb_io_buffer *buffer) -{ - if (buffer->flags & RB_IO_BUFFER_LOCKED) { - return 0; - } - - buffer->flags |= RB_IO_BUFFER_LOCKED; - return 1; -} - static void io_buffer_lock(struct rb_io_buffer *buffer) { - if (!io_buffer_try_lock(buffer)) { - rb_raise(rb_eIOBufferLockedError, "Buffer already locked!"); + struct rb_io_buffer *owner = io_buffer_lock_owner(buffer); + + if (owner->lock_count == SIZE_MAX) { + rb_raise(rb_eIOBufferLockedError, "It's locks all the way down!"); } + + owner->lock_count += 1; } VALUE @@ -1531,11 +1557,13 @@ rb_io_buffer_lock(VALUE self) static void io_buffer_unlock(struct rb_io_buffer *buffer) { - if (!(buffer->flags & RB_IO_BUFFER_LOCKED)) { + struct rb_io_buffer *owner = io_buffer_lock_owner(buffer); + + if (owner->lock_count == 0) { rb_raise(rb_eIOBufferLockedError, "Buffer not locked!"); } - buffer->flags &= ~RB_IO_BUFFER_LOCKED; + owner->lock_count -= 1; } VALUE @@ -1552,9 +1580,11 @@ int rb_io_buffer_try_unlock(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); + struct rb_io_buffer *owner = io_buffer_lock_owner(buffer); + + if (owner->lock_count > 0) { + owner->lock_count -= 1; - if (buffer->flags & RB_IO_BUFFER_LOCKED) { - buffer->flags &= ~RB_IO_BUFFER_LOCKED; return 1; } @@ -1566,7 +1596,7 @@ rb_io_buffer_locked_ensure(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - buffer->flags &= ~RB_IO_BUFFER_LOCKED; + io_buffer_unlock(buffer); return Qnil; } @@ -1574,16 +1604,14 @@ rb_io_buffer_locked_ensure(VALUE self) /* * call-seq: locked { ... } * - * Allows to process a buffer in exclusive way, for concurrency-safety. While - * the block is performed, the buffer is considered locked, and no other code - * can enter the lock. Also, locked buffer can't be changed with #resize or - * #free. + * Prevents the buffer or its buffer source from being moved or freed while + * the block is executing. Locks are nested and shared with slices backed by + * the same buffer source. The source remains locked until every nested lock + * has been released. * - * The following operations acquire a lock: #resize, #free. - * - * Locking is not thread safe. It is designed as a safety net around - * non-blocking system calls. You can only share a buffer between threads with - * appropriate synchronisation techniques. + * Locking protects allocation lifetime; it does not serialize access to the + * bytes. Code that shares mutable buffer contents between threads must still + * use appropriate synchronization. * * buffer = IO::Buffer.new(4) * buffer.locked? #=> false @@ -1595,9 +1623,8 @@ rb_io_buffer_locked_ensure(VALUE self) * end * * Fiber.schedule do - * # in `locked': Buffer already locked! (IO::Buffer::LockedError) * buffer.locked do - * buffer.set_string("test", 0) + * buffer.set_string("test", 0) # Nested locking is allowed. * end * end */ @@ -1606,11 +1633,11 @@ rb_io_buffer_locked(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - if (buffer->flags & RB_IO_BUFFER_LOCKED) { - rb_raise(rb_eIOBufferLockedError, "Buffer already locked!"); - } + // Only yield the block for a currently valid view. In particular, an + // invalid slice should not lock its source. + io_buffer_validate_for_reading(buffer); - buffer->flags |= RB_IO_BUFFER_LOCKED; + io_buffer_lock(buffer); return rb_ensure(rb_yield, self, rb_io_buffer_locked_ensure, self); } @@ -1645,7 +1672,7 @@ rb_io_buffer_free(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - if (buffer->flags & RB_IO_BUFFER_LOCKED) { + if (io_buffer_locked(buffer)) { rb_raise(rb_eIOBufferLockedError, "Buffer is locked!"); } @@ -1657,6 +1684,14 @@ rb_io_buffer_free(VALUE self) VALUE rb_io_buffer_free_locked(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); + struct rb_io_buffer *owner = io_buffer_lock_owner(buffer); + + // This function is used to invalidate temporary wrappers around borrowed + // memory. If another lock remains, the owner cannot safely end the + // lifetime of that memory while another operation still retains it. + if (owner->lock_count != 1) { + rb_bug("rb_io_buffer_free_locked: expected lock count 1, got %" PRIuSIZE, owner->lock_count); + } io_buffer_unlock(buffer); io_buffer_free(buffer); @@ -1741,8 +1776,9 @@ rb_io_buffer_slice(struct rb_io_buffer *buffer, VALUE self, size_t offset, size_ slice->base = (char*)buffer->base + offset; slice->size = length; - // The source should be the root buffer: - if (buffer->source != Qnil) { + // Slices retain their root buffer. If this buffer is already a slice, + // retain its root directly rather than building a chain of slices: + if (rb_typeddata_is_kind_of(buffer->source, &rb_io_buffer_type)) { RB_OBJ_WRITE(instance, &slice->source, buffer->source); } else { @@ -1758,8 +1794,9 @@ rb_io_buffer_slice(struct rb_io_buffer *buffer, VALUE self, size_t offset, size_ * Produce another IO::Buffer which is a slice (or view into) the current one * starting at +offset+ bytes and going for +length+ bytes. * - * The slicing happens without copying of memory, and the slice keeps being - * associated with the original buffer's source (string, or file), if any. + * The slicing happens without copying memory. The slice retains its root + * buffer and becomes invalid if that root is freed, transferred, resized so + * that the slice is outside its bounds, or otherwise invalidated. * * If the offset is not given, it will be zero. If the offset is negative, it * will raise an ArgumentError. @@ -1836,7 +1873,7 @@ rb_io_buffer_transfer(VALUE self) { struct rb_io_buffer *buffer = get_io_buffer(self); - if (buffer->flags & RB_IO_BUFFER_LOCKED) { + if (io_buffer_locked(buffer)) { rb_raise(rb_eIOBufferLockedError, "Cannot transfer ownership of locked buffer!"); } @@ -1884,7 +1921,7 @@ rb_io_buffer_resize(VALUE self, size_t size) io_buffer_validate_for_reading(buffer); - if (buffer->flags & RB_IO_BUFFER_LOCKED) { + if (io_buffer_locked(buffer)) { rb_raise(rb_eIOBufferLockedError, "Cannot resize locked buffer!"); } @@ -1897,6 +1934,11 @@ rb_io_buffer_resize(VALUE self, size_t size) rb_raise(rb_eIOBufferAccessError, "Cannot resize external buffer!"); } + if (size == 0) { + io_buffer_free(buffer); + return; + } + #if defined(HAVE_MREMAP) && defined(MREMAP_MAYMOVE) if (buffer->flags & RB_IO_BUFFER_MAPPED) { void *base = mremap(buffer->base, buffer->size, size, MREMAP_MAYMOVE); @@ -1915,11 +1957,6 @@ rb_io_buffer_resize(VALUE self, size_t size) #endif if (buffer->flags & RB_IO_BUFFER_INTERNAL) { - if (size == 0) { - io_buffer_free(buffer); - return; - } - void *base = realloc(buffer->base, size); if (!base) { @@ -2376,26 +2413,19 @@ io_buffer_extract_offset_count(ID buffer_type, size_t size, int argc, VALUE *arg } } -/* - * call-seq: - * each(buffer_type, [offset, [count]]) {|offset, value| ...} -> self - * each(buffer_type, [offset, [count]]) -> enumerator - * - * Iterates over the buffer, yielding each +value+ of +buffer_type+ starting - * from +offset+. - * - * If +count+ is given, only +count+ values will be yielded. - * - * IO::Buffer.for("Hello World").each(:U8, 2, 2) do |offset, value| - * puts "#{offset}: #{value}" - * end - * # 2: 108 - * # 3: 108 - */ +struct io_buffer_each_arguments { + VALUE self; + int argc; + VALUE *argv; +}; + static VALUE -io_buffer_each(int argc, VALUE *argv, VALUE self) +io_buffer_each_locked(VALUE _arguments) { - RETURN_ENUMERATOR_KW(self, argc, argv, RB_NO_KEYWORDS); + struct io_buffer_each_arguments *arguments = (void *)_arguments; + VALUE self = arguments->self; + int argc = arguments->argc; + VALUE *argv = arguments->argv; const void *base; size_t size; @@ -2422,6 +2452,37 @@ io_buffer_each(int argc, VALUE *argv, VALUE self) return self; } +/* + * call-seq: + * each(buffer_type, [offset, [count]]) {|offset, value| ...} -> self + * each(buffer_type, [offset, [count]]) -> enumerator + * + * Iterates over the buffer, yielding each +value+ of +buffer_type+ starting + * from +offset+. + * + * If +count+ is given, only +count+ values will be yielded. + * + * IO::Buffer.for("Hello World").each(:U8, 2, 2) do |offset, value| + * puts "#{offset}: #{value}" + * end + * # 2: 108 + * # 3: 108 + */ +static VALUE +io_buffer_each(int argc, VALUE *argv, VALUE self) +{ + RETURN_ENUMERATOR_KW(self, argc, argv, RB_NO_KEYWORDS); + + struct io_buffer_each_arguments arguments = { + .self = self, + .argc = argc, + .argv = argv, + }; + + rb_io_buffer_lock(self); + return rb_ensure(io_buffer_each_locked, (VALUE)&arguments, rb_io_buffer_locked_ensure, self); +} + /* * call-seq: values(buffer_type, [offset, [count]]) -> array * @@ -2461,25 +2522,13 @@ io_buffer_values(int argc, VALUE *argv, VALUE self) return array; } -/* - * call-seq: - * each_byte([offset, [count]]) {|byte| ...} -> self - * each_byte([offset, [count]]) -> enumerator - * - * Iterates over the buffer, yielding each byte starting from +offset+. - * - * If +count+ is given, only +count+ bytes will be yielded. - * - * IO::Buffer.for("Hello World").each_byte(2, 2) do |offset, byte| - * puts "#{offset}: #{byte}" - * end - * # 2: 108 - * # 3: 108 - */ static VALUE -io_buffer_each_byte(int argc, VALUE *argv, VALUE self) +io_buffer_each_byte_locked(VALUE _arguments) { - RETURN_ENUMERATOR_KW(self, argc, argv, RB_NO_KEYWORDS); + struct io_buffer_each_arguments *arguments = (void *)_arguments; + VALUE self = arguments->self; + int argc = arguments->argc; + VALUE *argv = arguments->argv; const void *base; size_t size; @@ -2501,6 +2550,36 @@ io_buffer_each_byte(int argc, VALUE *argv, VALUE self) return self; } +/* + * call-seq: + * each_byte([offset, [count]]) {|byte| ...} -> self + * each_byte([offset, [count]]) -> enumerator + * + * Iterates over the buffer, yielding each byte starting from +offset+. + * + * If +count+ is given, only +count+ bytes will be yielded. + * + * IO::Buffer.for("Hello World").each_byte(2, 2) do |offset, byte| + * puts "#{offset}: #{byte}" + * end + * # 2: 108 + * # 3: 108 + */ +static VALUE +io_buffer_each_byte(int argc, VALUE *argv, VALUE self) +{ + RETURN_ENUMERATOR_KW(self, argc, argv, RB_NO_KEYWORDS); + + struct io_buffer_each_arguments arguments = { + .self = self, + .argc = argc, + .argv = argv, + }; + + rb_io_buffer_lock(self); + return rb_ensure(io_buffer_each_byte_locked, (VALUE)&arguments, rb_io_buffer_locked_ensure, self); +} + static inline void rb_io_buffer_set_value(struct rb_io_buffer *buffer, VALUE buffer_type, size_t *offset, VALUE value) { @@ -3042,16 +3121,12 @@ io_buffer_blocking_region(VALUE io, struct rb_io_buffer *buffer, rb_blocking_fun .data = data, }; - // If the buffer is already locked, we can skip the ensure (unlock): - if (buffer->flags & RB_IO_BUFFER_LOCKED) { - return io_buffer_blocking_region_begin((VALUE)&argument); - } - else { - // The buffer should be locked for the duration of the blocking region: - io_buffer_lock(buffer); + // The buffer should be locked for the duration of the blocking region. We + // always acquire our own reference so another operation cannot release the + // allocation while this operation is still using it: + io_buffer_lock(buffer); - return rb_ensure(io_buffer_blocking_region_begin, (VALUE)&argument, io_buffer_blocking_region_ensure, (VALUE)&argument); - } + return rb_ensure(io_buffer_blocking_region_begin, (VALUE)&argument, io_buffer_blocking_region_ensure, (VALUE)&argument); } struct io_buffer_read_internal_argument { @@ -4113,9 +4188,6 @@ Init_IO_Buffer(void) /* Indicates that the memory in the buffer is also mapped such that it can be shared with other processes. See #shared? for more details. */ rb_define_const(rb_cIOBuffer, "SHARED", RB_INT2NUM(RB_IO_BUFFER_SHARED)); - /* Indicates that the memory in the buffer is locked and cannot be resized or freed. See #locked? and #locked for more details. */ - rb_define_const(rb_cIOBuffer, "LOCKED", RB_INT2NUM(RB_IO_BUFFER_LOCKED)); - /* Indicates that the memory in the buffer is mapped privately and changes won't be replicated to the underlying file. See #private? for more details. */ rb_define_const(rb_cIOBuffer, "PRIVATE", RB_INT2NUM(RB_IO_BUFFER_PRIVATE)); diff --git a/scheduler.c b/scheduler.c index 2e0d50d163f373..2a41bb1d2717a8 100644 --- a/scheduler.c +++ b/scheduler.c @@ -976,7 +976,7 @@ rb_fiber_scheduler_io_pwrite(VALUE scheduler, VALUE io, rb_off_t from, VALUE buf VALUE rb_fiber_scheduler_io_read_memory(VALUE scheduler, VALUE io, void *base, size_t size, size_t length) { - VALUE buffer = rb_io_buffer_new(base, size, RB_IO_BUFFER_LOCKED); + VALUE buffer = rb_io_buffer_new_locked(base, size, 0); VALUE result = rb_fiber_scheduler_io_read(scheduler, io, buffer, length, 0); @@ -988,7 +988,7 @@ rb_fiber_scheduler_io_read_memory(VALUE scheduler, VALUE io, void *base, size_t VALUE rb_fiber_scheduler_io_write_memory(VALUE scheduler, VALUE io, const void *base, size_t size, size_t length) { - VALUE buffer = rb_io_buffer_new((void*)base, size, RB_IO_BUFFER_LOCKED|RB_IO_BUFFER_READONLY); + VALUE buffer = rb_io_buffer_new_locked((void*)base, size, RB_IO_BUFFER_READONLY); VALUE result = rb_fiber_scheduler_io_write(scheduler, io, buffer, length, 0); @@ -1000,7 +1000,7 @@ rb_fiber_scheduler_io_write_memory(VALUE scheduler, VALUE io, const void *base, VALUE rb_fiber_scheduler_io_pread_memory(VALUE scheduler, VALUE io, rb_off_t from, void *base, size_t size, size_t length) { - VALUE buffer = rb_io_buffer_new(base, size, RB_IO_BUFFER_LOCKED); + VALUE buffer = rb_io_buffer_new_locked(base, size, 0); VALUE result = rb_fiber_scheduler_io_pread(scheduler, io, from, buffer, length, 0); @@ -1012,7 +1012,7 @@ rb_fiber_scheduler_io_pread_memory(VALUE scheduler, VALUE io, rb_off_t from, voi VALUE rb_fiber_scheduler_io_pwrite_memory(VALUE scheduler, VALUE io, rb_off_t from, const void *base, size_t size, size_t length) { - VALUE buffer = rb_io_buffer_new((void*)base, size, RB_IO_BUFFER_LOCKED|RB_IO_BUFFER_READONLY); + VALUE buffer = rb_io_buffer_new_locked((void*)base, size, RB_IO_BUFFER_READONLY); VALUE result = rb_fiber_scheduler_io_pwrite(scheduler, io, from, buffer, length, 0); diff --git a/spec/ruby/core/io/buffer/for_spec.rb b/spec/ruby/core/io/buffer/for_spec.rb index 4c614f74b05a6a..ce35757f7a5eda 100644 --- a/spec/ruby/core/io/buffer/for_spec.rb +++ b/spec/ruby/core/io/buffer/for_spec.rb @@ -59,6 +59,20 @@ @buffer.should.null? end + ruby_version_is "4.1" do + it "invalidates slices when the block ends" do + slice = nil + + IO::Buffer.for(@string) do |buffer| + slice = buffer.slice(0, 2) + slice.should.valid? + end + + slice.should_not.valid? + -> { slice.get_string }.should.raise(IO::Buffer::InvalidatedError) + end + end + context "if string is not frozen" do it "creates a modifiable string-backed buffer" do IO::Buffer.for(@string) do |buffer| diff --git a/spec/ruby/core/io/buffer/locked_spec.rb b/spec/ruby/core/io/buffer/locked_spec.rb index 249026aa8a9d6f..93baf7e7d6540c 100644 --- a/spec/ruby/core/io/buffer/locked_spec.rb +++ b/spec/ruby/core/io/buffer/locked_spec.rb @@ -26,30 +26,98 @@ end end - it "disallows reentrant locking, raising IO::Buffer::LockedError" do - @buffer = IO::Buffer.new(4) - @buffer.locked do - -> { @buffer.locked {} }.should.raise(IO::Buffer::LockedError, "Buffer already locked!") + ruby_version_is ""..."4.1" do + it "disallows reentrant locking, raising IO::Buffer::LockedError" do + @buffer = IO::Buffer.new(4) + @buffer.locked do + -> { @buffer.locked {} }.should.raise(IO::Buffer::LockedError, "Buffer already locked!") + end end - end - it "does not propagate to buffer's slices" do - @buffer = IO::Buffer.new(4) - slice = @buffer.slice(0, 2) - @buffer.locked do - @buffer.locked?.should == true - slice.locked?.should == false - slice.locked { slice.locked?.should == true } + it "does not propagate to buffer's slices" do + @buffer = IO::Buffer.new(4) + slice = @buffer.slice(0, 2) + @buffer.locked do + @buffer.locked?.should == true + slice.locked?.should == false + slice.locked { slice.locked?.should == true } + end + end + + it "does not propagate backwards from buffer's slices" do + @buffer = IO::Buffer.new(4) + slice = @buffer.slice(0, 2) + slice.locked do + slice.locked?.should == true + @buffer.locked?.should == false + @buffer.locked { @buffer.locked?.should == true } + end end end - it "does not propagate backwards from buffer's slices" do - @buffer = IO::Buffer.new(4) - slice = @buffer.slice(0, 2) - slice.locked do - slice.locked?.should == true + ruby_version_is "4.1" do + it "allows nested locking" do + @buffer = IO::Buffer.new(4) + @buffer.locked do + @buffer.locked do + @buffer.locked?.should == true + end + @buffer.locked?.should == true + end @buffer.locked?.should == false - @buffer.locked { @buffer.locked?.should == true } + end + + it "propagates to buffer's slices" do + @buffer = IO::Buffer.new(4) + slice = @buffer.slice(0, 2) + @buffer.locked do + @buffer.locked?.should == true + slice.locked?.should == true + slice.locked do + slice.locked?.should == true + end + @buffer.locked?.should == true + end + end + + it "propagates backwards from buffer's slices" do + @buffer = IO::Buffer.new(4) + slice = @buffer.slice(0, 2) + slice.locked do + slice.locked?.should == true + @buffer.locked?.should == true + @buffer.locked do + @buffer.locked?.should == true + end + slice.locked?.should == true + end + end + + it "shares locks with string-backed slices" do + @buffer = IO::Buffer.for("test") + slice = @buffer.slice(0, 2) + + slice.locked do + @buffer.locked?.should == true + -> { @buffer.free }.should.raise(IO::Buffer::LockedError) + end + end + + it "validates a slice before locking its source" do + @buffer = IO::Buffer.new(4) + slice = @buffer.slice + @buffer.free + + yielded = false + + -> { + slice.locked do + yielded = true + end + }.should.raise(IO::Buffer::InvalidatedError) + + yielded.should == false + slice.locked?.should == false end end end diff --git a/spec/ruby/core/io/buffer/valid_spec.rb b/spec/ruby/core/io/buffer/valid_spec.rb index b84bdd0cfd942e..e4be10645b6c0b 100644 --- a/spec/ruby/core/io/buffer/valid_spec.rb +++ b/spec/ruby/core/io/buffer/valid_spec.rb @@ -83,12 +83,24 @@ end end - it "is true for a slice of a freed string-backed buffer while string is alive" do - @buffer = IO::Buffer.for("alive") - slice = @buffer.slice(0, 2) - slice.valid?.should == true - @buffer.free - slice.valid?.should == true + ruby_version_is ""..."4.1" do + it "is true for a slice of a freed string-backed buffer while string is alive" do + @buffer = IO::Buffer.for("alive") + slice = @buffer.slice(0, 2) + slice.valid?.should == true + @buffer.free + slice.valid?.should == true + end + end + + ruby_version_is "4.1" do + it "is false for a slice of a freed string-backed buffer" do + @buffer = IO::Buffer.for("alive") + slice = @buffer.slice(0, 2) + slice.valid?.should == true + @buffer.free + slice.valid?.should == false + end end # There probably should be a test with a garbage-collected string, diff --git a/test/ruby/test_io_buffer.rb b/test/ruby/test_io_buffer.rb index 0917f1517ac485..2c0135b970a7bd 100644 --- a/test/ruby/test_io_buffer.rb +++ b/test/ruby/test_io_buffer.rb @@ -2,6 +2,7 @@ require 'tempfile' require 'rbconfig/sizeof' +require 'io/nonblock' require '-test-/io_buffer' class TestIOBuffer < Test::Unit::TestCase @@ -26,7 +27,6 @@ def test_flags assert_equal 2, IO::Buffer::INTERNAL assert_equal 4, IO::Buffer::MAPPED - assert_equal 32, IO::Buffer::LOCKED assert_equal 64, IO::Buffer::PRIVATE assert_equal 128, IO::Buffer::READONLY @@ -309,6 +309,30 @@ def test_resize_zero_internal assert_equal 1, buffer.size end + def test_resize_zero_mapped + buffer = IO::Buffer.new(IO::Buffer::PAGE_SIZE) + assert_predicate buffer, :mapped? + + buffer.resize(0) + assert_predicate buffer, :null? + assert_equal "", buffer.get_string + + buffer.resize(1) + assert_equal 1, buffer.size + end + + def test_resize_zero_slice + buffer = IO::Buffer.new(64) + slice = buffer.slice(0, 8) + + slice.resize(0) + assert_predicate slice, :null? + assert_equal 64, buffer.size + + slice.resize(1) + assert_equal 1, slice.size + end + def test_resize_zero_external buffer = IO::Buffer.for('1') @@ -396,6 +420,33 @@ def test_slice_readonly assert_equal "Hello World", hello end + def test_string_backed_slice_is_invalidated_when_root_is_freed + buffer = IO::Buffer.for("Hello World") + slice = buffer.slice(0, 5) + + assert_predicate slice, :valid? + buffer.free + refute_predicate slice, :valid? + assert_raise(IO::Buffer::InvalidatedError) {slice.get_string} + ensure + slice&.free unless slice&.null? + buffer&.free unless buffer&.null? + end + + def test_string_backed_slice_escaping_block_is_invalidated + slice = nil + + IO::Buffer.for(+"Hello World") do |buffer| + slice = buffer.slice(0, 5) + assert_predicate slice, :valid? + end + + refute_predicate slice, :valid? + assert_raise(IO::Buffer::InvalidatedError) {slice.get_string} + ensure + slice&.free unless slice&.null? + end + def test_transfer hello = %w"Hello World".join(" ") buffer = IO::Buffer.for(hello) @@ -421,20 +472,102 @@ def test_transfer_in_block assert_equal "Ciao! World", hello end - def test_locked - buffer = IO::Buffer.new(128, IO::Buffer::INTERNAL|IO::Buffer::LOCKED) + def test_counted_locking + buffer = IO::Buffer.new(128) - assert_raise IO::Buffer::LockedError do - buffer.resize(256) - end + Bug::IOBuffer.lock(buffer) + Bug::IOBuffer.lock(buffer) - assert_equal 128, buffer.size + assert_predicate buffer, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.free} - assert_raise IO::Buffer::LockedError do - buffer.free + Bug::IOBuffer.unlock(buffer) + + assert_predicate buffer, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.resize(256)} + + Bug::IOBuffer.unlock(buffer) + + refute_predicate buffer, :locked? + buffer.resize(256) + assert_equal 256, buffer.size + ensure + Bug::IOBuffer.unlock(buffer) while buffer&.locked? + buffer&.free + end + + def test_new_locked + buffer = Bug::IOBuffer.new_locked(128) + + assert_predicate buffer, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.free} + + Bug::IOBuffer.free_locked(buffer) + + refute_predicate buffer, :locked? + assert_predicate buffer, :null? + ensure + Bug::IOBuffer.free_locked(buffer) if buffer&.locked? + buffer&.free unless buffer&.null? + end + + def test_slice_and_root_share_lock_count + buffer = IO::Buffer.new(128) + slice = buffer.slice(0, 64) + + Bug::IOBuffer.lock(buffer) + Bug::IOBuffer.lock(slice) + + Bug::IOBuffer.unlock(buffer) + + assert_predicate buffer, :locked? + assert_predicate slice, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.free} + + Bug::IOBuffer.unlock(slice) + + refute_predicate buffer, :locked? + refute_predicate slice, :locked? + buffer.free + refute_predicate slice, :valid? + ensure + Bug::IOBuffer.unlock(buffer) while buffer&.locked? + slice&.free unless slice&.null? + buffer&.free unless buffer&.null? + end + + def test_invalid_slice_does_not_lock_source + buffer = IO::Buffer.new(128) + slice = buffer.slice(0, 64) + + buffer.free + + yielded = false + assert_raise(IO::Buffer::InvalidatedError) do + slice.locked do + yielded = true + end end - assert_equal 128, buffer.size + refute yielded + refute_predicate buffer, :locked? + refute_predicate slice, :locked? + ensure + slice&.free unless slice&.null? + buffer&.free unless buffer&.null? + end + + def test_string_backed_slice_shares_root_lock + buffer = IO::Buffer.for("test") + slice = buffer.slice(0, 2) + + slice.locked do + assert_predicate buffer, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.free} + end + ensure + slice&.free unless slice&.null? + buffer&.free unless buffer&.null? end def test_get_string @@ -596,6 +729,39 @@ def test_each end end + def test_each_locks_backing_allocation + buffer = IO::Buffer.for("Hello World").dup + + assert_raise(IO::Buffer::LockedError) do + buffer.each(:U8) do + buffer.free + end + end + + assert_equal "Hello World", buffer.get_string + refute_predicate buffer, :locked? + ensure + buffer&.free + end + + def test_each_on_slice_locks_root_allocation + buffer = IO::Buffer.for("Hello World").dup + slice = buffer.slice + + assert_raise(IO::Buffer::LockedError) do + slice.each(:U8) do + buffer.resize(32) + end + end + + assert_equal "Hello World", slice.get_string + refute_predicate buffer, :locked? + refute_predicate slice, :locked? + ensure + slice&.free + buffer&.free + end + def test_zero_length_each buffer = IO::Buffer.new(0) @@ -610,6 +776,21 @@ def test_each_byte assert_equal string.bytes[3, 5], buffer.each_byte(3, 5).to_a end + def test_each_byte_locks_backing_allocation + buffer = IO::Buffer.for("Hello World").dup + + assert_raise(IO::Buffer::LockedError) do + buffer.each_byte do + buffer.transfer + end + end + + assert_equal "Hello World", buffer.get_string + refute_predicate buffer, :locked? + ensure + buffer&.free + end + def test_each_byte_bounds_error buffer = IO::Buffer.for("A") @@ -669,6 +850,42 @@ def test_invalidation input.close end + def test_overlapping_reads_retain_independent_locks + input1, output1 = IO.pipe + input2, output2 = IO.pipe + input1.nonblock = false + input2.nonblock = false + buffer = IO::Buffer.new(2) + + thread1 = Thread.new {buffer.read(input1, 1, 0)} + thread2 = Thread.new {buffer.read(input2, 1, 1)} + + Thread.pass until thread1.stop? && thread2.stop? + + assert_predicate buffer, :locked? + + output1.write("A") + assert_equal 1, thread1.value + + assert_predicate thread2, :alive? + assert_predicate buffer, :locked? + assert_raise(IO::Buffer::LockedError) {buffer.free} + + output2.write("B") + assert_equal 1, thread2.value + + refute_predicate buffer, :locked? + assert_equal "AB", buffer.get_string + ensure + thread1&.kill&.join + thread2&.kill&.join + input1&.close + output1&.close + input2&.close + output2&.close + buffer&.free unless buffer&.null? + end + def hello_world_tempfile(repeats = 1) io = Tempfile.new repeats.times do diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 2922e9002b8524..c9d76288204939 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -767,7 +767,11 @@ pub enum SendFallbackReason { SendBlockArgNotNil, CCallWithFrameTooManyArgs, ObjToStringNotString, + /// Too many arguments in a C call to fit in C ABI registers. TooManyArgsForLir, + /// An operand doesn't fit in the integer type that encodes it, + /// e.g. an argument count that overflows IseqCall's u16. + OperandTooLarge, /// The Proc object for a BMETHOD is not defined by an ISEQ. (See `enum rb_block_type`.) BmethodNonIseqProc, /// Caller supplies too few or too many arguments than what the callee's parameters expects. @@ -838,6 +842,7 @@ impl Display for SendFallbackReason { CCallWithFrameTooManyArgs => write!(f, "CCallWithFrame: too many arguments"), ObjToStringNotString => write!(f, "ObjToString: result is not a string"), TooManyArgsForLir => write!(f, "Too many arguments for LIR"), + OperandTooLarge => write!(f, "Operand doesn't fit in its encoding"), BmethodNonIseqProc => write!(f, "Bmethod: Proc object is not defined by an ISEQ"), ArgcParamMismatch => write!(f, "Argument count does not match parameter count"), ComplexArgPass => write!(f, "Complex argument passing"), @@ -2702,7 +2707,7 @@ fn can_direct_send(iseq: *const rb_iseq_t, ci: *const rb_callinfo, args: &[InsnI // IseqCall stores the JIT entry index and argc as u16. if u16::try_from(send_argc).is_err() { - return Err(SendDirectFailure::new(TooManyArgsForLir)); + return Err(SendDirectFailure::new(OperandTooLarge)); } Ok(()) @@ -4041,7 +4046,7 @@ impl Function { // rest packing changes the SendDirect argument count. // See: vm_args.c's setup_parameters_complex and args_setup_opt_parameters. let passed_opt_num = (positional_argc - min_positional_argc).min(opt_num); - let jit_entry_idx = passed_opt_num.try_into().map_err(|_| TooManyArgsForLir)?; + let jit_entry_idx = passed_opt_num.try_into().map_err(|_| OperandTooLarge)?; // Methods without *rest still need the jit_entry_idx computed above, // but their positional args do not need repacking. @@ -4728,7 +4733,7 @@ impl Function { { let native_index = (index as i64) * (SIZEOF_VALUE as i64); if native_index > (i32::MAX as i64) { - self.set_dynamic_send_reason(insn_id, TooManyArgsForLir); + self.set_dynamic_send_reason(insn_id, OperandTooLarge); self.push_insn_id(block, insn_id); continue; } } diff --git a/zjit/src/stats.rs b/zjit/src/stats.rs index 8601c56ac10d89..57774aa4d7db21 100644 --- a/zjit/src/stats.rs +++ b/zjit/src/stats.rs @@ -258,6 +258,7 @@ make_counters! { send_fallback_send_cfunc_not_variadic, send_fallback_send_not_optimized_method_type_optimized, send_fallback_too_many_args_for_lir, + send_fallback_operand_too_large, send_fallback_send_bop_redefined, send_fallback_send_operands_not_fixnum, send_fallback_send_polymorphic_fallback, @@ -682,6 +683,7 @@ pub fn send_fallback_counter(reason: crate::hir::SendFallbackReason) -> Counter SendNotOptimizedMethodTypeOptimized(_) => send_fallback_send_not_optimized_method_type_optimized, TooManyArgsForLir => send_fallback_too_many_args_for_lir, + OperandTooLarge => send_fallback_operand_too_large, SendBopRedefined => send_fallback_send_bop_redefined, SendOperandsNotFixnum => send_fallback_send_operands_not_fixnum, SendPolymorphicFallback => send_fallback_send_polymorphic_fallback,