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

[serguei.spitsyn at oracle.com]

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

  TestAddDeleteMethods.java:
   - removed confusion in redefinition string names and added comments 
recommended by Dan
   - always list methods in order: foo, publicFoo, finalFoo, staticFoo

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190425/4ece2e60/attachment.html>