From 2088a457981b0f71a3bfd14871ed5b6f0d090e6a Mon Sep 17 00:00:00 2001 From: "Eileen M. Uchitelle" Date: Mon, 14 Jun 2021 20:34:57 -0400 Subject: [PATCH] [Bug #17880] Set leaf false on opt_setinlinecache (#4565) This change fixes the bug described in https://bugs.ruby-lang.org/issues/17880. Checking `ractor_shareable_p` will cause the method to call back into Ruby. Anything calling this method can't be a leaf instruction, otherwise it could crash. By adding `attr bool leaf = false` we no longer crash because it marks the function as not a leaf. Here's a simplified reproduction script: ```ruby require "set" class Id attr_reader :db_id def initialize(db_id) @db_id = db_id end def ==(other) other.class == self.class && other.db_id == db_id end alias_method :eql?, :== def hash 10 end def <=>(other) db_id <=> other.db_id if other.is_a?(self.class) end end class Namespace IDS = Set[ Id.new(1).freeze, Id.new(2).freeze, Id.new(3).freeze, Id.new(4).freeze, ].freeze class << self def test?(id) IDS.include?(id) end end end p Namespace.test?(Id.new(1)) p Namespace.test?(Id.new(5)) ``` Co-authored-by: Aaron Patterson Co-authored-by: Aaron Patterson --- insns.def | 1 + test/ruby/test_insns_leaf.rb | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test/ruby/test_insns_leaf.rb diff --git a/insns.def b/insns.def index 8d609927b5..c6ebf7401b 100644 --- a/insns.def +++ b/insns.def @@ -1023,6 +1023,7 @@ opt_setinlinecache (IC ic) (VALUE val) (VALUE val) +// attr bool leaf = false; { vm_ic_update(GET_ISEQ(), ic, val, GET_EP()); } diff --git a/test/ruby/test_insns_leaf.rb b/test/ruby/test_insns_leaf.rb new file mode 100644 index 0000000000..9c9a4324cb --- /dev/null +++ b/test/ruby/test_insns_leaf.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: false +require 'test/unit' + +class TestInsnsLeaf < Test::Unit::TestCase + require "set" + + class Id + attr_reader :db_id + def initialize(db_id) + @db_id = db_id + end + + def ==(other) + other.class == self.class && other.db_id == db_id + end + alias_method :eql?, :== + + def hash + 10 + end + + def <=>(other) + db_id <=> other.db_id if other.is_a?(self.class) + end + end + + class Namespace + IDS = Set[ + Id.new(1).freeze, + Id.new(2).freeze, + Id.new(3).freeze, + Id.new(4).freeze, + ].freeze + + class << self + def test?(id) + IDS.include?(id) + end + end + end + + def test_insns_leaf + assert Namespace.test?(Id.new(1)), "IDS should include 1" + assert !Namespace.test?(Id.new(5)), "IDS should not include 5" + end +end