diff options
54 files changed, 1580 insertions, 321 deletions
diff --git a/.github/workflows/zjit-macos.yml b/.github/workflows/zjit-macos.yml index 3340ba08a7..4921d9ef70 100644 --- a/.github/workflows/zjit-macos.yml +++ b/.github/workflows/zjit-macos.yml @@ -93,7 +93,7 @@ jobs: rustup install ${{ matrix.rust_version }} --profile minimal rustup default ${{ matrix.rust_version }} - - uses: taiki-e/install-action@7a79fe8c3a13344501c80d99cae481c1c9085912 # v2.81.10 + - uses: taiki-e/install-action@15449e3094499af05d8d964a1c884208e4b8b595 # v2.81.11 with: tool: nextest@0.9 if: ${{ matrix.test_task == 'zjit-check' }} diff --git a/.github/workflows/zjit-ubuntu.yml b/.github/workflows/zjit-ubuntu.yml index 8fd1ef9b7a..30c6138466 100644 --- a/.github/workflows/zjit-ubuntu.yml +++ b/.github/workflows/zjit-ubuntu.yml @@ -125,7 +125,7 @@ jobs: ruby-version: '3.1' bundler: none - - uses: taiki-e/install-action@7a79fe8c3a13344501c80d99cae481c1c9085912 # v2.81.10 + - uses: taiki-e/install-action@15449e3094499af05d8d964a1c884208e4b8b595 # v2.81.11 with: tool: nextest@0.9 if: ${{ matrix.test_task == 'zjit-check' }} diff --git a/doc/file/timestamps.md b/doc/file/timestamps.md index c8ad616567..60934edb05 100644 --- a/doc/file/timestamps.md +++ b/doc/file/timestamps.md @@ -51,6 +51,13 @@ Each of these methods returns the modification time for an entry as a Time objec - File::Stat#mtime. - Pathname#mtime. +The modification time (along with the access time) may also be updated explicitly: + +- File::lutime. +- File::utime. +- Pathname#lutime. +- Pathname#utime. + ## Access \Time The access time for an entry is the time the entry last read. @@ -64,6 +71,13 @@ Each of these methods returns the access time for an entry as a Time object: - File::Stat#atime. - Pathname#atime. +The access time (along with the modification time) may also be updated explicitly: + +- File::lutime. +- File::utime. +- Pathname#lutime. +- Pathname#utime. + ## Metadata-Change \Time The metadata-change time for an entry is the time the entry last read. diff --git a/doc/string/split.rdoc b/doc/string/split.rdoc index 8679149003..dc6292d182 100644 --- a/doc/string/split.rdoc +++ b/doc/string/split.rdoc @@ -68,7 +68,7 @@ and trailing empty strings are included: When +limit+ is negative, there is no limit on the size of the array, -and trailing empty strings are omitted: +and trailing empty strings are included: 'abracadabra'.split('', -1) # => ["a", "b", "r", "a", "c", "a", "d", "a", "b", "r", "a", ""] 'abracadabra'.split('a', -1) # => ["", "br", "c", "d", "br", ""] @@ -3374,14 +3374,55 @@ rb_file_s_utime(int argc, VALUE *argv, VALUE _) #if defined(HAVE_UTIMES) && (defined(HAVE_LUTIMES) || (defined(HAVE_UTIMENSAT) && defined(AT_SYMLINK_NOFOLLOW))) /* + * :markup: markdown + * * call-seq: - * File.lutime(atime, mtime, file_name, ...) -> integer + * File.lutime(atime, mtime, *paths) -> path_count + * + * Like File#utime, but does not follow symbolic links, + * and therefore changes the times of the entries given by `paths`, + * regardless of whether they are symbolic links; + * returns the number of `paths` given: + * + * ```ruby + * # Create a file and a link to it. + * file_path = 't.tmp' + * File.write(file_path, '') + * link_path = 'link' + * File.symlink(file_path, link_path) + * # Take snapshots of both. + * file_stat = File.stat(file_path) + * link_stat = File.lstat(link_path) + * # Fetch access times and modification times of both. + * file_stat.atime # => 2026-06-15 10:45:11.376753268 -0500 + * file_stat.mtime # => 2026-06-15 10:44:47.335854904 -0500 + * link_stat.atime # => 2026-06-15 10:44:59.788801128 -0500 + * link_stat.mtime # => 2026-06-15 10:44:49.367845961 -0500 + * # Update access time and modification time of the link. + * time = Time.now # => 2026-06-15 10:48:57.847422496 -0500 + * File.lutime(time, time, link_path) + * # Take fresh snapshots of both. + * file_stat = File.stat(file_path) + * link_stat = File.lstat(link_path) + * # Fetch access time and modification time of file (not changed). + * file_stat.atime # => 2026-06-15 10:45:11.376753268 -0500 + * file_stat.mtime # => 2026-06-15 10:44:47.335854904 -0500 + * # Fetch access time and modification time of link (changed). + * link_stat.atime # => 2026-06-15 10:49:27.119146136 -0500 + * link_stat.mtime # => 2026-06-15 10:48:57.847422496 -0500 + * # Clean up. + * File.delete(file_path) + * File.delete(link_path) + * ``` + * + * Arguments `atime` and `mtime` may be Time objects (as above). + * + * Either or both may be integers; + * when an integer `i` is passed, `Time.new(i)` is used. + * + * Either or both may be `nil`, in which case `Time.now` is used. * - * Sets the access and modification times of each named file to the - * first two arguments. If a file is a symlink, this method acts upon - * the link itself as opposed to its referent; for the inverse - * behavior, see File.utime. Returns the number of file - * names in the argument list. + * See {File System Timestamps}[rdoc-ref:file/timestamps.md]. */ static VALUE @@ -3427,15 +3468,28 @@ syserr_fail2_in(const char *func, int e, VALUE s1, VALUE s2) #ifdef HAVE_LINK /* + * :markup: markdown + * call-seq: - * File.link(old_name, new_name) -> 0 + * File.link(path, new_path) -> 0 + * + * Not available on some systems. + * + * Creates a new entry at `new_path` for the existing entry at `path` + * using a [hard link](https://en.wikipedia.org/wiki/Hard_link): * - * Creates a new name for an existing file using a hard link. Will not - * overwrite <i>new_name</i> if it already exists (raising a subclass - * of SystemCallError). Not available on all platforms. + * ```ruby + * File.write('doc/t.tmp', 'foo') + * File.link('doc/t.tmp', 'lib/u.tmp') + * File.read('lib/u.tmp') # => "foo" + * File.write('lib/u.tmp', 'bar') + * File.read('doc/t.tmp') # => "bar" + * File.delete('doc/t.tmp') + * File.read('lib/u.tmp') # => "bar" + * File.delete('lib/u.tmp') + * ``` * - * File.link("testfile", ".testfile") #=> 0 - * IO.readlines(".testfile")[0] #=> "This is line one\n" + * Raises an exception if the entry at `new_path` exists. */ static VALUE diff --git a/lib/bundled_gems.rb b/lib/bundled_gems.rb index e6b643678b..fa13f9aa09 100644 --- a/lib/bundled_gems.rb +++ b/lib/bundled_gems.rb @@ -46,6 +46,14 @@ module Gem::BUNDLED_GEMS # :nodoc: DLEXT = /\.#{Regexp.union(dlext)}\z/ LIBEXT = /\.#{Regexp.union("rb", *dlext)}\z/ + # Decorate Kernel#require so that requiring a gem that is no longer a default + # gem emits a warning. Called by Bundler during setup + # (lib/bundler/rubygems_integration.rb), guarded by respond_to?(:replace_require) + # because older Ruby versions ship without this file. + # + # The decorating method is defined via define_method, so its base_label is + # "replace_require" rather than "require"; this is why uplevel has to recognize + # both labels (build_message instead skips our own frames by file path). def self.replace_require(specs) return if [::Kernel.singleton_class, ::Kernel].any? {|klass| klass.respond_to?(:no_warning_require) } @@ -67,20 +75,28 @@ module Gem::BUNDLED_GEMS # :nodoc: end end + # base_labels that belong to the require machinery: "replace_require" is the + # define_method block installed by replace_require, "require" covers both + # Kernel#require and the wrappers that Bootsnap and Zeitwerk install on top. + REQUIRE_LABELS = ["replace_require", "require"].freeze + + # Compute the uplevel: argument for Kernel#warn so the warning points at the + # user's require call site rather than at our machinery. We walk down the run + # of require frames (there can be several when Bootsnap/Zeitwerk are present) + # and stop at the first frame past them. def self.uplevel frame_count = 0 - require_labels = ["replace_require", "require"] uplevel = 0 require_found = false Thread.each_caller_location do |cl| frame_count += 1 if require_found - unless require_labels.include?(cl.base_label) + unless REQUIRE_LABELS.include?(cl.base_label) return uplevel end else - if require_labels.include?(cl.base_label) + if REQUIRE_LABELS.include?(cl.base_label) require_found = true end end @@ -93,35 +109,53 @@ module Gem::BUNDLED_GEMS # :nodoc: require_found ? 1 : (frame_count - 1).nonzero? end - def self.warning?(name, specs: nil) - # name can be a feature name or a file path with String or Pathname + # Resolve a require target to the bundled gem it belongs to. + # + # +name+ is a feature name or a file path (String or Pathname). Returns + # +[feature, gem_name, subfeature]+ where +feature+ is the normalized feature + # string (extension and bootsnap-expanded LIBDIR/ARCHDIR prefix removed), + # +gem_name+ is the matching SINCE key, and +subfeature+ is true when the + # require targets a nested path (e.g. "benchmark/ips") rather than the gem's + # top-level feature. Returns nil when the target is not a bundled gem. + # + # This is a deliberately cheap check: the SINCE membership test here excludes + # the vast majority of candidates before the costly checks in warning? + # (see [Bug #20641]). + def self.find_gem(name) feature = File.path(name).sub(LIBEXT, "") - # The actual checks needed to properly identify the gem being required - # are costly (see [Bug #20641]), so we first do a much cheaper check - # to exclude the vast majority of candidates. - subfeature = if feature.include?("/") + if feature.include?("/") # bootsnap expands `require "csv"` to `require "#{LIBDIR}/csv.rb"`, - # and `require "syslog"` to `require "#{ARCHDIR}/syslog.so"`. + # and `require "syslog"` to `require "#{ARCHDIR}/syslog.so"`, so strip + # those prefixes back off before matching. [Bug #20450] feature.delete_prefix!(ARCHDIR) feature.delete_prefix!(LIBDIR) # 1. A segment for the EXACT mapping and SINCE check # 2. A segment for the SINCE check for dashed names # 3. A segment to check if there's a subfeature segments = feature.split("/", 3) - name = segments.shift - name = EXACT[name] || name - if !SINCE[name] - name = "#{name}-#{segments.shift}" - return unless SINCE[name] + gem = segments.shift + gem = EXACT[gem] || gem + if !SINCE[gem] + gem = "#{gem}-#{segments.shift}" + return unless SINCE[gem] end - segments.any? + [feature, gem, segments.any?] else - name = EXACT[feature] || feature - return unless SINCE[name] - false + gem = EXACT[feature] || feature + return unless SINCE[gem] + [feature, gem, false] end + end + + def self.warning?(name, specs: nil) + # name can be a feature name or a file path with String or Pathname + feature, name, subfeature = find_gem(name) + return unless feature + # Callers can suppress warnings for specific gems/features for the duration + # of a block via this thread-local. prelude.rb uses it so that `binding.irb` + # can load irb (which pulls in reline and rdoc) without warning. [Bug #21723] if suppress_list = Thread.current[:__bundled_gems_warning_suppression] return if suppress_list.include?(name) || suppress_list.include?(feature) end @@ -202,6 +236,13 @@ module Gem::BUNDLED_GEMS # :nodoc: msg end + # Activate +gem+ even when it is not declared in the current Gemfile, by + # building and setting up a temporary Bundler definition. + # + # Unlike the rest of this file, this neither detects nor warns about bundled + # gems. It exists so that +binding.irb+ (see prelude.rb) and + # <tt>bundle console</tt> (lib/bundler/cli/console.rb) can load irb even when + # it is not declared. Only usable when Bundler is available. def self.force_activate(gem) require "bundler" Bundler.reset! diff --git a/lib/bundler/cli/outdated.rb b/lib/bundler/cli/outdated.rb index 465e56ada2..e5c9273383 100644 --- a/lib/bundler/cli/outdated.rb +++ b/lib/bundler/cli/outdated.rb @@ -77,7 +77,7 @@ module Bundler gemfile_specs + dependency_specs end - specs.sort_by(&:name).uniq(&:name).each do |current_spec| + specs_for_outdated_check(specs).each do |current_spec| next unless gems.empty? || gems.include?(current_spec.name) active_spec = retrieve_active_spec(definition, current_spec) @@ -152,6 +152,12 @@ module Bundler end end + def specs_for_outdated_check(specs) + specs.group_by(&:name).values.filter_map do |matching_specs| + MatchPlatform.select_best_platform_match(matching_specs, Bundler.local_platform).first || matching_specs.first + end.sort_by(&:name) + end + def retrieve_active_spec(definition, current_spec) active_spec = definition.resolve.find_by_name_and_platform(current_spec.name, current_spec.platform) return unless active_spec diff --git a/lib/bundler/lazy_specification.rb b/lib/bundler/lazy_specification.rb index 0da621d21f..86b2da4516 100644 --- a/lib/bundler/lazy_specification.rb +++ b/lib/bundler/lazy_specification.rb @@ -9,7 +9,8 @@ module Bundler include ForcePlatform attr_reader :name, :version, :platform, :materialization - attr_accessor :source, :remote, :force_ruby_platform, :dependencies, :required_ruby_version, :required_rubygems_version, :overrides + attr_accessor :source, :remote, :force_ruby_platform, :dependencies, :required_ruby_version, :required_rubygems_version + attr_accessor :overrides, :locked_platforms # # For backwards compatibility with existing lockfiles, if the most specific @@ -49,6 +50,7 @@ module Bundler @force_ruby_platform = default_force_ruby_platform @most_specific_locked_platform = nil @materialization = nil + @locked_platforms = nil end def missing? @@ -145,7 +147,7 @@ module Bundler # Exact spec is incompatible; in frozen mode, try to find a compatible platform variant # In non-frozen mode, return nil to trigger re-resolution and lockfile update if Bundler.frozen_bundle? - materialize([name, version]) {|specs| resolve_best_platform(specs) } + materialize([name, version]) {|specs| resolve_best_platform(specs, locked_platforms_only: true) } end else materialize([name, version]) {|specs| resolve_best_platform(specs) } @@ -186,12 +188,12 @@ module Bundler # Try platforms in order of preference until finding a compatible spec. # Used for legacy lockfiles and as a fallback when the exact locked spec # is incompatible. Falls back to frozen bundle behavior if none match. - def resolve_best_platform(specs) - find_compatible_platform_spec(specs) || frozen_bundle_fallback(specs) + def resolve_best_platform(specs, locked_platforms_only: false) + find_compatible_platform_spec(specs, locked_platforms_only: locked_platforms_only) || frozen_bundle_fallback(specs) end - def find_compatible_platform_spec(specs) - candidate_platforms.each do |plat| + def find_compatible_platform_spec(specs, locked_platforms_only: false) + candidate_platforms(locked_platforms_only: locked_platforms_only).each do |plat| candidates = MatchPlatform.select_best_platform_match(specs, plat) spec = choose_compatible(candidates, fallback_to_non_installable: false) return spec if spec @@ -201,9 +203,12 @@ module Bundler # Platforms to try in order of preference. Ruby platform is last since it # requires compilation, but works when precompiled gems are incompatible. - def candidate_platforms + def candidate_platforms(locked_platforms_only: false) target = source.is_a?(Source::Path) ? platform : Bundler.local_platform - [target, platform, Gem::Platform::RUBY].uniq + platforms = [target, platform, Gem::Platform::RUBY].uniq + return platforms unless locked_platforms_only && locked_platforms + + platforms & locked_platforms end # In frozen mode, accept any candidate. Will error at install time. diff --git a/lib/bundler/materialization.rb b/lib/bundler/materialization.rb index 82e48464a7..d73e9124a8 100644 --- a/lib/bundler/materialization.rb +++ b/lib/bundler/materialization.rb @@ -12,6 +12,7 @@ module Bundler @dep = dep @platform = platform @candidates = candidates + set_locked_platforms end def complete? @@ -55,5 +56,14 @@ module Bundler private attr_reader :dep, :platform + + def set_locked_platforms + return unless @candidates + + platforms = @candidates.map(&:platform) + @candidates.each do |candidate| + candidate.locked_platforms = platforms if candidate.respond_to?(:locked_platforms=) + end + end end end diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index 7dac202042..6c5331495b 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -305,11 +305,15 @@ module Bundler next groups if package.force_ruby_platform? end - platform_group = Resolver::SpecGroup.new(platform_specs.flatten.uniq) + platform_specs = platform_specs.flatten.uniq + platform_group = Resolver::SpecGroup.new((platform_specs + ruby_specs).uniq) next groups if platform_group == ruby_group groups << Resolver::Candidate.new(version, group: platform_group, priority: 1) + platform_only_group = Resolver::SpecGroup.new(platform_specs) + groups << Resolver::Candidate.new(version, group: platform_only_group, priority: 0) unless platform_only_group == platform_group + groups end end diff --git a/lib/prism/translation/parser/lexer.rb b/lib/prism/translation/parser/lexer.rb index e82042867f..dadc53d38e 100644 --- a/lib/prism/translation/parser/lexer.rb +++ b/lib/prism/translation/parser/lexer.rb @@ -192,17 +192,25 @@ module Prism EXPR_BEG = 0x1 EXPR_LABEL = 0x400 - # It is used to determine whether `do` is of the token type `kDO` or `kDO_LAMBDA`. + # It is used to determine whether `do` is of the token type `kDO` or + # `kDO_LAMBDA`. # - # NOTE: In edge cases like `-> (foo = -> (bar) {}) do end`, please note that `kDO` is still returned - # instead of `kDO_LAMBDA`, which is expected: https://github.com/ruby/prism/pull/3046 + # NOTE: In edge cases like `-> (foo = -> (bar) {}) do end`, please note + # that `kDO` is still returned instead of `kDO_LAMBDA`, which is + # expected: https://github.com/ruby/prism/pull/3046 LAMBDA_TOKEN_TYPES = Set.new([:kDO_LAMBDA, :tLAMBDA, :tLAMBEG]) - # The `PARENTHESIS_LEFT` token in Prism is classified as either `tLPAREN` or `tLPAREN2` in the Parser gem. - # The following token types are listed as those classified as `tLPAREN`. + # The `PARENTHESIS_LEFT` token in Prism is classified as either + # `tLPAREN` or `tLPAREN2` in the Parser gem. The following token types + # are listed as those classified as `tLPAREN`. LPAREN_CONVERSION_TOKEN_TYPES = Set.new([ - :kBREAK, :tCARET, :kCASE, :tDIVIDE, :kFOR, :kIF, :kNEXT, :kRETURN, :kUNTIL, :kWHILE, :tAMPER, :tANDOP, :tBANG, :tCOMMA, :tDOT2, :tDOT3, - :tEQL, :tLPAREN, :tLPAREN2, :tLPAREN_ARG, :tLSHFT, :tNL, :tOP_ASGN, :tOROP, :tPIPE, :tSEMI, :tSTRING_DBEG, :tUMINUS, :tUPLUS, :tLCURLY + :kAND, :kBEGIN, :kBREAK, :kCASE, :kDO_COND, :kDO_LAMBDA, :kDO, :kELSE, + :kELSIF, :kENSURE, :kFOR, :kIF_MOD, :kIF, :kIN, :kNEXT, :kOR, + :kRESCUE_MOD, :kRESCUE, :kRETURN, :kTHEN, :kUNLESS_MOD, :kUNLESS, + :kUNTIL_MOD, :kUNTIL, :kWHEN, :kWHILE_MOD, :kWHILE, + :tAMPER, :tANDOP, :tBANG, :tCARET, :tCOMMA, :tDIVIDE, :tDOT2, :tDOT3, + :tEQL, :tLCURLY, :tLPAREN_ARG, :tLPAREN, :tLPAREN2, :tLSHFT, :tNL, + :tOP_ASGN, :tOROP, :tPIPE, :tSEMI, :tSTRING_DBEG, :tUMINUS, :tUPLUS ]) # Types of tokens that are allowed to continue a method call with comments in-between. diff --git a/pathname_builtin.rb b/pathname_builtin.rb index a9e5ead00c..60ae6d3412 100644 --- a/pathname_builtin.rb +++ b/pathname_builtin.rb @@ -1436,7 +1436,28 @@ class Pathname # * File * # Returns <tt>'unknown'</tt> if the type cannot be determined. def ftype() File.ftype(@path) end - # See <tt>File.link</tt>. Creates a hard link. + # :markup: markdown + # + # call-seq: + # make_link(path) -> 0 + # + # Not available on some systems. + # + # Creates a new entry at the path in `self` for the existing entry at `path` + # using a [hard link](https://en.wikipedia.org/wiki/Hard_link): + # + # ```ruby + # File.write('doc/t.tmp', 'foo') + # Pathname('lib/u.tmp').make_link('doc/t.tmp') + # File.read('lib/u.tmp') # => "foo" + # File.write('lib/u.tmp', 'bar') + # File.read('doc/t.tmp') # => "bar" + # File.delete('doc/t.tmp') + # File.read('lib/u.tmp') # => "bar" + # File.delete('lib/u.tmp') + # ``` + # + # Raises an exception if the entry at the path in `self` exists. def make_link(old) File.link(old, @path) end # See <tt>File.open</tt>. Opens the file for reading or writing. @@ -1489,11 +1510,56 @@ class Pathname # * File * # See <tt>File.utime</tt>. Update the access and modification times. def utime(atime, mtime) File.utime(atime, mtime, @path) end - # Update the access and modification times of the file. + # :markup: markdown + # + # call-seq: + # lutime(atime, mtime) -> 1 + # + # Like Pathname#utime, but does not follow symbolic links, + # and therefore changes the times of the entry in `self`, + # regardless of whether it is a symbolic link: + # + # ```ruby + # # Create a file and a link to it. + # file_path = 't.tmp' + # link_path = 'link' + # File.write(file_path, '') + # File.symlink(file_path, link_path) + # # Take snapshots of both. + # file_stat = File.stat(file_path) + # link_stat = File.lstat(link_path) + # # Fetch access times and modification times of both. + # file_stat.atime # => 2026-06-15 11:03:29.600373255 -0500 + # file_stat.mtime # => 2026-06-15 11:03:22.247352211 -0500 + # link_stat.atime # => 2026-06-15 11:03:29.251372254 -0500 + # link_stat.mtime # => 2026-06-15 11:03:26.66436484 -0500 + # # Update access time and modification time of the link. + # pn = Pathname(link_path) + # time = Time.now # => 2026-06-15 11:08:07.384287523 -0500 + # pn.lutime(time, time) + # # Take fresh snapshots of both. + # file_stat = File.stat(file_path) + # link_stat = File.lstat(link_path) + # # Fetch access time and modification time of file (not changed). + # file_stat.atime # => 2026-06-15 11:03:29.600373255 -0500 + # file_stat.mtime # => 2026-06-15 11:03:22.247352211 -0500 + # # Fetch access time and modification time of link (changed). + # link_stat.atime # => 2026-06-15 11:08:29.847301399 -0500 + # link_stat.mtime # => 2026-06-15 11:08:07.384287523 -0500 + # # Clean up. + # File.delete(file_path) + # File.delete(link_path) + # ``` + # + # Arguments `atime` and `mtime` may be Time objects (as above). + # + # Either or both may be integers; + # when an integer `i` is passed, `Time.new(i)` is used. + # + # Either or both may be `nil`, in which case `Time.now` is used. # - # Same as Pathname#utime, but does not follow symbolic links. + # See {File System Timestamps}[rdoc-ref:file/timestamps.md]. # - # See File.lutime. def lutime(atime, mtime) File.lutime(atime, mtime, @path) end # call-seq: diff --git a/prism/prism.c b/prism/prism.c index 36a6028fe5..1a2cc64596 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -12736,6 +12736,17 @@ expect1_opening(pm_parser_t *parser, pm_token_type_t type, pm_diagnostic_id_t di #define PM_PARSE_ACCEPTS_DO_BLOCK ((uint8_t) 0x4) #define PM_PARSE_IN_ENDLESS_DEF ((uint8_t) 0x8) +/** + * Indicates that we are parsing a statement even though the binding power is + * below PM_BINDING_POWER_STATEMENT. Only used for the value of a `rescue` + * modifier, whose CRuby grammar is a `stmt` in statement, multiple-assignment, + * and command-call contexts. Permits statement-only constructs that the + * binding power would otherwise reject: a multiple-value right-hand side + * (`b = c, d`), a leading splat value (`b = *c`), a parenthesized target list + * (`(b, c), d = 1`), and `alias`/`undef`/`END {}`. + */ +#define PM_PARSE_ACCEPTS_STATEMENT ((uint8_t) 0x10) + static pm_node_t * parse_expression(pm_parser_t *parser, pm_binding_power_t binding_power, uint8_t flags, pm_diagnostic_id_t diag_id, uint16_t depth); @@ -18857,7 +18868,7 @@ parse_symbol_array(pm_parser_t *parser, uint16_t depth) { * assignment, or a set of statements. */ static pm_node_t * -parse_parentheses(pm_parser_t *parser, pm_binding_power_t binding_power, uint16_t depth) { +parse_parentheses(pm_parser_t *parser, pm_binding_power_t binding_power, uint8_t flags, uint16_t depth) { pm_token_t opening = parser->current; pm_node_flags_t paren_flags = 0; @@ -18952,6 +18963,13 @@ parse_parentheses(pm_parser_t *parser, pm_binding_power_t binding_power, uint16_ } else if (context_p(parser, PM_CONTEXT_FOR_INDEX) && match1(parser, PM_TOKEN_KEYWORD_IN)) { /* All set, we're inside a for loop and we're parsing multiple * targets. */ + } else if (flags & PM_PARSE_ACCEPTS_STATEMENT) { + /* The rescue-modifier value parser promotes this target on a + * following `=` or comma. Reject any other binary operator that + * would otherwise consume the target list (e.g. `(a, b) + c`). */ + if (pm_binding_powers[parser->current.type].binary && !match1(parser, PM_TOKEN_EQUAL)) { + pm_parser_err_node(parser, result, PM_ERR_WRITE_TARGET_UNEXPECTED); + } } else if (binding_power != PM_BINDING_POWER_STATEMENT) { /* Multi targets are not allowed when it's not a statement * level. */ @@ -19058,6 +19076,23 @@ parse_parentheses(pm_parser_t *parser, pm_binding_power_t binding_power, uint16_ } /** + * Parse a splat (`*expression`) whose `*` operator has just been lexed and is + * sitting in parser->previous. The expression is optional, since an anonymous + * splat is just `*` with no following name. + */ +static pm_node_t * +parse_splat(pm_parser_t *parser, uint8_t flags, uint16_t depth) { + pm_token_t operator = parser->previous; + pm_node_t *name = NULL; + + if (token_begins_expression_p(parser->current.type)) { + name = parse_expression(parser, PM_BINDING_POWER_INDEX, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_STAR, (uint16_t) (depth + 1)); + } + + return UP(pm_splat_node_create(parser, &operator, name)); +} + +/** * Parse an expression that begins with the previous node that we just lexed. */ static PRISM_INLINE pm_node_t * @@ -19178,7 +19213,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u } case PM_TOKEN_PARENTHESIS_LEFT: case PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES: - return parse_parentheses(parser, binding_power, depth); + return parse_parentheses(parser, binding_power, flags, depth); case PM_TOKEN_BRACE_LEFT: { // If we were passed a current_hash_keys via the parser, then that // means we're already parsing a hash and we want to share the set @@ -19580,7 +19615,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u parser_lex(parser); return UP(pm_source_line_node_create(parser, &parser->previous)); case PM_TOKEN_KEYWORD_ALIAS: { - if (binding_power != PM_BINDING_POWER_STATEMENT) { + if (binding_power != PM_BINDING_POWER_STATEMENT && !(flags & PM_PARSE_ACCEPTS_STATEMENT)) { pm_parser_err_current(parser, PM_ERR_STATEMENT_ALIAS); } @@ -19808,7 +19843,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u )); } case PM_TOKEN_KEYWORD_END_UPCASE: { - if (binding_power != PM_BINDING_POWER_STATEMENT) { + if (binding_power != PM_BINDING_POWER_STATEMENT && !(flags & PM_PARSE_ACCEPTS_STATEMENT)) { pm_parser_err_current(parser, PM_ERR_STATEMENT_POSTEXE_END); } @@ -19840,14 +19875,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u // First, parse out the first index expression. if (accept1(parser, PM_TOKEN_USTAR)) { - pm_token_t star_operator = parser->previous; - pm_node_t *name = NULL; - - if (token_begins_expression_p(parser->current.type)) { - name = parse_expression(parser, PM_BINDING_POWER_INDEX, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_STAR, (uint16_t) (depth + 1)); - } - - index = UP(pm_splat_node_create(parser, &star_operator, name)); + index = parse_splat(parser, flags, depth); } else if (token_begins_expression_p(parser->current.type)) { index = parse_expression(parser, PM_BINDING_POWER_INDEX, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_COMMA, (uint16_t) (depth + 1)); } else { @@ -19901,7 +19929,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u return parse_conditional(parser, PM_CONTEXT_IF, opening_newline_index, if_after_else, (uint16_t) (depth + 1)); case PM_TOKEN_KEYWORD_UNDEF: { - if (binding_power != PM_BINDING_POWER_STATEMENT) { + if (binding_power != PM_BINDING_POWER_STATEMENT && !(flags & PM_PARSE_ACCEPTS_STATEMENT)) { pm_parser_err_current(parser, PM_ERR_STATEMENT_UNDEF); } @@ -20361,14 +20389,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u return UP(pm_error_recovery_node_create(parser, PM_TOKEN_START(parser, &parser->previous), PM_TOKEN_LENGTH(&parser->previous))); } - pm_token_t operator = parser->previous; - pm_node_t *name = NULL; - - if (token_begins_expression_p(parser->current.type)) { - name = parse_expression(parser, PM_BINDING_POWER_INDEX, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_STAR, (uint16_t) (depth + 1)); - } - - pm_node_t *splat = UP(pm_splat_node_create(parser, &operator, name)); + pm_node_t *splat = parse_splat(parser, flags, depth); if (match1(parser, PM_TOKEN_COMMA)) { return parse_targets_validate(parser, splat, PM_BINDING_POWER_INDEX, (uint16_t) (depth + 1)); @@ -20574,6 +20595,25 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u } } +static pm_node_t * +parse_rescue_modifier_value(pm_parser_t *parser, uint8_t flags, bool statement, uint16_t depth); + +/** + * When a `rescue` modifier's handler is itself a statement (a multiple or + * implicit-array assignment, a pattern match, or a command call before + * `=>`/`in`), its parse stops before any trailing operator above the modifier + * level. Such an operator cannot follow a statement — only statement modifiers + * (`if`/`unless`/...) can — so report it and skip past it. + */ +static void +parse_rescue_modifier_terminator(pm_parser_t *parser, uint8_t flags, uint16_t depth) { + if (pm_binding_powers[parser->current.type].left > PM_BINDING_POWER_MODIFIER) { + PM_PARSER_ERR_TOKEN_FORMAT(parser, &parser->current, PM_ERR_EXPECT_EOL_AFTER_STATEMENT, pm_token_str(parser->current.type)); + parser_lex(parser); + parse_expression(parser, pm_binding_powers[parser->previous.type].right, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR, (uint16_t) (depth + 1)); + } +} + /** * Parse a value that is going to be written to some kind of variable or method * call. We need to handle this separately because the rescue modifier is @@ -20605,9 +20645,22 @@ parse_assignment_value(pm_parser_t *parser, pm_binding_power_t previous_binding_ pm_token_t rescue = parser->current; parser_lex(parser); - pm_node_t *right = parse_expression(parser, pm_binding_powers[PM_TOKEN_KEYWORD_RESCUE_MODIFIER].right, flags & PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_RESCUE_MODIFIER_VALUE, (uint16_t) (depth + 1)); + // As in parse_assignment_values, the resbody is a `stmt` (permitting a + // multiple assignment / command call) when the rescued value is itself a + // command call, and a plain `arg` otherwise. + bool statement_value = pm_command_call_value_p(value) || pm_block_call_p(value); + uint8_t rescue_flags = (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | (statement_value ? PM_PARSE_ACCEPTS_COMMAND_CALL : 0)); + + pm_node_t *right = parse_rescue_modifier_value(parser, rescue_flags, statement_value, (uint16_t) (depth + 1)); context_pop(parser); + // A pattern-match resbody is a statement, but here the rescue is nested + // in an assignment value where parse_expression_terminator cannot see + // it, so reject a trailing operator above the modifier level directly. + if (PM_NODE_TYPE_P(right, PM_MATCH_REQUIRED_NODE) || PM_NODE_TYPE_P(right, PM_MATCH_PREDICATE_NODE)) { + parse_rescue_modifier_terminator(parser, flags, depth); + } + return UP(pm_rescue_modifier_node_create(parser, value, &rescue, right)); } @@ -20664,10 +20717,19 @@ parse_assignment_value_local(pm_parser_t *parser, const pm_node_t *node) { */ static pm_node_t * parse_assignment_values(pm_parser_t *parser, pm_binding_power_t previous_binding_power, pm_binding_power_t binding_power, uint8_t flags, pm_diagnostic_id_t diag_id, uint16_t depth) { + bool statement_level = (previous_binding_power == PM_BINDING_POWER_STATEMENT) || (flags & PM_PARSE_ACCEPTS_STATEMENT); + bool permitted = true; - if (previous_binding_power != PM_BINDING_POWER_STATEMENT && match1(parser, PM_TOKEN_USTAR)) permitted = false; + if (!statement_level && match1(parser, PM_TOKEN_USTAR)) permitted = false; - pm_node_t *value = parse_starred_expression(parser, binding_power, (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | (previous_binding_power == PM_BINDING_POWER_ASSIGNMENT ? (flags & PM_PARSE_ACCEPTS_COMMAND_CALL) : (previous_binding_power < PM_BINDING_POWER_MODIFIER ? PM_PARSE_ACCEPTS_COMMAND_CALL : 0))), diag_id, (uint16_t) (depth + 1)); + // A command call (e.g. `x = y z`) is permitted as the value when assigning + // directly (carrying the caller's flag), or in any statement-level context + // — which includes a rescue modifier value via the flag. + uint8_t command_call_flag = (previous_binding_power == PM_BINDING_POWER_ASSIGNMENT) + ? (uint8_t) (flags & PM_PARSE_ACCEPTS_COMMAND_CALL) + : ((previous_binding_power < PM_BINDING_POWER_MODIFIER || statement_level) ? PM_PARSE_ACCEPTS_COMMAND_CALL : 0); + + pm_node_t *value = parse_starred_expression(parser, binding_power, (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | command_call_flag), diag_id, (uint16_t) (depth + 1)); if (!permitted) pm_parser_err_node(parser, value, PM_ERR_UNEXPECTED_MULTI_WRITE); parse_assignment_value_local(parser, value); @@ -20676,7 +20738,7 @@ parse_assignment_values(pm_parser_t *parser, pm_binding_power_t previous_binding // Block calls (command call + do block, e.g., `foo bar do end`) cannot // be followed by a comma to form a multi-value RHS because each element // of a multi-value assignment must be an `arg`, not a `block_call`. - if (previous_binding_power == PM_BINDING_POWER_STATEMENT && !pm_block_call_p(value) && (PM_NODE_TYPE_P(value, PM_SPLAT_NODE) || match1(parser, PM_TOKEN_COMMA))) { + if (statement_level && !pm_block_call_p(value) && (PM_NODE_TYPE_P(value, PM_SPLAT_NODE) || match1(parser, PM_TOKEN_COMMA))) { single_value = false; pm_array_node_t *array = pm_array_node_create(parser, NULL); @@ -20705,26 +20767,41 @@ parse_assignment_values(pm_parser_t *parser, pm_binding_power_t previous_binding // Contradicting binding powers, the right-hand-side value of the assignment // allows the `rescue` modifier. - if ((single_value || (binding_power == (PM_BINDING_POWER_MULTI_ASSIGNMENT + 1))) && match1(parser, PM_TOKEN_KEYWORD_RESCUE_MODIFIER)) { + bool multiple_assignment = (binding_power == (PM_BINDING_POWER_MULTI_ASSIGNMENT + 1)); + if ((single_value || multiple_assignment) && match1(parser, PM_TOKEN_KEYWORD_RESCUE_MODIFIER)) { + bool command_value = pm_command_call_value_p(value) || pm_block_call_p(value); + + // A multiple assignment whose value is a command call (`x, y = foo + // bar`) is a complete statement (parse.y: `mlhs '=' + // command_call_value`, which has no rescue), so a trailing `rescue` + // modifies the whole assignment rather than the value. Leave it for the + // statement-level rescue instead of binding it to the value here. For a + // non-command value the rescue does bind to the value (parse.y: + // `mlhs '=' mrhs_arg modifier_rescue stmt`). + if (multiple_assignment && command_value) return value; + context_push(parser, PM_CONTEXT_RESCUE_MODIFIER); pm_token_t rescue = parser->current; parser_lex(parser); - bool accepts_command_call_inner = false; + // The resbody is a `stmt` (parse.y: `command_rhs`/`mlhs '=' mrhs_arg`), + // which permits a multiple assignment and a command call, when this is a + // multiple assignment or the rescued value is itself a command call. + // Otherwise it is a plain `arg` (parse.y: `arg_rhs`). + bool statement_value = multiple_assignment || command_value; + uint8_t rescue_flags = (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | (statement_value ? PM_PARSE_ACCEPTS_COMMAND_CALL : 0)); - // RHS can accept command call iff the value is a call with arguments - // but without parenthesis. - if (PM_NODE_TYPE_P(value, PM_CALL_NODE)) { - pm_call_node_t *call_node = (pm_call_node_t *) value; - if ((call_node->arguments != NULL) && (call_node->opening_loc.length == 0)) { - accepts_command_call_inner = true; - } - } - - pm_node_t *right = parse_expression(parser, pm_binding_powers[PM_TOKEN_KEYWORD_RESCUE_MODIFIER].right, (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | (accepts_command_call_inner ? PM_PARSE_ACCEPTS_COMMAND_CALL : 0)), PM_ERR_RESCUE_MODIFIER_VALUE, (uint16_t) (depth + 1)); + pm_node_t *right = parse_rescue_modifier_value(parser, rescue_flags, statement_value, (uint16_t) (depth + 1)); context_pop(parser); + // A pattern-match resbody is a statement, but here the rescue is nested + // in an assignment value where parse_expression_terminator cannot see + // it, so reject a trailing operator above the modifier level directly. + if (PM_NODE_TYPE_P(right, PM_MATCH_REQUIRED_NODE) || PM_NODE_TYPE_P(right, PM_MATCH_PREDICATE_NODE)) { + parse_rescue_modifier_terminator(parser, flags, depth); + } + return UP(pm_rescue_modifier_node_create(parser, value, &rescue, right)); } @@ -20732,6 +20809,74 @@ parse_assignment_values(pm_parser_t *parser, pm_binding_power_t previous_binding } /** + * Parse the value of a `rescue` modifier. When `statement` is true the value + * is a `stmt` in CRuby's grammar (parse.y: `stmt modifier_rescue stmt`, + * `mlhs '=' mrhs_arg modifier_rescue stmt`, and `command_rhs`), which means it + * may be a multiple assignment, e.g. `a rescue b, c = 1`. We parse it at the + * rescue modifier's right binding power so that trailing statement modifiers + * (if/unless/while/until) stay outside the value, and promote to a multiple + * assignment exactly as the statement-level parser does when a comma (or + * leading splat) appears. Otherwise the value is a plain `arg` (parse.y: + * `arg modifier_rescue arg`), parsed above the `and`/`or`/`not` level so that + * those stay outside it. + */ +static pm_node_t * +parse_rescue_modifier_value(pm_parser_t *parser, uint8_t flags, bool statement, uint16_t depth) { + if (statement) { + pm_node_t *value; + bool multiple; + + if (match1(parser, PM_TOKEN_USTAR)) { + // A leading splat can only begin a multiple assignment target list. + parser_lex(parser); + value = parse_splat(parser, flags, depth); + multiple = true; + } else { + // The flag lets a single-target assignment take a multiple-value or + // splat right-hand side (`b = c, d` / `b = *c`); a comma _before_ an + // `=`, or a parenthesized target list (`(b, c), d = 1`), instead + // promotes to a multiple assignment target list below. + value = parse_expression(parser, pm_binding_powers[PM_TOKEN_KEYWORD_RESCUE_MODIFIER].right, flags | PM_PARSE_ACCEPTS_STATEMENT, PM_ERR_RESCUE_MODIFIER_VALUE, (uint16_t) (depth + 1)); + multiple = match1(parser, PM_TOKEN_COMMA) || PM_NODE_TYPE_P(value, PM_MULTI_TARGET_NODE); + } + + if (multiple) { + pm_node_t *target = parse_targets_validate(parser, value, PM_BINDING_POWER_INDEX, (uint16_t) (depth + 1)); + + // A promoted target list is only a valid rescue value as part of a + // complete `targets = values`. parse_targets_validate already + // reports a missing `=` for every terminator except `)` (which it + // permits for an enclosing mlhs paren that does not apply here), so + // reject that case. + if (!match1(parser, PM_TOKEN_EQUAL)) { + if (match1(parser, PM_TOKEN_PARENTHESIS_RIGHT)) pm_parser_err_node(parser, target, PM_ERR_WRITE_TARGET_UNEXPECTED); + return target; + } + + pm_token_t operator = parser->current; + parser_lex(parser); + + pm_node_t *values = parse_assignment_values(parser, PM_BINDING_POWER_STATEMENT, PM_BINDING_POWER_MULTI_ASSIGNMENT + 1, flags, PM_ERR_EXPECT_EXPRESSION_AFTER_EQUAL, (uint16_t) (depth + 1)); + value = parse_write(parser, target, &operator, values); + } + + // Reject a trailing operator that cannot follow a statement resbody. + // Pattern-match handlers are statements too, but for the bare statement + // form they are reported by parse_expression_terminator instead (which + // keeps its existing error-recovery), so they are excluded here. + if (!PM_NODE_TYPE_P(value, PM_MATCH_REQUIRED_NODE) && !PM_NODE_TYPE_P(value, PM_MATCH_PREDICATE_NODE)) { + parse_rescue_modifier_terminator(parser, flags, depth); + } + + return value; + } + + // Otherwise the resbody is a plain `arg` (parse.y: `arg modifier_rescue + // arg`), parsed above the `and`/`or`/`not` level so those stay outside it. + return parse_expression(parser, PM_BINDING_POWER_DEFINED, flags, PM_ERR_RESCUE_MODIFIER_VALUE, (uint16_t) (depth + 1)); +} + +/** * Ensure a call node that is about to become a call operator node does not * have arguments or a block attached. If it does, then we'll need to add an * error message and destroy the arguments/block. Ideally we would keep the node @@ -21014,7 +21159,7 @@ parse_expression_infix(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t parser_lex(parser); pm_node_t *value = parse_assignment_values(parser, previous_binding_power, PM_NODE_TYPE_P(node, PM_MULTI_TARGET_NODE) ? PM_BINDING_POWER_MULTI_ASSIGNMENT + 1 : binding_power, flags, PM_ERR_EXPECT_EXPRESSION_AFTER_EQUAL, (uint16_t) (depth + 1)); - if (PM_NODE_TYPE_P(node, PM_MULTI_TARGET_NODE) && previous_binding_power != PM_BINDING_POWER_STATEMENT) { + if (PM_NODE_TYPE_P(node, PM_MULTI_TARGET_NODE) && previous_binding_power != PM_BINDING_POWER_STATEMENT && !(flags & PM_PARSE_ACCEPTS_STATEMENT)) { pm_parser_err_node(parser, node, PM_ERR_UNEXPECTED_MULTI_WRITE); } @@ -21806,7 +21951,7 @@ parse_expression_infix(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t parser_lex(parser); accept1(parser, PM_TOKEN_NEWLINE); - pm_node_t *value = parse_expression(parser, binding_power, (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | PM_PARSE_ACCEPTS_COMMAND_CALL), PM_ERR_RESCUE_MODIFIER_VALUE, (uint16_t) (depth + 1)); + pm_node_t *value = parse_rescue_modifier_value(parser, (uint8_t) ((flags & PM_PARSE_ACCEPTS_DO_BLOCK) | PM_PARSE_ACCEPTS_COMMAND_CALL), previous_binding_power == PM_BINDING_POWER_STATEMENT, (uint16_t) (depth + 1)); context_pop(parser); return UP(pm_rescue_modifier_node_create(parser, node, &token, value)); @@ -3875,7 +3875,15 @@ curry(RB_BLOCK_CALL_FUNC_ARGLIST(_, args)) return arity; } else { - return rb_proc_call_with_block(proc, check_argc(RARRAY_LEN(passed)), RARRAY_CONST_PTR(passed), blockarg); + // `passed` is the only reference keeping this array (and thus the + // buffer that RARRAY_CONST_PTR points into) alive, but it is otherwise + // unused after this point. Without RB_GC_GUARD the compiler may drop it + // before the call returns, so conservative stack marking misses it and + // GC can reclaim the array while it is still being read as argv, + // crashing with "try to mark T_NONE object". + VALUE result = rb_proc_call_with_block(proc, check_argc(RARRAY_LEN(passed)), RARRAY_CONST_PTR(passed), blockarg); + RB_GC_GUARD(passed); + return result; } } diff --git a/spec/bundler/commands/lock_spec.rb b/spec/bundler/commands/lock_spec.rb index 8ab3cc7e8d..1a43400923 100644 --- a/spec/bundler/commands/lock_spec.rb +++ b/spec/bundler/commands/lock_spec.rb @@ -1083,8 +1083,10 @@ RSpec.describe "bundle lock" do simulate_platform("x86-mingw32") { bundle :lock } checksums = checksums_section_when_enabled do |c| + c.checksum gem_repo4, "ffi", "1.9.14" c.checksum gem_repo4, "ffi", "1.9.14", "x86-mingw32" c.checksum gem_repo4, "gssapi", "1.2.0" + c.checksum gem_repo4, "mixlib-shellout", "2.2.6" c.checksum gem_repo4, "mixlib-shellout", "2.2.6", "universal-mingw32" c.checksum gem_repo4, "win32-process", "0.8.3" end @@ -1093,9 +1095,11 @@ RSpec.describe "bundle lock" do GEM remote: https://gem.repo4/ specs: + ffi (1.9.14) ffi (1.9.14-x86-mingw32) gssapi (1.2.0) ffi (>= 1.0.1) + mixlib-shellout (2.2.6) mixlib-shellout (2.2.6-universal-mingw32) win32-process (~> 0.8.2) win32-process (0.8.3) diff --git a/spec/bundler/install/gemfile/platform_spec.rb b/spec/bundler/install/gemfile/platform_spec.rb index e12933ebcf..c28af3d4ab 100644 --- a/spec/bundler/install/gemfile/platform_spec.rb +++ b/spec/bundler/install/gemfile/platform_spec.rb @@ -208,6 +208,7 @@ RSpec.describe "bundle install across platforms" do c.checksum gem_repo4, "empyrean", "0.1.0" c.checksum gem_repo4, "ffi", "1.9.23", "java" c.checksum gem_repo4, "method_source", "0.9.0" + c.checksum gem_repo4, "pry", "0.11.3" c.checksum gem_repo4, "pry", "0.11.3", "java" c.checksum gem_repo4, "spoon", "0.0.6" end @@ -220,6 +221,9 @@ RSpec.describe "bundle install across platforms" do empyrean (0.1.0) ffi (1.9.23-java) method_source (0.9.0) + pry (0.11.3) + coderay (~> 1.1.0) + method_source (~> 0.9.0) pry (0.11.3-java) coderay (~> 1.1.0) method_source (~> 0.9.0) diff --git a/spec/bundler/install/gemfile/specific_platform_spec.rb b/spec/bundler/install/gemfile/specific_platform_spec.rb index 97b1d233bf..eeda764c39 100644 --- a/spec/bundler/install/gemfile/specific_platform_spec.rb +++ b/spec/bundler/install/gemfile/specific_platform_spec.rb @@ -261,6 +261,99 @@ RSpec.describe "bundle install with specific platforms" do expect(the_bundle).not_to include_gem("nokogiri 1.18.10 x86_64-linux") end end + + it "adds and installs the ruby variant when only an incompatible platform-specific variant was locked" do + build_repo4 do + build_gem "nokogiri", "1.18.10" + build_gem "nokogiri", "1.18.10" do |s| + s.platform = "x86_64-linux" + s.required_ruby_version = "< #{Gem.ruby_version}" + end + end + + gemfile <<~G + source "https://gem.repo4" + + gem "nokogiri" + G + + lockfile <<-L + GEM + remote: https://gem.repo4/ + specs: + nokogiri (1.18.10-x86_64-linux) + + PLATFORMS + x86_64-linux + + DEPENDENCIES + nokogiri + + BUNDLED WITH + #{Bundler::VERSION} + L + + simulate_platform "x86_64-linux" do + bundle "install --verbose" + expect(out).to include("Installing nokogiri 1.18.10") + expect(the_bundle).to include_gem("nokogiri 1.18.10") + end + + expect(lockfile).to eq <<~L + GEM + remote: https://gem.repo4/ + specs: + nokogiri (1.18.10) + nokogiri (1.18.10-x86_64-linux) + + PLATFORMS + x86_64-linux + + DEPENDENCIES + nokogiri + + BUNDLED WITH + #{Bundler::VERSION} + L + end + + it "fails at install time when only an incompatible platform-specific variant is locked" do + build_repo4 do + build_gem "nokogiri", "1.18.10" + build_gem "nokogiri", "1.18.10" do |s| + s.platform = "x86_64-linux" + s.required_ruby_version = "< #{Gem.ruby_version}" + end + end + + gemfile <<~G + source "https://gem.repo4" + + gem "nokogiri" + G + + lockfile <<-L + GEM + remote: https://gem.repo4/ + specs: + nokogiri (1.18.10-x86_64-linux) + + PLATFORMS + x86_64-linux + + DEPENDENCIES + nokogiri + + BUNDLED WITH + #{Bundler::VERSION} + L + + simulate_platform "x86_64-linux" do + bundle "install --verbose", env: { "BUNDLE_FROZEN" => "true" }, raise_on_error: false + expect(exitstatus).not_to eq(0) + expect(err).to include("nokogiri-1.18.10-x86_64-linux requires ruby version < #{Gem.ruby_version}") + end + end end it "doesn't discard previously installed platform specific gem and fall back to ruby on subsequent bundles" do @@ -766,10 +859,13 @@ RSpec.describe "bundle install with specific platforms" do bundle "update --conservative nokogiri" end + checksums.checksum gem_repo4, "nokogiri", "1.13.0" + expect(lockfile).to eq <<~L GEM remote: https://gem.repo4/ specs: + nokogiri (1.13.0) nokogiri (1.13.0-x86_64-darwin) sorbet-static (0.5.10601-x86_64-darwin) @@ -785,6 +881,61 @@ RSpec.describe "bundle install with specific platforms" do L end + it "locks ruby fallback variant dependencies without adding the ruby platform" do + build_repo4 do + build_gem "native_tool", "1.0" do |s| + s.add_dependency "rake" + end + + build_gem "native_tool", "1.0" do |s| + s.platform = "x86_64-linux" + end + + build_gem "rake" + + build_gem "sorbet-static", "0.5.10601" do |s| + s.platform = "x86_64-linux" + end + end + + simulate_platform "x86_64-linux" do + install_gemfile <<~G + source "https://gem.repo4" + + gem "native_tool" + gem "sorbet-static" + G + end + + checksums = checksums_section_when_enabled do |c| + c.checksum gem_repo4, "native_tool", "1.0" + c.checksum gem_repo4, "native_tool", "1.0", "x86_64-linux" + c.checksum gem_repo4, "rake", "1.0" + c.checksum gem_repo4, "sorbet-static", "0.5.10601", "x86_64-linux" + end + + expect(lockfile).to eq <<~L + GEM + remote: https://gem.repo4/ + specs: + native_tool (1.0) + rake + native_tool (1.0-x86_64-linux) + rake (1.0) + sorbet-static (0.5.10601-x86_64-linux) + + PLATFORMS + x86_64-linux + + DEPENDENCIES + native_tool + sorbet-static + #{checksums} + BUNDLED WITH + #{Bundler::VERSION} + L + end + it "automatically fixes the lockfile if only ruby platform is locked and some gem has no ruby variant available" do build_repo4 do build_gem("sorbet-static-and-runtime", "0.5.10160") do |s| diff --git a/spec/bundler/lock/lockfile_spec.rb b/spec/bundler/lock/lockfile_spec.rb index 654ac02aa7..0a2aa8aca8 100644 --- a/spec/bundler/lock/lockfile_spec.rb +++ b/spec/bundler/lock/lockfile_spec.rb @@ -1324,6 +1324,7 @@ RSpec.describe "the lockfile format" do G checksums = checksums_section_when_enabled do |c| + c.checksum gem_repo2, "platform_specific", "1.0" c.checksum gem_repo2, "platform_specific", "1.0", "universal-java-16" end @@ -1331,6 +1332,7 @@ RSpec.describe "the lockfile format" do GEM remote: https://gem.repo2/ specs: + platform_specific (1.0) platform_specific (1.0-universal-java-16) PLATFORMS diff --git a/spec/bundler/resolver/platform_spec.rb b/spec/bundler/resolver/platform_spec.rb index a1d095d024..83bec610a6 100644 --- a/spec/bundler/resolver/platform_spec.rb +++ b/spec/bundler/resolver/platform_spec.rb @@ -71,7 +71,7 @@ RSpec.describe "Resolving platform craziness" do should_resolve_as %w[foo-1.0.0] end - it "prefers the platform specific gem to the ruby version" do + it "prefers the platform specific gem to the ruby version, but keeps the ruby fallback" do @index = build_index do gem "foo", "1.0.0" gem "foo", "1.0.0", "x64-mingw-ucrt" @@ -79,7 +79,7 @@ RSpec.describe "Resolving platform craziness" do dep "foo" platforms "x64-mingw-ucrt" - should_resolve_as %w[foo-1.0.0-x64-mingw-ucrt] + should_resolve_as %w[foo-1.0.0 foo-1.0.0-x64-mingw-ucrt] end describe "on a linux platform" do @@ -88,7 +88,7 @@ RSpec.describe "Resolving platform craziness" do # Gem's platform is *-linux => gem is glibc + maybe musl compatible # Gem's platform is *-linux-musl => gem is musl compatible but not glibc - it "favors the platform version-specific gem on a version-specifying linux platform" do + it "favors the platform version-specific gem on a version-specifying linux platform, but keeps the ruby fallback" do @index = build_index do gem "foo", "1.0.0" gem "foo", "1.0.0", "x86_64-linux" @@ -97,10 +97,10 @@ RSpec.describe "Resolving platform craziness" do dep "foo" platforms "x86_64-linux-musl" - should_resolve_as %w[foo-1.0.0-x86_64-linux-musl] + should_resolve_as %w[foo-1.0.0 foo-1.0.0-x86_64-linux-musl] end - it "favors the version-less gem over the version-specific gem on a gnu linux platform" do + it "favors the version-less gem over the version-specific gem on a gnu linux platform, but keeps the ruby fallback" do @index = build_index do gem "foo", "1.0.0" gem "foo", "1.0.0", "x86_64-linux" @@ -109,7 +109,7 @@ RSpec.describe "Resolving platform craziness" do dep "foo" platforms "x86_64-linux" - should_resolve_as %w[foo-1.0.0-x86_64-linux] + should_resolve_as %w[foo-1.0.0 foo-1.0.0-x86_64-linux] end it "ignores the platform version-specific gem on a gnu linux platform" do @@ -122,7 +122,7 @@ RSpec.describe "Resolving platform craziness" do should_not_resolve end - it "falls back to the platform version-less gem on a linux platform with a version" do + it "falls back to the platform version-less gem on a linux platform with a version, but keeps the ruby fallback" do @index = build_index do gem "foo", "1.0.0" gem "foo", "1.0.0", "x86_64-linux" @@ -130,7 +130,7 @@ RSpec.describe "Resolving platform craziness" do dep "foo" platforms "x86_64-linux-musl" - should_resolve_as %w[foo-1.0.0-x86_64-linux] + should_resolve_as %w[foo-1.0.0 foo-1.0.0-x86_64-linux] end it "falls back to the ruby platform gem on a gnu linux platform when only a version-specifying gem is available" do diff --git a/test/digest/test_digest.rb b/test/digest/test_digest.rb index 084eda8b1b..95cce93940 100644 --- a/test/digest/test_digest.rb +++ b/test/digest/test_digest.rb @@ -204,7 +204,7 @@ module TestDigest Data1 => "352441c2", Data2 => "171a3f5f", } - end + end if defined?(Digest::CRC32) class TestBase < Test::Unit::TestCase def test_base diff --git a/test/prism/errors/rescue_modifier_argument_value_alias.txt b/test/prism/errors/rescue_modifier_argument_value_alias.txt new file mode 100644 index 0000000000..e4d0b9dc90 --- /dev/null +++ b/test/prism/errors/rescue_modifier_argument_value_alias.txt @@ -0,0 +1,3 @@ +x = 1 rescue alias a b + ^~~~~ unexpected an `alias` at a non-statement position + diff --git a/test/prism/errors/rescue_modifier_argument_value_not.txt b/test/prism/errors/rescue_modifier_argument_value_not.txt new file mode 100644 index 0000000000..5d066eb43a --- /dev/null +++ b/test/prism/errors/rescue_modifier_argument_value_not.txt @@ -0,0 +1,4 @@ +x = 1 rescue not b + ^ expected a `(` after `not` + ^ unexpected local variable or method, expecting end-of-input + diff --git a/test/prism/errors/rescue_modifier_endless_method_multiple_assignment.txt b/test/prism/errors/rescue_modifier_endless_method_multiple_assignment.txt new file mode 100644 index 0000000000..892f78cc9b --- /dev/null +++ b/test/prism/errors/rescue_modifier_endless_method_multiple_assignment.txt @@ -0,0 +1,4 @@ +def m = foo bar rescue a, b = 1 + ^ unexpected ',', expecting end-of-input + ^ unexpected ',', ignoring it + diff --git a/test/prism/errors/rescue_modifier_multiple_assignment_logical_operator.txt b/test/prism/errors/rescue_modifier_multiple_assignment_logical_operator.txt new file mode 100644 index 0000000000..faca9bf49b --- /dev/null +++ b/test/prism/errors/rescue_modifier_multiple_assignment_logical_operator.txt @@ -0,0 +1,3 @@ +a rescue b, c = d and e + ^~~ unexpected 'and', expecting end-of-input + diff --git a/test/prism/errors/rescue_modifier_multiple_assignment_trailing_comma.txt b/test/prism/errors/rescue_modifier_multiple_assignment_trailing_comma.txt new file mode 100644 index 0000000000..bffd9e7280 --- /dev/null +++ b/test/prism/errors/rescue_modifier_multiple_assignment_trailing_comma.txt @@ -0,0 +1,3 @@ +(a rescue b,) + ^~ unexpected write target + diff --git a/test/prism/errors/rescue_modifier_single_assignment_multiple_assignment.txt b/test/prism/errors/rescue_modifier_single_assignment_multiple_assignment.txt new file mode 100644 index 0000000000..be666ef886 --- /dev/null +++ b/test/prism/errors/rescue_modifier_single_assignment_multiple_assignment.txt @@ -0,0 +1,4 @@ +z = 1 rescue a, b = 1 + ^ unexpected ',', expecting end-of-input + ^ unexpected ',', ignoring it + diff --git a/test/prism/fixtures/rescue_modifier_assignment.txt b/test/prism/fixtures/rescue_modifier_assignment.txt new file mode 100644 index 0000000000..4882835d77 --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_assignment.txt @@ -0,0 +1,3 @@ +x, y = 1 rescue a, b = 1 +x, y = a rescue b, c = 1 +x, y = a rescue b = c d diff --git a/test/prism/fixtures/rescue_modifier_block_command_value.txt b/test/prism/fixtures/rescue_modifier_block_command_value.txt new file mode 100644 index 0000000000..94091f072a --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_block_command_value.txt @@ -0,0 +1,5 @@ +x = foo a do end rescue b +x = foo a do end rescue b, c = 1 +x = a.foo b do end rescue c, d = 1 +x, y = foo a do end rescue b +x += foo a do end rescue b, c = 1 diff --git a/test/prism/fixtures/rescue_modifier_command_value.txt b/test/prism/fixtures/rescue_modifier_command_value.txt new file mode 100644 index 0000000000..7b31adcd2d --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_command_value.txt @@ -0,0 +1,6 @@ +a rescue b = c d +x = foo bar rescue b, c = 1 +x = foo bar rescue b = c d +x += foo bar rescue b, c = 1 +x, y = foo bar rescue b +x, y = foo bar rescue b, c = 1 diff --git a/test/prism/fixtures/rescue_modifier_multiple_assignment.txt b/test/prism/fixtures/rescue_modifier_multiple_assignment.txt new file mode 100644 index 0000000000..79ca21f577 --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_multiple_assignment.txt @@ -0,0 +1,9 @@ +a rescue b, c = 1 +a rescue b, c, d = 1 +a rescue *b, c = 1 +a rescue b, *c = 1 +a rescue b, *c, d = 1 +a rescue *b = 1 +a rescue (b, c), d = 1 +a rescue b.c, d = 1 +a rescue b[0], c = 1 diff --git a/test/prism/fixtures/rescue_modifier_multiple_value.txt b/test/prism/fixtures/rescue_modifier_multiple_value.txt new file mode 100644 index 0000000000..bac7b039a0 --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_multiple_value.txt @@ -0,0 +1,5 @@ +a rescue b = c, d +a rescue b = c, d, e +a rescue b = *c +a rescue b = c, *d +a rescue b = *c, d diff --git a/test/prism/fixtures/rescue_modifier_statement_value.txt b/test/prism/fixtures/rescue_modifier_statement_value.txt new file mode 100644 index 0000000000..d3951a9608 --- /dev/null +++ b/test/prism/fixtures/rescue_modifier_statement_value.txt @@ -0,0 +1,8 @@ +a rescue alias x y +a rescue undef m +a rescue END { b } +a rescue retry +alias x y rescue b +undef m rescue b +END { b } rescue b +x = foo bar rescue retry diff --git a/test/prism/locals_test.rb b/test/prism/locals_test.rb index 417730a8a7..c4514edd51 100644 --- a/test/prism/locals_test.rb +++ b/test/prism/locals_test.rb @@ -77,6 +77,10 @@ module Prism parts[10] end + def catch_table + parts[12] + end + def instructions parts[13] end @@ -97,6 +101,20 @@ module Prism yield ISeq.new(opnd) end end + + # The handler for a rescue clause is compiled into its own iseq that is + # reachable only through the catch table. Its local table only ever + # holds the implicit `$!` error variable (rescue clauses do not + # introduce user locals of their own -- `rescue => e` puts `e` in the + # enclosing scope), and prism does not model it as a scope, so we treat + # it as transparent and descend straight into any iseqs nested within + # it (e.g. a block or an `END {}` inside a rescue clause). Only + # `:rescue` entries are followed: `:break`/`:next`/`:redo`/`:retry` + # entries either have no iseq or reference one already reachable through + # the instructions, and `:ensure` bodies are compiled inline. + catch_table.each do |entry| + ISeq.new(entry[1]).each_child { |child| yield child } if entry[0] == :rescue && entry[1].is_a?(Array) + end end end diff --git a/test/prism/ruby/parser_test.rb b/test/prism/ruby/parser_test.rb index ad9fa0c92c..856ecedc1d 100644 --- a/test/prism/ruby/parser_test.rb +++ b/test/prism/ruby/parser_test.rb @@ -132,7 +132,6 @@ module Prism "whitequark/lbrace_arg_after_command_args.txt", "whitequark/multiple_pattern_matches.txt", "whitequark/newline_in_hash_argument.txt", - "whitequark/pattern_matching_expr_in_paren.txt", "whitequark/pattern_matching_hash.txt", "whitequark/ruby_bug_14690.txt", "whitequark/ruby_bug_9669.txt", diff --git a/test/ruby/test_proc.rb b/test/ruby/test_proc.rb index f74342322f..eefd5b189b 100644 --- a/test/ruby/test_proc.rb +++ b/test/ruby/test_proc.rb @@ -342,6 +342,25 @@ class TestProc < Test::Unit::TestCase assert_equal(9, b) end + # Not named test_curry_* so that test_curry_with_trace does not re-run it + # under set_trace_func (which would be needlessly slow with GC.stress). + def test_proc_curry_keeps_args_alive + # The argument array passed down by `curry` must stay alive across the + # inner call; otherwise GC may reclaim it while it is still read as argv + # and crash with "try to mark T_NONE object". See the RB_GC_GUARD in `curry`. + assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}", timeout: 60) + begin; + GC.stress = true + l = ->(a, b, c) { a + b + c } + 30.times do + l1 = l.curry.call(1) + l2 = l1.curry.call(2) + assert_equal(6, l2.curry.call(3)) + assert_equal(6, l1.curry.call(2, 3)) + end + end; + end + def test_lambda? l = proc {} assert_equal(false, l.lambda?) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index df7054c101..33f13511cb 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -1041,6 +1041,55 @@ class TestYJIT < Test::Unit::TestCase RUBY end + def test_bmethod_super_block_forwarding + # super() in a block method forwards the block handler of environment + # it is in. In this test, the block is in a class, which never has + # a block handler, so Parent#foo gets no block. + assert_compiles(<<~RUBY, result: nil) + class Parent + def foo = defined?(yield) + end + + class BmethodSuper < Parent + define_method(:foo) { super() } + end + + BmethodSuper.new.foo {} + RUBY + + # For contrast, the super() here forwards the block passed to add_then_override + # because block with super() is rooted in add_then_override, a "def" method. + assert_compiles(<<~RUBY, result: [1, 2]) + def add_then_override(object) + object.define_singleton_method(:then) { super() } + end + + add_then_override(one = Object.new) { 1 } + add_then_override(two = Object.new) { 2 } + [one.then {}, two.then {}] + RUBY + end + + def test_bug_22116 + # Regression test from report which used to crash during environment escape + # to pass block to Hash.new + assert_compiles(<<~RUBY, call_threshold: 1) + class C + def foo + Hash.new {|h, k| h[k] = [] } + end + end + + class D < C + define_method(:foo) do + super() + end + end + + D.new.foo + RUBY + end + # Tests calling a variadic cfunc with many args def test_build_large_struct assert_compiles(<<~RUBY, insns: %i[opt_send_without_block], call_threshold: 2) diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index a56fea6d51..198301d933 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -382,6 +382,23 @@ class TestZJIT < Test::Unit::TestCase }, call_threshold: 14, num_profiles: 5 end + def test_regression_gc_stress_with_lazy_block_code + assert_compiles ':ok', %q{ + def allocate_array + [1, 2, 3] + end + + begin + GC.stress = true + allocate_array + allocate_array + :ok + ensure + GC.stress = false + end + } + end + def test_exit_tracing # Smoke test: --zjit-trace-exits writes a Fuchsia trace (.fxt) file to /tmp assert_compiles('true', <<~RUBY, extra_args: ['--zjit-trace-exits']) diff --git a/test/test_bundled_gems.rb b/test/test_bundled_gems.rb index 0889584185..884412b416 100644 --- a/test/test_bundled_gems.rb +++ b/test/test_bundled_gems.rb @@ -16,6 +16,39 @@ class TestBundlerGem < Gem::TestCase assert_nil Gem::BUNDLED_GEMS.warning?("csv", specs: {}) end + def test_find_gem_plain_name + assert_equal ["csv", "csv", false], Gem::BUNDLED_GEMS.find_gem("csv") + end + + def test_find_gem_returns_nil_for_non_bundled_gem + assert_nil Gem::BUNDLED_GEMS.find_gem("some_gem") + assert_nil Gem::BUNDLED_GEMS.find_gem("some/nested/gem") + end + + def test_find_gem_exact_mapping + # kconv is provided by the nkf gem; the feature keeps its original name + assert_equal ["kconv", "nkf", false], Gem::BUNDLED_GEMS.find_gem("kconv") + end + + def test_find_gem_dashed_name + # resolv/replace is provided by the resolv-replace gem + assert_equal ["resolv/replace", "resolv-replace", false], Gem::BUNDLED_GEMS.find_gem("resolv/replace") + end + + def test_find_gem_subfeature + assert_equal ["benchmark/ips", "benchmark", true], Gem::BUNDLED_GEMS.find_gem("benchmark/ips") + end + + def test_find_gem_strips_libdir_prefix + path = File.join(::RbConfig::CONFIG.fetch("rubylibdir"), "csv.rb") + assert_equal ["csv", "csv", false], Gem::BUNDLED_GEMS.find_gem(path) + end + + def test_find_gem_strips_archdir_prefix + path = File.join(::RbConfig::CONFIG.fetch("rubyarchdir"), "syslog.so") + assert_equal ["syslog", "syslog", false], Gem::BUNDLED_GEMS.find_gem(path) + end + def test_no_warning_warning assert_nil Gem::BUNDLED_GEMS.warning?("some_gem", specs: {}) assert_nil Gem::BUNDLED_GEMS.warning?("/path/to/some_gem.rb", specs: {}) @@ -2849,6 +2849,27 @@ rb_zjit_materialize_frames(const rb_execution_context_t *ec, rb_control_frame_t if (jit_frame->materialize_block_code) { cfp->block_code = NULL; } + + // Materialize Ruby stack slots kept off the VM stack. On side exit, + // the exiting frame is already written by compile_exit_save_state() + // and skipped here after materialize_exit_trampoline clears its + // jit_return, so this restores older ZJIT frames from stack maps. + int32_t stack_size = (int32_t)jit_frame->stack_size; + if (stack_size > 0) { + VALUE *stack = cfp->sp - stack_size; + for (int32_t i = 0; i < stack_size; i++) { + VALUE entry = jit_frame->stack[i]; + if (ZJIT_STACK_MAP_VREG_P(entry)) { + // Decode a native stack slot offset generated by ZJIT's backend. + // It's an offset from NATIVE_BASE_PTR, which is copied into + // cfp->jit_return, to the encoded stack slot. + stack[i] = ((VALUE *)cfp->jit_return)[-(ssize_t)ZJIT_STACK_MAP_VREG_INDEX(entry)]; + } + else { + stack[i] = entry; + } + } + } cfp->jit_return = 0; } if (end_cfp == cfp) break; @@ -3641,11 +3662,15 @@ rb_execution_context_update(rb_execution_context_t *ec) rb_control_frame_t *cfp = ec->cfp; rb_control_frame_t *limit_cfp = (void *)(ec->vm_stack + ec->vm_stack_size); - for (i = 0; i < (long)(sp - p); i++) { - VALUE ref = p[i]; - VALUE update = rb_gc_location(ref); - if (ref != update) { - p[i] = update; + // ZJIT leaves uninitialized slots on the VM stack, so we cannot + // safely use rb_gc_location on such slots. + if (!rb_zjit_enabled_p) { + for (i = 0; i < (long)(sp - p); i++) { + VALUE ref = p[i]; + VALUE update = rb_gc_location(ref); + if (ref != update) { + p[i] = update; + } } } @@ -3653,12 +3678,15 @@ rb_execution_context_update(rb_execution_context_t *ec) const VALUE *ep = cfp->ep; cfp->self = rb_gc_location(cfp->self); if (CFP_ZJIT_FRAME_P(cfp)) { - rb_zjit_jit_frame_update_references((zjit_jit_frame_t *)CFP_ZJIT_FRAME(cfp)); - // block_code must always be relocated. For ISEQ frames, the JIT caller - // may have written it (gen_block_handler_specval) for passing blocks. - // For C frames, rb_iterate0 may have written an ifunc to block_code - // after the JIT pushed the frame. NULL is safe to pass to rb_gc_location. - cfp->block_code = (void *)rb_gc_location((VALUE)cfp->block_code); + const zjit_jit_frame_t *jit_frame = CFP_ZJIT_FRAME(cfp); + rb_zjit_jit_frame_update_references((zjit_jit_frame_t *)jit_frame); + // materialize_block_code means cfp->block_code is lazy and may + // still contain stale data from a previous frame. Otherwise it + // was initialized by ZJIT and may have been written later by + // vm_caller_setup_arg_block (ISEQ frames) or rb_iterate0 (C frames). + if (!jit_frame->materialize_block_code) { + cfp->block_code = (void *)rb_gc_location((VALUE)cfp->block_code); + } } else { cfp->_iseq = (rb_iseq_t *)rb_gc_location((VALUE)cfp->_iseq); @@ -3706,7 +3734,14 @@ rb_execution_context_mark(const rb_execution_context_t *ec) rb_control_frame_t *limit_cfp = (void *)(ec->vm_stack + ec->vm_stack_size); for (long i = 0; i < (long)(sp - p); i++) { - rb_gc_mark_movable(p[i]); + if (rb_zjit_enabled_p) { + // ZJIT leaves uninitialized slots on the VM stack, so we need + // to mark such slots conservatively. + rb_gc_mark_maybe(p[i]); + } + else { + rb_gc_mark_movable(p[i]); + } } while (cfp != limit_cfp) { @@ -3714,10 +3749,21 @@ rb_execution_context_mark(const rb_execution_context_t *ec) VM_ASSERT(!!VM_ENV_FLAGS(ep, VM_ENV_FLAG_ESCAPED) == vm_ep_in_heap_p_(ec, ep)); rb_gc_mark_movable(cfp->self); - rb_gc_mark_movable((VALUE)CFP_ISEQ(cfp)); - // Mark block_code directly (not through rb_zjit_cfp_block_code) - // because rb_iterate0 may write a valid ifunc after JIT frame push. - rb_gc_mark_movable((VALUE)cfp->block_code); + if (CFP_ZJIT_FRAME_P(cfp)) { + const zjit_jit_frame_t *jit_frame = CFP_ZJIT_FRAME(cfp); + rb_gc_mark_movable((VALUE)jit_frame->iseq); + // materialize_block_code means cfp->block_code is lazy and may + // still contain stale data from a previous frame. Otherwise it + // was initialized by ZJIT and may have been written later by + // vm_caller_setup_arg_block (ISEQ frames) or rb_iterate0 (C frames). + if (!jit_frame->materialize_block_code) { + rb_gc_mark_movable((VALUE)cfp->block_code); + } + } + else { + rb_gc_mark_movable((VALUE)cfp->_iseq); + rb_gc_mark_movable((VALUE)cfp->block_code); + } if (VM_ENV_LOCAL_P(ep) && VM_ENV_BOXED_P(ep)) { const rb_box_t *box = VM_ENV_BOX(ep); diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index 743a10e15a..fcddb18ac7 100644 --- a/yjit/src/codegen.rs +++ b/yjit/src/codegen.rs @@ -6805,8 +6805,8 @@ enum SpecVal { pub enum BlockHandler { // send, invokesuper: blockiseq operand BlockISeq(IseqPtr), - // invokesuper: GET_BLOCK_HANDLER() (GET_LEP()[VM_ENV_DATA_INDEX_SPECVAL]) - LEPSpecVal, + // For invokesuper; equivalent to GET_BLOCK_HANDLER() + FindFromCurrentFrame, // part of the allocate-free block forwarding scheme BlockParamProxy, // To avoid holding the block arg (e.g. proc and symbol) across C calls, @@ -6870,9 +6870,20 @@ fn gen_push_frame( let cfp_self = asm.lea(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_SELF)); asm.or(cfp_self, Opnd::Imm(1)) } - BlockHandler::LEPSpecVal => { - let lep_opnd = gen_get_lep(jit, asm); - asm.load(Opnd::mem(64, lep_opnd, SIZEOF_VALUE_I32 * VM_ENV_DATA_INDEX_SPECVAL)) + BlockHandler::FindFromCurrentFrame => { + // Find the calling frame's block handler. Similar shape to implementation for + // defined?(yield). See also: vm_caller_setup_arg_block() with is_super, which + // ends up in VM_CF_BLOCK_HANDLER(). + // + // When the calling ISEQ is rooted in a ISEQ_TYPE_METHOD, the block handler is + // at local_ep[VM_ENV_DATA_INDEX_SPECVAL]. Otherwise, the frame has no block + // handler. + if ISEQ_TYPE_METHOD == unsafe { rb_get_iseq_body_type(rb_get_iseq_body_local_iseq(jit.iseq)) } { + let lep_opnd = gen_get_lep(jit, asm); + asm.load(Opnd::mem(64, lep_opnd, SIZEOF_VALUE_I32 * VM_ENV_DATA_INDEX_SPECVAL)) + } else { + VM_BLOCK_HANDLER_NONE.into() + } } BlockHandler::BlockParamProxy => { let ep_opnd = gen_get_lep(jit, asm); @@ -9880,7 +9891,7 @@ fn gen_invokesuper_specialized( let block = if let Some(iseq) = jit.get_arg(1).as_optional_ptr() { BlockHandler::BlockISeq(iseq) } else { - BlockHandler::LEPSpecVal + BlockHandler::FindFromCurrentFrame }; // Fallback to dynamic dispatch if this callsite is megamorphic @@ -9,6 +9,25 @@ # define ZJIT_STATS (USE_ZJIT && RUBY_DEBUG) #endif +// Stack map entries are either immediate Ruby VALUEs or tagged native-stack +// locations. Stack maps never contain heap VALUEs, so 0x08 is available: it is +// not Qfalse (0), and its low 3 bits are zero, so RB_SPECIAL_CONST_P is false. +#define ZJIT_STACK_MAP_VREG_TAG 0x08 +#define ZJIT_STACK_MAP_TAG_MASK 0xff +#define ZJIT_STACK_MAP_SHIFT 8 + +static inline bool +ZJIT_STACK_MAP_VREG_P(VALUE entry) +{ + return (entry & ZJIT_STACK_MAP_TAG_MASK) == ZJIT_STACK_MAP_VREG_TAG; +} + +static inline size_t +ZJIT_STACK_MAP_VREG_INDEX(VALUE entry) +{ + return entry >> ZJIT_STACK_MAP_SHIFT; +} + // JITFrame is defined here as the single source of truth and imported into // Rust via bindgen. C code reads fields directly; Rust uses an impl block. typedef struct zjit_jit_frame { @@ -23,6 +42,14 @@ typedef struct zjit_jit_frame { // (which write block_code themselves), so we must restore it. // Always false for C frames. bool materialize_block_code; + + // Number of Ruby stack slots described by stack[]. + // rb_zjit_materialize_frames() copies them to cfp->sp - stack_size. + uint32_t stack_size; + // Flexible array of stack map entries. Each entry is either an immediate + // VALUE or a tagged native-stack index from cfp->jit_return for a value + // kept by the JIT. + VALUE stack[]; } zjit_jit_frame_t; #if USE_ZJIT diff --git a/zjit/bindgen/src/main.rs b/zjit/bindgen/src/main.rs index 7c2925641c..041949be81 100644 --- a/zjit/bindgen/src/main.rs +++ b/zjit/bindgen/src/main.rs @@ -316,6 +316,8 @@ fn main() { .allowlist_function("rb_zjit_insn_leaf") .allowlist_type("jit_bindgen_constants") .allowlist_type("zjit_struct_offsets") + .allowlist_var("ZJIT_STACK_MAP_SHIFT") + .allowlist_var("ZJIT_STACK_MAP_VREG_TAG") .allowlist_var("ZJIT_JIT_RETURN_C_FRAME") .allowlist_function("rb_assert_holding_vm_lock") .allowlist_function("rb_jit_shape_complex_p") @@ -458,12 +460,25 @@ fn main() { // Unwrap the Result and panic on failure. .expect("Unable to generate bindings"); + // Write to a Vec for post-processing + let mut bindings_string = Vec::new(); + bindings.write(Box::new(&mut bindings_string)).expect("Couldn't write bindings!"); + + // Use i32 for this type since that's what the assembler APIs expect + const JIT_CONSTANTS_NEEDLE: &[u8] = b"pub type jit_bindgen_constants = u32;"; + const JIT_CONSTANTS_REPLACEMENT: &[u8] = b"pub type jit_bindgen_constants = i32;"; + // Yes, this search-and-replace could be faster, but it's a small file. + for line in bindings_string.as_mut_slice().split_mut(|&byte| byte == b'\n') { + if line == JIT_CONSTANTS_NEEDLE { + line.copy_from_slice(JIT_CONSTANTS_REPLACEMENT); + break; + } + } + + // Write out to file let mut out_path: PathBuf = src_root; out_path.push(jit_name); out_path.push("src"); out_path.push("cruby_bindings.inc.rs"); - - bindings - .write_to_file(out_path) - .expect("Couldn't write bindings!"); + std::fs::write(out_path, bindings_string).expect("file output failed"); } diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index ff1661f6fc..87b388eefa 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -263,6 +263,9 @@ impl Assembler { Opnd::Mem(mem) => { if mem_disp_fits_bits(mem.disp) { opnd + } else if asm.accept_scratch_reg { + asm.lea_into(SCRATCH1_OPND, Opnd::Mem(Mem { num_bits: 64, ..mem })); + Opnd::mem(mem.num_bits, SCRATCH1_OPND, 0) } else { let base = asm.lea(Opnd::Mem(Mem { num_bits: 64, ..mem })); Opnd::mem(mem.num_bits, base, 0) @@ -627,6 +630,12 @@ impl Assembler { *opnd = split_load_operand(asm, *opnd); asm.push_insn(insn); }, + Insn::Store { dest, .. } => { + if asm.accept_scratch_reg && matches!(dest, Opnd::Mem(_)) { + *dest = split_memory_address(asm, *dest); + } + asm.push_insn(insn); + }, Insn::Mul { left, right, .. } => { *left = split_load_operand(asm, *left); *right = split_load_operand(asm, *right); @@ -1673,7 +1682,7 @@ impl Assembler { }); trace_compile_phase("resolve_ssa", || { - asm.handle_caller_saved_regs(&intervals, &assignments, &C_ARG_REGREGS); + asm.handle_caller_saved_regs(&intervals, &assignments, &C_ARG_REGREGS, total_stack_slots); asm.resolve_ssa(&intervals, &assignments); }); @@ -2193,6 +2202,19 @@ mod tests { } #[test] + fn test_store_with_scratch_reg_and_large_displacement() { + let (mut asm, mut cb, _) = setup_asm_with_scratch_reg(); + asm.store(Opnd::mem(64, SP, -0x140), C_RET_OPND); + + asm.compile_with_num_regs(&mut cb, 0); + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: sub x17, x21, #0x140 + 0x4: stur x0, [x17] + "); + assert_snapshot!(cb.hexdump(), @"b10205d1200200f8"); + } + + #[test] #[should_panic] fn test_store_with_invalid_scratch_reg() { let (_, scratch_reg) = Assembler::new_with_scratch_reg(); diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index f394062d51..84c8c8ba0f 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -4,11 +4,10 @@ use std::mem::take; use std::rc::Rc; use crate::bitset::BitSet; use crate::codegen::{local_size_and_idx_to_ep_offset, perf_symbol_range_start, perf_symbol_range_end}; -use crate::cruby::{IseqPtr, RUBY_OFFSET_CFP_ISEQ, RUBY_OFFSET_CFP_JIT_RETURN, RUBY_OFFSET_CFP_PC, RUBY_OFFSET_CFP_SP, SIZEOF_VALUE_I32, vm_stack_canary, YarvInsnIdx }; +use crate::cruby::{IseqPtr, RUBY_OFFSET_CFP_ISEQ, RUBY_OFFSET_CFP_JIT_RETURN, RUBY_OFFSET_CFP_PC, RUBY_OFFSET_CFP_SP, SIZEOF_VALUE_I32, VALUE, ZJIT_STACK_MAP_SHIFT, ZJIT_STACK_MAP_VREG_TAG, vm_stack_canary, YarvInsnIdx, zjit_jit_frame}; use crate::hir::{Invariant, SideExitReason}; use crate::hir; use crate::options::{TraceExits, PerfMap, get_option}; -use crate::cruby::VALUE; use crate::payload::{IseqVersionRef, get_or_create_iseq_payload}; use crate::stats::{exit_counter_ptr, exit_counter_ptr_for_opcode, side_exit_counter, CompileError}; use crate::virtualmem::CodePtr; @@ -698,6 +697,7 @@ pub enum Insn { /// that are split from this CCall during register assignment. end_marker: Option<PosMarkerFn>, out: Opnd, + stack_map: Option<StackMap>, }, // C function return @@ -940,8 +940,11 @@ macro_rules! for_each_operand_impl { visit_one!(opnd0); visit_one!(opnd1); } - Insn::CCall { opnds, .. } => { + Insn::CCall { opnds, stack_map, .. } => { visit_many!(opnds); + if let Some(StackMap { stack, .. }) = stack_map { + visit_many!(stack); + } } // only iterate over preserved in the const iterator #[allow(unused_variables)] @@ -1387,6 +1390,18 @@ impl StackState { } } +/// Stack map to materialize Ruby stack slots from JIT-kept values. +#[derive(Clone, Debug)] +pub struct StackMap { + /// Ruby stack slots to reconstruct if this frame is materialized. + /// Each operand must be either an immediate Ruby VALUE or a VReg whose + /// final register/spill location will be encoded after register allocation. + stack: Vec<Opnd>, + /// Heap-allocated JITFrame whose trailing stack map storage receives the + /// encoded entries once this CCall's register allocation is known. + jit_frame: *const zjit_jit_frame, +} + /// Initial capacity for asm.insns vector const ASSEMBLER_INSNS_CAPACITY: usize = 256; @@ -1420,6 +1435,11 @@ pub struct Assembler { /// Current instruction index, incremented for each instruction pushed idx: usize, + + /// Pending stack map to attach to the next CCall. The register allocator + /// consumes this through Insn::CCall, after it knows whether each live VReg + /// is in a saved register or an allocator spill slot. + stack_map: Option<StackMap>, } impl Assembler @@ -1435,6 +1455,7 @@ impl Assembler current_block_id: BlockId(0), num_vregs: 0, idx: 0, + stack_map: None, } } @@ -2103,6 +2124,7 @@ impl Assembler intervals: &[Interval], assignments: &[Option<Allocation>], regs: &[Reg], + total_stack_slots: usize, ) { use crate::backend::parcopy; use crate::backend::current::{C_RET_OPND, SCRATCH_REG, ALLOC_REGS}; @@ -2116,7 +2138,7 @@ impl Assembler let mut new_ids = Vec::with_capacity(old_ids.len()); for (insn, insn_id) in old_insns.into_iter().zip(old_ids.into_iter()) { - if let Insn::CCall { opnds, out, start_marker, end_marker, fptr } = insn { + if let Insn::CCall { opnds, out, start_marker, end_marker, fptr, stack_map } = insn { let insn_number = insn_id.map(|id| id.0).unwrap_or(0); // Do we have a case where a ccall is emitted, but nobody // uses the result? @@ -2126,15 +2148,29 @@ impl Assembler .end .is_some_and(|end| end > insn_number); + // Build a set of VRegIds that can be referenced by JITFrame for materializing the VM stack + let stack_vreg_ids: HashSet<usize> = if let Some(StackMap { stack, .. }) = &stack_map { + stack.iter().filter_map(|opnd| match opnd { + Opnd::VReg { idx: VRegId(vreg_id), .. } => Some(*vreg_id), + _ => None, + }).collect() + } else { + HashSet::default() + }; + // Find survivors: intervals that survive this Call instruction // We need to preserve the "surviving" registers past the ccall, // so we're going to push them all on the stack, then pop // after we make the ccall let survivors: Vec<usize> = intervals.iter() .filter(|interval| { - interval.has_bounds() - && interval.survives(insn_number) - && assignments[interval.id].and_then(|alloc| alloc.alloc_pool_index(ALLOC_REGS.len())).is_some() + // We need to spill register intervals on this CCall in two cases: + // 1) The VReg is referenced in an instruction after the CCall + let survives_call = interval.has_bounds() && interval.survives(insn_number); + // 2) The VReg is referenced by the stack map for the CCall + let stack_map_reg = stack_vreg_ids.contains(&interval.id); + let is_register = assignments[interval.id].and_then(|alloc| alloc.alloc_pool_index(ALLOC_REGS.len())).is_some(); + is_register && (survives_call || stack_map_reg) }) .map(|interval| interval.id) .collect(); @@ -2156,6 +2192,59 @@ impl Assembler } new_ids.push(None); } + + if let Some(StackMap { stack, jit_frame }) = stack_map { + assert_eq!(unsafe { (*jit_frame).stack_size } as usize, stack.len()); + for (idx, stack_opnd) in stack.iter().enumerate() { + let entry = match stack_opnd { + Opnd::UImm(value) => { + let value = VALUE(*value as usize); + // TODO: Investigate using a constant pool to track any value reference in the stack map + assert!(value.special_const_p(), "StackMap should only materialize immediate VALUEs, but got: {value:?}"); + value + } + Opnd::VReg { idx: VRegId(vreg_id), .. } => { + // Calculate the offset from NATIVE_BASE_PTR to the stack slot for this VReg. + let vreg_stack_index = match assignments[*vreg_id].expect("StackMap VReg should have an allocation") { + Allocation::Reg(_) | Allocation::Fixed(_) => { + let position = survivors.iter().position(|&survivor_id| survivor_id == *vreg_id).unwrap(); + // See Assembler::alloc_stack's native stack diagram. rb_zjit_materialize_frames() + // reads vreg_stack_index as cfp->jit_return[-vreg_stack_index]. + // For both arches, FrameSetup may add one native stack slot for + // alignment before these CPushPairs. + // TODO: Centralize stack slot offset calculation in StackState. + let frame_alignment_slots = if total_stack_slots % 2 == 1 { + 1 + } else { + 0 + }; + (total_stack_slots + frame_alignment_slots) + .checked_add(1) + .expect("StackMap requires a JITFrame slot") + + position + } + Allocation::Stack(stack_idx) => { + // StackState places allocator spills at: + // cfp->jit_return[-(self.stack_base_idx + stack_idx + 1)] + // so encode the matching materializer index. + self.stack_base_idx + .checked_add(1) + .expect("StackMap requires a JITFrame slot") + + stack_idx + } + }; + + // Encode the offset as a shifted-and-tagged integer. + let encoded = (vreg_stack_index << ZJIT_STACK_MAP_SHIFT) | ZJIT_STACK_MAP_VREG_TAG as usize; + debug_assert!(!VALUE(encoded).special_const_p(), "encoded StackMap VReg should not look like an immediate VALUE"); + VALUE(encoded) + } + _ => unreachable!("unexpected operand in StackMap: {stack_opnd:?}"), + }; + unsafe { (*jit_frame.cast_mut()).stack.as_mut_ptr().add(idx).write(entry); } + } + } + // Extract arguments from CCall, clear opnds assert!(opnds.len() <= regs.len()); @@ -2198,7 +2287,8 @@ impl Assembler // be empty now start_marker: None, end_marker: None, - fptr + fptr, + stack_map: None, }); new_ids.push(insn_id); @@ -3228,7 +3318,8 @@ impl Assembler { let canary_opnd = self.set_stack_canary(); let out = self.new_vreg(Opnd::match_num_bits(&opnds)); let fptr = Opnd::const_ptr(fptr); - self.push_insn(Insn::CCall { fptr, opnds, start_marker: None, end_marker: None, out }); + let stack_map = self.stack_map.take(); + self.push_insn(Insn::CCall { fptr, opnds, start_marker: None, end_marker: None, out, stack_map }); self.clear_stack_canary(canary_opnd); out } @@ -3237,14 +3328,16 @@ impl Assembler { /// new vreg for the result. pub fn ccall_into(&mut self, out: Opnd, fptr: *const u8, opnds: Vec<Opnd>) { let fptr = Opnd::const_ptr(fptr); - self.push_insn(Insn::CCall { fptr, opnds, start_marker: None, end_marker: None, out }); + let stack_map = self.stack_map.take(); + self.push_insn(Insn::CCall { fptr, opnds, start_marker: None, end_marker: None, out, stack_map }); } /// Call a C function stored in a register pub fn ccall_reg(&mut self, fptr: Opnd, num_bits: u8) -> Opnd { assert!(matches!(fptr, Opnd::Reg(_)), "ccall_reg must be called with Opnd::Reg: {fptr:?}"); let out = self.new_vreg(num_bits); - self.push_insn(Insn::CCall { fptr, opnds: vec![], start_marker: None, end_marker: None, out }); + let stack_map = self.stack_map.take(); + self.push_insn(Insn::CCall { fptr, opnds: vec![], start_marker: None, end_marker: None, out, stack_map }); out } @@ -3258,12 +3351,14 @@ impl Assembler { end_marker: impl Fn(CodePtr, &CodeBlock) + 'static, ) -> Opnd { let out = self.new_vreg(Opnd::match_num_bits(&opnds)); + let stack_map = self.stack_map.take(); self.push_insn(Insn::CCall { fptr: Opnd::const_ptr(fptr), opnds, start_marker: Some(Rc::new(start_marker)), end_marker: Some(Rc::new(end_marker)), out, + stack_map, }); out } @@ -3492,6 +3587,15 @@ impl Assembler { out } + /// Attach a stack map to the next CCall emitted by this Assembler. + /// The map is queued here because gen_stack_map() runs before the CCall is + /// emitted, but the map is filled only after register allocation assigns + /// locations to the VRegs it references. + pub fn stack_map(&mut self, stack: Vec<Opnd>, jit_frame: *const zjit_jit_frame) { + assert!(self.stack_map.is_none()); + self.stack_map = Some(StackMap { stack, jit_frame }); + } + pub fn store(&mut self, dest: Opnd, src: Opnd) { assert!(!matches!(dest, Opnd::VReg { .. }), "Destination of store must not be Opnd::VReg, got: {dest:?}"); self.push_insn(Insn::Store { dest, src }); @@ -4405,6 +4509,7 @@ mod tests { start_marker: None, end_marker: None, out: v3, + stack_map: None, }); // v4 = Add(v3, v1) @@ -4436,7 +4541,7 @@ mod tests { "v1 should be in a register"); // Run the pipeline: handle_caller_saved_regs then resolve_ssa - asm.handle_caller_saved_regs(&intervals, &assignments, regs); + asm.handle_caller_saved_regs(&intervals, &assignments, regs, 0); asm.resolve_ssa(&intervals, &assignments); let insns = &asm.basic_blocks[b1.0].insns; diff --git a/zjit/src/backend/tests.rs b/zjit/src/backend/tests.rs index 7174ac4c80..e38dc4707f 100644 --- a/zjit/src/backend/tests.rs +++ b/zjit/src/backend/tests.rs @@ -182,9 +182,9 @@ fn test_jcc_ptr() let (mut asm, mut cb) = setup_asm(); let side_exit = Target::CodePtr(cb.get_write_ptr().add_bytes(4)); - let not_mask = asm.not(Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_MASK as i32)); + let not_mask = asm.not(Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_MASK)); asm.test( - Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_FLAG as i32), + Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_FLAG), not_mask, ); asm.push_insn(Insn::Jnz(side_exit)); diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index d3bf847ab2..0f4acf1cef 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -1195,7 +1195,7 @@ impl Assembler { }); trace_compile_phase("resolve_ssa", || { - asm.handle_caller_saved_regs(&intervals, &assignments, &C_ARG_REGREGS); + asm.handle_caller_saved_regs(&intervals, &assignments, &C_ARG_REGREGS, total_stack_slots); asm.resolve_ssa(&intervals, &assignments); }); diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index c53e0a0f9b..bb21c3dda2 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -626,9 +626,6 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio Insn::ObjectAlloc { val, state } => gen_object_alloc(jit, asm, opnd!(val), &function.frame_state(*state)), &Insn::ObjectAllocClass { class, state } => gen_object_alloc_class(asm, class, &function.frame_state(state)), Insn::StringCopy { val, chilled, state } => gen_string_copy(asm, opnd!(val), *chilled, &function.frame_state(*state)), - // concatstrings shouldn't have 0 strings - // If it happens we abort the compilation for now - Insn::StringConcat { strings, state, .. } if strings.is_empty() => return Err(*state), Insn::StringConcat { strings, state } => gen_string_concat(jit, asm, opnds!(strings), &function.frame_state(*state)), &Insn::StringGetbyte { string, index } => gen_string_getbyte(asm, opnd!(string), opnd!(index)), Insn::StringSetbyteFixnum { string, index, value } => gen_string_setbyte_fixnum(asm, opnd!(string), opnd!(index), opnd!(value)), @@ -659,9 +656,6 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio &Insn::InvokeBlock { cd, state, reason, .. } => gen_invokeblock(jit, asm, cd, &function.frame_state(state), reason), Insn::InvokeBlockIfunc { cd, block_handler, args, state, .. } => gen_invokeblock_ifunc(jit, asm, *cd, opnd!(block_handler), opnds!(args), &function.frame_state(*state)), Insn::InvokeProc { recv, args, state, kw_splat } => gen_invokeproc(jit, asm, opnd!(recv), opnds!(args), *kw_splat, &function.frame_state(*state)), - // Ensure we have enough room fit ec, self, and arguments - // TODO remove this check when we have stack args (we can use Time.new to test it) - Insn::InvokeBuiltin { bf, state, .. } if bf.argc + 2 > (C_ARG_OPNDS.len() as i32) => return Err(*state), Insn::InvokeBuiltin { bf, leaf, args, state, .. } => gen_invokebuiltin(jit, asm, &function.frame_state(*state), bf, *leaf, opnds!(args)), &Insn::EntryPoint { jit_entry_idx } => no_output!(gen_entry_point(jit, asm, jit_entry_idx)), Insn::Return { val } => no_output!(gen_return(asm, opnd!(val))), @@ -722,12 +716,6 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio &Insn::GuardGreaterEq { left, right, state, .. } => gen_guard_greater_eq(jit, asm, opnd!(left), opnd!(right), &function.frame_state(state)), Insn::PatchPoint { invariant, state } => no_output!(gen_patch_point(jit, asm, invariant, &function.frame_state(*state))), Insn::CCall { cfunc, recv, args, name, owner: _, return_type: _, elidable: _ } => gen_ccall(asm, *cfunc, *name, opnd!(recv), opnds!(args)), - // Give up CCallWithFrame for 7+ args since asm.ccall() supports at most 6 args (recv + args). - // We're currently emitting a CCallWithFrame for `super` in to a cfunction. - // We can't lower to `gen_send_without_block` because the - // source opcode isn't necessarily `opt_send_without_block` - // and so the interpreter stack layout may be incompatible. - Insn::CCallWithFrame { cd, state, args, block, .. } if args.len() + 1 > C_ARG_OPNDS.len() => return Err(*state), Insn::CCallWithFrame { cfunc, recv, name, args, cme, state, block, .. } => gen_ccall_with_frame(jit, asm, *cfunc, *name, opnd!(recv), opnds!(args), *cme, *block, &function.frame_state(*state)), Insn::CCallVariadic { cfunc, recv, name, args, cme, state, block, return_type: _, elidable: _ } => { @@ -770,7 +758,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio Insn::LoadEC => gen_load_ec(), Insn::LoadSP => gen_load_sp(), &Insn::GetEP { level } => gen_get_ep(asm, level), - Insn::LoadSelf => gen_load_self(), + Insn::LoadSelf => gen_load_self(asm), &Insn::LoadField { recv, id, offset, return_type } => gen_load_field(asm, opnd!(recv), id, offset, return_type), &Insn::StoreField { recv, id, offset, val } => no_output!(gen_store_field(asm, opnd!(recv), id, offset, opnd!(val), function.type_of(val))), &Insn::WriteBarrier { recv, val } => no_output!(gen_write_barrier(jit, asm, opnd!(recv), opnd!(val), function.type_of(val))), @@ -967,6 +955,7 @@ fn gen_fixnum_bit_check(asm: &mut Assembler, val: Opnd, index: u8) -> Opnd { } fn gen_invokebuiltin(jit: &JITState, asm: &mut Assembler, state: &FrameState, bf: &rb_builtin_function, leaf: bool, args: Vec<Opnd>) -> lir::Opnd { + // +2 for ec, self assert!(bf.argc + 2 <= C_ARG_OPNDS.len() as i32, "gen_invokebuiltin should not be called for builtin function {} with too many arguments: {}", unsafe { std::ffi::CStr::from_ptr(bf.name).to_str().unwrap() }, @@ -1052,11 +1041,14 @@ fn gen_ccall_with_frame( gen_stack_overflow_check(jit, asm, state, state.stack_size()); let args_with_recv_len = args.len() + 1; + if args_with_recv_len > C_ARG_OPNDS.len() { + unimplemented!("Passing C call arguments on the stack"); + } let caller_stack_size = state.stack().len() - args_with_recv_len; // Can't use gen_prepare_non_leaf_call() because we need to adjust the SP // to account for the receiver and arguments (and block arguments if any) - gen_save_pc_for_gc(asm, state); + gen_save_pc_for_gc(asm, state, 0); gen_save_sp(asm, caller_stack_size); gen_spill_stack(jit, asm, state); gen_spill_locals(jit, asm, state); @@ -1089,7 +1081,7 @@ fn gen_ccall_with_frame( asm_comment!(asm, "switch to new CFP"); let new_cfp = asm.sub(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, new_cfp); - asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); let mut cfunc_args = vec![recv]; cfunc_args.extend(args); @@ -1099,7 +1091,7 @@ fn gen_ccall_with_frame( asm_comment!(asm, "pop C frame"); let new_cfp = asm.add(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, new_cfp); - asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); asm_comment!(asm, "restore SP register for the caller"); let new_sp = asm.sub(SP, sp_offset.into()); @@ -1150,7 +1142,7 @@ fn gen_ccall_variadic( // Can't use gen_prepare_non_leaf_call() because we need to adjust the SP // to account for the receiver and arguments (like gen_ccall_with_frame does) - gen_save_pc_for_gc(asm, state); + gen_save_pc_for_gc(asm, state, 0); gen_save_sp(asm, caller_stack_size); gen_spill_stack(jit, asm, state); gen_spill_locals(jit, asm, state); @@ -1178,7 +1170,7 @@ fn gen_ccall_variadic( asm_comment!(asm, "switch to new CFP"); let new_cfp = asm.sub(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, new_cfp); - asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); let argv_ptr = gen_push_opnds(jit, asm, &args); asm.count_call_to(&name.contents_lossy()); @@ -1187,7 +1179,7 @@ fn gen_ccall_variadic( asm_comment!(asm, "pop C frame"); let new_cfp = asm.add(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, new_cfp); - asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); asm_comment!(asm, "restore SP register for the caller"); let new_sp = asm.sub(SP, sp_offset.into()); @@ -1312,7 +1304,7 @@ fn gen_check_interrupts(jit: &mut JITState, asm: &mut Assembler, state: &FrameSt asm_comment!(asm, "RUBY_VM_CHECK_INTS(ec)"); // Not checking interrupt_mask since it's zero outside finalize_deferred_heap_pages, // signal_exec, or rb_postponed_job_flush. - let interrupt_flag = asm.load(Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_FLAG as i32)); + let interrupt_flag = asm.load(Opnd::mem(32, EC, RUBY_OFFSET_EC_INTERRUPT_FLAG)); asm.test(interrupt_flag, interrupt_flag); asm.jnz(jit, side_exit(jit, state, SideExitReason::Interrupt)); } @@ -1382,8 +1374,8 @@ fn gen_load_sp() -> Opnd { SP } -fn gen_load_self() -> Opnd { - Opnd::mem(64, CFP, RUBY_OFFSET_CFP_SELF) +fn gen_load_self(asm: &mut Assembler) -> Opnd { + asm.load(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_SELF)) } fn gen_load_field(asm: &mut Assembler, recv: Opnd, id: FieldName, offset: i32, return_type: Type) -> Opnd { @@ -1491,7 +1483,7 @@ fn gen_send( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call #{} with dynamic dispatch", ruby_call_method_name(cd)); unsafe extern "C" { fn rb_vm_send(ec: EcPtr, cfp: CfpPtr, cd: VALUE, blockiseq: IseqPtr) -> VALUE; @@ -1514,7 +1506,7 @@ fn gen_send_forward( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call #{} with dynamic dispatch", ruby_call_method_name(cd)); unsafe extern "C" { @@ -1537,7 +1529,7 @@ fn gen_send_without_block( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call #{} with dynamic dispatch", ruby_call_method_name(cd)); unsafe extern "C" { fn rb_vm_opt_send_without_block(ec: EcPtr, cfp: CfpPtr, cd: VALUE) -> VALUE; @@ -1568,8 +1560,9 @@ fn gen_push_inline_frame( // Save cfp->pc and cfp->sp for the caller frame. // Cannot use gen_prepare_non_leaf_call because we need special SP math. - gen_save_pc_for_gc(asm, state); - gen_save_sp(asm, state.stack().len() - args.len() - 1); // -1 for receiver + let stack_size = state.stack().len() - args.len() - 1; // -1 for receiver + gen_save_pc_for_gc(asm, state, 0); + gen_save_sp(asm, stack_size); gen_spill_locals(jit, asm, state); gen_spill_stack(jit, asm, state); @@ -1636,7 +1629,7 @@ fn gen_push_inline_frame( } let callee_depth = state.depth + 1; let callee_entry_pc = unsafe { rb_iseq_pc_at_idx(iseq, 0) }; - let callee_entry_frame = JITFrame::new_iseq(callee_entry_pc, iseq); + let callee_entry_frame = JITFrame::new_iseq(callee_entry_pc, iseq, 0); asm_comment!(asm, "install entry JITFrame for inlined callee"); asm.mov(Opnd::mem(64, NATIVE_BASE_PTR, jit_frame_slot_offset(callee_depth)), Opnd::const_ptr(callee_entry_frame)); let callee_jit_return = cfp_jit_return_for_depth(asm, callee_depth); @@ -1704,11 +1697,12 @@ fn gen_send_iseq_direct( // Save cfp->pc and cfp->sp for the caller frame // Can't use gen_prepare_non_leaf_call because we need special SP math. - gen_save_pc_for_gc(asm, state); - gen_save_sp(asm, state.stack().len() - args.len() - 1); // -1 for receiver + let stack_size = state.stack().len() - args.len() - 1; // -1 for receiver + let jit_frame = gen_save_pc_for_gc(asm, state, stack_size); + gen_save_sp(asm, stack_size); gen_spill_locals(jit, asm, state); - gen_spill_stack(jit, asm, state); + gen_stack_map(jit, asm, state, stack_size, jit_frame); // This mirrors vm_caller_setup_arg_block() in for the `blockiseq != NULL` case. // The HIR specialization guards ensure we will only reach here for literal blocks, @@ -1767,7 +1761,7 @@ fn gen_send_iseq_direct( asm_comment!(asm, "switch to new CFP"); let new_cfp = asm.sub(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, new_cfp); - asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.store(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); let params = unsafe { iseq.params() }; @@ -1843,7 +1837,7 @@ fn gen_invokeblock( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call invokeblock"); unsafe extern "C" { @@ -1868,7 +1862,7 @@ fn gen_invokeblock_ifunc( ) -> lir::Opnd { let _ = cd; // cd is not needed for the direct call - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); // Push args to memory so we can pass argv pointer let argv_ptr = gen_push_opnds(jit, asm, &args); @@ -1898,7 +1892,7 @@ fn gen_invokeproc( kw_splat: bool, state: &FrameState, ) -> lir::Opnd { - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call invokeproc"); @@ -1927,7 +1921,7 @@ fn gen_invokesuper( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call super with dynamic dispatch"); unsafe extern "C" { fn rb_vm_invokesuper(ec: EcPtr, cfp: CfpPtr, cd: VALUE, blockiseq: IseqPtr) -> VALUE; @@ -1950,7 +1944,7 @@ fn gen_invokesuperforward( ) -> lir::Opnd { gen_incr_send_fallback_counter(asm, reason); - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); asm_comment!(asm, "call super with dynamic dispatch (forwarding)"); unsafe extern "C" { fn rb_vm_invokesuperforward(ec: EcPtr, cfp: CfpPtr, cd: VALUE, blockiseq: IseqPtr) -> VALUE; @@ -2072,7 +2066,7 @@ fn gen_opt_newarray_hash( state: &FrameState, ) -> lir::Opnd { // `Array#hash` will hash the elements of the array. - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); let array_len: c_long = elements.len().try_into().expect("Unable to fit length of elements into c_long"); @@ -2098,7 +2092,7 @@ fn gen_array_max( elements: Vec<Opnd>, state: &FrameState, ) -> lir::Opnd { - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); let array_len: u32 = elements.len().try_into().expect("Unable to fit length of elements into u32"); @@ -2124,7 +2118,7 @@ fn gen_array_min( elements: Vec<Opnd>, state: &FrameState, ) -> lir::Opnd { - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); let array_len: u32 = elements.len().try_into().expect("Unable to fit length of elements into u32"); @@ -2150,7 +2144,7 @@ fn gen_array_include( target: Opnd, state: &FrameState, ) -> lir::Opnd { - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); let array_len: c_long = elements.len().try_into().expect("Unable to fit length of elements into c_long"); @@ -2178,7 +2172,7 @@ fn gen_array_pack_buffer( buffer: Option<Opnd>, state: &FrameState, ) -> lir::Opnd { - gen_prepare_non_leaf_call(jit, asm, state); + gen_prepare_fallback_call(jit, asm, state); let array_len: c_long = elements.len().try_into().expect("Unable to fit length of elements into c_long"); @@ -2365,7 +2359,7 @@ fn gen_entry_point(jit: &mut JITState, asm: &mut Assembler, jit_entry_idx: Optio // Publish a valid entry JITFrame before setting cfp->jit_return. The entry point is // always the top-level frame (depth 0). Inlined frames get their own deeper // slots in gen_push_lightweight_frame(). - let jit_frame = JITFrame::new_iseq(entry_pc(jit.iseq(), jit_entry_idx), jit.iseq()); + let jit_frame = JITFrame::new_iseq(entry_pc(jit.iseq(), jit_entry_idx), jit.iseq(), 0); asm.mov(Opnd::mem(64, NATIVE_BASE_PTR, -SIZEOF_VALUE_I32), Opnd::const_ptr(jit_frame)); asm.mov(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_JIT_RETURN), NATIVE_BASE_PTR); } @@ -2377,7 +2371,7 @@ fn gen_return(asm: &mut Assembler, val: lir::Opnd) { asm_comment!(asm, "pop stack frame"); let incr_cfp = asm.add(CFP, RUBY_SIZEOF_CONTROL_FRAME.into()); asm.mov(CFP, incr_cfp); - asm.mov(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP as i32), CFP); + asm.mov(Opnd::mem(64, EC, RUBY_OFFSET_EC_CFP), CFP); // Order here is important. Because we're about to tear down the frame, // we need to load the return value, which might be part of the frame. @@ -2891,13 +2885,13 @@ fn cfp_jit_return_for_depth(asm: &mut Assembler, depth: InlineDepth) -> Opnd { /// Save only the PC to CFP. Use this when you need to call gen_save_sp() /// immediately after with a custom stack size (e.g., gen_ccall_with_frame /// adjusts SP to exclude receiver and arguments). -fn gen_save_pc_for_gc(asm: &mut Assembler, state: &FrameState) { +fn gen_save_pc_for_gc(asm: &mut Assembler, state: &FrameState, stack_map_size: usize) -> *const zjit_jit_frame { let opcode: usize = state.get_opcode().try_into().unwrap(); let next_pc: *const VALUE = unsafe { state.pc.offset(insn_len(opcode) as isize) }; gen_incr_counter(asm, Counter::vm_write_jit_frame_count); asm_comment!(asm, "save JITFrame to CFP"); - let jit_frame = JITFrame::new_iseq(next_pc, state.iseq); + let jit_frame = JITFrame::new_iseq(next_pc, state.iseq, stack_map_size); asm.mov(Opnd::mem(64, NATIVE_BASE_PTR, jit_frame_slot_offset(state.depth)), Opnd::const_ptr(jit_frame)); // CFP_PC for a live JIT frame routes through the JITFrame on the native @@ -2907,6 +2901,7 @@ fn gen_save_pc_for_gc(asm: &mut Assembler, state: &FrameState) { // jit_frame->pc into cfp->pc and cleared cfp->jit_return: the JIT keeps // running, lands on this routine again, and the poison would replace // the valid materialized pc behind the GC's back. + jit_frame } /// Save the current PC on the CFP as a preparation for calling a C function @@ -2917,12 +2912,13 @@ fn gen_save_pc_for_gc(asm: &mut Assembler, state: &FrameState) { /// because the backend spills all live registers onto the C stack on CCall. /// However, to avoid marking uninitialized stack slots, this also updates SP, /// which may have cfp->sp for a past frame or a past non-leaf call. -fn gen_prepare_call_with_gc(asm: &mut Assembler, state: &FrameState, leaf: bool) { - gen_save_pc_for_gc(asm, state); +fn gen_prepare_call_with_gc(asm: &mut Assembler, state: &FrameState, leaf: bool, stack_map_size: usize) -> *const zjit_jit_frame { + let jit_frame = gen_save_pc_for_gc(asm, state, stack_map_size); gen_save_sp(asm, state.stack_size()); if leaf { asm.expect_leaf_ccall(state.stack_size()); } + jit_frame } fn gen_prepare_leaf_call_with_gc(asm: &mut Assembler, state: &FrameState) { @@ -2939,7 +2935,7 @@ fn gen_prepare_leaf_call_with_gc(asm: &mut Assembler, state: &FrameState) { // We use state.without_stack() to pass stack_size=0 to gen_save_sp() because we don't write // VM stack slots on leaf calls, which leaves those stack slots uninitialized. ZJIT keeps // live objects on the C stack, so they are protected from GC properly. - gen_prepare_call_with_gc(asm, &state.without_stack(), true); + gen_prepare_call_with_gc(asm, &state.without_stack(), true, 0); } /// Save the current SP on the CFP @@ -2976,17 +2972,43 @@ fn gen_spill_stack(jit: &JITState, asm: &mut Assembler, state: &FrameState) { } } +/// Prepare for VM fallback helpers that read arguments from the VM stack. +/// +/// Direct JIT-to-JIT calls keep cfp->sp lazy, so this must publish SP before +/// writing stack slots. Otherwise spilling the stack can overwrite frame +/// metadata below the real VM-stack base. +fn gen_prepare_fallback_call(jit: &JITState, asm: &mut Assembler, state: &FrameState) { + gen_save_pc_for_gc(asm, state, 0); + gen_save_sp(asm, state.stack_size()); + gen_spill_locals(jit, asm, state); + gen_spill_stack(jit, asm, state); +} + +/// Record the Ruby stack values needed to materialize this frame after the next +/// non-leaf C call. The actual JITFrame entries are encoded by the register +/// allocator, where VReg locations on the native stack are known. +fn gen_stack_map(jit: &JITState, asm: &mut Assembler, state: &FrameState, stack_size: usize, jit_frame: *const zjit_jit_frame) { + let mut stack = Vec::new(); + for &insn_id in state.stack().take(stack_size) { + let opnd = jit.get_opnd(insn_id); + // JITFrame only supports materializing Opnd::Value or Opnd::VReg out of the frame + assert!(matches!(opnd, Opnd::Value(_) | Opnd::VReg { .. }), "FrameState should only reference Opnd::Value or Opnd::VReg, but got: {opnd:?}"); + stack.push(opnd); + } + asm.stack_map(stack, jit_frame); +} + /// Prepare for calling a C function that may call an arbitrary method. /// Use gen_prepare_leaf_call_with_gc() if the method is leaf but allocates objects. fn gen_prepare_non_leaf_call(jit: &JITState, asm: &mut Assembler, state: &FrameState) { // TODO: Lazily materialize caller frames when needed // Save PC for backtraces and allocation tracing // and SP to avoid marking uninitialized stack slots - gen_prepare_call_with_gc(asm, state, false); + let jit_frame = gen_prepare_call_with_gc(asm, state, false, state.stack_size()); // Spill the virtual stack in case it raises an exception // and the interpreter uses the stack for handling the exception - gen_spill_stack(jit, asm, state); + gen_stack_map(jit, asm, state, state.stack_size(), jit_frame); // Spill locals in case the method looks at caller Bindings gen_spill_locals(jit, asm, state); @@ -3436,6 +3458,20 @@ fn gen_function_stub(cb: &mut CodeBlock, iseq_call: IseqCallRef) -> Result<CodeP asm.new_block_without_id("gen_function_stub"); asm_comment!(asm, "Stub: {}", iseq_get_location(iseq_call.iseq.get(), 0)); + // If the stubbed ISEQ fails to compile, function_stub_hit exits to the + // interpreter with this callee frame. Direct JIT-to-JIT calls pass arguments + // in C argument registers, so spill the packed argument locals first. The + // fallback path will reshape these around any optional positional gaps. + let argc = iseq_call.argc.to_usize(); + assert!(argc < C_ARG_OPNDS.len(), "SendDirect must fit receiver plus arguments in C argument registers"); + let local_size = unsafe { get_iseq_body_local_table_size(iseq_call.iseq.get()) }.to_usize(); + for arg_idx in 0..argc { + asm.store( + Opnd::mem(64, SP, -local_size_and_idx_to_bp_offset(local_size, arg_idx) * SIZEOF_VALUE_I32), + C_ARG_OPNDS[arg_idx + 1], + ); + } + // Call function_stub_hit using the shared trampoline. See `gen_function_stub_hit_trampoline`. // Use load_into instead of mov, which is split on arm64, to avoid clobbering ALLOC_REGS. asm.load_into(scratch_reg, Opnd::const_ptr(Rc::into_raw(iseq_call))); @@ -3541,8 +3577,10 @@ pub fn gen_materialize_exit_trampoline(cb: &mut CodeBlock, exit_trampoline: Code let mut asm = Assembler::new(); asm.new_block_without_id("materialize_exit_trampoline"); - asm_comment!(asm, "materialize ZJIT frames"); + asm_comment!(asm, "clear JITFrame materialized by exit code"); asm.store(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_JIT_RETURN), 0.into()); + + asm_comment!(asm, "materialize ZJIT frames"); asm_ccall!(asm, rb_zjit_materialize_frames, EC, CFP); asm.jmp(Target::CodePtr(exit_trampoline)); diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index b052147add..99d5f90516 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -1294,6 +1294,69 @@ fn test_invokesuper_to_cfunc_with_too_many_args_exits() { "#), @"[1, 2, 3, 4, 5, 6]"); } +// Repro for the production "Failed to get_opnd(vN)" panic +// (PriceRs::PricingService#build_rust_adjustment_from_row, introduced by #17186). +// +// A regular send to a C method with 7 fixed args is reduced to a CCallWithFrame +// with recv + 7 = 8 operands, which exceeds C_ARG_OPNDS.len() (6). gen_insn bails +// with `return Err(*state)`; the caller emits a side exit and `break`s out of the +// block. But the call's *result* is stored in a local and used in a *later* basic +// block (the `if` arm here). Because codegen bailed before assigning a LIR operand +// to the result, compiling that later block calls get_opnd(result) on a None entry +// and panics. The existing `test_invokesuper_to_cfunc_with_too_many_args_exits` does +// not catch this because there the call result is the method's tail value and is not +// referenced past the bailed block. +// +// NOTE: This currently ABORTS with `Failed to get_opnd(vN)` (the bug). The snapshot +// below is the expected behavior once the backend exits cleanly: `flag` is true so +// `test` returns the cfunc's result, the array [1, 2, 3, 4, 5, 6, 7]. +#[test] +fn test_ccall_with_frame_too_many_args_result_used_in_later_block() { + unsafe extern "C" fn test_seven_args( + _self: VALUE, + a: VALUE, + b: VALUE, + c: VALUE, + d: VALUE, + e: VALUE, + f: VALUE, + g: VALUE, + ) -> VALUE { + unsafe { rb_ary_new_from_args(7, a, b, c, d, e, f, g) } + } + + with_rubyvm(|| { + let klass = define_class("ZJITSevenArgs", unsafe { rb_cObject }); + unsafe { + rb_define_method( + klass, + c"seven".as_ptr(), + Some(std::mem::transmute::< + unsafe extern "C" fn(VALUE, VALUE, VALUE, VALUE, VALUE, VALUE, VALUE, VALUE) -> VALUE, + unsafe extern "C" fn(VALUE) -> VALUE, + >(test_seven_args)), + 7, + ); + } + }); + + assert_snapshot!(assert_compiles_allowing_exits(r#" + def test(obj, flag) + priceable = obj.seven(1, 2, 3, 4, 5, 6, 7) + if flag + priceable + else + nil + end + end + + obj = ZJITSevenArgs.new + test(obj, true) # profile receiver class + test(obj, true) # compile -> currently panics: Failed to get_opnd(vN) + test(obj, true) + "#), @"[1, 2, 3, 4, 5, 6, 7]"); +} + #[test] fn test_string_new_preserves_string_arg() { assert_snapshot!(inspect(r#" diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index 75535272af..261cdf2c51 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -234,6 +234,8 @@ pub const VM_ENV_DATA_INDEX_SPECVAL: i32 = -1; pub const VM_ENV_DATA_INDEX_FLAGS: u32 = 0; pub const VM_BLOCK_HANDLER_NONE: u32 = 0; pub const SHAPE_ID_NUM_BITS: u32 = 32; +pub const ZJIT_STACK_MAP_VREG_TAG: u32 = 8; +pub const ZJIT_STACK_MAP_SHIFT: u32 = 8; pub const ZJIT_JIT_RETURN_C_FRAME: u32 = 1; pub type rb_alloc_func_t = ::std::option::Option<unsafe extern "C" fn(klass: VALUE) -> VALUE>; pub const RUBY_Qfalse: ruby_special_consts = 0; @@ -1921,11 +1923,12 @@ pub const DEFINED_FUNC: defined_type = 16; pub const DEFINED_CONST_FROM: defined_type = 17; pub type defined_type = u32; #[repr(C)] -#[derive(Debug, Copy, Clone)] pub struct zjit_jit_frame { pub pc: *const VALUE, pub iseq: *const rb_iseq_t, pub materialize_block_code: bool, + pub stack_size: u32, + pub stack: __IncompleteArrayField<VALUE>, } pub const ISEQ_BODY_OFFSET_PARAM: zjit_struct_offsets = 16; pub type zjit_struct_offsets = u32; @@ -1940,7 +1943,7 @@ pub const RUBY_OFFSET_EC_INTERRUPT_FLAG: jit_bindgen_constants = 32; pub const RUBY_OFFSET_EC_INTERRUPT_MASK: jit_bindgen_constants = 36; pub const RUBY_OFFSET_EC_THREAD_PTR: jit_bindgen_constants = 48; pub const RUBY_OFFSET_EC_RACTOR_ID: jit_bindgen_constants = 64; -pub type jit_bindgen_constants = u32; +pub type jit_bindgen_constants = i32; pub const rb_invalid_shape_id: shape_id_t = 524287; pub type rb_iseq_param_keyword_struct = rb_iseq_constant_body_rb_iseq_parameters_rb_iseq_param_keyword; diff --git a/zjit/src/cruby_methods.rs b/zjit/src/cruby_methods.rs index b8d314b858..68c0ea4d90 100644 --- a/zjit/src/cruby_methods.rs +++ b/zjit/src/cruby_methods.rs @@ -321,13 +321,13 @@ fn inline_thread_current(fun: &mut hir::Function, block: hir::BlockId, _recv: hi let thread_ptr = fun.push_insn(block, hir::Insn::LoadField { recv: ec, id: FieldName::thread_ptr, - offset: RUBY_OFFSET_EC_THREAD_PTR as i32, + offset: RUBY_OFFSET_EC_THREAD_PTR, return_type: types::CPtr, }); let thread_self = fun.push_insn(block, hir::Insn::LoadField { recv: thread_ptr, id: FieldName::SelfParam, - offset: RUBY_OFFSET_THREAD_SELF as i32, + offset: RUBY_OFFSET_THREAD_SELF, // TODO(max): Add Thread type. But Thread.current is not guaranteed to be an exact Thread. // You can make subclasses... return_type: types::BasicObject, @@ -467,7 +467,7 @@ fn inline_string_bytesize(fun: &mut hir::Function, block: hir::BlockId, recv: hi let len = fun.push_insn(block, hir::Insn::LoadField { recv, id: FieldName::len, - offset: RUBY_OFFSET_RSTRING_LEN as i32, + offset: RUBY_OFFSET_RSTRING_LEN, return_type: types::CInt64, }); @@ -491,7 +491,7 @@ fn inline_string_getbyte(fun: &mut hir::Function, block: hir::BlockId, recv: hir let len = fun.push_insn(block, hir::Insn::LoadField { recv, id: FieldName::len, - offset: RUBY_OFFSET_RSTRING_LEN as i32, + offset: RUBY_OFFSET_RSTRING_LEN, return_type: types::CInt64, }); // TODO(max): Find a way to mark these guards as not needed for correctness... as in, once @@ -519,7 +519,7 @@ fn inline_string_setbyte(fun: &mut hir::Function, block: hir::BlockId, recv: hir let len = fun.push_insn(block, hir::Insn::LoadField { recv, id: FieldName::len, - offset: RUBY_OFFSET_RSTRING_LEN as i32, + offset: RUBY_OFFSET_RSTRING_LEN, return_type: types::CInt64, }); let unboxed_index = fun.push_insn(block, hir::Insn::GuardLess { left: unboxed_index, right: len, reason: SideExitReason::GuardLess, state }); @@ -542,7 +542,7 @@ fn inline_string_empty_p(fun: &mut hir::Function, block: hir::BlockId, recv: hir let len = fun.push_insn(block, hir::Insn::LoadField { recv, id: FieldName::len, - offset: RUBY_OFFSET_RSTRING_LEN as i32, + offset: RUBY_OFFSET_RSTRING_LEN, return_type: types::CInt64, }); let zero = fun.push_insn(block, hir::Insn::Const { val: hir::Const::CInt64(0) }); diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index bd2b4e29d6..013996ba9f 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -531,6 +531,7 @@ pub enum SideExitReason { UnhandledCallType(CallType), UnhandledBlockArg, TooManyKeywordParameters, + TooManyArgsForLir, FixnumAddOverflow, FixnumSubOverflow, FixnumMultOverflow, @@ -2578,6 +2579,7 @@ fn can_direct_send(function: &mut Function, block: BlockId, iseq: *const rb_iseq // See: https://github.com/ruby/ruby/pull/15911#discussion_r2710544982 let block_arg = if 0 != params.flags.has_block() { 1 } else { 0 }; let final_argc = caller_positional + kw_total_num as usize + block_arg; + // TODO: Support passing arguments on the stack in C calls if final_argc + 1 > C_ARG_OPNDS.len() { // +1 for self function.set_dynamic_send_reason(send_insn, TooManyArgsForLir); return false; @@ -4257,6 +4259,13 @@ impl Function { self.set_dynamic_send_reason(insn_id, ArgcParamMismatch); continue; } + // TODO: Support passing arguments on the stack in C calls + // +1 for self + if args.len()+1 > C_ARG_OPNDS.len() { + self.push_insn_id(block, insn_id); + self.set_dynamic_send_reason(insn_id, TooManyArgsForLir); + continue; + } emit_super_call_guards(self, block, super_cme, current_cme, mid, state, frame_state.iseq); @@ -4841,7 +4850,7 @@ impl Function { // See ROBJECT_FIELDS() from include/ruby/internal/core/robject.h let ptr = self.push_insn(block, Insn::LoadField { recv, id: FieldName::as_heap, - offset: ROBJECT_OFFSET_AS_HEAP_FIELDS as i32, + offset: ROBJECT_OFFSET_AS_HEAP_FIELDS, return_type: types::CPtr, }); let offset = SIZEOF_VALUE_I32 * ivar_index as i32; @@ -4853,7 +4862,7 @@ impl Function { fn load_ivar_embedded(&mut self, block: BlockId, recv: InsnId, id: ID, ivar_index: attr_index_t) -> InsnId { // See ROBJECT_FIELDS() from include/ruby/internal/core/robject.h - let offset = ROBJECT_OFFSET_AS_ARY as i32 + let offset = ROBJECT_OFFSET_AS_ARY + (SIZEOF_VALUE * ivar_index.to_usize()) as i32; self.push_insn(block, Insn::LoadField { recv, id: id.into(), offset, @@ -4883,9 +4892,9 @@ impl Function { match layout { ShapeLayout::RClass | ShapeLayout::RData => { let offset = if layout == ShapeLayout::RClass { - RCLASS_OFFSET_PRIME_FIELDS_OBJ as i32 + RCLASS_OFFSET_PRIME_FIELDS_OBJ } else { - TDATA_OFFSET_FIELDS_OBJ as i32 + TDATA_OFFSET_FIELDS_OBJ }; let fields_obj = self.push_insn(block, Insn::LoadField { @@ -5061,10 +5070,10 @@ impl Function { // Current shape contains this ivar let (ivar_storage, offset) = if recv_type.flags().is_embedded() { // See ROBJECT_FIELDS() from include/ruby/internal/core/robject.h - let offset = ROBJECT_OFFSET_AS_ARY as i32 + (SIZEOF_VALUE * ivar_index.to_usize()) as i32; + let offset = ROBJECT_OFFSET_AS_ARY + (SIZEOF_VALUE * ivar_index.to_usize()) as i32; (self_val, offset) } else { - let as_heap = self.push_insn(block, Insn::LoadField { recv: self_val, id: FieldName::as_heap, offset: ROBJECT_OFFSET_AS_HEAP_FIELDS as i32, return_type: types::CPtr }); + let as_heap = self.push_insn(block, Insn::LoadField { recv: self_val, id: FieldName::as_heap, offset: ROBJECT_OFFSET_AS_HEAP_FIELDS, return_type: types::CPtr }); let offset = SIZEOF_VALUE_I32 * ivar_index as i32; (as_heap, offset) }; @@ -5236,6 +5245,13 @@ impl Function { return Err(()); } + // TODO: Support passing arguments on the stack in C calls + // +1 for self + if (argc as usize)+1 > C_ARG_OPNDS.len() { + fun.set_dynamic_send_reason(send_insn_id, TooManyArgsForLir); + return Err(()); + } + // Check singleton class assumption first, before emitting other patchpoints if !fun.assume_no_singleton_classes(block, recv_class, state) { fun.set_dynamic_send_reason(send_insn_id, SingletonClassSeen); @@ -7626,6 +7642,7 @@ fn add_iseq_to_hir( } YARVINSN_concatstrings => { let count = get_arg(pc, 0).as_u32(); + debug_assert!(count > 0, "concatstrings should have arguments"); let strings = state.stack_pop_n(count as usize)?; let insn_id = fun.push_insn(block, Insn::StringConcat { strings, state: exit_id }); state.stack_push(insn_id); @@ -9136,6 +9153,12 @@ fn add_iseq_to_hir( } YARVINSN_invokebuiltin => { let bf: rb_builtin_function = unsafe { *get_arg(pc, 0).as_ptr() }; + // TODO: Support passing arguments on the stack in C calls + // +2 for ec, self + if (bf.argc + 2) as usize > C_ARG_OPNDS.len() { + fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::TooManyArgsForLir, recompile: None }); + break; // End the block + } let mut args = vec![]; for _ in 0..bf.argc { @@ -9165,6 +9188,13 @@ fn add_iseq_to_hir( YARVINSN_opt_invokebuiltin_delegate | YARVINSN_opt_invokebuiltin_delegate_leave => { let bf: rb_builtin_function = unsafe { *get_arg(pc, 0).as_ptr() }; + // TODO: Support passing arguments on the stack in C calls + // +2 for ec, self + if (bf.argc + 2) as usize > C_ARG_OPNDS.len() { + fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::TooManyArgsForLir, recompile: None }); + break; // End the block + } + let index = get_arg(pc, 1).as_usize(); let argc = bf.argc as usize; @@ -9396,6 +9426,10 @@ fn compile_jit_entry_state(fun: &mut Function, jit_entry_block: BlockId, jit_ent let opt_num: usize = params.opt_num.try_into().expect("iseq param opt_num >= 0"); let lead_num: usize = params.lead_num.try_into().expect("iseq param lead_num >= 0"); let passed_opt_num = jit_entry_idx; + // We don't need to check crate::cruby::iseq_escapes_ep because we + // don't enter ISEQ_TYPE_MAIN/ISEQ_TYPE_EVAL using JIT-to-JIT calls. + // TODO: Stop compiling such JIT entries (Shopify/ruby#992) + let iseq_escapes_ep = crate::invariants::iseq_escapes_ep(iseq); // If the iseq has keyword parameters, the keyword bits local will be appended to the local table. let kw_bits_idx: Option<usize> = if unsafe { rb_get_iseq_flags_has_kw(iseq) } { @@ -9418,9 +9452,9 @@ fn compile_jit_entry_state(fun: &mut Function, jit_entry_block: BlockId, jit_ent let mut entry_state = FrameState::new(iseq); let mut ep: Option<InsnId> = None; for local_idx in 0..num_locals(iseq) { - if (lead_num + passed_opt_num..lead_num + opt_num).contains(&local_idx) { + let local = if (lead_num + passed_opt_num..lead_num + opt_num).contains(&local_idx) { // Omitted optionals are locals, so they start as nils before their code run - entry_state.locals.push(fun.push_insn(jit_entry_block, Insn::Const { val: Const::Value(Qnil) })); + fun.push_insn(jit_entry_block, Insn::Const { val: Const::Value(Qnil) }) } else if Some(local_idx) == kw_bits_idx { // Read the kw_bits value written by the caller to the callee frame. // This tells us which optional keywords were NOT provided and need their defaults evaluated. @@ -9430,20 +9464,37 @@ fn compile_jit_entry_state(fun: &mut Function, jit_entry_block: BlockId, jit_ent let ep_offset_u32 = u32::try_from(ep_offset) .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); let ep = *ep.get_or_insert_with(|| fun.push_insn(jit_entry_block, Insn::GetEP { level: 0 })); - entry_state.locals.push(fun.get_local_from_ep( + fun.get_local_from_ep( jit_entry_block, iseq, ep, ep_offset_u32, 0, types::BasicObject, - )); + ) } else if local_idx < param_size { let id = unsafe { rb_zjit_local_id(iseq, local_idx.try_into().unwrap()) }; - entry_state.locals.push(fun.push_insn(jit_entry_block, Insn::LoadArg { idx: arg_idx, id: id.into(), val_type: types::BasicObject })); + let local = fun.push_insn(jit_entry_block, Insn::LoadArg { idx: arg_idx, id: id.into(), val_type: types::BasicObject }); arg_idx += 1; + local } else { - entry_state.locals.push(fun.push_insn(jit_entry_block, Insn::Const { val: Const::Value(Qnil) })); + fun.push_insn(jit_entry_block, Insn::Const { val: Const::Value(Qnil) }) + }; + entry_state.locals.push(local); + + // Once an ISEQ has escaped EP, HIR getlocal may need to read from the + // VM frame instead of FrameState. Direct JIT-to-JIT entry passes locals + // as C arguments, so initialize the frame slots here before such reads. + if iseq_escapes_ep { + let ep_offset = local_idx_to_ep_offset(iseq, local_idx); + let local_id = unsafe { rb_zjit_local_id(iseq, local_idx.try_into().unwrap()) }; + let ep = *ep.get_or_insert_with(|| fun.push_insn(jit_entry_block, Insn::GetEP { level: 0 })); + fun.push_insn(jit_entry_block, Insn::StoreField { + recv: ep, + id: local_id.into(), + offset: -(SIZEOF_VALUE_I32 * ep_offset), + val: local, + }); } } (self_param, entry_state) @@ -10040,13 +10091,13 @@ mod validation_tests { function.push_insn(entry, Insn::LoadField { recv, id: FieldName::as_heap, - offset: ROBJECT_OFFSET_AS_HEAP_FIELDS as i32, + offset: ROBJECT_OFFSET_AS_HEAP_FIELDS, return_type: types::CPtr, }); let ivar = function.push_insn(entry, Insn::LoadField { recv, id: FieldName::Id(ID(1)), - offset: ROBJECT_OFFSET_AS_ARY as i32, + offset: ROBJECT_OFFSET_AS_ARY, return_type: types::BasicObject, }); function.push_insn(entry, Insn::Return { val: ivar }); @@ -10073,13 +10124,13 @@ mod validation_tests { function.push_insn(entry, Insn::LoadField { recv, id: FieldName::as_heap, - offset: ROBJECT_OFFSET_AS_HEAP_FIELDS as i32, + offset: ROBJECT_OFFSET_AS_HEAP_FIELDS, return_type: types::BasicObject, }); let ivar = function.push_insn(entry, Insn::LoadField { recv, id: FieldName::Id(ID(1)), - offset: ROBJECT_OFFSET_AS_ARY as i32, + offset: ROBJECT_OFFSET_AS_ARY, return_type: types::Array, }); function.push_insn(entry, Insn::Return { val: ivar }); diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 5b94aed90e..0953905c19 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -3849,20 +3849,22 @@ mod hir_opt_tests { EntryPoint JIT(0) v5:BasicObject = LoadArg :self@0 v6:NilClass = Const Value(nil) + v7:CPtr = GetEP 0 + StoreField v7, :a@0x1000, v6 Jump bb3(v5, v6) - bb3(v8:BasicObject, v9:NilClass): - v13:Fixnum[1] = Const Value(1) - SetLocal :a, l0, EP@3, v13 - PatchPoint MethodRedefined(Object@0x1000, lambda@0x1008, cme:0x1010) - v45:ObjectSubclass[class_exact*:Object@VALUE(0x1000)] = GuardType v8, ObjectSubclass[class_exact*:Object@VALUE(0x1000)] recompile - v46:BasicObject = CCallWithFrame v45, :Kernel#lambda@0x1038, block=0x1040 - v20:CPtr = GetEP 0 - v21:BasicObject = LoadField v20, :a@0x1048 - PatchPoint MethodRedefined(Object@0x1000, foo@0x1049, cme:0x1050) - v32:CPtr = GetEP 0 - v33:BasicObject = LoadField v32, :a@0x1048 + bb3(v10:BasicObject, v11:NilClass): + v15:Fixnum[1] = Const Value(1) + SetLocal :a, l0, EP@3, v15 + PatchPoint MethodRedefined(Object@0x1008, lambda@0x1010, cme:0x1018) + v47:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v10, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + v48:BasicObject = CCallWithFrame v47, :Kernel#lambda@0x1040, block=0x1048 + v22:CPtr = GetEP 0 + v23:BasicObject = LoadField v22, :a@0x1000 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1050, cme:0x1058) + v34:CPtr = GetEP 0 + v35:BasicObject = LoadField v34, :a@0x1000 CheckInterrupts - Return v33 + Return v35 "); } @@ -14486,25 +14488,30 @@ mod hir_opt_tests { EntryPoint JIT(0) v9:BasicObject = LoadArg :self@0 v10:BasicObject = LoadArg :a@1 - v11:BasicObject = LoadArg :_b@2 - v12:BasicObject = LoadArg :_c@3 - v13:NilClass = Const Value(nil) - Jump bb3(v9, v10, v11, v12, v13) - bb3(v15:BasicObject, v16:BasicObject, v17:BasicObject, v18:BasicObject, v19:NilClass): + v11:CPtr = GetEP 0 + StoreField v11, :a@0x1001, v10 + v13:BasicObject = LoadArg :_b@2 + StoreField v11, :_b@0x1002, v13 + v15:BasicObject = LoadArg :_c@3 + StoreField v11, :_c@0x1003, v15 + v17:NilClass = Const Value(nil) + StoreField v11, :formatted@0x1004, v17 + Jump bb3(v9, v10, v13, v15, v17) + bb3(v20:BasicObject, v21:BasicObject, v22:BasicObject, v23:BasicObject, v24:NilClass): CheckInterrupts - SetLocal :formatted, l0, EP@3, v16 + SetLocal :formatted, l0, EP@3, v21 PatchPoint SingleRactorMode - SetIvar v15, :@formatted, v16 - v47:ClassSubclass[VMFrozenCore] = Const Value(VALUE(0x1008)) + SetIvar v20, :@formatted, v21 + v52:ClassSubclass[VMFrozenCore] = Const Value(VALUE(0x1008)) PatchPoint MethodRedefined(Class@0x1010, lambda@0x1018, cme:0x1020) - v63:BasicObject = CCallWithFrame v47, :RubyVM::FrozenCore.lambda@0x1048, block=0x1050 - v50:CPtr = GetEP 0 - v51:BasicObject = LoadField v50, :a@0x1001 - v52:BasicObject = LoadField v50, :_b@0x1002 - v53:BasicObject = LoadField v50, :_c@0x1058 - v54:BasicObject = LoadField v50, :formatted@0x1059 + v68:BasicObject = CCallWithFrame v52, :RubyVM::FrozenCore.lambda@0x1048, block=0x1050 + v55:CPtr = GetEP 0 + v56:BasicObject = LoadField v55, :a@0x1001 + v57:BasicObject = LoadField v55, :_b@0x1002 + v58:BasicObject = LoadField v55, :_c@0x1003 + v59:BasicObject = LoadField v55, :formatted@0x1004 CheckInterrupts - Return v63 + Return v68 "); } @@ -15729,21 +15736,24 @@ mod hir_opt_tests { EntryPoint JIT(0) v7:HeapBasicObject = LoadArg :self@0 v8:BasicObject = LoadArg :blk@1 - v9:NilClass = Const Value(nil) - Jump bb3(v7, v8, v9) - bb3(v11:HeapBasicObject, v12:BasicObject, v13:NilClass): + v9:CPtr = GetEP 0 + StoreField v9, :blk@0x1001, v8 + v11:NilClass = Const Value(nil) + StoreField v9, :other_block@0x1002, v11 + Jump bb3(v7, v8, v11) + bb3(v14:HeapBasicObject, v15:BasicObject, v16:NilClass): PatchPoint NoSingletonClass(B@0x1008) PatchPoint MethodRedefined(B@0x1008, proc@0x1010, cme:0x1018) - v39:ObjectSubclass[class_exact:B] = GuardType v11, ObjectSubclass[class_exact:B] recompile - v40:BasicObject = CCallWithFrame v39, :Kernel#proc@0x1040, block=0x1048 - v19:CPtr = GetEP 0 - v20:BasicObject = LoadField v19, :blk@0x1050 - SetLocal :other_block, l0, EP@3, v40 - v27:CPtr = GetEP 0 - v28:BasicObject = LoadField v27, :other_block@0x1051 - v30:BasicObject = InvokeSuper v39, 0x1058, v28 # SendFallbackReason: super: complex argument passing to `super` call + v42:ObjectSubclass[class_exact:B] = GuardType v14, ObjectSubclass[class_exact:B] recompile + v43:BasicObject = CCallWithFrame v42, :Kernel#proc@0x1040, block=0x1048 + v22:CPtr = GetEP 0 + v23:BasicObject = LoadField v22, :blk@0x1001 + SetLocal :other_block, l0, EP@3, v43 + v30:CPtr = GetEP 0 + v31:BasicObject = LoadField v30, :other_block@0x1002 + v33:BasicObject = InvokeSuper v42, 0x1050, v31 # SendFallbackReason: super: complex argument passing to `super` call CheckInterrupts - Return v30 + Return v33 "); } @@ -16328,67 +16338,82 @@ mod hir_opt_tests { EntryPoint JIT(0) v16:BasicObject = LoadArg :self@0 v17:BasicObject = LoadArg :list@1 - v18:NilClass = Const Value(nil) - v19:NilClass = Const Value(nil) + v18:CPtr = GetEP 0 + StoreField v18, :list@0x1001, v17 v20:NilClass = Const Value(nil) - Jump bb3(v16, v17, v18, v19, v20) - bb3(v36:BasicObject, v37:BasicObject, v38:BasicObject, v39:BasicObject, v40:NilClass): - v43:NilClass = Const Value(nil) - SetLocal :sep, l0, EP@5, v43 - Jump bb5(v36, v37, v43, v39, v40) + StoreField v18, :sep@0x1002, v20 + v22:NilClass = Const Value(nil) + StoreField v18, :iter_method@0x1005, v22 + v24:NilClass = Const Value(nil) + StoreField v18, :kwsplat@0x1006, v24 + Jump bb3(v16, v17, v20, v22, v24) + bb3(v51:BasicObject, v52:BasicObject, v53:BasicObject, v54:BasicObject, v55:NilClass): + v58:NilClass = Const Value(nil) + SetLocal :sep, l0, EP@5, v58 + Jump bb5(v51, v52, v58, v54, v55) bb4(): EntryPoint JIT(1) - v23:BasicObject = LoadArg :self@0 - v24:BasicObject = LoadArg :list@1 - v25:BasicObject = LoadArg :sep@2 - v26:NilClass = Const Value(nil) - v27:NilClass = Const Value(nil) - Jump bb5(v23, v24, v25, v26, v27) - bb5(v47:BasicObject, v48:BasicObject, v49:BasicObject, v50:BasicObject, v51:NilClass): - v54:StaticSymbol[:each] = Const Value(VALUE(0x1008)) - SetLocal :iter_method, l0, EP@4, v54 - Jump bb7(v47, v48, v49, v54, v51) - bb6(): - EntryPoint JIT(2) - v30:BasicObject = LoadArg :self@0 - v31:BasicObject = LoadArg :list@1 + v28:BasicObject = LoadArg :self@0 + v29:BasicObject = LoadArg :list@1 + v30:CPtr = GetEP 0 + StoreField v30, :list@0x1001, v29 v32:BasicObject = LoadArg :sep@2 - v33:BasicObject = LoadArg :iter_method@3 + StoreField v30, :sep@0x1002, v32 v34:NilClass = Const Value(nil) - Jump bb7(v30, v31, v32, v33, v34) - bb7(v58:BasicObject, v59:BasicObject, v60:BasicObject, v61:BasicObject, v62:NilClass): - CheckInterrupts - v68:CBool = Test v60 - v69:Truthy = RefineType v60, Truthy - CondBranch v68, bb8(v58, v59, v69, v61, v62), bb11() + StoreField v30, :iter_method@0x1005, v34 + v36:NilClass = Const Value(nil) + StoreField v30, :kwsplat@0x1006, v36 + Jump bb5(v28, v29, v32, v34, v36) + bb5(v62:BasicObject, v63:BasicObject, v64:BasicObject, v65:BasicObject, v66:NilClass): + v69:StaticSymbol[:each] = Const Value(VALUE(0x1008)) + SetLocal :iter_method, l0, EP@4, v69 + Jump bb7(v62, v63, v64, v69, v66) + bb6(): + EntryPoint JIT(2) + v40:BasicObject = LoadArg :self@0 + v41:BasicObject = LoadArg :list@1 + v42:CPtr = GetEP 0 + StoreField v42, :list@0x1001, v41 + v44:BasicObject = LoadArg :sep@2 + StoreField v42, :sep@0x1002, v44 + v46:BasicObject = LoadArg :iter_method@3 + StoreField v42, :iter_method@0x1005, v46 + v48:NilClass = Const Value(nil) + StoreField v42, :kwsplat@0x1006, v48 + Jump bb7(v40, v41, v44, v46, v48) + bb7(v73:BasicObject, v74:BasicObject, v75:BasicObject, v76:BasicObject, v77:NilClass): + CheckInterrupts + v83:CBool = Test v75 + v84:Truthy = RefineType v75, Truthy + CondBranch v83, bb8(v73, v74, v84, v76, v77), bb11() bb11(): - v71:Falsy = RefineType v60, Falsy + v86:Falsy = RefineType v75, Falsy PatchPoint MethodRedefined(Object@0x1010, lambda@0x1018, cme:0x1020) - v118:ObjectSubclass[class_exact*:Object@VALUE(0x1010)] = GuardType v58, ObjectSubclass[class_exact*:Object@VALUE(0x1010)] recompile - v119:BasicObject = CCallWithFrame v118, :Kernel#lambda@0x1048, block=0x1050 - v75:CPtr = GetEP 0 - v76:BasicObject = LoadField v75, :list@0x1001 - v78:BasicObject = LoadField v75, :iter_method@0x1058 - v79:BasicObject = LoadField v75, :kwsplat@0x1059 - SetLocal :sep, l0, EP@5, v119 - Jump bb8(v118, v76, v119, v78, v79) - bb8(v83:BasicObject, v84:BasicObject, v85:BasicObject, v86:BasicObject, v87:BasicObject): + v133:ObjectSubclass[class_exact*:Object@VALUE(0x1010)] = GuardType v73, ObjectSubclass[class_exact*:Object@VALUE(0x1010)] recompile + v134:BasicObject = CCallWithFrame v133, :Kernel#lambda@0x1048, block=0x1050 + v90:CPtr = GetEP 0 + v91:BasicObject = LoadField v90, :list@0x1001 + v93:BasicObject = LoadField v90, :iter_method@0x1005 + v94:BasicObject = LoadField v90, :kwsplat@0x1006 + SetLocal :sep, l0, EP@5, v134 + Jump bb8(v133, v91, v134, v93, v94) + bb8(v98:BasicObject, v99:BasicObject, v100:BasicObject, v101:BasicObject, v102:BasicObject): PatchPoint SingleRactorMode - PatchPoint StableConstantNames(0x1060, CONST) - v115:HashExact[VALUE(0x1068)] = Const Value(VALUE(0x1068)) - SetLocal :kwsplat, l0, EP@3, v115 - v96:CPtr = GetEP 0 - v97:BasicObject = LoadField v96, :list@0x1001 - v99:CPtr = GetEP 0 - v100:BasicObject = LoadField v99, :iter_method@0x1058 - v102:BasicObject = Send v97, 0x1070, :__send__, v100 # SendFallbackReason: Send: unsupported method type Optimized - v103:CPtr = GetEP 0 - v104:BasicObject = LoadField v103, :list@0x1001 - v105:BasicObject = LoadField v103, :sep@0x1002 - v106:BasicObject = LoadField v103, :iter_method@0x1058 - v107:BasicObject = LoadField v103, :kwsplat@0x1059 + PatchPoint StableConstantNames(0x1058, CONST) + v130:HashExact[VALUE(0x1060)] = Const Value(VALUE(0x1060)) + SetLocal :kwsplat, l0, EP@3, v130 + v111:CPtr = GetEP 0 + v112:BasicObject = LoadField v111, :list@0x1001 + v114:CPtr = GetEP 0 + v115:BasicObject = LoadField v114, :iter_method@0x1005 + v117:BasicObject = Send v112, 0x1068, :__send__, v115 # SendFallbackReason: Send: unsupported method type Optimized + v118:CPtr = GetEP 0 + v119:BasicObject = LoadField v118, :list@0x1001 + v120:BasicObject = LoadField v118, :sep@0x1002 + v121:BasicObject = LoadField v118, :iter_method@0x1005 + v122:BasicObject = LoadField v118, :kwsplat@0x1006 CheckInterrupts - Return v102 + Return v117 "); } @@ -18779,4 +18804,89 @@ mod hir_opt_tests { Return v200 "); } + + #[test] + fn test_ccall_with_frame_too_many_args_result_used_in_later_block() { + unsafe extern "C" fn test_seven_args( + _self: VALUE, + a: VALUE, + b: VALUE, + c: VALUE, + d: VALUE, + e: VALUE, + f: VALUE, + g: VALUE, + ) -> VALUE { + unsafe { rb_ary_new_from_args(7, a, b, c, d, e, f, g) } + } + + with_rubyvm(|| { + let klass = define_class("ZJITSevenArgs", unsafe { rb_cObject }); + unsafe { + rb_define_method( + klass, + c"seven".as_ptr(), + Some(std::mem::transmute::< + unsafe extern "C" fn(VALUE, VALUE, VALUE, VALUE, VALUE, VALUE, VALUE, VALUE) -> VALUE, + unsafe extern "C" fn(VALUE) -> VALUE, + >(test_seven_args)), + 7, + ); + } + }); + + eval(r#" + def test(obj, flag) + priceable = obj.seven(1, 2, 3, 4, 5, 6, 7) + if flag + priceable + else + nil + end + end + + obj = ZJITSevenArgs.new + test(obj, true) # profile receiver class + "#); + assert_snapshot!(hir_string("test"), @" + fn test@<compiled>:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :obj@0x1000 + v4:BasicObject = LoadField v2, :flag@0x1001 + v5:NilClass = Const Value(nil) + Jump bb3(v1, v3, v4, v5) + bb2(): + EntryPoint JIT(0) + v8:BasicObject = LoadArg :self@0 + v9:BasicObject = LoadArg :obj@1 + v10:BasicObject = LoadArg :flag@2 + v11:NilClass = Const Value(nil) + Jump bb3(v8, v9, v10, v11) + bb3(v13:BasicObject, v14:BasicObject, v15:BasicObject, v16:NilClass): + v21:Fixnum[1] = Const Value(1) + v23:Fixnum[2] = Const Value(2) + v25:Fixnum[3] = Const Value(3) + v27:Fixnum[4] = Const Value(4) + v29:Fixnum[5] = Const Value(5) + v31:Fixnum[6] = Const Value(6) + v33:Fixnum[7] = Const Value(7) + v35:BasicObject = Send v14, :seven, v21, v23, v25, v27, v29, v31, v33 # SendFallbackReason: Too many arguments for LIR + PatchPoint NoEPEscape(test) + CheckInterrupts + v43:CBool = Test v15 + v44:Falsy = RefineType v15, Falsy + CondBranch v43, bb5(), bb4(v13, v14, v44, v35) + bb5(): + v46:Truthy = RefineType v15, Truthy + CheckInterrupts + Return v35 + bb4(v53:BasicObject, v54:BasicObject, v55:Falsy, v56:BasicObject): + v60:NilClass = Const Value(nil) + CheckInterrupts + Return v60 + "); + } } diff --git a/zjit/src/jit_frame.rs b/zjit/src/jit_frame.rs index 3dafd385be..eaedcd0a60 100644 --- a/zjit/src/jit_frame.rs +++ b/zjit/src/jit_frame.rs @@ -1,4 +1,8 @@ -use crate::cruby::{IseqPtr, VALUE, rb_gc_mark_movable, rb_gc_location}; +use std::alloc::{alloc, handle_alloc_error, Layout}; +use std::mem::{align_of, size_of}; +use std::ptr; + +use crate::cruby::{__IncompleteArrayField, IseqPtr, VALUE, rb_gc_mark_movable, rb_gc_location}; use crate::cruby::zjit_jit_frame; use crate::codegen::iseq_may_write_block_code; use crate::state::ZJITState; @@ -8,18 +12,43 @@ use crate::state::ZJITState; pub type JITFrame = zjit_jit_frame; impl JITFrame { - /// Allocate a JITFrame on the heap, register it with ZJITState, and return - /// a raw pointer that remains valid for the lifetime of the process. - fn alloc(jit_frame: JITFrame) -> *const Self { - let raw_ptr = Box::into_raw(Box::new(jit_frame)); + /// Allocate a JITFrame and its trailing stack map on the heap, register it + /// with ZJITState, and return a raw pointer that remains valid for the + /// lifetime of the process. + fn alloc( + pc: *const VALUE, + iseq: IseqPtr, + materialize_block_code: bool, + stack_size: usize, + ) -> *const Self { + // JITFrame ends with a flexible stack[] array, so allocate enough + // space for the fixed fields plus the requested stack map entries. + let frame_size = size_of::<JITFrame>() + .checked_add(stack_size.checked_mul(size_of::<VALUE>()).unwrap()) + .unwrap(); + let layout = Layout::from_size_align(frame_size, align_of::<JITFrame>()).unwrap(); + let raw_ptr = unsafe { alloc(layout) as *mut JITFrame }; + if raw_ptr.is_null() { + handle_alloc_error(layout); + } + + unsafe { + ptr::write(raw_ptr, JITFrame { + pc, + iseq, + materialize_block_code, + stack_size: stack_size.try_into().unwrap(), + stack: __IncompleteArrayField::new(), + }); + } ZJITState::get_jit_frames().push(raw_ptr); raw_ptr as *const _ } /// Create a JITFrame for an ISEQ frame. - pub fn new_iseq(pc: *const VALUE, iseq: IseqPtr) -> *const Self { + pub fn new_iseq(pc: *const VALUE, iseq: IseqPtr, stack_size: usize) -> *const Self { let materialize_block_code = !iseq_may_write_block_code(iseq); - Self::alloc(JITFrame { pc, iseq, materialize_block_code }) + Self::alloc(pc, iseq, materialize_block_code, stack_size) } /// Mark the iseq pointer for GC. Called from rb_zjit_root_mark. @@ -94,6 +123,29 @@ mod tests { "), @"1"); } + // Direct JIT-to-JIT entry passes callee locals as native arguments. If the + // callee ISEQ has already escaped EP, later getlocal reads use EP memory, + // so JIT entry must materialize those locals into the callee frame. + #[test] + fn test_jit_entry_materializes_ep_escaped_locals() { + assert_snapshot!(inspect(" + def poison(*) = nil + + def victim(a, b, c) + lambda { a } + a + end + + def jit_entry + poison([], [], [], []) + victim(:expected, 1, 2) + end + + jit_entry + Array.new(100) { jit_entry }.uniq + "), @"[:expected]"); + } + // Materialize frames on side exit: a type guard triggers a side exit with // multiple JIT frames on the stack. All frames must be materialized before // the interpreter resumes. diff --git a/zjit/src/stats.rs b/zjit/src/stats.rs index b019ba0b87..a769005fb9 100644 --- a/zjit/src/stats.rs +++ b/zjit/src/stats.rs @@ -234,6 +234,7 @@ make_counters! { exit_block_param_proxy_profile_not_covered, exit_block_param_wb_required, exit_too_many_keyword_parameters, + exit_too_many_args_for_lir, exit_no_profile_send, exit_splatkw_not_nil_or_hash, exit_splatkw_polymorphic, @@ -630,6 +631,7 @@ pub fn side_exit_counter(reason: crate::hir::SideExitReason) -> Counter { BlockParamProxyProfileNotCovered => exit_block_param_proxy_profile_not_covered, BlockParamWbRequired => exit_block_param_wb_required, TooManyKeywordParameters => exit_too_many_keyword_parameters, + TooManyArgsForLir => exit_too_many_args_for_lir, SplatKwNotNilOrHash => exit_splatkw_not_nil_or_hash, SplatKwPolymorphic => exit_splatkw_polymorphic, SplatKwNotProfiled => exit_splatkw_not_profiled, |