RFR (S) 8181171: Deleting method for RedefineClasses breaks ResolvedMethodName
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Feb 25 22:40:18 UTC 2019
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
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list