RFS(S): 8222934: mark new VM option AllowRedefinitionToAddOrDeleteMethods as deprecated

[serguei.spitsyn at oracle.com]

Hi David,

Thank you for the review!
I'll remove this import.

Thanks,
Serguei


On 4/25/19 22:54, David Holmes wrote:
> Hi Serguei,
>
> Thanks for making these additional changes.
>
> On 26/04/2019 12:57 pm, serguei.spitsyn at oracle.com wrote:
>> Hi Coleen and Dan,
>>
>> Updated webrev is:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.2/ 
>>
>>
>> This implements the suggestions:
>>   VMDeprecatedOptions.java:
>>    - moved the option to the deprecated non-alias flags section
>
> Yep that's fine.
>
>>   TestAddDeleteMethods.java:
>>    - removed confusion in redefinition string names and added 
>> comments recommended by Dan
>>    - always list methods in order: foo, publicFoo, finalFoo, staticFoo
>
> One nit:
>
>   39 import java.lang.Runnable;
>
> java.lang.* is implicitly imported so we don't list any imports for 
> java.lang types.
>
> Thanks,
> David
> -----
>
>> Thanks,
>> Serguei
>>
>>
>> On 4/25/19 2:41 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Coleen,
>>>
>>> Thank you a lot for looking at this!
>>>
>>>
>>> On 4/25/19 2:18 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 4/25/19 4:19 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Dan,
>>>>>
>>>>> Thank you a lot fore reviewing this!
>>>>>
>>>>> On 4/25/19 12:40, Daniel D. Daugherty wrote:
>>>>>> On 4/24/19 6:18 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Please, review fix for:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8222934
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.1/ 
>>>>>>>
>>>>>>
>>>>>> src/hotspot/share/runtime/globals.hpp
>>>>>>     No comments.
>>>>>>
>>>>>> test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java
>>>>>>     L42:         // deprecated class redefinition flags:
>>>>>>     L43:         {"AllowRedefinitionToAddDeleteMethods", "true"},
>>>>>>     L44:
>>>>>>     L45:         // deprecated non-alias flags:
>>>>>>         I think your new flag entry should have been added to the
>>>>>>         "deprecated non-alias flags" section. You don't need to
>>>>>>         call out that this is a "class redefinition" flag.
>>>>>>
>>>>>>         The reason for the "// deprecated alias flags (see also 
>>>>>> aliased_jvm_flags):"
>>>>>>         section (below what you changed) is because there is more
>>>>>>         work to do for those flags.
>>>>>
>>>>> Okay, I'm not very familiar with this test, will check how to 
>>>>> change it.
>>>>>
>>>>>>
>>>>>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java 
>>>>>>
>>>>>>     L94:     public static String ADeleteStaticFoo =
>>>>>>         This case is deleting both "staticFoo" and "finalFoo".
>>>>>>         Is that what you really want? If so, then the test case
>>>>>>         is misnamed.
>>>>>
>>>>> I see your confusion here.
>>>>> The ADeleteStaticFoo is used after the ADeleteFinalFoo.
>>>>> So, the "finalFoo" has been already deleted before.
>>>>> Then the ADeleteStaticFoo only deletes the "staticFoo".
>>>>>
>>>>> The same was not the case for ADeleteFinalFoo.
>>>>> It is because the redefinitions with ADeleteFoo and ADeletePublicFoo
>>>>> are expected to be rejected with UOE.
>>>>>
>>>>>>
>>>>>>     L119     public static String BAddStaticBar =
>>>>>>         This case is added "staticBar" and "finalBar". Is that what
>>>>>>         you really want? If so, then the test case is misnamed.
>>>>>
>>>>> This one is similar to the above.
>>>>> The "finalBar" has already been added bythe BAddFinalBar 
>>>>> redefinition.
>>>>>
>>>>> Please, let me know if you are Okay with it as it is or prefer to 
>>>>> add a comment with clarification.
>>>>>
>>>>>>
>>>>>>     Still a really cool test here!
>>>>>
>>>>> The test was initially written by Coleen (thanks, Coleen!)
>>>>> I've spoiled it a little bit though. :)
>>>>
>>>> Hi Serguei,  You added a lot to it, which is taking me a while to 
>>>> understand.
>>>>
>>>> Why did you make class A inherit from Runnable?
>>>
>>> In fact, nothing foxy.
>>> It implements Runnable, not inherits. :)
>>> There were two steps.
>>> First was to decide if we there is a point to call methods in the 
>>> redefined classes A and B.
>>> You did it with the in the original test version but you made 
>>> publicFoo to call others.
>>> So, I decided that it is useful to make sure the methods are 
>>> executed well after redefinition.
>>> Then I decided to use another class B for added methods.
>>> Calling other methods from publicFoo did not work for me.
>>> I had to generalize it with run() method and then made
>>> classes A and B to implement Runnable to make it more clear.
>>>
>>>
>>>> Can you maintain order of the function declarations?
>>>>
>>>>    78     public static String ADeletePublicFoo =
>>>>
>>>> finalFoo should be before staticFoo in this one.
>>> Nice catch, thanks!
>>> Will fix it in the webrev update.
>>>
>>>>
>>>> Oh, now I see what Dan is talking about.  In ADeleteStaticFoo, 
>>>> finalFoo has already been deleted so you didn't want to also test 
>>>> adding it back.
>>>
>>> Right.
>>>
>>>>
>>>> Thank you for enhancing the test.  I guess it's good that it tests 
>>>> the new option.
>>>
>>> Thanks!
>>> Serguei
>>>
>>>>
>>>> Coleen
>>>>
>>>>>
>>>>>>
>>>>>> Thumbs up. Your call on whether to tweak the test.
>>>>>
>>>>> I'll send a VMDeprecatedOptions related update later.
>>>>>
>>>>>
>>>>> Thanks!
>>>>> Serguei
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>   David, in review for 
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8222934 suggested:
>>>>>>>    1. I would have suggested to add "(Deprecated)" to the 
>>>>>>> description of the new flag in globals.hpp
>>>>>>>    2. The new flag should have been added to the deprecated VM 
>>>>>>> options tests.
>>>>>>>    3. The new test should run in both a positive and negative 
>>>>>>> mode so that it also checks that the new flag works.
>>>>>>>
>>>>>>>   The webrev above implements this suggestion.
>>>>>>>
>>>>>>>
>>>>>>> Testing:
>>>>>>>   In progress: Submit mach5 run for the updated tests.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>