RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Sep 20 14:39:02 UTC 2019
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.
More information about the core-libs-dev
mailing list