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

[Staffan Larsen]

This is better!

But won’t the new WB_IsDebugVMFlag() return true for flags that don’t exist at all? 

/Staffan


On 23 okt 2014, at 10:57, Tobias Hartmann <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> 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