Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Always emit code for pushing a block in the JIT

There are situations where we create incorrect code before this fix.

Imagine the following code:

def x
  y do
    true
  end
end

def y
  z { yield }
end

def z
  yield
end

Now if in this case we decide to compile 'x', we can inline y into x and
z into y. The problem occurs when we try to inline z but we can't due to
it being too complex, or when it blows the inline budget. In that case
the block hasn't been pushed so z will raise a NoJumpError because it
can't find the block.

In the future we need to make the JIT logic here smarter. We need to
keep track of whether we end up using the block in the whole chain down
when we compile a method and if we don't end up using the block, we can
remove the code for pushing it.

Fixes #1940
  • Loading branch information...
commit 897570e8ced9b2c902f867316fec56837c7e7081 1 parent 9ad741f
@dbussink dbussink authored
Showing with 2 additions and 42 deletions.
  1. +2 −42 vm/llvm/jit_visit.hpp
View
44 vm/llvm/jit_visit.hpp
@@ -1685,25 +1685,14 @@ namespace rubinius {
}
void visit_create_block(opcode which) {
- // If we're creating a block to pass directly to
- // send_stack_with_block, delay doing so.
- if(next_op() == InstructionSequence::insn_send_stack_with_block) {
- // Push a placeholder and register which literal we would
- // use for the block. The later send handles whether to actually
- // emit the call to create the block (replacing the placeholder)
- stack_push(constant(cNil));
- current_block_ = (int)which;
- } else {
- current_block_ = -1;
- emit_create_block(which, true);
- }
+ emit_create_block(which, true);
+ current_block_ = (int) which;
}
void visit_send_stack_with_block(opcode which, opcode args) {
set_has_side_effects();
bool has_literal_block = (current_block_ >= 0);
- bool block_on_stack = !has_literal_block;
InlineCache* cache = reinterpret_cast<InlineCache*>(which);
CompiledCode* block_code = 0;
@@ -1768,10 +1757,6 @@ namespace rubinius {
b().CreateBr(cleanup);
set_block(failure);
- if(!block_on_stack) {
- emit_create_block(current_block_);
- block_on_stack = true;
- }
emit_uncommon();
set_block(cleanup);
@@ -1797,10 +1782,6 @@ namespace rubinius {
b().CreateBr(cleanup);
set_block(failure);
- if(!block_on_stack) {
- emit_create_block(current_block_);
- block_on_stack = true;
- }
Value* send_res = block_send(cache, args, allow_private_);
b().CreateBr(cleanup);
@@ -1830,11 +1811,6 @@ namespace rubinius {
use_send:
- // Detect a literal block being created and passed here.
- if(!block_on_stack) {
- emit_create_block(current_block_);
- }
-
Value* ret = block_send(cache, args, allow_private_);
stack_remove(args + 2);
check_for_return(ret);
@@ -1847,10 +1823,6 @@ namespace rubinius {
void visit_send_stack_with_splat(opcode which, opcode args) {
set_has_side_effects();
- if(current_block_ >= 0) {
- emit_create_block(current_block_);
- }
-
InlineCache* cache = reinterpret_cast<InlineCache*>(which);
Value* ret = splat_send(cache->name, args, allow_private_);
stack_remove(args + 3);
@@ -1953,10 +1925,6 @@ namespace rubinius {
void visit_send_super_stack_with_block(opcode which, opcode args) {
set_has_side_effects();
- if(current_block_ >= 0) {
- emit_create_block(current_block_);
- }
-
InlineCache* cache = reinterpret_cast<InlineCache*>(which);
b().CreateStore(get_self(), out_args_recv_);
@@ -1979,10 +1947,6 @@ namespace rubinius {
void visit_send_super_stack_with_splat(opcode which, opcode args) {
set_has_side_effects();
- if(current_block_ >= 0) {
- emit_create_block(current_block_);
- }
-
InlineCache* cache = reinterpret_cast<InlineCache*>(which);
Value* ret = super_send(cache->name, args, true);
stack_remove(args + 2);
@@ -1995,10 +1959,6 @@ namespace rubinius {
void visit_zsuper(opcode which) {
set_has_side_effects();
- if(current_block_ >= 0) {
- emit_create_block(current_block_);
- }
-
InlineCache* cache = reinterpret_cast<InlineCache*>(which);
Signature sig(ls_, ObjType);

4 comments on commit 897570e

@ryoqun
Collaborator

The reason I love Rubinius is that its commits are well explained in commit messages. It sufficiently records the relational of commits and at the same time other programmers like me can learn a lot from such commits like this one!

Really thanks @thedarkone for finding this bug and @dbussink for fixing it. I got busy recently...

@jc00ke
Owner

@ryoqun I've noticed the same :wink:
Check the CONTRIBUTING doc

@ileitch
Collaborator

Yes, I love reading these commits :)

@ryoqun
Collaborator

@jc00ke Wow, I didn't know that. Thanks! My commit is mentioned as an example. Yay!

Please sign in to comment.
Something went wrong with that request. Please try again.