Bug #17725
closedPrepend breaks ability to override optimized methods
Description
Prepending any module to String
and Hash
(and possibly other or all classes?) blocks the ability to alias
As an example:
module Dummy; end
# String.prepend(Dummy)
class String
alias_method(:old_plus, :+)
def + other
puts 'blah blah'
old_plus(other)
end
end
'a' + 'b'
> blah blah
module Dummy; end
String.prepend(Dummy)
class String
alias_method(:old_plus, :+)
def + other
puts 'blah blah'
old_plus(other)
end
end
'a' + 'b'
>
Prepending after an alias
does not affect the previous alias
Updated by joshuadreed (Josh Reed) over 3 years ago
module Dummy; end
class Foo
def ten
10
end
end
Foo.prepend(Dummy)
class Foo
alias_method(:old_ten, :ten)
def ten
puts 'blah blah'
old_ten
end
end
Foo.new.ten
> blah blah
Okay, doesn't seem to affect every class.
I do suspect it may be to do with refinement, so I will check into that next.
Updated by joshuadreed (Josh Reed) over 3 years ago
So, I set out to test my hypothesis regarding refinements. I've never used refinements prior to this, so it's still possible there's an interaction somewhere, but at least not in the way I was thinking.
module Dummy; end
class Foo
def ten
10
end
end
module Bar
refine Foo do
def ten
11
end
end
end
class Baz
using Bar
def check
Foo.new.ten
end
end
Baz.prepend(Dummy)
class Baz
alias_method(:old_check, :check)
def check
puts 'blah blah'
old_check
end
end
p Baz.new.check
>blah blah
>11
Updated by joshuadreed (Josh Reed) over 3 years ago
One more detail.
module Dummy; end
class String
alias_method(:old_plus, :+)
def + other
puts 'blah blah'
old_plus(other)
end
end
String.prepend(Dummy)
class String
alias_method(:old_plus2, :+)
def + other
puts 'blah blah2'
old_plus2(other)
end
end
>blah blah2
>blah blah
If the method was aliased prior to prepending, the method is perfectly re-alias-able.
Updated by nobu (Nobuyoshi Nakada) over 3 years ago
- Subject changed from Prepend Breaks Ability to Alias to Prepend breaks ability to override optimized methods
Alias is not concerned.
# bug-17725.rb
String.prepend(Module.new) unless ARGV.empty?
class String
def + other
'blah blah'
end
end
p 'a' + 'b'
$ ruby bug-17725.rb
"blah blah"
$ ruby bug-17725.rb 1
"ab"
Seems the redefinition flag is not set by prepend.
Updated by nobu (Nobuyoshi Nakada) over 3 years ago
nobu (Nobuyoshi Nakada) wrote in #note-4:
Seems the redefinition flag is not set by prepend.
This is not accurate, actual method owner class needs to be checked too.
Updated by nobu (Nobuyoshi Nakada) over 3 years ago
Hmmmm?
$ ruby -v
ruby 3.1.0dev (2021-03-16T15:19:37Z master a47697aa44) [x86_64-darwin19]
$ ruby --enable=gems bug-17725.rb
"blah blah"
$ ruby --enable=gems bug-17725.rb 1
"ab"
$ ruby --disable=gems bug-17725.rb
"blah blah"
$ ruby --disable=gems bug-17725.rb 1
"blah blah"
$ ruby --disable=gems -rrubygems bug-17725.rb
"blah blah"
$ ruby --disable=gems -rrubygems bug-17725.rb 1
"ab"
Updated by xtkoba (Tee KOBAYASHI) over 3 years ago
FWIW,
# bug17725-test.rb
eval <<EOS
# encoding: ascii-8bit
MY_STRING = ''
EOS
MY_STRING + ''
String.prepend(Module.new)
class String
def + other
'blah blah'
end
end
p 'a' + 'b'
gives
$ ruby --disable-gems bug17725-test.rb
"ab"
Updated by nobu (Nobuyoshi Nakada) over 3 years ago
- Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED
Updated by xtkoba (Tee KOBAYASHI) over 3 years ago
Another example:
'' != nil
module Blah
def == other
'blah blah'
end
end
String.prepend(Blah)
p 'a' == 'b' # => false
It seems that rb_vm_check_redefinition_opt_method
is not working correctly in these cases. If the issue were only with String#==
and String#+
, a (dirty) workaround would be as follows:
--- a/vm.c
+++ b/vm.c
@@ -1854,6 +1854,12 @@ rb_vm_check_redefinition_opt_method(cons
ruby_vm_redefined_flag[bop] |= flag;
}
+ else if (me->def->body.iseq.iseqptr == (rb_iseq_t *)rb_str_equal) {
+ ruby_vm_redefined_flag[BOP_EQ] |= STRING_REDEFINED_OP_FLAG;
+ }
+ else if (me->def->body.iseq.iseqptr == (rb_iseq_t *)rb_str_plus) {
+ ruby_vm_redefined_flag[BOP_PLUS] |= STRING_REDEFINED_OP_FLAG;
+ }
}
}
Updated by jeremyevans0 (Jeremy Evans) over 3 years ago
I've submitted a pull request to fix this: https://github.com/ruby/ruby/pull/4376
Updated by ko1 (Koichi Sasada) over 3 years ago
- Status changed from Open to Closed
Applied in changeset git|fb4cf204a662a8cd9dafef6f31f2bd0db9129abe.
use me->def instead of me for opt_table
vm_opt_method_table
is me=>bop table to manage the optimized
methods (by specialized instruction). However, me
can be invalidated
to invalidate the method cache entry.
[Bug #17725]
To solve the issue, use me-def
instead of me
which simply copied
at invalidation timing.
A test by @jeremyevans https://github.com/ruby/ruby/pull/4376
Updated by nagachika (Tomoyuki Chikanaga) over 3 years ago
I think fa0279d947c3962c3f8c32852278d3ebb964cb19 should be backported too.
But it could introduce a tiny incompatibility. I'd like to investigate the another workarounds of the issue.
Updated by mk (Matthias Käppler) almost 3 years ago
@nagachika (Tomoyuki Chikanaga) @ko1 (Koichi Sasada) I noticed neither commit appears to be included in 3.0.3. Just wondering why? We currently need to build a patched 3.0.2 image that includes these two commits since we were running into this problem, and it has been working fine for us.
Updated by nagachika (Tomoyuki Chikanaga) almost 3 years ago
- Backport changed from 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED to 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED
I gave up to find a workaround. I will backport fb4cf204a662a8cd9dafef6f31f2bd0db9129abe and fa0279d947c3962c3f8c32852278d3ebb964cb19 even though they introduce an incompatibility. I believe the incompatibility (the Array#size is going to have an independent method entry instead of being defined as the alias of Array#length) couldn't cause any problem in the real world applications...
Updated by nagachika (Tomoyuki Chikanaga) almost 3 years ago
- Backport changed from 2.6: DONTNEED, 2.7: DONTNEED, 3.0: REQUIRED to 2.6: DONTNEED, 2.7: DONTNEED, 3.0: DONE
ruby_3_0 545d6820715a48a17d6182128c0db4198dfa76c1 merged revision(s) fb4cf204a662a8cd9dafef6f31f2bd0db9129abe,fa0279d947c3962c3f8c32852278d3ebb964cb19.