Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jitted blocks do not scope evals properly #3368

Closed
headius opened this issue Oct 3, 2015 · 1 comment
Closed

Jitted blocks do not scope evals properly #3368

headius opened this issue Oct 3, 2015 · 1 comment

Comments

@headius
Copy link
Member

headius commented Oct 3, 2015

This is the cause of the current TestMarshal failures.

Reduced case follows.

blah.rb:

class X
  def initialize
    p :here
  end
  Class.new { eval 'def initialize; p :there; end' }
end

Command line and bug (:there should be :here):

$ jruby -Xjit.threshold=0 -e "load './blah.rb'; X.new"
2015-10-03T02:18:02.890-05:00: Ruby: done compiling target script: -e
2015-10-03T02:18:02.910-05:00: JITCompiler: done jitting:<block> at /Users/headius/projects/jruby/blah.rb:4
2015-10-03T02:18:02.916-05:00: JITCompiler: done jitting:X.X.initialize at (eval):0
:there

The problem here seems to be that when a block is jitted, it's not framing itself or rooting the eval properly. I have not investigated further than that, but obviously the eval'ed def initialize is going into the surrounding class X rather than into the Class.new that launched the block.

This breaks TestMarshal because of test_marshal.rb:263-266.

  classISO8859_1 = Class.new do
    attr_accessor "r\xe9sum\xe9".force_encoding(iso_8859_1)
    eval("def initialize(x); @r\xe9sum\xe9 = x; end".force_encoding(iso_8859_1))
  end

This initialize becomes TestMarshal's initialize, As a result, the name of the test passed into TestMarshal.new (as part of test suite initialization) never reaches the TestCase superclass, and the __name__ remains nil. That's the source of the errors we see in the test run:

[1/1] TestMarshal#test_symbol2 = 0.17 s
  1) Error:
TestMarshal#:
TypeError: no implicit conversion of nil into String
    /Users/headius/projects/jruby/test/mri/lib/test/unit/testcase.rb:17:in `run'
    org/jruby/RubyArray.java:2300:in `map'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:682:in `block in _run_suites'
    org/jruby/RubyArray.java:1560:in `each'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:680:in `_run_suites'
    org/jruby/RubyArray.java:1560:in `each'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:31:in `run'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:799:in `run'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:859:in `run'
    /Users/headius/projects/jruby/test/mri/lib/test/unit.rb:863:in `run'
    test/mri/runner.rb:41:in `<top>'

Note that the method name in the error report is missing.

cc @enebo

@headius headius closed this as completed in e9d5c94 Oct 5, 2015
@headius
Copy link
Member Author

headius commented Oct 5, 2015

Closing the loop here...

The problem was that the evalType threadlocal in *IRBlockBody was getting out of sync, due to having MixedModeIRBlockBody wrap a CompiledIRBlockBody. As a result, when CompiledIRBlockBody ran, it did not see that e.g. Class.new had set MixedModeIRBlockBody as an eval'ed block, and as a result the search for method target failed to stop at the Class.new block.

The fix in e9d5c94 shares a single evalType threadlocal between both the container MixedModeIRBlockBody and the contained CompiledIRBlockBody.

There's remaining work needed to clean up all this shared state and do a better job of unifying the different container objects for interpreted and jitted blocks and methods.

@headius headius added this to the JRuby 9.0.2.0 milestone Oct 5, 2015
@headius headius self-assigned this Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant