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

Thu Oct 23 17:51:54 UTC 2014

Thanks, Vladimir.

Best,
Tobias

On 23.10.2014 19:49, Vladimir Kozlov wrote:
> Good.
> 
> Thanks,
> Vladimir
> 
> On 10/23/14 10:45 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 23.10.2014 19:28, Vladimir Kozlov wrote:
>>> whitebox.cpp I don't why you need to split next lines:
>>>
>>> +   {CC"isConstantVMFlag",      CC"(Ljava/lang/String;)Z",
>>> +
>>> (void*)&WB_IsConstantVMFlag},
>>> +   {CC"isLockedVMFlag",     CC"(Ljava/lang/String;)Z",
>>> +                                                      
>>> (void*)&WB_IsLockedVMFlag},
>>>
>>> VmFlagTest.java add line to the next comment:
>>>
>>> // JMM cannot access debug flags in product builds or locked flags,
>>> // use whitebox methods to get such flags value.
>>
>> Done. New webrev:
>> http://cr.openjdk.java.net/~thartmann/8061443/webrev.04/
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Otherwise seems correct.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 10/23/14 4:15 AM, Tobias Hartmann 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
>>>>>>>
>>>>>