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

[Tobias Hartmann]

Thanks, Stefan.

Best,
Tobias

On 23.10.2014 14:05, Staffan Larsen wrote:
> 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
>>>>>
>>>
>