Use correct scope by setting CompiledCode::scope #2234

Merged
merged 1 commit into from Mar 25, 2013

Projects

None yet

3 participants

@ryoqun
Member
ryoqun commented Mar 25, 2013

In some cases, wrong ConstantScope can be used when JIT is enabled.

For example, ConstantScope should be different with each invocation of
class_exec in the following code:

definition_block = proc do
  proc do
    def baz
    end
  end.call
end
Foo.class_exec(&definition_block)
Bar.class_exec(&definition_block)

But, it becomes always same after this code is JIT-ted.

The cause is the following commit's modification for perfmance. It changed to
only initialize CompiledCode::scope if it's nil:

e3e230646aa1330c19727b3fde6bf38248733a6c Improve block creation speed

// TODO: We don't need to be doing this everytime.
-    cm->scope(state, call_frame->static_scope());
+    if(cm->scope()->nil_p()) {
+      cm->scope(state, call_frame->static_scope());
+    }

But this is wrong, as shown above, there are cases where unconditional
overwriting of ConstantScope is needed.

Also, this diverges from the code of the create_block VM instruction defined
at vm/instructions.def. This also indicates something wrong.

So, always initialize CompiledCode::scope here too.

@ryoqun ryoqun Use correct scope by setting CompiledCode::scope
In some cases, wrong ConstantScope can be used when JIT is enabled.

For example, ConstantScope should be different with each invocation of
class_exec in the following code:

    definition_block = proc do
      proc do
        def baz
        end
      end.call
    end
    Foo.class_exec(&definition_block)
    Bar.class_exec(&definition_block)

But, it becomes always same after this code is JIT-ted.

The cause is the following commit's modification for perfmance. It changed to
only initialize CompiledCode::scope if it's nil:

    e3e2306 Improve block creation speed

    // TODO: We don't need to be doing this everytime.
    -    cm->scope(state, call_frame->static_scope());
    +    if(cm->scope()->nil_p()) {
    +      cm->scope(state, call_frame->static_scope());
    +    }

But this is wrong, as shown above, there are cases where unconditional
overwriting of ConstantScope is needed.

Also, this diverges from the code of the create_block VM instruction defined
at vm/instructions.def. This also indicates something wrong.

So, always initialize CompiledCode::scope here too.
d07df0e
@dbussink dbussink merged commit baab48d into rubinius:master Mar 25, 2013

1 check was pending

default The Travis build is in progress
Details
Member
ryoqun commented Mar 26, 2013

@Locke23rus Thanks for reviewing my commits! well, the comment is still valid. The comment describes the code in the if clauses, not the whole if statement. This code is copied from this create_block instruction: https://github.com/rubinius/rubinius/blob/master/vm/instructions.def#L1214

There you can see the comment still alive. I'll work on removing it later in near future, though.

@Locke23rus This is the issue for the future work I just mentioned: #2221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment