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

Fri Oct 24 00:59:06 UTC 2014

Thanks Tobias this seems much better to me.

David

On 24/10/2014 3: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
>>>>>>
>>>>