Dan, Thank you for your review.
On 2/25/19 4:13 PM, Daniel D. Daugherty wrote:
On 2/22/19 6:36 PM, coleen.phillim...@oracle.com wrote:
8210457: JVM crash in ResolvedMethodTable::add_method(Handle)
Summary: Add a function to call NSME in ResolvedMethodTable to
replace deleted methods.
This Unsafe.throwX trick is also used for vtable initialization for
throwing IllegalAccessError. Tested with redefinition tests in the
repository and tier1-3, and added tests.
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8181171.01/webrev
src/hotspot/share/classfile/javaClasses.cpp
No comments.
src/hotspot/share/memory/universe.hpp
No comments.
src/hotspot/share/memory/universe.cpp
No comments.
src/hotspot/share/oops/method.cpp
No comments.
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
L3533: // There is a jmethodID, change it to point to the NSME.
Please consider:
// Change the jmethodID to point to NSME.
Ok, change it.
src/hotspot/share/prims/resolvedMethodTable.hpp
No comments.
src/hotspot/share/prims/resolvedMethodTable.cpp
L134: // Method may have been deleted, replace with NSME
L135: if (method == NULL) {
L136: method = Universe::throw_no_such_method_error();
Please consider:
if (method == NULL) {
// Replace deleted method with NSME.
method = Universe::throw_no_such_method_error();
In addition to the comment on L134?
src/java.base/share/classes/jdk/internal/misc/Unsafe.java
No comments beyond Serguei's.
test/jdk/java/lang/instrument/NamedBuffer.java
L91: static boolean checkMatch(byte[] buf, byte[] name, int
begIdx) {
L92: for (int i = 0; i < name.length; i++) {
L93: if (buf[i + begIdx] != name[i]) {
This code assumes the buf.length >= name.length + begIdx
which could lead to array index problem. Perhaps add this
before L92:
if (buf.length < name.length + begIdx) {
return false;
}
Oh yeah, this is bad. Your solution looks good, so I added it. Thank you!
L101: bytesForHostClass(char replace, String className) throws
Throwable {
A short header comment about what this function is doing would
be a good idea. Update: David asked for something similar.
Yes, I added a header comment for what this does.
// This function reads a class file from disk and replaces the
first character of
// the name with the one passed as "replace". Then goes through
the bytecodes to
// replace all instances of the name of the class with the new
name. The
// redefinition tests use this to redefine Host$ classes with
precompiled class files
// Xost.java, Yost.java and Zost.java.
L121: if(checkMatch(buf, ptrnBytes, i)) {
nit - please add space after 'if' and before '('
Fixed.
test/hotspot/jtreg/runtime/RedefineTests/RedefineDeleteJmethod.java
No comments.
test/hotspot/jtreg/runtime/RedefineTests/libRedefineDeleteJmethod.c
L46: // printf("visible 0x%x expected %d\n", visible, expected);
This commented out line appears unrelated to this test.
Removed it.
test/jdk/java/lang/instrument/RedefineAddDeleteMethod/DeleteMethodHandle/MethodHandleDeletedMethod.java
L34: * @compile redef/Xost.java redef/Yost.java
File
test/jdk/java/lang/instrument/RedefineAddDeleteMethod/DeleteMethodHandle/redef/Yost.java
appears to be missing from the webrev. However, there appears
to be
no reference to Yost other than this compile line.
Thank you for catching that. Yost is out of this test case but it's
still in my repository, so it would have bombed when I checked it in :(
L87: System.out.println("Passed, expected NSME");
L93: System.out.println("Passed, expected NSME");
Perhaps both of these should be:
System.out.println("Received expected NSME");
and the indication of "Passed" should be just before main()
returns.
Ok. I changed it. I like a lot of positive reinforcement ...
test/jdk/java/lang/instrument/RedefineAddDeleteMethod/DeleteMethodHandle/redef/Xost.java
L28: // try to redefine Xhost.
Should 'Xhost' be 'Xost'? Or??? Update: Serguei also mentioned
this one.
I missed this from Serguei's comments. I fixed it now.
Thanks for the code review. I made the above changes and I'll sanity
check it before checking it in.
Coleen
Dan
bug link https://bugs.openjdk.java.net/browse/JDK-8181171
Thanks,
Coleen
