define_method with empty body throws RuntimeError in interpreter #3483

Closed
annawinkler opened this Issue Nov 20, 2015 · 14 comments

Comments

Projects
None yet
4 participants
@annawinkler

If you do the following in jruby 9.0.4.0:

> define_method(:foo) { |*| }
> foo("hi")

You get this RuntimeError:

RuntimeError: BUG: interpreter fell through to end unexpectedly
    from (irb):2:in `foo'
    from (irb):2:in `<eval>'
    from org/jruby/RubyKernel.java:978:in `eval'
    from org/jruby/RubyKernel.java:1291:in `loop'
    from org/jruby/RubyKernel.java:1098:in `catch'
    from org/jruby/RubyKernel.java:1098:in `catch'
    from /home/vagrant/.rbenv/versions/jruby-9.0.4.0/bin/irb:13:in `<top>'

I tried the same in jruby-9.0.1.0, and it works as follows:

irb(main):005:0> define_method(:foo) { |*| }
=> :foo
irb(main):006:0> foo("hi")
=> nil
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 20, 2015

Member

Ah-ha! This makes sense. In 9030 we made a change that converts define_method methods into regular methods. It seems something about this conversion is causing the code to fall off the end of the method.

This is definitely enough for us to reproduce and find the problem.

You can work around the issue by adding an explicit return to the method, so it seems like this is related to how blocks return. For example, this works:

define_method(:foo) {|*| return}
foo("hi")
Member

headius commented Nov 20, 2015

Ah-ha! This makes sense. In 9030 we made a change that converts define_method methods into regular methods. It seems something about this conversion is causing the code to fall off the end of the method.

This is definitely enough for us to reproduce and find the problem.

You can work around the issue by adding an explicit return to the method, so it seems like this is related to how blocks return. For example, this works:

define_method(:foo) {|*| return}
foo("hi")
@annawinkler

This comment has been minimized.

Show comment
Hide comment
@annawinkler

annawinkler Nov 20, 2015

oh i'll try that - thanks!

oh i'll try that - thanks!

@annawinkler

This comment has been minimized.

Show comment
Hide comment
@annawinkler

annawinkler Nov 20, 2015

And for the record, this is why we're doing what we're doing for testing:

class NullLogger
  [ :add, :debug, :info, :warn, :error, :fatal, :unknown, :write ].each do |level|
    define_method(level) { |*| }
  end
end

So... one could argue that that's terrible :)

And for the record, this is why we're doing what we're doing for testing:

class NullLogger
  [ :add, :debug, :info, :warn, :error, :fatal, :unknown, :write ].each do |level|
    define_method(level) { |*| }
  end
end

So... one could argue that that's terrible :)

@annawinkler

This comment has been minimized.

Show comment
Hide comment
@annawinkler

annawinkler Nov 20, 2015

The addition of the return statement fixes the problem! Thanks very much!

The addition of the return statement fixes the problem! Thanks very much!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 20, 2015

Member

@annafw I knew it would be something like that...basically stubbing out methods you don't want to make noise. Glad to hear the workaround works. We'll get this fixed up for 9.0.5.0. Thanks for the report!

Member

headius commented Nov 20, 2015

@annafw I knew it would be something like that...basically stubbing out methods you don't want to make noise. Glad to hear the workaround works. We'll get this fixed up for 9.0.5.0. Thanks for the report!

@headius headius added this to the JRuby 9.0.5.0 milestone Nov 20, 2015

@headius headius changed the title from define_method with * args throws RuntimeError to define_method with empty body throws RuntimeError in interpreter Nov 23, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

Looking into this now.

Member

headius commented Nov 23, 2015

Looking into this now.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

This only appears to affect running in the StartupInterpreterEngine:

[] ~/projects/jruby $ jruby -X-C -Xjit.threshold=-1 -e "define_method(:foo) { }; foo"

[] ~/projects/jruby $ jruby -X-C -Xjit.threshold=0 -e "define_method(:foo) { }; foo"

[] ~/projects/jruby $ jruby -Xjit.threshold=-1 -e "define_method(:foo) { }; foo"
RuntimeError: BUG: interpreter fell through to end unexpectedly
    foo at -e:1
  <top> at -e:1

Here's the full backtrace:

RuntimeError: BUG: interpreter fell through to end unexpectedly
        getStackTrace at java/lang/Thread.java:1552
     getBacktraceData at org/jruby/runtime/backtrace/TraceType.java:183
         getBacktrace at org/jruby/runtime/backtrace/TraceType.java:47
     prepareBacktrace at org/jruby/RubyException.java:223
             preRaise at org/jruby/exceptions/RaiseException.java:229
             preRaise at org/jruby/exceptions/RaiseException.java:196
               <init> at org/jruby/exceptions/RaiseException.java:111
    newRaiseException at org/jruby/Ruby.java:4058
      newRuntimeError at org/jruby/Ruby.java:3535
            interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:142
            interpret at org/jruby/ir/interpreter/InterpreterEngine.java:77
     INTERPRET_METHOD at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:162
                  foo at -e:1
                 call at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:148
                 call at org/jruby/internal/runtime/methods/DefineMethodMethod.java:32
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:189
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:293
                 call at org/jruby/runtime/callsite/CachingCallSite.java:131
                <top> at -e:1
  invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                 load at org/jruby/ir/Compiler.java:111
            runScript at org/jruby/Ruby.java:821
            runScript at org/jruby/Ruby.java:813
          runNormally at org/jruby/Ruby.java:751
          runFromMain at org/jruby/Ruby.java:573
        doRunFromMain at org/jruby/Main.java:409
          internalRun at org/jruby/Main.java:304
                  run at org/jruby/Main.java:233
                 main at org/jruby/Main.java:200
Member

headius commented Nov 23, 2015

This only appears to affect running in the StartupInterpreterEngine:

[] ~/projects/jruby $ jruby -X-C -Xjit.threshold=-1 -e "define_method(:foo) { }; foo"

[] ~/projects/jruby $ jruby -X-C -Xjit.threshold=0 -e "define_method(:foo) { }; foo"

[] ~/projects/jruby $ jruby -Xjit.threshold=-1 -e "define_method(:foo) { }; foo"
RuntimeError: BUG: interpreter fell through to end unexpectedly
    foo at -e:1
  <top> at -e:1

Here's the full backtrace:

RuntimeError: BUG: interpreter fell through to end unexpectedly
        getStackTrace at java/lang/Thread.java:1552
     getBacktraceData at org/jruby/runtime/backtrace/TraceType.java:183
         getBacktrace at org/jruby/runtime/backtrace/TraceType.java:47
     prepareBacktrace at org/jruby/RubyException.java:223
             preRaise at org/jruby/exceptions/RaiseException.java:229
             preRaise at org/jruby/exceptions/RaiseException.java:196
               <init> at org/jruby/exceptions/RaiseException.java:111
    newRaiseException at org/jruby/Ruby.java:4058
      newRuntimeError at org/jruby/Ruby.java:3535
            interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:142
            interpret at org/jruby/ir/interpreter/InterpreterEngine.java:77
     INTERPRET_METHOD at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:162
                  foo at -e:1
                 call at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:148
                 call at org/jruby/internal/runtime/methods/DefineMethodMethod.java:32
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:189
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:293
                 call at org/jruby/runtime/callsite/CachingCallSite.java:131
                <top> at -e:1
  invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                 load at org/jruby/ir/Compiler.java:111
            runScript at org/jruby/Ruby.java:821
            runScript at org/jruby/Ruby.java:813
          runNormally at org/jruby/Ruby.java:751
          runFromMain at org/jruby/Ruby.java:573
        doRunFromMain at org/jruby/Main.java:409
          internalRun at org/jruby/Main.java:304
                  run at org/jruby/Main.java:233
                 main at org/jruby/Main.java:200
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

Ah-ha...I think I see the problem. In the startup interpreter, blocks are allowed to fall off the end without an explicit return instructions. Here's a screencap from my debug session:

image

So there's a few ways to fix this, and I'm not sure how to proceed.

  1. Modify the startup interpreter/compiler to add this missing return instruction, so we don't fall off the end.
  2. Force the full interpreter's passes to run before turning the define_method block into a regular method.
  3. Fix up the define_method block when converting it to a method.

What do you think, @enebo and @subbuss?

Member

headius commented Nov 23, 2015

Ah-ha...I think I see the problem. In the startup interpreter, blocks are allowed to fall off the end without an explicit return instructions. Here's a screencap from my debug session:

image

So there's a few ways to fix this, and I'm not sure how to proceed.

  1. Modify the startup interpreter/compiler to add this missing return instruction, so we don't fall off the end.
  2. Force the full interpreter's passes to run before turning the define_method block into a regular method.
  3. Fix up the define_method block when converting it to a method.

What do you think, @enebo and @subbuss?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

Ok, my naive forcing of a full build did not work.

Patch:

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d487251..e06610b 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1800,6 +1800,9 @@ public class RubyModule extends RubyObject {
             // Ask closure to give us a method equivalent.
             IRMethod method = closure.convertToMethod(name);
             if (method != null) {
+                // Run passes against the method to fix #3483
+                method.prepareFullBuild();
+                
                 newMethod = new DefineMethodMethod(method, visibility, this, context.getFrameBlock());
                 Helpers.addInstanceMethod(this, name, newMethod, visibility, context, runtime);
                 return nameSym;

NPE:

Unhandled Java exception: java.lang.NullPointerException
java.lang.NullPointerException: null
             cloneInstrs at org/jruby/ir/IRScope.java:522
  prepareFullBuildCommon at org/jruby/ir/IRScope.java:539
        prepareFullBuild at org/jruby/ir/IRScope.java:558
           define_method at org/jruby/RubyModule.java:1804
Member

headius commented Nov 23, 2015

Ok, my naive forcing of a full build did not work.

Patch:

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d487251..e06610b 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1800,6 +1800,9 @@ public class RubyModule extends RubyObject {
             // Ask closure to give us a method equivalent.
             IRMethod method = closure.convertToMethod(name);
             if (method != null) {
+                // Run passes against the method to fix #3483
+                method.prepareFullBuild();
+                
                 newMethod = new DefineMethodMethod(method, visibility, this, context.getFrameBlock());
                 Helpers.addInstanceMethod(this, name, newMethod, visibility, context, runtime);
                 return nameSym;

NPE:

Unhandled Java exception: java.lang.NullPointerException
java.lang.NullPointerException: null
             cloneInstrs at org/jruby/ir/IRScope.java:522
  prepareFullBuildCommon at org/jruby/ir/IRScope.java:539
        prepareFullBuild at org/jruby/ir/IRScope.java:558
           define_method at org/jruby/RubyModule.java:1804
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

Ok, something is culling out the return before we even get to StartupInterpreter.

Here's the full instructions immediately after build:

image

I'll keep tracing through to figure out where this is happening.

Member

headius commented Nov 23, 2015

Ok, something is culling out the return before we even get to StartupInterpreter.

Here's the full instructions immediately after build:

image

I'll keep tracing through to figure out where this is happening.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 23, 2015

Member

Bleh, ok. I'm stumped for now.

Member

headius commented Nov 23, 2015

Bleh, ok. I'm stumped for now.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 23, 2015

Member

This should be fully converting to a method where we make a new IRMethod and args and body nodes are being treated as if they were method args and body node. I am guessing some presence of some node in AST is setting a flag in a bad way. We should not need to force full build prep since first access of this new "method" should lazilyAcquire the startup interp instrs and it should also run basic passes on it. If a return is getting stripped it is because of the presence of something normally a method would not have. At least that is my current hunch.

Member

enebo commented Nov 23, 2015

This should be fully converting to a method where we make a new IRMethod and args and body nodes are being treated as if they were method args and body node. I am guessing some presence of some node in AST is setting a flag in a bad way. We should not need to force full build prep since first access of this new "method" should lazilyAcquire the startup interp instrs and it should also run basic passes on it. If a return is getting stripped it is because of the presence of something normally a method would not have. At least that is my current hunch.

@enebo enebo closed this in 1ee9007 Nov 23, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 23, 2015

Member

The weird thing about this bug involved println and displaying the AST. The optimization would compile the argsNode and the bodyNode as if the iternode was a def{n,s}node but def nodes will add an implicitnilnode whereas iter nodes would not if the body is empty. For some reason the ast command would not display nilimplicitnode in the output so it was not possible to see the difference. So adding an implicitnilnode for iternodes empty bodies fixes thing.

Member

enebo commented Nov 23, 2015

The weird thing about this bug involved println and displaying the AST. The optimization would compile the argsNode and the bodyNode as if the iternode was a def{n,s}node but def nodes will add an implicitnilnode whereas iter nodes would not if the body is empty. For some reason the ast command would not display nilimplicitnode in the output so it was not possible to see the difference. So adding an implicitnilnode for iternodes empty bodies fixes thing.

kares added a commit that referenced this issue Nov 25, 2015

Merge branch 'master' into ruby-2.3
* master: (29 commits)
  [travis-ci] revert jdk 9 testing from 2fa04aa
  Eliminate Block.Type arg in BlockBody yield/call signatures
  Pass Block instead of Binding in BlockBody.yield/call
  Minor: fix typo in name of runtime helper instr method name
  Update to jnr-unixsocket 0.10.
  Improve CamelCase package splitting. Fixes #3493.
  Update to jnr-unixsocket 0.10-SNAPSHOT for forFD.
  [Truffle] j+tr: support for passing any extra ruby options
  each_object(cls.singleton_class) should not walk special classes.
  This test has been moved to RubySpec.
  [Truffle] Typo.
  [Truffle] Tag failing regex specs.
  [Truffle] Tag failing Time.at spec.
  The bin200 distribution has grown in size.
  [Truffle] Fixed two typos of the same word on the same line of code.
  [Truffle] Simplified the fix in 5446cc2.
  [Truffle] Fixed an NPE when a SharedMethodInfo has a null argumentsDescriptors.
  [Truffle] Fixed the interpreter_path when the root JRuby distribution directory is not named "jruby."
  Fixes #3483. define_method with empty body throws RuntimeError in interpreter
  Test main on Java 9
  ...
@westlakem

This comment has been minimized.

Show comment
Hide comment
@westlakem

westlakem Dec 4, 2015

Do we know when we are planning on this going live? I believe it's effecting RAKE tasks as well (at least cucumber::task rake tasks)

EDIT: I found where the issue was, it's in the cucumber rerun formatter

Do we know when we are planning on this going live? I believe it's effecting RAKE tasks as well (at least cucumber::task rake tasks)

EDIT: I found where the issue was, it's in the cucumber rerun formatter

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