JIT mode allows nulled out IRubyObject array to propagate null

[headius]

The following test attempts to call into a Java method Reflector.invoke which calls back into a reified Ruby method via Java reflection. In the process, it eventually passes an empty IRubyObject[1] to the Ruby bar method.

jruby/spec/java_integration/reify/become_java_spec.rb

Lines 67 to 68 in d7a87d4

	args = org.jruby.runtime.builtin.IRubyObject[1].new
	expect( Reflector.invoke(nil, method, args) ).to eql 'BAR-nil'

When the JIT is disabled, the null value contained in the array never ends up in the arg argument as a null; it is a nil and the subsequent method calls work against that nil value.

When the JIT is disabled, however, the null ends up propagating all the way to the calls, eventually causing NPE.

I have not found an explanation for this, but I have managed to confirm that in both modes, an IRubyObject[] {null} does indeed propagate through the Reflector.invoke method, but in the interpreter the null value becomes a nil before it is visible in `arg.

I suspect that the interpreter has additional null guards or ends up populating arg with nil before it can become null. In any case, I don't believe this is a JIT bug, because the test logic in both cases clearly passes a non-zero-length IRubyObject[] with a null element, and that is never allowed. Perhaps there should be a better error than to propagate the null value, but propagation is part of what the IR and JIT optimizations are supposed to do.

I'm unclear what the original intent of this particular test was. It has existed since 2f363d0 in 2016, probably around the time we were formalizing the "become_java" feature, but the way this tries to invoke the generated static method should always fail for a {null} argument array.

As such, I'm going to disable the offending test lines and we'll have to figure out whether they should be expected to pass.

[headius]

@kares I believe you added the test originally. Can you explain why you expected it to pass? Perhaps it just accidentally passed and you codified that behavior in the expectation?

@enebo Any thoughts on why this gets by the interpreter? The arg value should clearly end up null given the unpopulated IRubyObject array used for passing arguments.

[enebo]

@headius if it indirects through a temporary variable we will null to nil the value on access. Part of me wonders what breaks if we remove that. I could see that we added it because we do not properly deal with uninitialized lvars (which is odd because access of an lvar will be nil).

[headius]

@enebo That's what I figured when I punted on this, but I was unable to locate where that actually happens.

I did come up with a short reproduction to show that the interpreter allows this nulled array to work as an argument array, but JIT propagates the null and fails:

class Foo
  def foo(a)
    p a
  end
end

foo = Foo.new
jfoo = JRuby.ref(foo)
ary = org.jruby.runtime.builtin.IRubyObject[1].new
jfoo.callMethod(JRuby.runtime.current_context, "foo", ary)

This is essentially what the test is doing, though it calls through a generated method from become_java. As with the test case, the interpreter sees nil and the JIT sees and propagates null.

[] jruby $ jruby -X-C blah.rb
nil
[] jruby $ jruby blah.rb     
Unhandled Java exception: java.lang.NullPointerException
java.lang.NullPointerException: null
                       getMetaClass at org/jruby/RubyBasicObject.java:472
                          rbInspect at org/jruby/RubyBasicObject.java:1096
                              pBody at org/jruby/RubyKernel.java:552
                    handleInterrupt at org/jruby/RubyThread.java:836
                    uninterruptible at org/jruby/RubyThread.java:2507
                                  p at org/jruby/RubyKernel.java:535
                               call at org/jruby/RubyKernel$INVOKER$s$0$0$p.gen:-1
                     invokeOther1:p at blah.rb:5
                                foo at blah.rb:5
                            finvoke at org/jruby/RubyClass.java:792
                         callMethod at org/jruby/RubyBasicObject.java:367
                             invoke at java/lang/reflect/Method.java:498
  invokeDirectWithExceptionHandling at org/jruby/javasupport/JavaMethod.java:285
                       invokeDirect at org/jruby/javasupport/JavaMethod.java:152
           invokeOther13:callMethod at blah.rb:12
                             <main> at blah.rb:12
                                run at blah.rb:-1
                invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                          runScript at org/jruby/Ruby.java:1277
                        runNormally at org/jruby/Ruby.java:1194
                        runNormally at org/jruby/Ruby.java:1176
                        runNormally at org/jruby/Ruby.java:1212
                        runFromMain at org/jruby/Ruby.java:990
                      doRunFromMain at org/jruby/Main.java:398
                        internalRun at org/jruby/Main.java:282
                                run at org/jruby/Main.java:227
                               main at org/jruby/Main.java:199

Edit: a does not need to be an optional variable so I removed that.

[enebo]

@headius a in this case is an lvar and since we have initialization issues with lvars it will convert null to nil. My statement above is also true that it will also happen with temps but I think in this case it is just the lvar init thing.

[enebo]

The snippet as IR:

     0	[DEAD]%self = recv_self()
     1	%v_0 = load_implicit_closure()
     2	%current_module = copy(module<0>)
     3	%v_2 = recv_kw()
     4	check_arity(%v_2 ;req: 1, opt: 0, *r: false)
     5	a(0:0) = recv_pre_reqd_arg(%v_2)
     6	line_num(;n: 1)
     7	return(a(0:0))

So a is assigned null and we do not seem to check that at all on the way in but the return will access a which will notice it is null and return nil instead.

[headius]

So I guess the question is whether IR should be failing here (invalid arguments in the argument array) or if JIT should gain some enhancement to avoid this null propagating further. I'm currently of the opinion that this is an invalid argument array. It would (should?) never happen during normal execution, and this example and the test above are going through a back door to force a null into the call stack. But I don't like when these bugs come up and nulls start to propagate.

Perhaps we need a debug mode that will null check arguments, variable lookups (ivars, constants, globals), and return values for null, raising immediately when one is seen?

This was difficult to track down and my first thought was that something was broken in the JIT or invokedynamic logic, so I spun my wheels trying to fix something that perhaps wasn't actually broken. A null-debugging flag we could turn on might help future cases.

[enebo]

@headius I agree. A potential compromise where it will still work but we can at least detect it would be to make the recv instrs assert() is null. Then ordinary execution would work but we should see it fail in our test suite.

[enebo]

I guess I should clarify that assert() is not really a compromise but just another variant of your debug mode idea.

[headius]

I scanned through JVMVisitor and it wouldn't be too hard to decorate all the incoming-argument logic with some conditional verification. I don't want it to be always there (like a Java assert) so it should still be behind a flag I think.