RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Ralf,

thanks for looking at this code and this thorough review!

New webrevs:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-incremental/

Find my comments inline:
 
> In javaClasses.cpp:
> 
> > #define CLASS_FIELDS_DO(macro) \
> >  macro(classRedefinedCount_offset, k, "classRedefinedCount", int_signature,
> false); \
> >  macro(_class_loader_offset,       k, "classLoader",         classloader_signature,
> false); \
> 
> The field name should match the other field names. So
> _class_redefined_count_offset instead of classRedefinedCount_offset.
That's right. But the field was there before my change, I just removed
the spurious space. So I don't feel like changing this.

> The method java_lang_Throwable::get_method_and_bci() should be renamed
> java_lang_Throwable::get_top_method_and_bci().
Changed.

> The usage of java.lang.Boolean(true) as the marker pointer leads to more code
> than a simpler, but more hackish solution (e.g. just reusing a reference to the
> backtrace array itself). I'm not sure it is really worth it. Considering you in the
> end just test for != NULL.
I think the design is clearer the way I did it. 
The Boolean should almost never be allocated. In general, hidden frames should
not throw NPEs. 

> In jvm.cpp:
> 
> The method trim_well_known_class_names() would convert a name like
> test.java.lang.String to test.String. I think you have to be more specific when
> replacing a name.
Ah, you are right. A pity, this was nice and compact.
I moved the code into bytecodeUtils.cpp.

> In bytecodeUtils.cpp:
> 
> The method get_slotData() should be renamed get_slot_data() in class
> SimulatedOperandStack.
Done.

> The parameter innerExpr should be renamed inner_expr.
Done.
 
> In method print_local_var() you could initialize param_index to 1 instead of 0
> and remove adding 1 later.
Fixed.

> In ExceptionMessageBuilder::ExceptionMessageBuilder you should add spaces
> in '(len+1)'.
Fixed.

> Since the ExceptionMessageBuilder is only used with a bci >= 0, you could
> remove the code which handles the case for bci < 0 (where we would create
> the simulated stack for every bci).
I'd like to leave this as-is, as the code could well analyze the whole
method.

> In ExceptionMessageBuilder::do_instruction() the dup2 bytecode seems to be
> implemented wrongly, since you seemingly push two times the same value. It
> isn't, since you change the stack with the push, so in the first push you push
> the top of stack - 1 entry, and in the next push the former top of stack. A
> comment should be added to make this clearer or change to the code to use
> temporary variables.
Added comment.

> In ExceptionMessageBuilder::do_instruction() the is_wide variable is never
> used, so it should be removed.
Done.

> In ExceptionMessageBuilder::do_instruction() when handling the invoke*
> bytecodes, there is the following code:
> 
> if ((code != Bytecodes::_invokestatic) && (code !=
> Bytecodes::_invokedynamic)) {
>   // Pop class.
>   stack->pop(1);
> }
> 
> The // Pop class comment is misleading, because the receiver is popped in
> reality.
Fixed.

> In ExceptionMessageBuilder::get_NPE_null_slot() the opening brace for the
> invoke* cases should be consistent with the rest of the code.
Fixed.

> In ExceptionMessageBuilder::get_NPE_null_slot() maybe we should use defines
> for -2 and -1 which convey the meaning of that return values.
Done.

> In ExceptionMessageBuilder::print_NPE_failed_action() you assert that the
> method holder is not the NativeConstructorAccessorImpl, noting that this
> should already have been checked In get_NPE_null_slot(). But I don't see any
> code in that message which would check this, it seemed to be check in
> BytecodeUtils::get_NPE_message_at() instead.
I moved that code up when I reviewed my own code.
It seems better placed here, as also the check for 
NPE_EXPLICIT_CONSTRUCTED is here.
I removed the assertion.
 
> In NullPointerExceptionTest.java:
> 
> It seems you don't have tests for invokeinterface or invokespecial calls to cause
> an NPE (e.g. by calling a null interface variable or a private non-static method
> of a null objects).
That is because the code is the same as for invokevirtual in all places in 
bytecodeUtils.cpp.

Thanks and best regards,
  Goetz.