RFR(S): 8061443: Whitebox get*VMFlag() methods fail with develop flags in product builds

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

On 23 okt 2014, at 13:15, Tobias Hartmann <Tobias.Hartmann at oracle.com> wrote:

> 
> On 23.10.2014 12:13, Staffan Larsen wrote:
>> I don’t agree that locked flags are the same as debug flags. Perhaps two queries? One for constant flags and one for locked flags?
> 
> Okay, I added the methods 'isLockedVMFlag' and 'isConstantVMFlag' and adapted
> the tests.
> 
> New webrev: http://cr.openjdk.java.net/~thartmann/8061443/webrev.03/
> 
> Thanks,
> Tobias
> 
>> /Staffan
>> 
>> 
>> On 23 okt 2014, at 12:02, Tobias Hartmann <Tobias.Hartmann at oracle.com> wrote:
>> 
>>> Hi Staffan,
>>> 
>>> On 23.10.2014 11:29, Staffan Larsen wrote:
>>>> This is better!
>>>> 
>>>> But won’t the new WB_IsDebugVMFlag() return true for flags that don’t exist at all? 
>>> 
>>> Of course, you are right. I fixed it and added a call to 'isDebugVMFlag' to
>>> 'VmFlagTest.testWriteNegative' to check that it always returns false for
>>> non-existing flags.
>>> 
>>> New webrev: http://cr.openjdk.java.net/~thartmann/8061443/webrev.02/
>>> 
>>> Thanks,
>>> Tobias
>>> 
>>>> 
>>>> /Staffan
>>>> 
>>>> 
>>>> On 23 okt 2014, at 10:57, Tobias Hartmann <Tobias.Hartmann at oracle.com
>>>> <mailto:Tobias.Hartmann at oracle.com>> wrote:
>>>> 
>>>>> Vladimir, David, Staffan,
>>>>> 
>>>>> thanks for the reviews.
>>>>> 
>>>>> On 23.10.2014 09:06, Staffan Larsen wrote:
>>>>>> 
>>>>>> On 23 okt 2014, at 04:35, David Holmes <david.holmes at oracle.com
>>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>> 
>>>>>>> On 23/10/2014 7:22 AM, Vladimir Kozlov wrote:
>>>>>>>> getVMOption() specification does not say anything about product, locked
>>>>>>>> or develop flags. "If a VM option of the given name does not exist" can
>>>>>>>> interpreted different ways.
>>>>>>> 
>>>>>>> Perhaps, but conceptually develop flags are not part of a product build and
>>>>>>> by definition non-product flags are not part of a product build, so I think
>>>>>>> the existing jmm code is quite right to exclude them and the new code should
>>>>>>> do the same. It doesn't make sense to me to present someone with a list of
>>>>>>> flags that can't actually be used.
>>>>>> 
>>>>>> I agree with David here. Instead of changing the JMM API, you can add
>>>>>> functionality to the WhiteBox API to get the value of non-product flags.
>>>>> 
>>>>> I agree. I reverted the JMM changes and added a Whitebox API method
>>>>> 'isDebugVMFlag' to check whether a flag is debug/notproduct.
>>>>> 
>>>>> I changed the tests such that we only use JMM to verify the values returned by
>>>>> the Whitebox API if the flag is accessible (i.e. is not a debug/notproduct flag
>>>>> in a product build).
>>>>> 
>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8061443/webrev.01/
>>>>> 
>>>>> Thanks,
>>>>> Tobias
>>>>> 
>>>>>> /Staffan
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> 
>>>>>>>> http://docs.oracle.com/javase/8/docs/jre/api/management/extension/com/sun/management/HotSpotDiagnosticMXBean.html
>>>>>>>> 
>>>>>>>> 
>>>>>>>> VMOption getVMOption(String name)
>>>>>>>> 
>>>>>>>>  Returns a VMOption object for a VM option of the given name.
>>>>>>>> 
>>>>>>>>  Returns:
>>>>>>>>      a VMOption object for a VM option of the given name.
>>>>>>>>  Throws:
>>>>>>>>      NullPointerException - if name is null.
>>>>>>>>      IllegalArgumentException - if a VM option of the given name
>>>>>>>> does not exist.
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 10/22/14 6:46 AM, Tobias Hartmann wrote:
>>>>>>>>> Hi David,
>>>>>>>>> 
>>>>>>>>> On 22.10.2014 14:33, David Holmes wrote:
>>>>>>>>>> On 22/10/2014 6:48 PM, Tobias Hartmann wrote:
>>>>>>>>>>> Hi David,
>>>>>>>>>>> 
>>>>>>>>>>> On 22.10.2014 04:16, David Holmes wrote:
>>>>>>>>>>>> Hi Tobias,
>>>>>>>>>>>> 
>>>>>>>>>>>> On 21/10/2014 11:08 PM, Tobias Hartmann wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>> 
>>>>>>>>>>>>> please review the following patch.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8061443
>>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8061443/webrev.00/
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Problem:
>>>>>>>>>>>>> The Whitebox API methods get*VMFlag() fail with develop/notproduct
>>>>>>>>>>>>> flags in
>>>>>>>>>>>>> product builds.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Solution:
>>>>>>>>>>>>> The code is changed to invoke 'Flag::find_flag' with
>>>>>>>>>>>>> allow_locked=true and
>>>>>>>>>>>>> return_flag=true to return both locked and notproduct/develop
>>>>>>>>>>>>> flags. I also
>>>>>>>>>>>>> changed the JVM monitoring and management code 'jmm_GetVMGlobals()'.
>>>>>>>>>>>> 
>>>>>>>>>>>> Why did you change the jmm code? You seemed to have changed its
>>>>>>>>>>>> semantics -
>>>>>>>>>>>> which may require a CCC request.
>>>>>>>>>>> 
>>>>>>>>>>> I changed the jmm code because the whitebox tests use a JMM call to
>>>>>>>>>>> verify the
>>>>>>>>>>> return value (see 'VmFlagTest<T>.getVMOptionAsString()').
>>>>>>>>>>> 
>>>>>>>>>>> Do you think a CCC request is necessary here?
>>>>>>>>>> 
>>>>>>>>>> Depends what the jmm code is supposed to return - I don't know the
>>>>>>>>>> spec. But
>>>>>>>>>> changing the behaviour for the sake of a test without consideration
>>>>>>>>>> of real
>>>>>>>>>> users is not something that should be done lightly.
>>>>>>>>> 
>>>>>>>>> I agree. I filed a CCC request and attached the link to the bug.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Tobias
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> David
>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Tobias
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>> 
>>>>>>>>>>>>> I adapted the existing tests to check for develop flags as well. I
>>>>>>>>>>>>> noticed that
>>>>>>>>>>>>> HotspotDiagnosticMXBean.getVMOption() fails with double flags and
>>>>>>>>>>>>> filed
>>>>>>>>>>>>> JDK-8061616 [1].
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Testing:
>>>>>>>>>>>>> - New testcases
>>>>>>>>>>>>> - JPRT
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Tobias
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8061616
>>>> 
>>