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
