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

[Schmelter, Ralf]

Hi Goetz,

here are my review remarks:


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.

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

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.

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.



In bytecodeUtils.cpp:

The method get_slotData() should be renamed get_slot_data() in class SimulatedOperandStack.
The parameter innerExpr should be renamed inner_expr.

In method print_local_var() you could initialize param_index to 1 instead of 0 and remove adding 1 later.

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

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).

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.

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

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.

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

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

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.



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).


The rest looks good to me.

Best regards,
Ralf