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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Apr 26 15:27:06 UTC 2019 

On 4/25/19 10: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/

src/hotspot/share/runtime/globals.hpp
     No comments.

test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java
     No comments.

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java
     L100:     // So, this redefinition will add it back which is 
expected to work.
         There's a space at the end of this comment line. jcheck may 
complain.

     L132:     // So, this redefinition will deleate it back which is 
expected to work.
         Typo: s/deleate it back/delete it/

         There's a space at the end of this comment line. jcheck may 
complain.

Thumbs up.

Dan



>
> 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/20190426/c214d4cb/attachment-0001.html>

More information about the serviceability-dev mailing list