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

[coleen.phillimore at oracle.com]

On 2/25/19 5:39 PM, Daniel D. Daugherty wrote:
>> 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.

Okay. thanks,
Coleen
>
> 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
>>>>
>>>
>>
>