From 6612731a4ffd41c01e836ed2ffd473d46b685ab1 Mon Sep 17 00:00:00 2001 From: tomoya ishida Date: Thu, 6 Aug 2026 05:26:30 +0900 Subject: [PATCH 1/9] Pin IRB to latest master (#18214) --- gems/bundled_gems | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gems/bundled_gems b/gems/bundled_gems index 3e4355a07d92e7..c10d746627ceb8 100644 --- a/gems/bundled_gems +++ b/gems/bundled_gems @@ -39,7 +39,7 @@ benchmark 0.5.0 https://github.com/ruby/benchmark logger 1.7.0 https://github.com/ruby/logger rdoc 8.0.0 https://github.com/ruby/rdoc win32ole 1.9.3 https://github.com/ruby/win32ole -irb 1.18.0 https://github.com/ruby/irb +irb 1.18.0 https://github.com/ruby/irb ac42eaaa88e6399384c1a56cc948c4b2528a9cc2 reline 0.6.3 https://github.com/ruby/reline readline 0.0.4 https://github.com/ruby/readline fiddle 1.1.8 https://github.com/ruby/fiddle From 54d9f627d4f9e8b9af032242c83a7208eebb664c Mon Sep 17 00:00:00 2001 From: michaelg100 Date: Wed, 5 Aug 2026 09:14:58 -0400 Subject: [PATCH 2/9] [ruby/mmtk] Adding unit tests to ruby_heap_trigger.rs https://github.com/ruby/mmtk/commit/4089d04c49 --- gc/mmtk/src/heap/ruby_heap_trigger.rs | 51 +++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/gc/mmtk/src/heap/ruby_heap_trigger.rs b/gc/mmtk/src/heap/ruby_heap_trigger.rs index 63ee22b8bec13e..6188c7f0ced62a 100644 --- a/gc/mmtk/src/heap/ruby_heap_trigger.rs +++ b/gc/mmtk/src/heap/ruby_heap_trigger.rs @@ -103,3 +103,54 @@ impl RubyHeapTrigger { .expect("Attempt to use RUBY_HEAP_TRIGGER_CONFIG before it is initialized") } } + +#[cfg(test)] +mod tests { + use super::*; + + const MIN_HEAP_PAGES: usize = 256; + const MAX_HEAP_PAGES: usize = 4096; + + fn init_config() { + RUBY_HEAP_TRIGGER_CONFIG.get_or_init(|| RubyHeapTriggerConfig { + min_heap_pages: MIN_HEAP_PAGES, + max_heap_pages: MAX_HEAP_PAGES, + heap_pages_min_ratio: 0.2, + heap_pages_goal_ratio: 0.4, + heap_pages_max_ratio: 0.65, + }); + } + + fn trigger_with_target(target_heap_pages: usize) -> RubyHeapTrigger { + init_config(); + + RubyHeapTrigger { + target_heap_pages: AtomicUsize::new(target_heap_pages), + } + } + + #[test] + fn starts_at_the_min_heap_size() { + init_config(); + + let trigger = RubyHeapTrigger::default(); + + assert_eq!(trigger.get_current_heap_size_in_pages(), MIN_HEAP_PAGES); + } + + #[test] + fn reports_the_configured_max_heap_size() { + let trigger = trigger_with_target(MIN_HEAP_PAGES); + + assert_eq!(trigger.get_max_heap_size_in_pages(), MAX_HEAP_PAGES); + } + + #[test] + fn current_heap_size_tracks_the_target() { + let trigger = trigger_with_target(1024); + assert_eq!(trigger.get_current_heap_size_in_pages(), 1024); + + trigger.target_heap_pages.store(2048, Ordering::Relaxed); + assert_eq!(trigger.get_current_heap_size_in_pages(), 2048); + } +} From f01bbd415617ef14cf0bae542e87b959ee1feb05 Mon Sep 17 00:00:00 2001 From: Jun Aruga Date: Tue, 4 Aug 2026 19:18:50 +0100 Subject: [PATCH 3/9] [ruby/rubygems] Centralize testing key/cert constants in test/rubygems/pem_utilities.rb This commit refactors testing key/cert constants in a more consistent way. The key/cert constants were duplicated across the files, and there was a different constant naming `*_PATH` vs `*_FILE`. Create test/rubygems/pem_utilities.rb moving .pem methods/constants from test/rubygems/helper.rb to manage testing key/cert constants in one file. test/rubygems/local_ssl_server_utilities.rb, used by both RubyGems and Bundler, also uses these constants, so the new pem_utilities.rb must not depend on test-unit. In test/rubygems/pem_utilities.rb, added the following features compared to test/rubygems/helper.rb. * Add pem_file method for combined cert+key PEM files * Rename helper methods from key_path to key_file, from cert_path to cert_file, to consistently use the `*_FILE` suffix for constants. * Align test/rubygems/*.pem file names and constants with the pattern `_`. If the `` is RSA 2048 bits, there is no `_` prefix as a default algorithm. * Rename `EXPIRED_PUBLIC_CERT` to `EXPIRED_CERT`. CERT is only public. There is no private cert file. * Update local_ssl_server_utilities.rb to use constants directly, removing unused methods. * Handle OpenSSL::PKey::PKeyError in load_key for unsupported algorithms on old OpenSSL versions Assisted-by: Claude:claude-opus-4-6[1m] https://github.com/ruby/rubygems/commit/35d72d71e2 --- ...em_remote_fetcher_local_ssl_server_spec.rb | 13 +- ...{private_ec_key.pem => ec_private_key.pem} | 0 test/rubygems/helper.rb | 71 +--------- test/rubygems/local_ssl_server_utilities.rb | 32 ++--- test/rubygems/pem_utilities.rb | 130 ++++++++++++++++++ ...te3072_key.pem => rsa3072_private_key.pem} | 0 ...c3072_cert.pem => rsa3072_public_cert.pem} | 0 .../test_gem_commands_build_command.rb | 12 +- .../test_gem_commands_cert_command.rb | 41 ++---- test/rubygems/test_gem_package.rb | 4 +- ...est_gem_remote_fetcher_local_ssl_server.rb | 18 +-- test/rubygems/test_gem_request.rb | 7 - test/rubygems/test_gem_security.rb | 11 +- test/rubygems/test_gem_security_policy.rb | 17 --- test/rubygems/test_gem_security_signer.rb | 12 +- test/rubygems/test_gem_security_trust_dir.rb | 2 - 16 files changed, 180 insertions(+), 190 deletions(-) rename test/rubygems/{private_ec_key.pem => ec_private_key.pem} (100%) create mode 100644 test/rubygems/pem_utilities.rb rename test/rubygems/{private3072_key.pem => rsa3072_private_key.pem} (100%) rename test/rubygems/{public3072_cert.pem => rsa3072_public_cert.pem} (100%) diff --git a/spec/bundler/bundler/fetcher/gem_remote_fetcher_local_ssl_server_spec.rb b/spec/bundler/bundler/fetcher/gem_remote_fetcher_local_ssl_server_spec.rb index 91f02005586fc3..2a287af19587b6 100644 --- a/spec/bundler/bundler/fetcher/gem_remote_fetcher_local_ssl_server_spec.rb +++ b/spec/bundler/bundler/fetcher/gem_remote_fetcher_local_ssl_server_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "bundler/fetcher" +require Spec::Path.rubygems_test_dir.join("pem_utilities") require Spec::Path.rubygems_test_dir.join("local_ssl_server_utilities") RSpec.describe "Bundler::Fetcher local SSL server", if: Gem::HAVE_OPENSSL do @@ -19,7 +20,7 @@ it "connects" do ssl_server = start_ssl_server allow(Bundler.settings).to receive(:[]).and_call_original - allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(File.join(certs_dir, "ca_cert.pem")) + allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(Gem::PemUtilities::CA_CERT_FILE) response = fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") expect(response.code).to eq("200") end @@ -29,8 +30,8 @@ verify_mode: OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT ) allow(Bundler.settings).to receive(:[]).and_call_original - allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(File.join(certs_dir, "ca_cert.pem")) - allow(Bundler.settings).to receive(:[]).with(:ssl_client_cert).and_return(File.join(certs_dir, "client.pem")) + allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(Gem::PemUtilities::CA_CERT_FILE) + allow(Bundler.settings).to receive(:[]).with(:ssl_client_cert).and_return(Gem::PemUtilities::CLIENT_FILE) response = fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") expect(response.code).to eq("200") end @@ -44,7 +45,7 @@ it "connects" do ssl_server = start_ssl_server(mode: :pqc) allow(Bundler.settings).to receive(:[]).and_call_original - allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(File.join(certs_dir, "mldsa65_ca_cert.pem")) + allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(Gem::PemUtilities::MLDSA65_CA_CERT_FILE) response = fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") expect(response.code).to eq("200") end @@ -55,8 +56,8 @@ verify_mode: OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT ) allow(Bundler.settings).to receive(:[]).and_call_original - allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(File.join(certs_dir, "mldsa65_ca_cert.pem")) - allow(Bundler.settings).to receive(:[]).with(:ssl_client_cert).and_return(File.join(certs_dir, "mldsa65_client.pem")) + allow(Bundler.settings).to receive(:[]).with(:ssl_ca_cert).and_return(Gem::PemUtilities::MLDSA65_CA_CERT_FILE) + allow(Bundler.settings).to receive(:[]).with(:ssl_client_cert).and_return(Gem::PemUtilities::MLDSA65_CLIENT_FILE) response = fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") expect(response.code).to eq("200") end diff --git a/test/rubygems/private_ec_key.pem b/test/rubygems/ec_private_key.pem similarity index 100% rename from test/rubygems/private_ec_key.pem rename to test/rubygems/ec_private_key.pem diff --git a/test/rubygems/helper.rb b/test/rubygems/helper.rb index 1a4f49ad64170e..174bd258168b0d 100644 --- a/test/rubygems/helper.rb +++ b/test/rubygems/helper.rb @@ -46,6 +46,7 @@ require "rubygems/vendor/uri/lib/uri" require "zlib" require_relative "mock_gem_ui" +require_relative "pem_utilities" # JRuby on Windows raises TypeError inside File.symlink (the wincode helper # trips on a nil path), so any test that exercises Gem::Installer's symlink @@ -1658,75 +1659,7 @@ def prefetch(reqs) # :nodoc: end end - ## - # Loads certificate named +cert_name+ from test/rubygems/. - - def self.load_cert(cert_name) - cert_file = cert_path cert_name - - cert = File.read cert_file - - OpenSSL::X509::Certificate.new cert - end - - ## - # Returns the path to the certificate named +cert_name+ from - # test/rubygems/. - - def self.cert_path(cert_name) - if begin - Time.at(2**32) - rescue StandardError - 32 - end == 32 - cert_file = "#{__dir__}/#{cert_name}_cert_32.pem" - - return cert_file if File.exist? cert_file - end - - "#{__dir__}/#{cert_name}_cert.pem" - end - - ## - # Loads a private key named +key_name+ with +passphrase+ in test/rubygems/ - - def self.load_key(key_name, passphrase = nil) - key_file = key_path key_name - - key = File.read key_file - - OpenSSL::PKey.read key, passphrase - end - - ## - # Returns the path to the key named +key_name+ from test/rubygems - - def self.key_path(key_name) - "#{__dir__}/#{key_name}_key.pem" - end - - # :stopdoc: - # only available in RubyGems tests - - PRIVATE_KEY_PASSPHRASE = "Foo bar" - - begin - PRIVATE_KEY = load_key "private" - PRIVATE_KEY_PATH = key_path "private" - - # ENCRYPTED_PRIVATE_KEY is PRIVATE_KEY encrypted with PRIVATE_KEY_PASSPHRASE - ENCRYPTED_PRIVATE_KEY = load_key "encrypted_private", PRIVATE_KEY_PASSPHRASE - ENCRYPTED_PRIVATE_KEY_PATH = key_path "encrypted_private" - - PUBLIC_KEY = PRIVATE_KEY.public_key - - PUBLIC_CERT = load_cert "public" - PUBLIC_CERT_PATH = cert_path "public" - rescue Errno::ENOENT - PRIVATE_KEY = nil - PUBLIC_KEY = nil - PUBLIC_CERT = nil - end if Gem::HAVE_OPENSSL + include Gem::PemUtilities end # https://github.com/seattlerb/minitest/blob/13c48a03d84a2a87855a4de0c959f96800100357/lib/minitest/mock.rb#L192 diff --git a/test/rubygems/local_ssl_server_utilities.rb b/test/rubygems/local_ssl_server_utilities.rb index a068efa9642d96..d0d73dbd1219bf 100644 --- a/test/rubygems/local_ssl_server_utilities.rb +++ b/test/rubygems/local_ssl_server_utilities.rb @@ -5,14 +5,10 @@ require "socket" require "openssl" +require_relative "pem_utilities" module Gem::LocalSSLServerUtilities - CERTS_DIR = __dir__ - - def certs_dir - CERTS_DIR - end - + include Gem::PemUtilities def initialize_ssl_server @ssl_server_thread = nil @ssl_server = nil @@ -40,13 +36,13 @@ def start_ssl_server(config = {}) case mode when :non_pqc - ctx.cert = cert("ssl_cert.pem") - ctx.key = key("ssl_key.pem") - ctx.ca_file = File.join(certs_dir, "ca_cert.pem") + ctx.cert = SSL_CERT + ctx.key = SSL_KEY + ctx.ca_file = CA_CERT_FILE when :pqc - ctx.cert = cert("mldsa65_ssl_cert.pem") - ctx.key = key("mldsa65_ssl_key.pem") - ctx.ca_file = File.join(certs_dir, "mldsa65_ca_cert.pem") + ctx.cert = MLDSA65_SSL_CERT + ctx.key = MLDSA65_SSL_KEY + ctx.ca_file = MLDSA65_CA_CERT_FILE ctx.groups = "X25519MLKEM768" end @@ -79,14 +75,6 @@ def handle_request(client) end end - def cert(filename) - OpenSSL::X509::Certificate.new(File.read(File.join(certs_dir, filename))) - end - - def key(filename) - OpenSSL::PKey.read(File.read(File.join(certs_dir, filename))) - end - def without_pqc_support(&block) # PQC algorithms ML-KEM and ML-DSA require OpenSSL >= 3.5. # https://openssl-library.org/post/2025-04-08-openssl-35-final-release/ @@ -123,8 +111,8 @@ def self.support_pqc_handshake? def self.probe_pqc_handshake server = TCPServer.new("127.0.0.1", 0) ctx = OpenSSL::SSL::SSLContext.new - ctx.cert = OpenSSL::X509::Certificate.new(File.read(File.join(CERTS_DIR, "mldsa65_ssl_cert.pem"))) - ctx.key = OpenSSL::PKey.read(File.read(File.join(CERTS_DIR, "mldsa65_ssl_key.pem"))) + ctx.cert = Gem::PemUtilities::MLDSA65_SSL_CERT + ctx.key = Gem::PemUtilities::MLDSA65_SSL_KEY ctx.groups = "X25519MLKEM768" ssl_server = OpenSSL::SSL::SSLServer.new(server, ctx) diff --git a/test/rubygems/pem_utilities.rb b/test/rubygems/pem_utilities.rb new file mode 100644 index 00000000000000..bec08d9a38dd4f --- /dev/null +++ b/test/rubygems/pem_utilities.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +# This file can be loaded by RubyGems test-unit files and Bundler rspec files. +# Don't add test-unit or rspec dependent logic in this file. + +require "rubygems/openssl" + +module Gem::PemUtilities + ## + # Loads certificate named +cert_name+ from test/rubygems/. + + def self.load_cert(cert_name) + cert = File.read(cert_file(cert_name)) + OpenSSL::X509::Certificate.new cert + end + + ## + # Returns the file path to the certificate named +cert_name+ from + # test/rubygems/. + + def self.cert_file(cert_name) + if begin + Time.at(2**32) + rescue StandardError + 32 + end == 32 + cert_file = "#{__dir__}/#{cert_name}_cert_32.pem" + + return cert_file if File.exist? cert_file + end + + "#{__dir__}/#{cert_name}_cert.pem" + end + + ## + # Loads a private key named +key_name+ with +passphrase+ in test/rubygems/ + + def self.load_key(key_name, passphrase = nil) + key = File.read(key_file(key_name)) + + # Rescue if unsupported key algorithm's file is read with old OpenSSL versions. + begin + OpenSSL::PKey.read key, passphrase + rescue OpenSSL::PKey::PKeyError + nil + end + end + + ## + # Returns the file path to the key named +key_name+ from test/rubygems + + def self.key_file(key_name) + "#{__dir__}/#{key_name}_key.pem" + end + + ## + # Returns the file path to the PEM file named +pem_name+ from test/rubygems + + def self.pem_file(pem_name) + "#{__dir__}/#{pem_name}.pem" + end + + # :stopdoc: + + PRIVATE_KEY_PASSPHRASE = "Foo bar" + + if Gem::HAVE_OPENSSL + # Only the key and certificate constants used in tests are managed here. Add + # constants here when adding or using new .pem files. The constant naming + # convention is _. + + # Keys and certificates mostly generated by create_certs.sh + # RSA CA + CA_CERT = load_cert "ca" + CA_CERT_FILE = cert_file "ca" + # RSA server + SSL_KEY = load_key "ssl" + SSL_KEY_FILE = key_file "ssl" + SSL_CERT = load_cert "ssl" + SSL_CERT_FILE = cert_file "ssl" + # RSA client key/cert pair + CLIENT_FILE = pem_file "client" + # RSA invalid client manually created without script + INVALID_CLIENT_FILE = pem_file "invalid_client" + # ML-DSA-65 CA + MLDSA65_CA_CERT_FILE = cert_file "mldsa65_ca" + # ML-DSA-65 server + MLDSA65_SSL_KEY = load_key "mldsa65_ssl" + MLDSA65_SSL_KEY_FILE = key_file "mldsa65_ssl" + MLDSA65_SSL_CERT = load_cert "mldsa65_ssl" + MLDSA65_SSL_CERT_FILE = cert_file "mldsa65_ssl" + # ML-DSA-65 client key/cert pair + MLDSA65_CLIENT_FILE = pem_file "mldsa65_client" + + # Keys and certificates generated by create_certs.rb + PRIVATE_KEY = load_key "private" + PRIVATE_KEY_FILE = key_file "private" + # ENCRYPTED_PRIVATE_KEY is PRIVATE_KEY encrypted with PRIVATE_KEY_PASSPHRASE + ENCRYPTED_PRIVATE_KEY = load_key "encrypted_private", PRIVATE_KEY_PASSPHRASE + ENCRYPTED_PRIVATE_KEY_FILE = key_file "encrypted_private" + PUBLIC_KEY = PRIVATE_KEY.public_key + PUBLIC_KEY_FILE = key_file "public" + PUBLIC_CERT = load_cert "public" + PUBLIC_CERT_FILE = cert_file "public" + ALTERNATE_KEY = load_key "alternate" + ALTERNATE_KEY_FILE = key_file "alternate" + ALTERNATE_CERT = load_cert "alternate" + ALTERNATE_CERT_FILE = cert_file "alternate" + CHILD_KEY = load_key "child" + CHILD_CERT = load_cert "child" + CHILD_CERT_FILE = cert_file "child" + GRANDCHILD_CERT = load_cert "grandchild" + INVALID_ISSUER_CERT = load_cert "invalid_issuer" + INVALID_SIGNER_CERT = load_cert "invalid_signer" + INVALIDCHILD_CERT = load_cert "invalidchild" + EXPIRED_CERT = load_cert "expired" + EXPIRED_CERT_FILE = cert_file "expired" + FUTURE_CERT = load_cert "future" + WRONG_KEY_CERT = load_cert "wrong_key" + + # Keys and certificates manually created without script + # RSA 3072 bits + RSA3072_PRIVATE_KEY_FILE = key_file "rsa3072_private" + RSA3072_PUBLIC_CERT = load_cert "rsa3072_public" + RSA3072_PUBLIC_CERT_FILE = cert_file "rsa3072_public" + # EC + EC_PRIVATE_KEY = load_key "ec_private", PRIVATE_KEY_PASSPHRASE + EC_PRIVATE_KEY_FILE = key_file "ec_private" + end +end diff --git a/test/rubygems/private3072_key.pem b/test/rubygems/rsa3072_private_key.pem similarity index 100% rename from test/rubygems/private3072_key.pem rename to test/rubygems/rsa3072_private_key.pem diff --git a/test/rubygems/public3072_cert.pem b/test/rubygems/rsa3072_public_cert.pem similarity index 100% rename from test/rubygems/public3072_cert.pem rename to test/rubygems/rsa3072_public_cert.pem diff --git a/test/rubygems/test_gem_commands_build_command.rb b/test/rubygems/test_gem_commands_build_command.rb index 9339f41f7cb875..cd88421c0754ff 100644 --- a/test/rubygems/test_gem_commands_build_command.rb +++ b/test/rubygems/test_gem_commands_build_command.rb @@ -5,12 +5,6 @@ require "rubygems/package" class TestGemCommandsBuildCommand < Gem::TestCase - CERT_FILE = cert_path "public3072" - SIGNING_KEY = key_path "private3072" - - EXPIRED_CERT_FILE = cert_path "expired" - PRIVATE_KEY_FILE = key_path "private" - def setup super @@ -591,8 +585,8 @@ def test_build_signed_gem trust_dir = Gem::Security.trust_dir spec = util_spec "some_gem" do |s| - s.signing_key = SIGNING_KEY - s.cert_chain = [CERT_FILE] + s.signing_key = RSA3072_PRIVATE_KEY_FILE + s.cert_chain = [RSA3072_PUBLIC_CERT_FILE] end gemspec_file = File.join(@tempdir, spec.spec_name) @@ -605,7 +599,7 @@ def test_build_signed_gem util_test_build_gem spec - trust_dir.trust_cert OpenSSL::X509::Certificate.new(File.read(CERT_FILE)) + trust_dir.trust_cert RSA3072_PUBLIC_CERT gem = Gem::Package.new(File.join(@tempdir, spec.file_name), Gem::Security::HighSecurity) diff --git a/test/rubygems/test_gem_commands_cert_command.rb b/test/rubygems/test_gem_commands_cert_command.rb index b9207cdcbcd39a..17fe3d789a6f8c 100644 --- a/test/rubygems/test_gem_commands_cert_command.rb +++ b/test/rubygems/test_gem_commands_cert_command.rb @@ -12,19 +12,6 @@ end class TestGemCommandsCertCommand < Gem::TestCase - ALTERNATE_CERT = load_cert "alternate" - EXPIRED_PUBLIC_CERT = load_cert "expired" - - ALTERNATE_KEY_FILE = key_path "alternate" - PRIVATE_KEY_FILE = key_path "private" - PRIVATE_EC_KEY_FILE = key_path "private_ec" - PUBLIC_KEY_FILE = key_path "public" - - ALTERNATE_CERT_FILE = cert_path "alternate" - CHILD_CERT_FILE = cert_path "child" - PUBLIC_CERT_FILE = cert_path "public" - EXPIRED_PUBLIC_CERT_FILE = cert_path "expired" - def setup super @@ -84,8 +71,6 @@ def test_execute_add end def test_execute_add_twice - self.class.cert_path "alternate" - @cmd.handle_options %W[ --add #{PUBLIC_CERT_FILE} --add #{ALTERNATE_CERT_FILE} @@ -289,7 +274,7 @@ def test_execute_build_key def test_execute_build_encrypted_key @cmd.handle_options %W[ --build nobody@example.com - --private-key #{ENCRYPTED_PRIVATE_KEY_PATH} + --private-key #{ENCRYPTED_PRIVATE_KEY_FILE} ] use_ui @ui do @@ -310,7 +295,7 @@ def test_execute_build_encrypted_key def test_execute_build_ec_key @cmd.handle_options %W[ --build nobody@example.com - --private-key #{PRIVATE_EC_KEY_FILE} + --private-key #{EC_PRIVATE_KEY_FILE} ] use_ui @ui do @@ -397,7 +382,7 @@ def test_execute_private_key def test_execute_encrypted_private_key use_ui @ui do - @cmd.send :handle_options, %W[--private-key #{ENCRYPTED_PRIVATE_KEY_PATH}] + @cmd.send :handle_options, %W[--private-key #{ENCRYPTED_PRIVATE_KEY_FILE}] end assert_equal "", @ui.output @@ -517,7 +502,7 @@ def test_execute_sign_encrypted_key assert_equal "/CN=alternate/DC=example", ALTERNATE_CERT.issuer.to_s @cmd.handle_options %W[ - --private-key #{ENCRYPTED_PRIVATE_KEY_PATH} + --private-key #{ENCRYPTED_PRIVATE_KEY_FILE} --certificate #{PUBLIC_CERT_FILE} --sign #{path} @@ -684,12 +669,12 @@ def test_execute_re_sign Dir.mkdir gem_path path = File.join @tempdir, "cert.pem" - Gem::Security.write_certificate EXPIRED_PUBLIC_CERT, path, 0o600 + Gem::Security.write_certificate EXPIRED_CERT, path, 0o600 - assert_equal "/CN=nobody/DC=example", EXPIRED_PUBLIC_CERT.issuer.to_s + assert_equal "/CN=nobody/DC=example", EXPIRED_CERT.issuer.to_s - tmp_expired_cert_file = File.join(@tempdir, File.basename(EXPIRED_PUBLIC_CERT_FILE)) - File.write(tmp_expired_cert_file, File.read(EXPIRED_PUBLIC_CERT_FILE)) + tmp_expired_cert_file = File.join(@tempdir, File.basename(EXPIRED_CERT_FILE)) + File.write(tmp_expired_cert_file, File.read(EXPIRED_CERT_FILE)) @cmd.handle_options %W[ --private-key #{PRIVATE_KEY_FILE} @@ -716,12 +701,12 @@ def test_execute_re_sign_with_cert_expiration_length_days Dir.mkdir gem_path path = File.join @tempdir, "cert.pem" - Gem::Security.write_certificate EXPIRED_PUBLIC_CERT, path, 0o600 + Gem::Security.write_certificate EXPIRED_CERT, path, 0o600 - assert_equal "/CN=nobody/DC=example", EXPIRED_PUBLIC_CERT.issuer.to_s + assert_equal "/CN=nobody/DC=example", EXPIRED_CERT.issuer.to_s - tmp_expired_cert_file = File.join(@tempdir, File.basename(EXPIRED_PUBLIC_CERT_FILE)) - File.write(tmp_expired_cert_file, File.read(EXPIRED_PUBLIC_CERT_FILE)) + tmp_expired_cert_file = File.join(@tempdir, File.basename(EXPIRED_CERT_FILE)) + File.write(tmp_expired_cert_file, File.read(EXPIRED_CERT_FILE)) @cmd.handle_options %W[ --private-key #{PRIVATE_KEY_FILE} @@ -851,7 +836,7 @@ def test_handle_options_sign def test_handle_options_sign_encrypted_key @cmd.handle_options %W[ --private-key #{ALTERNATE_KEY_FILE} - --private-key #{ENCRYPTED_PRIVATE_KEY_PATH} + --private-key #{ENCRYPTED_PRIVATE_KEY_FILE} --certificate #{ALTERNATE_CERT_FILE} --certificate #{PUBLIC_CERT_FILE} diff --git a/test/rubygems/test_gem_package.rb b/test/rubygems/test_gem_package.rb index 99763f4df19ec2..7b8ac4736d2f46 100644 --- a/test/rubygems/test_gem_package.rb +++ b/test/rubygems/test_gem_package.rb @@ -275,7 +275,7 @@ def test_build_auto_signed Gem::Security.write_private_key PRIVATE_KEY, private_key_path public_cert_path = File.join Gem.user_home, ".gem", "gem-public_cert.pem" - FileUtils.cp PUBLIC_CERT_PATH, public_cert_path + FileUtils.cp PUBLIC_CERT_FILE, public_cert_path spec = Gem::Specification.new "build", "1" spec.summary = "build" @@ -315,7 +315,7 @@ def test_build_auto_signed_encrypted_key FileUtils.mkdir_p File.join(Gem.user_home, ".gem") private_key_path = File.join Gem.user_home, ".gem", "gem-private_key.pem" - FileUtils.cp ENCRYPTED_PRIVATE_KEY_PATH, private_key_path + FileUtils.cp ENCRYPTED_PRIVATE_KEY_FILE, private_key_path public_cert_path = File.join Gem.user_home, ".gem", "gem-public_cert.pem" Gem::Security.write_certificate PUBLIC_CERT, public_cert_path diff --git a/test/rubygems/test_gem_remote_fetcher_local_ssl_server.rb b/test/rubygems/test_gem_remote_fetcher_local_ssl_server.rb index 95e6d3ac65a80a..813e8e2f026567 100644 --- a/test/rubygems/test_gem_remote_fetcher_local_ssl_server.rb +++ b/test/rubygems/test_gem_remote_fetcher_local_ssl_server.rb @@ -26,7 +26,7 @@ def teardown def test_ssl_connection ssl_server = start_ssl_server - temp_ca_cert = File.join(certs_dir, "ca_cert.pem") + temp_ca_cert = CA_CERT_FILE with_configured_fetcher(":ssl_ca_cert: #{temp_ca_cert}") do |fetcher| fetcher.fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") end @@ -36,7 +36,7 @@ def test_pqc_ssl_connection omit_unless_support_pqc ssl_server = start_ssl_server(mode: :pqc) - temp_ca_cert = File.join(certs_dir, "mldsa65_ca_cert.pem") + temp_ca_cert = MLDSA65_CA_CERT_FILE with_configured_fetcher(":ssl_ca_cert: #{temp_ca_cert}") do |fetcher| fetcher.fetch_path("https://localhost:#{ssl_server.addr[1]}/yaml") end @@ -47,8 +47,8 @@ def test_ssl_client_cert_auth_connection { verify_mode: OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT } ) - temp_ca_cert = File.join(certs_dir, "ca_cert.pem") - temp_client_cert = File.join(certs_dir, "client.pem") + temp_ca_cert = CA_CERT_FILE + temp_client_cert = CLIENT_FILE with_configured_fetcher( ":ssl_ca_cert: #{temp_ca_cert}\n" \ @@ -66,8 +66,8 @@ def test_pqc_ssl_client_cert_auth_connection verify_mode: OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT ) - temp_ca_cert = File.join(certs_dir, "mldsa65_ca_cert.pem") - temp_client_cert = File.join(certs_dir, "mldsa65_client.pem") + temp_ca_cert = MLDSA65_CA_CERT_FILE + temp_client_cert = MLDSA65_CLIENT_FILE with_configured_fetcher( ":ssl_ca_cert: #{temp_ca_cert}\n" \ @@ -82,8 +82,8 @@ def test_do_not_allow_invalid_client_cert_auth_connection { verify_mode: OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT } ) - temp_ca_cert = File.join(certs_dir, "ca_cert.pem") - temp_client_cert = File.join(certs_dir, "invalid_client.pem") + temp_ca_cert = CA_CERT_FILE + temp_client_cert = INVALID_CLIENT_FILE with_configured_fetcher( ":ssl_ca_cert: #{temp_ca_cert}\n" \ @@ -114,7 +114,7 @@ def test_ssl_connection_allow_verify_none def test_do_not_follow_insecure_redirect @server_uri = "http://example.com" ssl_server = start_ssl_server - temp_ca_cert = File.join(certs_dir, "ca_cert.pem") + temp_ca_cert = CA_CERT_FILE expected_error_message = "redirecting to non-https resource: #{@server_uri} (https://localhost:#{ssl_server.addr[1]}/insecure_redirect?to=#{@server_uri})" diff --git a/test/rubygems/test_gem_request.rb b/test/rubygems/test_gem_request.rb index cd0a416e79c02d..cee7e8943f232e 100644 --- a/test/rubygems/test_gem_request.rb +++ b/test/rubygems/test_gem_request.rb @@ -8,13 +8,6 @@ end class TestGemRequest < Gem::TestCase - CA_CERT_FILE = cert_path "ca" - CHILD_CERT = load_cert "child" - EXPIRED_CERT = load_cert "expired" - PUBLIC_CERT = load_cert "public" - PUBLIC_CERT_FILE = cert_path "public" - SSL_CERT = load_cert "ssl" - def make_request(uri, request_class, last_modified, proxy) Gem::Request.create_with_proxy uri, request_class, last_modified, proxy end diff --git a/test/rubygems/test_gem_security.rb b/test/rubygems/test_gem_security.rb index e97af3b959e95c..20f4b9fa8951cb 100644 --- a/test/rubygems/test_gem_security.rb +++ b/test/rubygems/test_gem_security.rb @@ -12,13 +12,6 @@ end class TestGemSecurity < Gem::TestCase - CHILD_KEY = load_key "child" - EC_KEY = load_key "private_ec", "Foo bar" - - ALTERNATE_CERT = load_cert "child" - CHILD_CERT = load_cert "child" - EXPIRED_CERT = load_cert "expired" - def test_class_create_cert name = PUBLIC_CERT.subject key = PRIVATE_KEY @@ -122,7 +115,7 @@ def test_class_get_public_key_rsa end def test_class_get_public_key_ec - pkey = Gem::Security.get_public_key(EC_KEY) + pkey = Gem::Security.get_public_key(EC_PRIVATE_KEY) assert_respond_to pkey, :public_to_pem end @@ -163,7 +156,7 @@ def test_class_re_sign_not_self_signed end assert_equal "#{child_alt_name.value} is not self-signed, contact " \ - "#{ALTERNATE_CERT.issuer} to obtain a valid certificate", + "#{CHILD_CERT.issuer} to obtain a valid certificate", e.message end diff --git a/test/rubygems/test_gem_security_policy.rb b/test/rubygems/test_gem_security_policy.rb index 2f4fb1ce287597..30116ae9ef428d 100644 --- a/test/rubygems/test_gem_security_policy.rb +++ b/test/rubygems/test_gem_security_policy.rb @@ -7,23 +7,6 @@ end class TestGemSecurityPolicy < Gem::TestCase - ALTERNATE_KEY = load_key "alternate" - INVALID_KEY = load_key "invalid" - CHILD_KEY = load_key "child" - GRANDCHILD_KEY = load_key "grandchild" - INVALIDCHILD_KEY = load_key "invalidchild" - - ALTERNATE_CERT = load_cert "alternate" - CA_CERT = load_cert "ca" - CHILD_CERT = load_cert "child" - EXPIRED_CERT = load_cert "expired" - FUTURE_CERT = load_cert "future" - GRANDCHILD_CERT = load_cert "grandchild" - INVALIDCHILD_CERT = load_cert "invalidchild" - INVALID_ISSUER_CERT = load_cert "invalid_issuer" - INVALID_SIGNER_CERT = load_cert "invalid_signer" - WRONG_KEY_CERT = load_cert "wrong_key" - def setup super diff --git a/test/rubygems/test_gem_security_signer.rb b/test/rubygems/test_gem_security_signer.rb index d0541b0a341f80..59a379322b6340 100644 --- a/test/rubygems/test_gem_security_signer.rb +++ b/test/rubygems/test_gem_security_signer.rb @@ -7,14 +7,6 @@ end class TestGemSecuritySigner < Gem::TestCase - ALTERNATE_KEY = load_key "alternate" - CHILD_KEY = load_key "child" - GRANDCHILD_KEY = load_key "grandchild" - - CHILD_CERT = load_cert "child" - GRANDCHILD_CERT = load_cert "grandchild" - EXPIRED_CERT = load_cert "expired" - def setup super @@ -70,7 +62,7 @@ def test_initialize_default end def test_initialize_key_path - key_file = PRIVATE_KEY_PATH + key_file = PRIVATE_KEY_FILE signer = Gem::Security::Signer.new key_file, nil @@ -78,7 +70,7 @@ def test_initialize_key_path end def test_initialize_encrypted_key_path - key_file = ENCRYPTED_PRIVATE_KEY_PATH + key_file = ENCRYPTED_PRIVATE_KEY_FILE signer = Gem::Security::Signer.new key_file, nil, PRIVATE_KEY_PASSPHRASE diff --git a/test/rubygems/test_gem_security_trust_dir.rb b/test/rubygems/test_gem_security_trust_dir.rb index bd3dfb86c231ca..57d7d1e4e8c056 100644 --- a/test/rubygems/test_gem_security_trust_dir.rb +++ b/test/rubygems/test_gem_security_trust_dir.rb @@ -7,8 +7,6 @@ end class TestGemSecurityTrustDir < Gem::TestCase - CHILD_CERT = load_cert "child" - def setup super From 8593c2befc6432a9e5d52045491743b9bd7aa60e Mon Sep 17 00:00:00 2001 From: XrXr Date: Fri, 31 Jul 2026 10:56:15 -0400 Subject: [PATCH 4/9] ZJIT: Add LIR `test_fallthrough_to_a_patchpoint` snapshot tests --- zjit/src/backend/arm64/mod.rs | 50 +++++++++++++++++++++++++++++++++ zjit/src/backend/x86_64/mod.rs | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index f74098f900c1b9..1251f65c5a0b57 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -1760,6 +1760,56 @@ mod tests { assert_eq!(20, cb.jmp_ptr_bytes()); } + #[test] + fn test_fallthrough_to_a_patchpoint() { + // At one point, this generated unnecessary nop padding + use crate::cruby::test_utils::{compile_to_iseq, with_rubyvm}; + use crate::hir::Invariant; + use crate::payload::IseqVersion; + + let version = IseqVersion::new(compile_to_iseq("nil")); + + // The PosMarker that split_patch_point() installs registers the patch point + // with ZJITState's invariant table while emitting, so the VM has to be booted. + let cb = with_rubyvm(|| { + crate::options::rb_zjit_prepare_options(); // Allow `get_option!` in Assembler + let mut asm = Assembler::new(); + let mut cb = CodeBlock::new_dummy(); + + let bb0 = asm.new_block(crate::hir::BlockId(0), true, 0); + let bb1 = asm.new_block(crate::hir::BlockId(1), false, 1); + + // The patch point's target only has to resolve to some address for the + // PosMarker that records it, so bb0 stands in for the side exit code. + let side_exit = asm.new_label("side_exit"); + + // bb0 falls through to bb1 + asm.set_current_block(bb0); + let label_bb0 = asm.new_label("bb0"); + asm.write_label(label_bb0); + asm.write_label(side_exit.clone()); + asm.mov(Opnd::Reg(TEMP_REGS[0]), Opnd::UImm(1)); + asm.push_insn(Insn::Jmp(Target::Block(Box::new(BranchEdge { target: bb1, args: vec![] })))); + + asm.set_current_block(bb1); + let label_bb1 = asm.new_label("bb1"); + asm.write_label(label_bb1); + asm.patch_point(side_exit.clone(), Invariant::SingleRactorMode, version); + asm.cret(Opnd::Reg(TEMP_REGS[0])); + + asm.compile_with_num_regs(&mut cb, 0); + cb + }); + + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: mov x1, #1 + 0x4: nop + 0x8: mov x0, x1 + 0xc: ret + "); + assert_snapshot!(cb.hexdump(), @"210080d21f2003d5e00301aac0035fd6"); + } + #[test] fn test_lir_string() { use crate::hir::SideExitReason; diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index e83ad9f2e7ad73..18f964e600850c 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -1377,6 +1377,57 @@ mod tests { } } + #[test] + fn test_fallthrough_to_a_patchpoint() { + // At one point, this generated unnecessary nop padding + use crate::cruby::test_utils::{compile_to_iseq, with_rubyvm}; + use crate::hir::Invariant; + use crate::payload::IseqVersion; + + let version = IseqVersion::new(compile_to_iseq("nil")); + + // The PosMarker that split_patch_point() installs registers the patch point + // with ZJITState's invariant table while emitting, so the VM has to be booted. + let cb = with_rubyvm(|| { + crate::options::rb_zjit_prepare_options(); // Allow `get_option!` in Assembler + let mut asm = Assembler::new(); + let mut cb = CodeBlock::new_dummy(); + + let bb0 = asm.new_block(crate::hir::BlockId(0), true, 0); + let bb1 = asm.new_block(crate::hir::BlockId(1), false, 1); + + // The patch point's target only has to resolve to some address for the + // PosMarker that records it, so bb0 stands in for the side exit code. + let side_exit = asm.new_label("side_exit"); + + // bb0 falls through to bb1 + asm.set_current_block(bb0); + let label_bb0 = asm.new_label("bb0"); + asm.write_label(label_bb0); + asm.write_label(side_exit.clone()); + asm.mov(C_ARG_OPNDS[0], Opnd::UImm(1)); + asm.push_insn(Insn::Jmp(Target::Block(Box::new(BranchEdge { target: bb1, args: vec![] })))); + + asm.set_current_block(bb1); + let label_bb1 = asm.new_label("bb1"); + asm.write_label(label_bb1); + asm.patch_point(side_exit.clone(), Invariant::SingleRactorMode, version); + asm.cret(C_ARG_OPNDS[0]); + + asm.compile_with_num_regs(&mut cb, 0); + cb + }); + + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: mov edi, 1 + 0x5: nop dword ptr [rax + rax] + 0xa: mov rax, rdi + 0xd: ret + 0xe: nop + "); + assert_snapshot!(cb.hexdump(), @"bf010000000f1f4400004889f8c390"); + } + #[test] fn test_lir_string() { use crate::hir::SideExitReason; From a540312b63020e5136ecaa33176a8eba61d892fe Mon Sep 17 00:00:00 2001 From: XrXr Date: Tue, 28 Jul 2026 16:07:24 -0400 Subject: [PATCH 5/9] ZJIT: Remove nops for PatchPoint too close to start of LIR BB Previously we put a `nop` at the beginning of a block because any two `PadPatchPoint` checked for minimum distance. The `PadPatchPoint` inserted at block boundary isn't itself a patch point position, so when we had: PadPatchPoint // Enough space PadPatchPoint // block boundary // Not enough space PadPatchPoint We got an unnecessary nop. Add a new `BoundaryPad` instruction that only pads for `PadPatchPoint` preceding it. Note that not all padding for patchpoints are gone, as we still pad for when two patchpoints are too close to each other. We'd need to tweak bookkeeping in invariants.rs to support two patchpoints having the same code address. --- zjit/src/backend/arm64/mod.rs | 30 ++++++++++++++++++--------- zjit/src/backend/lir.rs | 38 +++++++++++++++++++++++++++------- zjit/src/backend/x86_64/mod.rs | 34 +++++++++++++++++++----------- 3 files changed, 72 insertions(+), 30 deletions(-) diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index 1251f65c5a0b57..31d0b71c79b3c0 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -1068,6 +1068,15 @@ impl Assembler { ldr_post(cb, opnd, A64Opnd::new_mem(64, C_SP_REG, C_SP_STEP)); } + /// Fill nops until a jump written at `last_patch_pos` can no longer reach + /// past the current write position. + fn emit_pad_after_patch_point(cb: &mut CodeBlock, last_patch_pos: Option) { + let Some(last_patch_pos) = last_patch_pos else { return }; + while cb.get_write_pos().saturating_sub(last_patch_pos) < cb.jmp_ptr_bytes() && !cb.has_dropped_bytes() { + nop(cb); + } + } + // List of GC offsets let mut gc_offsets: Vec = Vec::new(); @@ -1521,14 +1530,16 @@ impl Assembler { }, Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PadPatchPoint in arm64_scratch_split"), Insn::PadPatchPoint => { - // If patch points are too close to each other or the end of the block, fill nop instructions - if let Some(last_patch_pos) = last_patch_pos { - while cb.get_write_pos().saturating_sub(last_patch_pos) < cb.jmp_ptr_bytes() && !cb.has_dropped_bytes() { - nop(cb); - } - } + emit_pad_after_patch_point(cb, last_patch_pos); + // This position is itself where a jump gets written on invalidation, so it + // becomes what following code has to keep its distance from. last_patch_pos = Some(cb.get_write_pos()); }, + Insn::BoundaryPad => { + // A boundary is never patched, so it doesn't become a position to keep away + // from. The last patch point stays that, and the pad just gave it its room. + emit_pad_after_patch_point(cb, last_patch_pos); + }, Insn::IncrCounter { mem, value } => { // Get the status register allocated by arm64_scratch_split let Some(Insn::Cmp { @@ -1803,11 +1814,10 @@ mod tests { assert_disasm_snapshot!(cb.disasm(), @" 0x0: mov x1, #1 - 0x4: nop - 0x8: mov x0, x1 - 0xc: ret + 0x4: mov x0, x1 + 0x8: ret "); - assert_snapshot!(cb.hexdump(), @"210080d21f2003d5e00301aac0035fd6"); + assert_snapshot!(cb.hexdump(), @"210080d2e00301aac0035fd6"); } #[test] diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index c284bab6e9e096..e9958caf4fa96a 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -910,11 +910,27 @@ pub enum Insn { /// Cold fields are boxed (see `PatchPointData`) to keep `Insn` small. PatchPoint(Box), - /// Make sure the last PatchPoint has enough space to insert a jump. - /// We insert this instruction at the end of each block so that the jump - /// will not overwrite the next block or a side exit. + /// Space reserved immediately before a PatchPoint, keeping the *preceding* + /// patch point's invalidation jump from running over this one's address. + /// The position right after this pad is itself where a jump gets written on + /// invalidation, so whatever follows has to keep `jmp_ptr_bytes()` of + /// distance from it in turn. + /// + /// Zero-width whenever there is already enough room, which is the common + /// case. PadPatchPoint, + /// Space reserved at a boundary that a preceding PatchPoint's invalidation + /// jump must not cross: the start of a non-entry block, the start of a side + /// exit, or the end of the last block. Nothing is ever patched at a + /// boundary, so unlike [`Insn::PadPatchPoint`] this only protects what comes + /// after it, and it does not become something later code has to keep away + /// from — the first patch point of a block needs no padding in front of it. + /// + /// Zero-width whenever there is already enough room, which is the common + /// case. + BoundaryPad, + // Mark a position in the generated code PosMarker(PosMarkerFn), @@ -1001,6 +1017,7 @@ macro_rules! for_each_operand_impl { } Insn::BakeString(_) | + Insn::BoundaryPad | Insn::Breakpoint | Insn::Abort | Insn::Comment(_) | Insn::CPop { .. } | @@ -1139,6 +1156,7 @@ impl Insn { Insn::Add { .. } => "Add", Insn::And { .. } => "And", Insn::BakeString(_) => "BakeString", + Insn::BoundaryPad => "BoundaryPad", Insn::Breakpoint => "Breakpoint", Insn::Abort => "Abort", Insn::Comment(_) => "Comment", @@ -1888,8 +1906,8 @@ impl Assembler // Entry blocks shouldn't ever be preceded by something that can // stomp on this block. if !block.is_entry { - push_insns_with_perf_symbol(&mut insns, "PadPatchPoint", |insns| { - insns.push(Insn::PadPatchPoint); + push_insns_with_perf_symbol(&mut insns, "BoundaryPad", |insns| { + insns.push(Insn::BoundaryPad); }); } @@ -1922,8 +1940,8 @@ impl Assembler // Make sure we don't stomp on the next function if block_id.0 == num_blocks - 1 { - push_insns_with_perf_symbol(&mut insns, "PadPatchPoint", |insns| { - insns.push(Insn::PadPatchPoint); + push_insns_with_perf_symbol(&mut insns, "BoundaryPad", |insns| { + insns.push(Insn::BoundaryPad); }); } } @@ -2857,7 +2875,7 @@ impl Assembler // Side exit blocks are not part of the CFG at the moment, // so we need to manually ensure that patchpoints get padded // so that nobody stomps on us - asm.pad_patch_point(); + asm.boundary_pad(); asm_comment!(asm, "save cfp->pc"); asm.store(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_PC), *pc); @@ -3977,6 +3995,10 @@ impl Assembler { self.push_insn(Insn::PadPatchPoint); } + pub fn boundary_pad(&mut self) { + self.push_insn(Insn::BoundaryPad); + } + pub fn pos_marker(&mut self, marker_fn: impl Fn(CodePtr, &CodeBlock) + 'static) { self.push_insn(Insn::PosMarker(Rc::new(marker_fn))); } diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index 18f964e600850c..4755f7d3b5728b 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -744,6 +744,16 @@ impl Assembler { gc_offsets.push(ptr_offset); } + /// Fill nops until a jump written at `last_patch_pos` can no longer reach + /// past the current write position. + fn emit_pad_after_patch_point(cb: &mut CodeBlock, last_patch_pos: Option) { + let Some(last_patch_pos) = last_patch_pos else { return }; + let code_size = cb.get_write_pos().saturating_sub(last_patch_pos); + if code_size < cb.jmp_ptr_bytes() { + nop(cb, (cb.jmp_ptr_bytes() - code_size) as u32); + } + } + // List of GC offsets let mut gc_offsets: Vec = Vec::new(); @@ -1061,15 +1071,16 @@ impl Assembler { Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PadPatchPoint in x86_scratch_split"), Insn::PadPatchPoint => { - // If patch points are too close to each other or the end of the block, fill nop instructions - if let Some(last_patch_pos) = last_patch_pos { - let code_size = cb.get_write_pos().saturating_sub(last_patch_pos); - if code_size < cb.jmp_ptr_bytes() { - nop(cb, (cb.jmp_ptr_bytes() - code_size) as u32); - } - } + emit_pad_after_patch_point(cb, last_patch_pos); + // This position is itself where a jump gets written on invalidation, so it + // becomes what following code has to keep its distance from. last_patch_pos = Some(cb.get_write_pos()); }, + Insn::BoundaryPad => { + // A boundary is never patched, so it doesn't become a position to keep away + // from. The last patch point stays that, and the pad just gave it its room. + emit_pad_after_patch_point(cb, last_patch_pos); + }, // Atomically increment a counter at a given memory location Insn::IncrCounter { mem, value } => { @@ -1420,12 +1431,11 @@ mod tests { assert_disasm_snapshot!(cb.disasm(), @" 0x0: mov edi, 1 - 0x5: nop dword ptr [rax + rax] - 0xa: mov rax, rdi - 0xd: ret - 0xe: nop + 0x5: mov rax, rdi + 0x8: ret + 0x9: nop "); - assert_snapshot!(cb.hexdump(), @"bf010000000f1f4400004889f8c390"); + assert_snapshot!(cb.hexdump(), @"bf010000004889f8c390"); } #[test] From ac0e68da5228a0b2996c677eb647d4c6a4983d20 Mon Sep 17 00:00:00 2001 From: XrXr Date: Thu, 30 Jul 2026 15:50:23 -0400 Subject: [PATCH 6/9] ZJIT: Pull end-of-function pad out of RPO loop I think there was a bug there because it checked for block id while iterating with reverse-post order. No need to check when we can do it unconditionally outside the loop. (Do we ever try to compile an empty assembler?) --- zjit/src/backend/lir.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index e9958caf4fa96a..6b44424dd91420 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -1897,9 +1897,7 @@ impl Assembler // Emit instructions with labels, expanding branch parameters let mut insns = Vec::with_capacity(ASSEMBLER_INSNS_CAPACITY); - let block_ids = self.block_order(); - let num_blocks = block_ids.len(); for (i, block_id) in block_ids.iter().enumerate() { let block = &self.basic_blocks[block_id.0]; @@ -1937,14 +1935,12 @@ impl Assembler if let Some(marker) = block_end_pos_marker { insns.push(Insn::PosMarker(marker)); } - - // Make sure we don't stomp on the next function - if block_id.0 == num_blocks - 1 { - push_insns_with_perf_symbol(&mut insns, "BoundaryPad", |insns| { - insns.push(Insn::BoundaryPad); - }); - } } + // Make sure we don't stomp on the next function + push_insns_with_perf_symbol(&mut insns, "BoundaryPad", |insns| { + insns.push(Insn::BoundaryPad); + }); + insns } From 69b13f2a1b77c13c0fa3615688bde354b50039f5 Mon Sep 17 00:00:00 2001 From: XrXr Date: Thu, 30 Jul 2026 15:56:24 -0400 Subject: [PATCH 7/9] ZJIT: Rename `PadPatchPoint` to `PatchPointPad` Two reasons for this: 1. It's less weird for a pad to be zero width than for an imperative to pad to not actually pad. 2. PatchPoint is a noun so its related padding reads better when can only be interpreted as a noun. --- zjit/src/backend/arm64/mod.rs | 4 ++-- zjit/src/backend/lir.rs | 12 ++++++------ zjit/src/backend/x86_64/mod.rs | 4 ++-- zjit/src/codegen.rs | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index 31d0b71c79b3c0..21812829763cda 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -1528,8 +1528,8 @@ impl Assembler { Insn::Jonz(opnd, target) => { emit_cmp_zero_jump(cb, opnd.into(), false, target.clone()); }, - Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PadPatchPoint in arm64_scratch_split"), - Insn::PadPatchPoint => { + Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PatchPointPad in arm64_scratch_split"), + Insn::PatchPointPad => { emit_pad_after_patch_point(cb, last_patch_pos); // This position is itself where a jump gets written on invalidation, so it // becomes what following code has to keep its distance from. diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index 6b44424dd91420..ca2d5e53ddcbb6 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -918,12 +918,12 @@ pub enum Insn { /// /// Zero-width whenever there is already enough room, which is the common /// case. - PadPatchPoint, + PatchPointPad, /// Space reserved at a boundary that a preceding PatchPoint's invalidation /// jump must not cross: the start of a non-entry block, the start of a side /// exit, or the end of the last block. Nothing is ever patched at a - /// boundary, so unlike [`Insn::PadPatchPoint`] this only protects what comes + /// boundary, so unlike [`Insn::PatchPointPad`] this only protects what comes /// after it, and it does not become something later code has to keep away /// from — the first patch point of a block needs no padding in front of it. /// @@ -1021,7 +1021,7 @@ macro_rules! for_each_operand_impl { Insn::Breakpoint | Insn::Abort | Insn::Comment(_) | Insn::CPop { .. } | - Insn::PadPatchPoint | + Insn::PatchPointPad | Insn::PosMarker(_) | Insn::PosMarkerAtBlockEnd(_) => {}, @@ -1205,7 +1205,7 @@ impl Insn { Insn::Not { .. } => "Not", Insn::Or { .. } => "Or", Insn::PatchPoint(..) => "PatchPoint", - Insn::PadPatchPoint => "PadPatchPoint", + Insn::PatchPointPad => "PatchPointPad", Insn::PosMarker(_) => "PosMarker", Insn::PosMarkerAtBlockEnd(_) => "PosMarkerAtBlockEnd", Insn::RShift { .. } => "RShift", @@ -3987,8 +3987,8 @@ impl Assembler { self.push_insn(Insn::PatchPoint(Box::new(PatchPointData { target, invariant, version }))); } - pub fn pad_patch_point(&mut self) { - self.push_insn(Insn::PadPatchPoint); + pub fn patch_point_pad(&mut self) { + self.push_insn(Insn::PatchPointPad); } pub fn boundary_pad(&mut self) { diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index 4755f7d3b5728b..35fff351148df9 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -1069,8 +1069,8 @@ impl Assembler { Insn::Joz(..) | Insn::Jonz(..) => unreachable!("Joz/Jonz should be unused for now"), - Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PadPatchPoint in x86_scratch_split"), - Insn::PadPatchPoint => { + Insn::PatchPoint(..) => unreachable!("PatchPoint should have been lowered to PatchPointPad in x86_scratch_split"), + Insn::PatchPointPad => { emit_pad_after_patch_point(cb, last_patch_pos); // This position is itself where a jump gets written on invalidation, so it // becomes what following code has to keep its distance from. diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index dace5d4b5dec5e..43670f5561b6e1 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -980,7 +980,7 @@ fn gen_patch_point(jit: &mut JITState, asm: &mut Assembler, function: &Function, asm.patch_point(Target::SideExit(Box::new(SideExitTarget { exit, reason: PatchPoint(invariant) })), invariant, jit.version); } -/// This is used by scratch_split to lower PatchPoint into PadPatchPoint and PosMarker. +/// This is used by scratch_split to lower PatchPoint into PatchPointPad and PosMarker. /// It's called at scratch_split so that we can use the Label after side-exit deduplication in compile_exits. pub fn split_patch_point(asm: &mut Assembler, target: &Target, invariant: Invariant, version: IseqVersionRef) { let Target::Label(exit_label) = *target else { @@ -988,7 +988,7 @@ pub fn split_patch_point(asm: &mut Assembler, target: &Target, invariant: Invari }; // Fill nop instructions if the last patch point is too close. - asm.pad_patch_point(); + asm.patch_point_pad(); // Remember the current address as a patch point asm.pos_marker(move |code_ptr, cb| { From 19f5b1248dfda47c48aa419a050bf596244a19b3 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Wed, 5 Aug 2026 17:56:00 -0400 Subject: [PATCH 8/9] Reintroduce recent PRISM changes This reverts commit 895084e65bee280bddc36dc31f19ec3a0e80587e. IRB failures are fixed by 6612731a4ffd41c01e836ed2ffd473d46b685ab1. --- lib/prism/lex_compat.rb | 4 ++ lib/prism/translation/parser/lexer.rb | 43 +++----------- prism/config.yml | 10 +++- prism/prism.c | 81 +++++++++++++++++--------- prism/templates/src/tokens.c.erb | 8 +++ test/prism/errors/command_calls_25.txt | 3 +- test/prism/ruby/parser_test.rb | 10 +--- 7 files changed, 85 insertions(+), 74 deletions(-) diff --git a/lib/prism/lex_compat.rb b/lib/prism/lex_compat.rb index 4d92842bda7515..749f11173a42aa 100644 --- a/lib/prism/lex_compat.rb +++ b/lib/prism/lex_compat.rb @@ -78,6 +78,8 @@ def deconstruct_keys(keys) # :nodoc: BANG_EQUAL: :on_op, BANG_TILDE: :on_op, BRACE_LEFT: :on_lbrace, + BRACE_LEFT_ARGUMENT: :on_lbrace, + BRACE_LEFT_HASH: :on_lbrace, BRACE_RIGHT: :on_rbrace, BRACKET_LEFT: :on_lbracket, BRACKET_LEFT_ARRAY: :on_lbracket, @@ -191,6 +193,7 @@ def deconstruct_keys(keys) # :nodoc: NEWLINE: :on_nl, NUMBERED_REFERENCE: :on_backref, PARENTHESIS_LEFT: :on_lparen, + PARENTHESIS_LEFT_GROUPING: :on_lparen, PARENTHESIS_LEFT_PARENTHESES: :on_lparen, PARENTHESIS_RIGHT: :on_rparen, PERCENT: :on_op, @@ -233,6 +236,7 @@ def deconstruct_keys(keys) # :nodoc: USTAR: :on_op, USTAR_STAR: :on_op, WORDS_SEP: :on_words_sep, + XSTRING_BEGIN: :on_backtick, "__END__": :on___end__ }.freeze diff --git a/lib/prism/translation/parser/lexer.rb b/lib/prism/translation/parser/lexer.rb index 0b2f4b9da76cb1..c26f48bcfbfda7 100644 --- a/lib/prism/translation/parser/lexer.rb +++ b/lib/prism/translation/parser/lexer.rb @@ -28,11 +28,13 @@ class Lexer # :nodoc: AMPERSAND_DOT: :tANDDOT, AMPERSAND_EQUAL: :tOP_ASGN, BACK_REFERENCE: :tBACK_REF, - BACKTICK: :tXSTRING_BEG, + BACKTICK: :tBACK_REF2, BANG: :tBANG, BANG_EQUAL: :tNEQ, BANG_TILDE: :tNMATCH, BRACE_LEFT: :tLCURLY, + BRACE_LEFT_ARGUMENT: :tLBRACE_ARG, + BRACE_LEFT_HASH: :tLBRACE, BRACE_RIGHT: :tRCURLY, BRACKET_LEFT: :tLBRACK2, BRACKET_LEFT_ARRAY: :tLBRACK, @@ -141,6 +143,7 @@ class Lexer # :nodoc: NEWLINE: :tNL, NUMBERED_REFERENCE: :tNTH_REF, PARENTHESIS_LEFT: :tLPAREN2, + PARENTHESIS_LEFT_GROUPING: :tLPAREN, PARENTHESIS_LEFT_PARENTHESES: :tLPAREN_ARG, PARENTHESIS_RIGHT: :tRPAREN, PERCENT: :tPERCENT, @@ -180,32 +183,10 @@ class Lexer # :nodoc: UPLUS: :tUPLUS, USTAR: :tSTAR, USTAR_STAR: :tDSTAR, - WORDS_SEP: :tSPACE + WORDS_SEP: :tSPACE, + XSTRING_BEGIN: :tXSTRING_BEG } - # These constants represent flags in our lex state. We really, really - # don't want to be using them and we really, really don't want to be - # exposing them as part of our public API. Unfortunately, we don't have - # another way of matching the exact tokens that the parser gem expects - # without them. We should find another way to do this, but in the - # meantime we'll hide them from the documentation and mark them as - # private constants. - EXPR_BEG = 0x1 - EXPR_LABEL = 0x400 - - # 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([ - :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. # For these, the parser gem doesn't emit a newline token after the last comment. COMMENT_CONTINUATION_TYPES = Set.new([:COMMENT, :AMPERSAND_DOT, :DOT]) @@ -214,7 +195,7 @@ class Lexer # :nodoc: # Heredocs are complex and require us to keep track of a bit of info to refer to later HeredocData = Struct.new(:identifier, :common_whitespace, keyword_init: true) - private_constant :TYPES, :EXPR_BEG, :EXPR_LABEL, :LPAREN_CONVERSION_TOKEN_TYPES, :HeredocData + private_constant :TYPES, :HeredocData # The Parser::Source::Buffer that the tokens were lexed from. attr_reader :source_buffer @@ -253,7 +234,7 @@ def to_a comment_newline_location = nil while index < length - token, state = lexed[index] + token, _ = lexed[index] index += 1 next if TYPES_ALWAYS_SKIP.include?(token.type) @@ -324,10 +305,6 @@ def to_a value.chomp!(":") when :tLABEL_END value.chomp!(":") - when :tLCURLY - type = :tLBRACE if state == EXPR_BEG | EXPR_LABEL - when :tLPAREN2 - type = :tLPAREN if tokens.empty? || LPAREN_CONVERSION_TOKEN_TYPES.include?(tokens.dig(-1, 0)) when :tNTH_REF value = parse_integer(value.delete_prefix("$")) when :tOP_ASGN @@ -506,10 +483,6 @@ def to_a type = :tIDENTIFIER end when :tXSTRING_BEG - if (next_token = lexed[index]&.first) && !%i[STRING_CONTENT STRING_END EMBEXPR_BEGIN].include?(next_token.type) - # self.`() - type = :tBACK_REF2 - end quote_stack.push(value) when :tSYMBOLS_BEG, :tQSYMBOLS_BEG, :tWORDS_BEG, :tQWORDS_BEG if (next_token = lexed[index]&.first) && next_token.type == :WORDS_SEP diff --git a/prism/config.yml b/prism/config.yml index 21502ea8ca9e91..cc5eb7e099c228 100644 --- a/prism/config.yml +++ b/prism/config.yml @@ -377,7 +377,7 @@ tokens: - name: AMPERSAND_EQUAL comment: "&=" - name: BACKTICK - comment: "`" + comment: "` as a method name" - name: BACK_REFERENCE comment: "a back reference" - name: BANG @@ -388,6 +388,10 @@ tokens: comment: "!~" - name: BRACE_LEFT comment: "{" + - name: BRACE_LEFT_ARGUMENT + comment: "{ for a block following a parenthesized argument" + - name: BRACE_LEFT_HASH + comment: "{ for a hash literal" - name: BRACKET_LEFT comment: "[" - name: BRACKET_LEFT_ARRAY @@ -584,6 +588,8 @@ tokens: comment: "a numbered reference to a capture group in the previous regular expression match" - name: PARENTHESIS_LEFT comment: "(" + - name: PARENTHESIS_LEFT_GROUPING + comment: "( scanned at the beginning of an expression" - name: PARENTHESIS_LEFT_PARENTHESES comment: "( for a parentheses node" - name: PERCENT @@ -658,6 +664,8 @@ tokens: comment: "unary **" - name: WORDS_SEP comment: "a separator between words in a list" + - name: XSTRING_BEGIN + comment: "the beginning of an execution string" - name: __END__ comment: "marker for the point in the file at which the parser should stop" flags: diff --git a/prism/prism.c b/prism/prism.c index 87bb03738fdf95..bd16a3f2822db4 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -10493,9 +10493,15 @@ parser_lex(pm_parser_t *parser) { // ( case '(': { + /* A parenthesis scanned at the beginning of an expression + * groups the expression it wraps, while one scanned in + * argument position with a preceding space wraps a command + * argument. Everything else opens an argument list. */ pm_token_type_t type = PM_TOKEN_PARENTHESIS_LEFT; - if (space_seen && (lex_state_arg_p(parser) || parser->lex_state == (PM_LEX_STATE_END | PM_LEX_STATE_LABEL))) { + if (lex_state_beg_p(parser)) { + type = PM_TOKEN_PARENTHESIS_LEFT_GROUPING; + } else if (space_seen && (lex_state_arg_p(parser) || parser->lex_state == (PM_LEX_STATE_END | PM_LEX_STATE_LABEL))) { type = PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES; } @@ -10554,24 +10560,28 @@ parser_lex(pm_parser_t *parser) { pm_token_type_t type = PM_TOKEN_BRACE_LEFT; if (parser->enclosure_nesting == parser->lambda_enclosure_nesting) { - // This { begins a lambda + /* This { begins a lambda */ parser->command_start = true; lex_state_set(parser, PM_LEX_STATE_BEG); type = PM_TOKEN_LAMBDA_BEGIN; } else if (lex_state_p(parser, PM_LEX_STATE_LABELED)) { - // This { begins a hash literal + /* This { begins a hash literal */ lex_state_set(parser, PM_LEX_STATE_BEG | PM_LEX_STATE_LABEL); + type = PM_TOKEN_BRACE_LEFT_HASH; } else if (lex_state_p(parser, PM_LEX_STATE_ARG_ANY | PM_LEX_STATE_END | PM_LEX_STATE_ENDFN)) { - // This { begins a block + /* This { begins a block */ parser->command_start = true; lex_state_set(parser, PM_LEX_STATE_BEG); } else if (lex_state_p(parser, PM_LEX_STATE_ENDARG)) { - // This { begins a block on a command + /* This { begins a block following a parenthesized + * command argument */ parser->command_start = true; lex_state_set(parser, PM_LEX_STATE_BEG); + type = PM_TOKEN_BRACE_LEFT_ARGUMENT; } else { - // This { begins a hash literal + /* This { begins a hash literal */ lex_state_set(parser, PM_LEX_STATE_BEG | PM_LEX_STATE_LABEL); + type = PM_TOKEN_BRACE_LEFT_HASH; } parser->enclosure_nesting++; @@ -10888,7 +10898,7 @@ parser_lex(pm_parser_t *parser) { } lex_mode_push_string(parser, true, false, '\0', '`'); - LEX(PM_TOKEN_BACKTICK); + LEX(PM_TOKEN_XSTRING_BEGIN); } // single-quoted string literal @@ -12751,6 +12761,14 @@ match4(const pm_parser_t *parser, pm_token_type_t type1, pm_token_type_t type2, return match1(parser, type1) || match1(parser, type2) || match1(parser, type3) || match1(parser, type4); } +/** + * Returns true if the current token is any of the five given types. + */ +static PRISM_INLINE bool +match5(const pm_parser_t *parser, pm_token_type_t type1, pm_token_type_t type2, pm_token_type_t type3, pm_token_type_t type4, pm_token_type_t type5) { + return match1(parser, type1) || match1(parser, type2) || match1(parser, type3) || match1(parser, type4) || match1(parser, type5); +} + /** * Returns true if the current token is any of the six given types. */ @@ -13571,7 +13589,7 @@ parse_targets(pm_parser_t *parser, pm_node_t *first_target, pm_binding_power_t b pm_node_t *splat = UP(pm_splat_node_create(parser, &star_operator, name)); pm_multi_target_node_targets_append(parser, result, splat); has_rest = true; - } else if (match1(parser, PM_TOKEN_PARENTHESIS_LEFT)) { + } else if (match1(parser, PM_TOKEN_PARENTHESIS_LEFT_GROUPING)) { context_push(parser, PM_CONTEXT_MULTI_TARGET); pm_node_t *target = parse_expression(parser, binding_power, PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_COMMA, (uint16_t) (depth + 1)); target = parse_target(parser, target, true, false); @@ -13782,7 +13800,7 @@ parse_assocs(pm_parser_t *parser, pm_static_literals_t *literals, pm_node_t *nod pm_token_t operator = parser->previous; pm_node_t *value = NULL; - if (match1(parser, PM_TOKEN_BRACE_LEFT)) { + if (match1(parser, PM_TOKEN_BRACE_LEFT_HASH)) { // If we're about to parse a nested hash that is being // pushed into this hash directly with **, then we want the // inner hash to share the static literals with the outer @@ -14335,7 +14353,7 @@ parse_arguments(pm_parser_t *parser, pm_arguments_t *arguments, bool accepts_for */ static pm_multi_target_node_t * parse_required_destructured_parameter(pm_parser_t *parser) { - expect1(parser, PM_TOKEN_PARENTHESIS_LEFT, PM_ERR_EXPECT_LPAREN_REQ_PARAMETER); + expect1(parser, PM_TOKEN_PARENTHESIS_LEFT_GROUPING, PM_ERR_EXPECT_LPAREN_REQ_PARAMETER); pm_multi_target_node_t *node = pm_multi_target_node_create(parser); pm_multi_target_node_opening_set(parser, node, &parser->previous); @@ -14354,7 +14372,7 @@ parse_required_destructured_parameter(pm_parser_t *parser) { break; } - if (match1(parser, PM_TOKEN_PARENTHESIS_LEFT)) { + if (match1(parser, PM_TOKEN_PARENTHESIS_LEFT_GROUPING)) { param = UP(parse_required_destructured_parameter(parser)); } else if (accept1(parser, PM_TOKEN_USTAR)) { pm_token_t star = parser->previous; @@ -14416,7 +14434,7 @@ static pm_parameters_order_t parameters_ordering[PM_TOKEN_MAXIMUM] = { [PM_TOKEN_AMPERSAND] = PM_PARAMETERS_ORDER_NOTHING_AFTER, [PM_TOKEN_UDOT_DOT_DOT] = PM_PARAMETERS_ORDER_NOTHING_AFTER, [PM_TOKEN_IDENTIFIER] = PM_PARAMETERS_ORDER_NAMED, - [PM_TOKEN_PARENTHESIS_LEFT] = PM_PARAMETERS_ORDER_NAMED, + [PM_TOKEN_PARENTHESIS_LEFT_GROUPING] = PM_PARAMETERS_ORDER_NAMED, [PM_TOKEN_EQUAL] = PM_PARAMETERS_ORDER_OPTIONAL, [PM_TOKEN_LABEL] = PM_PARAMETERS_ORDER_KEYWORDS, [PM_TOKEN_USTAR] = PM_PARAMETERS_ORDER_AFTER_OPTIONAL, @@ -14523,7 +14541,7 @@ parse_parameters( bool parsing = true; switch (parser->current.type) { - case PM_TOKEN_PARENTHESIS_LEFT: { + case PM_TOKEN_PARENTHESIS_LEFT_GROUPING: { update_parameter_state(parser, &parser->current, &order); pm_node_t *param = UP(parse_required_destructured_parameter(parser)); @@ -15400,7 +15418,7 @@ parse_block(pm_parser_t *parser, uint16_t depth) { * managed by the lexer. A `do`/`end` block is delimited by keywords, so we * push the frame here (covering the block parameters and body) and pop it * before consuming `end`, mirroring parse.y's `do_body` rule. */ - bool do_block = opening.type != PM_TOKEN_BRACE_LEFT; + bool do_block = opening.type != PM_TOKEN_BRACE_LEFT && opening.type != PM_TOKEN_BRACE_LEFT_ARGUMENT; if (do_block) pm_accepts_block_stack_push(parser, true); pm_parser_scope_push(parser, false); @@ -15425,7 +15443,7 @@ parse_block(pm_parser_t *parser, uint16_t depth) { accept1(parser, PM_TOKEN_NEWLINE); pm_node_t *statements = NULL; - if (opening.type == PM_TOKEN_BRACE_LEFT) { + if (!do_block) { if (!match1(parser, PM_TOKEN_BRACE_RIGHT)) { statements = UP(parse_statements(parser, PM_CONTEXT_BLOCK_BRACES, (uint16_t) (depth + 1))); } @@ -15522,7 +15540,7 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool full_a * it, so pop the delimiter frame, push the command-args frame, and then * restore the delimiter frame on top (the delimiter's closing token * will pop it back off during argument parsing). */ - bool lookahead_delimiter = match4(parser, PM_TOKEN_PARENTHESIS_LEFT, PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES, PM_TOKEN_BRACKET_LEFT, PM_TOKEN_BRACKET_LEFT_ARRAY); + bool lookahead_delimiter = match5(parser, PM_TOKEN_PARENTHESIS_LEFT, PM_TOKEN_PARENTHESIS_LEFT_GROUPING, PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES, PM_TOKEN_BRACKET_LEFT, PM_TOKEN_BRACKET_LEFT_ARRAY); if (lookahead_delimiter) pm_accepts_block_stack_pop(parser); pm_accepts_block_stack_push(parser, false); if (lookahead_delimiter) pm_accepts_block_stack_push(parser, true); @@ -15544,7 +15562,7 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool full_a * it, pop the command-args frame beneath it, and restore the block * frame so the block's `}` still pops it. This mirrors the `tLBRACE_ARG` * lookahead handling in parse.y's `command_args` rule. */ - bool lookahead_brace = match1(parser, PM_TOKEN_BRACE_LEFT); + bool lookahead_brace = match2(parser, PM_TOKEN_BRACE_LEFT, PM_TOKEN_BRACE_LEFT_ARGUMENT); if (lookahead_brace) pm_accepts_block_stack_pop(parser); pm_accepts_block_stack_pop(parser); if (lookahead_brace) pm_accepts_block_stack_push(parser, true); @@ -15556,7 +15574,7 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool full_a if (full_arguments) { pm_block_node_t *block = NULL; - if (accept1(parser, PM_TOKEN_BRACE_LEFT)) { + if (accept2(parser, PM_TOKEN_BRACE_LEFT, PM_TOKEN_BRACE_LEFT_ARGUMENT)) { found |= true; block = parse_block(parser, (uint16_t) (depth + 1)); pm_arguments_validate_block(parser, arguments, block); @@ -16018,7 +16036,7 @@ parse_conditional(pm_parser_t *parser, pm_context_t context, size_t opening_newl #define PM_CASE_PRIMITIVE PM_TOKEN_INTEGER: case PM_TOKEN_INTEGER_IMAGINARY: case PM_TOKEN_INTEGER_RATIONAL: \ case PM_TOKEN_INTEGER_RATIONAL_IMAGINARY: case PM_TOKEN_FLOAT: case PM_TOKEN_FLOAT_IMAGINARY: \ case PM_TOKEN_FLOAT_RATIONAL: case PM_TOKEN_FLOAT_RATIONAL_IMAGINARY: case PM_TOKEN_SYMBOL_BEGIN: \ - case PM_TOKEN_REGEXP_BEGIN: case PM_TOKEN_BACKTICK: case PM_TOKEN_PERCENT_LOWER_X: case PM_TOKEN_PERCENT_LOWER_I: \ + case PM_TOKEN_REGEXP_BEGIN: case PM_TOKEN_XSTRING_BEGIN: case PM_TOKEN_PERCENT_LOWER_X: case PM_TOKEN_PERCENT_LOWER_I: \ case PM_TOKEN_PERCENT_LOWER_W: case PM_TOKEN_PERCENT_UPPER_I: case PM_TOKEN_PERCENT_UPPER_W: \ case PM_TOKEN_STRING_BEGIN: case PM_TOKEN_KEYWORD_NIL: case PM_TOKEN_KEYWORD_SELF: case PM_TOKEN_KEYWORD_TRUE: \ case PM_TOKEN_KEYWORD_FALSE: case PM_TOKEN_KEYWORD___FILE__: case PM_TOKEN_KEYWORD___LINE__: \ @@ -17364,7 +17382,7 @@ parse_pattern_primitive(pm_parser_t *parser, pm_constant_id_list_t *captures, pm pm_array_pattern_node_requireds_append(parser->arena, node, inner); return UP(node); } - case PM_TOKEN_BRACE_LEFT: { + case PM_TOKEN_BRACE_LEFT_HASH: { bool previous_pattern_matching_newlines = parser->pattern_matching_newlines; parser->pattern_matching_newlines = false; @@ -17501,7 +17519,7 @@ parse_pattern_primitive(pm_parser_t *parser, pm_constant_id_list_t *captures, pm return UP(pm_pinned_variable_node_create(parser, &operator, variable)); } - case PM_TOKEN_PARENTHESIS_LEFT: { + case PM_TOKEN_PARENTHESIS_LEFT_GROUPING: { bool previous_pattern_matching_newlines = parser->pattern_matching_newlines; parser->pattern_matching_newlines = false; @@ -17585,7 +17603,7 @@ parse_pattern_primitives(pm_parser_t *parser, pm_constant_id_list_t *captures, p switch (parser->current.type) { case PM_TOKEN_IDENTIFIER: case PM_TOKEN_BRACKET_LEFT_ARRAY: - case PM_TOKEN_BRACE_LEFT: + case PM_TOKEN_BRACE_LEFT_HASH: case PM_TOKEN_CARET: case PM_TOKEN_CONSTANT: case PM_TOKEN_UCOLON_COLON: @@ -17604,7 +17622,7 @@ parse_pattern_primitives(pm_parser_t *parser, pm_constant_id_list_t *captures, p break; } - case PM_TOKEN_PARENTHESIS_LEFT: + case PM_TOKEN_PARENTHESIS_LEFT_GROUPING: case PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES: { pm_token_t operator = parser->previous; pm_token_t opening = parser->current; @@ -19186,6 +19204,13 @@ parse_parentheses(pm_parser_t *parser, pm_binding_power_t binding_power, uint8_t /* If this is the end of the file or we match a right parenthesis, then we * have an empty parentheses node, and we can immediately return. */ if (match2(parser, PM_TOKEN_PARENTHESIS_RIGHT, PM_TOKEN_EOF)) { + /* A command argument group sets EXPR_ENDARG before its ')' is + * consumed, even when the group is empty, so that a following '{' is + * scanned as a block brace. */ + if (match1(parser, PM_TOKEN_PARENTHESIS_RIGHT) && opening.type == PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES) { + lex_state_set(parser, PM_LEX_STATE_ENDARG); + } + expect1(parser, PM_TOKEN_PARENTHESIS_RIGHT, PM_ERR_EXPECT_RPAREN); pop_block_exits(parser, previous_block_exits); return UP(pm_parentheses_node_create(parser, &opening, NULL, &parser->previous, paren_flags)); @@ -19506,10 +19531,10 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u return UP(array); } - case PM_TOKEN_PARENTHESIS_LEFT: + case PM_TOKEN_PARENTHESIS_LEFT_GROUPING: case PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES: return parse_parentheses(parser, binding_power, flags, depth); - case PM_TOKEN_BRACE_LEFT: { + case PM_TOKEN_BRACE_LEFT_HASH: { // 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 // of hash keys with this inner hash we're about to parse for the @@ -20129,7 +20154,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u context_push(parser, PM_CONTEXT_DEFINED); bool newline = accept1(parser, PM_TOKEN_NEWLINE); - if (accept1(parser, PM_TOKEN_PARENTHESIS_LEFT)) { + if (accept2(parser, PM_TOKEN_PARENTHESIS_LEFT, PM_TOKEN_PARENTHESIS_LEFT_GROUPING)) { lparen = parser->previous; if (newline && accept1(parser, PM_TOKEN_PARENTHESIS_RIGHT)) { @@ -20298,7 +20323,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u accept1(parser, PM_TOKEN_NEWLINE); - if (accept1(parser, PM_TOKEN_PARENTHESIS_LEFT)) { + if (accept2(parser, PM_TOKEN_PARENTHESIS_LEFT, PM_TOKEN_PARENTHESIS_LEFT_GROUPING)) { pm_token_t lparen = parser->previous; if (accept1(parser, PM_TOKEN_PARENTHESIS_RIGHT)) { @@ -20619,7 +20644,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u pm_interpolated_regular_expression_node_closing_set(parser, interpolated, &closing); return UP(interpolated); } - case PM_TOKEN_BACKTICK: + case PM_TOKEN_XSTRING_BEGIN: case PM_TOKEN_PERCENT_LOWER_X: { parser_lex(parser); pm_token_t opening = parser->previous; diff --git a/prism/templates/src/tokens.c.erb b/prism/templates/src/tokens.c.erb index 472c82ea690939..fb71afe217f687 100644 --- a/prism/templates/src/tokens.c.erb +++ b/prism/templates/src/tokens.c.erb @@ -53,6 +53,10 @@ pm_token_str(pm_token_type_t token_type) { return "'!~'"; case PM_TOKEN_BRACE_LEFT: return "'{'"; + case PM_TOKEN_BRACE_LEFT_ARGUMENT: + return "'{'"; + case PM_TOKEN_BRACE_LEFT_HASH: + return "'{'"; case PM_TOKEN_BRACE_RIGHT: return "'}'"; case PM_TOKEN_BRACKET_LEFT: @@ -275,6 +279,8 @@ pm_token_str(pm_token_type_t token_type) { return "numbered reference"; case PM_TOKEN_PARENTHESIS_LEFT: return "'('"; + case PM_TOKEN_PARENTHESIS_LEFT_GROUPING: + return "'('"; case PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES: return "'('"; case PM_TOKEN_PARENTHESIS_RIGHT: @@ -355,6 +361,8 @@ pm_token_str(pm_token_type_t token_type) { return "**"; case PM_TOKEN_WORDS_SEP: return "string separator"; + case PM_TOKEN_XSTRING_BEGIN: + return "backtick string literal"; case PM_TOKEN___END__: return "'__END__'"; case PM_TOKEN_MAXIMUM: diff --git a/test/prism/errors/command_calls_25.txt b/test/prism/errors/command_calls_25.txt index cf04508f87d8a7..c8769c538ab75f 100644 --- a/test/prism/errors/command_calls_25.txt +++ b/test/prism/errors/command_calls_25.txt @@ -3,6 +3,7 @@ ^ expected a `do` keyword or a `{` to open the lambda block ^ unexpected ')', expecting end-of-input ^ unexpected ')', ignoring it - ^ unexpected end-of-input, assuming it is closing the parent top level context + ^ unexpected '{', ignoring it + ^ unexpected '}', ignoring it ^~ expected a lambda block beginning with `do` to end with `end` diff --git a/test/prism/ruby/parser_test.rb b/test/prism/ruby/parser_test.rb index 856ecedc1de39d..e44bc20d4dea0b 100644 --- a/test/prism/ruby/parser_test.rb +++ b/test/prism/ruby/parser_test.rb @@ -111,12 +111,8 @@ class ParserTest < TestCase skip_tokens = [ "dash_heredocs.txt", "embdoc_no_newline_at_end.txt", - "methods.txt", - "seattlerb/bug169.txt", "seattlerb/case_in.txt", "seattlerb/difficult4__leading_dots2.txt", - "seattlerb/difficult6__7.txt", - "seattlerb/difficult6__8.txt", "seattlerb/heredoc_unicode.txt", "seattlerb/parse_line_heredoc.txt", "seattlerb/pct_w_heredoc_interp_nested.txt", @@ -129,14 +125,10 @@ class ParserTest < TestCase "whitequark/beginless_irange_after_newline.txt", "whitequark/forward_arg_with_open_args.txt", "whitequark/kwarg_no_paren.txt", - "whitequark/lbrace_arg_after_command_args.txt", "whitequark/multiple_pattern_matches.txt", "whitequark/newline_in_hash_argument.txt", "whitequark/pattern_matching_hash.txt", - "whitequark/ruby_bug_14690.txt", - "whitequark/ruby_bug_9669.txt", - "whitequark/space_args_arg_block.txt", - "whitequark/space_args_block.txt" + "whitequark/ruby_bug_9669.txt" ] Fixture.each_for_version(except: skip_syntax_error, version: "3.3") do |fixture| From 8052f2492c3abb6cbceb7df8198c1d053960a043 Mon Sep 17 00:00:00 2001 From: Nozomi Hijikata <121233810+nozomemein@users.noreply.github.com> Date: Thu, 6 Aug 2026 07:42:16 +0900 Subject: [PATCH 9/9] ZJIT: Specialize keyword-to-positional-hash sends (#18061) Allow SendDirect to handle explicit caller keywords passed to callees with no keyword parameter table by materializing the keywords as one positional Hash. Keep VM dispatch for keyword splat, **nil, and ruby2_keywords cases because they need keyword-sensitive setup beyond constructing a plain Hash. --- zjit/src/codegen_tests.rs | 95 +++++++++++ zjit/src/hir.rs | 63 ++++++-- zjit/src/hir/opt_tests.rs | 330 +++++++++++++++++++++++++++++++++++++- 3 files changed, 474 insertions(+), 14 deletions(-) diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index b2c9bad57f93e5..58a1cfb94086a1 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -1090,6 +1090,46 @@ fn test_send_optional_return_default_with_argument() { assert_snapshot!(assert_compiles("entry"), @"1"); } +#[test] +fn test_send_keyword_to_positional_hash() { + eval(" + def test(arg) = arg + def entry = test(k: 1) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"{k: 1}"); +} + +#[test] +fn test_send_multiple_keywords_to_positional_hash() { + eval(" + def test(arg) = arg + def entry = test(k: 1, v: 2) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"{k: 1, v: 2}"); +} + +#[test] +fn test_send_positional_and_keyword_to_positional_hash() { + eval(" + def test(a, b) = [a, b] + def entry = test(1, k: 2) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"[1, {k: 2}]"); +} + +#[test] +fn test_send_optional_and_keyword_to_positional_hash() { + eval(" + def test(a, b = 2) = [a, b] + def entry = test(k: 1) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"[{k: 1}, 2]"); +} + #[test] fn test_send_rest_arguments_with_keyword_to_positional_hash() { eval(" @@ -1100,6 +1140,61 @@ fn test_send_rest_arguments_with_keyword_to_positional_hash() { assert_snapshot!(assert_compiles("entry"), @"[{k: 1}]"); } +#[test] +fn test_send_optional_and_rest_arguments_with_keyword_to_positional_hash() { + eval(" + def test(a, b = 2, *rest) = [a, b, rest] + def entry = test(1, k: 3) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"[1, {k: 3}, []]"); +} + +#[test] +fn test_send_rest_and_post_arguments_with_keyword_to_positional_hash() { + eval(" + def test(a, *rest, b) = [a, rest, b] + def entry = test(1, 2, k: 3) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"[1, [2], {k: 3}]"); +} + +#[test] +fn test_send_keyword_splat_to_positional_hash_fallback() { + eval(" + def test(arg) = arg + def entry = test(**{ k: 1 }) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"{k: 1}"); +} + +#[test] +fn test_send_no_kwarg_to_positional_hash_fallback() { + eval(" + def test(arg, **nil) = arg + def entry + test(k: 1) + rescue ArgumentError + :argument_error + end + entry + "); + assert_snapshot!(assert_compiles("entry"), @":argument_error"); +} + +#[test] +fn test_send_ruby2_keywords_to_positional_hash_fallback() { + eval(" + def target(k:) = k + ruby2_keywords def forward(*args) = target(*args) + def entry = forward(k: 1) + entry + "); + assert_snapshot!(assert_compiles("entry"), @"1"); +} + #[test] fn test_send_rest_arguments_with_block_literal() { eval(" diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 9d6fb7448b2f00..ac5cda92de1fdb 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -2642,15 +2642,21 @@ fn can_direct_send(function: &mut Function, block: BlockId, iseq: *const rb_iseq return false } - // SendDirect only models explicit keyword slots for now, so leave this - // conversion to VM dispatch. - if keywords_as_positional_hash { + // Plain keyword-to-positional-hash is safe to synthesize below. Keep VM + // dispatch for callee modes that need keyword-sensitive handling: **nil + // rejection and ruby2_keywords flag preservation. + if keywords_as_positional_hash + && (params.flags.accepts_no_kwarg() != 0 || params.flags.ruby2_keywords() != 0) + { function.count(block, complex_arg_pass_keyword_to_positional_hash); function.set_dynamic_send_reason(send_insn, ComplexArgPass); return false; } - let keyword_ok = c_int::try_from(caller_kw_count) + // After keyword-to-positional-hash, SendDirect receives no keyword slots; + // the caller keywords are represented by one extra positional Hash. + let effective_keyword_count = if keywords_as_positional_hash { 0 } else { caller_kw_count }; + let keyword_ok = c_int::try_from(effective_keyword_count) .as_ref() .map(|argc| (kw_req_num..=kw_total_num).contains(argc)) .unwrap_or(false); @@ -2670,7 +2676,9 @@ fn can_direct_send(function: &mut Function, block: BlockId, iseq: *const rb_iseq // With *rest, SendDirect receives one rest-array slot instead of each rest // element, so cap positional argc at required/post + filled opts + rest slot. let passed_opt_num = (caller_positional_i32 - min_positional).min(opt_num) as usize; - let send_positional_argc = if has_rest { min_positional as usize + passed_opt_num + 1 } else { caller_positional }; + // Without *rest, use the converted positional count so the synthesized + // keyword Hash is included in the SendDirect argument count. + let send_positional_argc = if has_rest { min_positional as usize + passed_opt_num + 1 } else { effective_positional }; let send_argc = send_positional_argc + kw_total_num as usize; let c_argc = 1 + send_argc + block_arg; // +1 for self @@ -3605,8 +3613,7 @@ impl Function { iseq: IseqPtr, state: InsnId, ) -> Result { - let kwarg = unsafe { rb_vm_ci_kwarg(ci) }; - let (processed_args, caller_argc, kw_bits) = self.setup_keyword_arguments(block, args, kwarg, iseq)?; + let (processed_args, caller_argc, kw_bits) = self.setup_keyword_arguments(block, args, ci, iseq, state)?; let (processed_args, jit_entry_idx) = self.setup_rest_parameter(block, processed_args, iseq, state)?; // If args were reordered or synthesized, create a new snapshot with the updated stack @@ -3639,17 +3646,49 @@ impl Function { &mut self, block: BlockId, args: &[InsnId], - kwarg: *const rb_callinfo_kwarg, + ci: *const rb_callinfo, iseq: IseqPtr, + state: InsnId, ) -> Result<(Vec, usize, u32), SendFallbackReason> { + let kwarg = unsafe { rb_vm_ci_kwarg(ci) }; let callee_keyword = unsafe { rb_get_iseq_body_param_keyword(iseq) }; if callee_keyword.is_null() { - if !kwarg.is_null() { - // Caller is passing kwargs but callee doesn't expect them. + if kwarg.is_null() { + // Neither caller nor callee have keywords - nothing to do + return Ok((args.to_vec(), args.len(), 0)); + } + + let params = unsafe { iseq.params() }; + let ci_flags = unsafe { rb_vm_ci_flag(ci) }; + if ci_flags & VM_CALL_KW_SPLAT != 0 { + // Caller **kw is one runtime Hash, not explicit keyword slots, so + // there is no static key/value list to repack here. return Err(SendDirectKeywordMismatch); } - // Neither caller nor callee have keywords - nothing to do - return Ok((args.to_vec(), args.len(), 0)); + + if params.flags.accepts_no_kwarg() != 0 || params.flags.ruby2_keywords() != 0 { + // These callee modes need VM keyword setup even without a keyword table: + // **nil rejects keywords, and ruby2_keywords requires RHASH_PASS_AS_KEYWORDS. + return Err(SendDirectKeywordMismatch); + } + + // Match vm_args.c's setup_parameters_complex via args_kw_argv_to_hash: + // explicit caller keywords passed to a method with no keyword table + // become one final positional Hash before regular parameter setup. + let caller_kw_count = unsafe { get_cikw_keyword_len(kwarg) } as usize; + let kw_args_start = args.len() - caller_kw_count; + let mut elements = Vec::with_capacity(caller_kw_count * 2); + for i in 0..caller_kw_count { + let keyword = unsafe { get_cikw_keywords_idx(kwarg, i as i32) }; + let key = self.push_insn(block, Insn::Const { val: Const::Value(keyword) }); + elements.push(key); + elements.push(args[kw_args_start + i]); + } + + let hash = self.push_insn(block, Insn::NewHash { elements, state }); + let mut processed_args = args[..kw_args_start].to_vec(); + processed_args.push(hash); + return Ok((processed_args, args.len(), 0)); } // kwarg may be null if caller passes no keywords but callee has optional keywords diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 81338e1bb24590..2cf29df1c7b3b1 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -4776,10 +4776,208 @@ mod hir_opt_tests { } #[test] - fn dont_specialize_call_to_rest_with_keyword_to_positional_hash() { + fn specialize_call_with_keyword_to_positional_hash() { + eval(" + def foo(arg) = arg.class + def test = foo(k: 1) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v19:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v20:HashExact = NewHash v19: v11 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v23:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v23 (0x1040), v20 + PatchPoint NoSingletonClass(Hash@0x1068) + PatchPoint MethodRedefined(Hash@0x1068, class@0x1070, cme:0x1078) + v44:ClassSubclass[Hash@0x1068] = Const Value(VALUE(0x1068)) + CheckInterrupts + PopInlineFrame + Return v44 + "); + } + + #[test] + fn specialize_call_with_multiple_keywords_to_positional_hash() { + eval(" + def foo(arg) = arg + def test = foo(k: 1, v: 2) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v13:Fixnum[2] = Const Value(2) + v21:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v22:StaticSymbol[:v] = Const Value(VALUE(0x1008)) + v23:HashExact = NewHash v21: v11, v22: v13 + PatchPoint MethodRedefined(Object@0x1010, foo@0x1018, cme:0x1020) + v26:ObjectSubclass[class_exact*:Object@VALUE(0x1010)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1010)] recompile + CheckInterrupts + Return v23 + "); + } + + #[test] + fn specialize_call_with_positional_and_keyword_to_positional_hash() { + eval(" + def foo(a, b) = [a, b] + def test = foo(1, k: 2) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v13:Fixnum[2] = Const Value(2) + v21:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v22:HashExact = NewHash v21: v13 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v25 (0x1040), v11, v22 + v36:ArrayExact = NewArray v11, v22 + CheckInterrupts + PopInlineFrame + Return v36 + "); + } + + #[test] + fn specialize_call_with_optional_and_keyword_to_positional_hash() { + eval(" + def foo(a, b = 2) = [a, b] + def test = foo(k: 1) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v19:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v20:HashExact = NewHash v19: v11 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v23:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v23 (0x1040), v20 + v31:Fixnum[2] = Const Value(2) + v42:ArrayExact = NewArray v20, v31 + CheckInterrupts + PopInlineFrame + Return v42 + "); + } + + #[test] + fn dont_specialize_keyword_splat_to_positional_hash() { enable_zjit_stats(); eval(" - def foo(*args) = args + def foo(arg) = arg + def test = foo(**{k: 1}) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + IncrCounterPtr + Jump bb3(v4) + bb3(v7:BasicObject): + IncrCounter zjit_insn_count + IncrCounter zjit_insn_count + v14:HashExact[VALUE(0x1000)] = Const Value(VALUE(0x1000)) + v15:HashExact = HashDup v14 + IncrCounter zjit_insn_count + IncrCounter complex_arg_pass_caller_kw_splat + v18:BasicObject = Send v7, :foo, v15 # SendFallbackReason: Complex argument passing + IncrCounter zjit_insn_count + CheckInterrupts + Return v18 + "); + } + + #[test] + fn dont_specialize_no_kwarg_to_positional_hash() { + enable_zjit_stats(); + eval(" + def foo(arg, **nil) = arg + def test = foo(k: 1) + begin; test; rescue ArgumentError; end + begin; test; rescue ArgumentError; end + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + IncrCounterPtr + Jump bb3(v4) + bb3(v7:BasicObject): + IncrCounter zjit_insn_count + IncrCounter zjit_insn_count + v14:Fixnum[1] = Const Value(1) + IncrCounter zjit_insn_count + IncrCounter complex_arg_pass_keyword_to_positional_hash + v17:BasicObject = Send v7, :foo, v14 # SendFallbackReason: Complex argument passing + IncrCounter zjit_insn_count + CheckInterrupts + Return v17 + "); + } + + #[test] + fn dont_specialize_ruby2_keywords_to_positional_hash() { + enable_zjit_stats(); + eval(" + ruby2_keywords def foo(*args) = args def test = foo(k: 1) test test @@ -4808,6 +5006,107 @@ mod hir_opt_tests { "); } + #[test] + fn specialize_call_to_rest_with_optional_and_keyword_to_positional_hash() { + eval(" + def foo(a, b = 2, *rest) = [a, b, rest] + def test = foo(1, k: 3) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v13:Fixnum[3] = Const Value(3) + v21:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v22:HashExact = NewHash v21: v13 + v23:ArrayExact = NewArray + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v26:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v26 (0x1040), v11, v22, v23 + v39:ArrayExact = NewArray v11, v22, v23 + CheckInterrupts + PopInlineFrame + Return v39 + "); + } + + #[test] + fn specialize_call_to_rest_with_keyword_to_positional_hash() { + eval(" + def foo(*args) = args + def test = foo(k: 1) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v19:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v20:HashExact = NewHash v19: v11 + v21:ArrayExact = NewArray v20 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v24:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v24 (0x1040), v21 + CheckInterrupts + PopInlineFrame + Return v21 + "); + } + + #[test] + fn specialize_call_to_rest_and_post_with_keyword_to_positional_hash() { + eval(" + def foo(a, *rest, b) = [a, rest, b] + def test = foo(1, 2, k: 3) + test + test + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v13:Fixnum[2] = Const Value(2) + v15:Fixnum[3] = Const Value(3) + v23:StaticSymbol[:k] = Const Value(VALUE(0x1000)) + v24:HashExact = NewHash v23: v15 + v25:ArrayExact = NewArray v13 + PatchPoint MethodRedefined(Object@0x1008, foo@0x1010, cme:0x1018) + v28:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v6, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v28 (0x1040), v11, v25, v24 + v41:ArrayExact = NewArray v11, v25, v24 + CheckInterrupts + PopInlineFrame + Return v41 + "); + } + #[test] fn dont_classify_keyword_to_positional_hash_argc_mismatch_as_complex_arg_pass() { eval(" @@ -4834,6 +5133,33 @@ mod hir_opt_tests { "); } + #[test] + fn dont_classify_keyword_to_positional_hash_too_many_args_as_complex_arg_pass() { + eval(" + def foo(a) = a + def test = foo(1, k: 2) + begin; test; rescue ArgumentError; end + begin; test; rescue ArgumentError; end + "); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb3(v1) + bb2(): + EntryPoint JIT(0) + v4:BasicObject = LoadArg :self@0 + Jump bb3(v4) + bb3(v6:BasicObject): + v11:Fixnum[1] = Const Value(1) + v13:Fixnum[2] = Const Value(2) + v15:BasicObject = Send v6, :foo, v11, v13 # SendFallbackReason: Argument count does not match parameter count + CheckInterrupts + Return v15 + "); + } + #[test] fn test_send_call_to_iseq_with_optional_kw() { eval("