RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows
Hi David, the canonicalize() method is never used by java.io or any Java code. Currently it is used by the hotspot in classloader.cpp (which I use in the test) and in libinstrument in InvocationAdapter.c. There is no way to test it in core-libs. One can argue if the canonicalize method is in the right file, but that should be a separate discussion. Best regards, Ralf -Original Message- From: David Holmes Sent: Mittwoch, 16. Oktober 2019 05:59 To: Schmelter, Ralf ; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR (S) 8232168: Fix non wide char canonicalization on Windows Hi Ralf, This isn't a hotspot issue but a core-libs one. The use of a hotspot-runtime test seems more opportunistic than anything else - is it just for code coverage? I would expect to find a more appropriate test somewhere in core-libs. Thanks, David
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz, thanks for the additional test. Looks good. Best regards, Ralf -Original Message- From: Lindenmaier, Goetz Sent: Montag, 30. September 2019 18:31 To: Schmelter, Ralf ; Hotspot dev runtime ; Java Core Libs Subject: RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null. Hi Ralf, > The test should not omit these two bytecodes because the current > implementation is the same. This can change and it is not much additional code > to add the two cases. I implemented test cases for the missing invokes: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20-incremental/ http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20/ Best regards, Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz, just one thing: >> 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. The test should not omit these two bytecodes because the current implementation is the same. This can change and it is not much additional code to add the two cases. Thanks and best regards, Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
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
RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.
Hi Goetz,
I've played with the messages a little bit and they generally look good. But
I've come across two which look strange:
- 'o[static Test.INDEX]' is null. Can not invoke method 'int
java.lang.Object.hashCode()'
- 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not
invoke method 'int java.lang.Object.hashCode()'.
Here is the test program to reproduce these:
public class Test {
public static int INDEX = 2;
public static void main(String[] args) {
Object[] o = new Object[10];
try {
o[INDEX].hashCode();
} catch (Exception e) {
e.printStackTrace();
}
try {
o["".hashCode()].hashCode();
} catch (Exception e) {
e.printStackTrace();
}
}
}
And 'Can not' should be 'Cannot'?
Best regards,
Ralf
