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

[Tobias Hartmann]

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
>