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.
