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

[Daniel D. Daugherty]

> 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? 

No. Drop the comment on L134.

Dan



On 2/25/19 5:34 PM, coleen.phillimore at oracle.com wrote:
>
> 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
>>>
>>
>