RFR (S) : 8025260 : Methodhandles/JSR292: NullPointerException (NPE) thrown instead of AbstractMethodError (AME)

[John Rose]

On Sep 25, 2013, at 8:26 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

> Could we use the same jump-over code for SPARC too?  It helps to keep the ports a similar as possible.

The "__ should_not_reach_here" potentially emits 100's of instructions, so it probably gets in the way of a short branch-around.

I would prefer that the should_not_reach_here be deleted from the change and the branch-around be replaced by branch-to-end as on x86.

The verify_thread also gets in the way of the short branch-to-end, because its code is also potentially large.

But the fall-through path should be the fast one, at least for compiled code.  So I think you should use the compact compare-and-branch for compiled code, and a looser but longer-reaching branch for the interpreter.

That is, push the compare-null-and-branch code into this conditional:
  if (!for_compiler_entry && JvmtiExport::can_post_interpreter_events()) {

The goal here is that vanilla compiled code should have a fast path that is as compact as possible.  Interpreted or "VerifyThread" code does not need to be compact.

If this goal cannot be met, then so be it, but I think it probably can.

> A general comment:  prefixing labels with L_ makes them easier to spot and follow the control flow.

I agree.  David, it looks like you are matching the ambient style, but I think you should add the L_ prefix to your label and also the pre-existing run_compiled_code (which Chris or I wrote!).

The test is well structured.  I hope it can be a model for other tests of bytecode corner cases.

Please add pseudocode to document what D and T are, so the casual reader (me) does not need to fully parse the ASM code.

A couple of nits:
+        Object[] arg_for_args = new Object[0];
+        try {
+        Object result = t.getMethod("test").invoke(null, arg_for_args);
+        } catch (InvocationTargetException e) {
The try body should be indented.  And you can delete arg_for_args completely since Method.invoke is varargs.

Reviewed.

— John

> 
> On Sep 25, 2013, at 6:01 PM, David Chase <david.r.chase at oracle.com> wrote:
> 
>> webrev: http://cr.openjdk.java.net/~drchase/8025260/webrev.00/
>> 
>> problem: Various places in hotspot adhere to a convention that if a method code pointer (store in a vtable, for instance) is null, then what would be a NullPointerException is instead converted to an AbstractMethodError.  This can be seen in vtableStubs_x86.cpp (search for ame_addr) and templateTable_x86_32.cpp (search for no_such_method), and also Sparc.  However, for methodhandles this convention was not followed and instead a NullPointerException is thrown, which is clearly wrong.
>> 
>> The bug itself causes 24 failures in the ute defmeth benchmark.
>> There is a related bug where NullPointerException is thrown instead of IllegalAccessError, and the fix for this bug converts those to AbstractMethodError, which is differently wrong, but not as completely wrong as NullPointerException.  See https://bugs.openjdk.java.net/browse/JDK-8016839 .  That needs to be handled elsewhere, or by passing more context in to the code that deals with this null pointer.
>> 
>> fix:
>> add a check and a throw, once per architecture.
>> extract a small self-contained test from defmeth
>> 
>> testing:
>> 1) try new against old and new to ensure that the test is a test.
>> 2) manually test across Sparc and x86
>> 3) test against a variety of Sparc and x86 using jprt.
>> 
>> David
>> 
>