Skip to content

Commit

Permalink
Fix bug involving .send and overwritten methods.
Browse files Browse the repository at this point in the history
@casperisfine reporting a bug in this gist https://gist.github.com/casperisfine/d59e297fba38eb3905a3d7152b9e9350

After investigating I found it was caused by a combination of send and a c_func that we have overwritten in the JIT. For send calls, we need to do some stack manipulation before making the call. Because of the way exits works, we need to do that stack manipulation at the last possible moment. In this case, we weren't doing that stack manipulation at all. Unfortunately, with how the code is structured there isn't a great place to do that stack manipulation for our overridden C funcs.

Each overridden C func can return a boolean stating that it shouldn't be used. We would need to do the stack manipulation after all of those checks are done. We could pass a lambda(?) or separate out the logic for "can I run this override" from "now generate the code for it". Since we are coming up on a release, I went with the path of least resistence and just decided to not use these overrides if we are in a send call.

We definitely should revist this in the future.
  • Loading branch information
jimmyhmiller committed Nov 17, 2022
1 parent 189e3c0 commit eeb40f7
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 1 deletion.
23 changes: 23 additions & 0 deletions bootstraptest/test_yjit.rb
Expand Up @@ -45,6 +45,29 @@ def call_graph_root
Object.send(:remove_const, :Foo)
}

assert_normal_exit %q{
# Test to ensure send on overriden c functions
# doesn't corrupt the stack
class Bar
def bar(x)
x
end
end
class Foo
def bar
Bar.new
end
end
foo = Foo.new
# before this change, this line would error
# because "s" would still be on the stack
# String.to_s is the overridden method here
p foo.bar.bar("s".__send__(:to_s))
}


assert_equal '[nil, nil, nil, nil, nil, nil]', %q{
[NilClass, TrueClass, FalseClass, Integer, Float, Symbol].each do |klass|
klass.class_eval("def foo = @foo")
Expand Down
2 changes: 1 addition & 1 deletion yjit/src/codegen.rs
Expand Up @@ -4179,7 +4179,7 @@ fn gen_send_cfunc(
}

// Delegate to codegen for C methods if we have it.
if kw_arg.is_null() {
if kw_arg.is_null() && flags & VM_CALL_OPT_SEND == 0 {
let codegen_p = lookup_cfunc_codegen(unsafe { (*cme).def });
if let Some(known_cfunc_codegen) = codegen_p {
if known_cfunc_codegen(jit, ctx, asm, ocb, ci, cme, block, argc, recv_known_klass) {
Expand Down

0 comments on commit eeb40f7

Please sign in to comment.