RFR(S): 8192936: RI does not follow the JVMTI RedefineClasses spec that is too strict in the definition

[coleen.phillimore at oracle.com]

On 4/20/19 4:47 PM, serguei.spitsyn at oracle.com wrote:
>
> On 4/20/19 11:29, Daniel D. Daugherty wrote:
>> On 4/16/19 5:40 AM, serguei.spitsyn at oracle.com wrote:
>>> Please, review the fix of:
>>> https://bugs.openjdk.java.net/browse/JDK-8192936
>>>
>>>
>>> Webrev (fix from Coleen):
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8192936-redef-add-delete.1/ 
>>>
>>
>> src/hotspot/share/prims/jvmtiRedefineClasses.cpp
>>     No comments.
>>
>> src/hotspot/share/runtime/arguments.cpp
>>     No comments.
>>
>> src/hotspot/share/runtime/globals.hpp
>>     No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineAddLambdaExpression.java 
>>
>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineDeleteJmethod.java 
>>
>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineSubtractLambdaExpression.java 
>>
>>     No comments.
>>
>> test/jdk/com/sun/jdi/RedefineAddPrivateMethod.java
>>     No comments.
>>
>> test/jdk/com/sun/jdi/lib/jdb/Debuggee.java
>>     No comments.
>>
>> test/jdk/com/sun/jdi/lib/jdb/JdbTest.java
>>     No comments.
>>
>> test/jdk/java/lang/instrument/RedefineAddDeleteMethod/DeleteMethodHandle/MethodHandleDeletedMethod.java 
>>
>> test/jdk/java/lang/instrument/RedefineMethodAddInvoke.sh
>> test/jdk/java/lang/instrument/RedefineMethodDelInvoke.sh
>> test/jdk/java/lang/instrument/RedefineMethodInBacktrace.sh
>>     No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java 
>>
>>     I like the new test! It gives me hope about converting
>>     the old shell tests into java tests.

Thank you for reviewing the test.  I'm glad you liked it.

>>
>> Thumbs up!
>
> Thank you a lot for the review, Dan!
>
>>
>> Update: I see the fix was already pushed. Sorry I didn't get
>> to this review in time.
>
> No worry.
> It is is nice to get your thumbs up anyway!
>
> Also, I want to say that I extremely appreciate the help from number 
> of HotSpot engineers:
> Coleen, David H. (especially, Coleen and David!), Karen, Alan, Mandy, 
> Dan and V. Ivanov!
> We had several rounds of discussions on this issue.
> It is on our plans to make this analysis publicly available.

We should add the internal pages to the openjdk wiki here.

https://wiki.openjdk.java.net/display/HotSpot/Serviceability

Coleen

>
> Thanks!
> Serguei
>
>>
>> Dan
>>
>>>
>>> I've already reviewed and updated the webrev with my suggestions.
>>>
>>>
>>> Reviewed and approved CSR:
>>> https://bugs.openjdk.java.net/browse/JDK-8221528
>>>
>>>
>>> Summary:
>>>   The fix introduces new VM option 
>>> -XX:AllowRedefinitionToAddOrDeleteMethods
>>>   for compatibility with previous releases. New option enables old 
>>> behavior
>>>   and allows the JVM TI RedefineClasses and RetransformClasses to 
>>> add/delete
>>>   private static and private final instance methods in the new class 
>>> versions.
>>>   Without this option the old behavior is disabled.
>>>
>>>   New option is deprecated right away.
>>>   The plan is to keep this option for several releases to allow 
>>> customers
>>>   (tool vendors) to remove dependency on old behavior from their tools.
>>>
>>>
>>> Testing:
>>>   Added new test to verify that class redefinitions which add or 
>>> delete methods
>>>   return expected JVMTI error codes:
>>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java 
>>>
>>>
>>>   Several jvmti, com/sun/jdi and java/lang/instrument tests which 
>>> need old behavior are updated to use new flag.
>>>
>>>   Run locally on Linux-x64 the following test suites in release and 
>>> fastdebug mode:
>>>     - open/test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/
>>>     - vmTestbase_nsk_jvmti
>>>     - vmTestbase_nsk_jdi
>>>     - vmTestbase_nsk_jdb
>>>     - vmTestbase_nsk_jdwp
>>>     - jdk_jdi
>>>     - jdk_instrument
>>>
>>>   Submission of corresponding mach5 jobs is in progress.
>>>
>>> Thanks,
>>> Serguei
>>>
>>
>