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

[serguei.spitsyn at oracle.com]

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/02f0aa8a/attachment.html>