RFR (S) 8181171: Deleting method for RedefineClasses breaks ResolvedMethodName

[coleen.phillimore at oracle.com]

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.phillimore at 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
>>
>