RFR(XS): 8213410: UseCompressedOops @requirement check fails fails on 32-bit system

[Boris Ulasevich]

 > Looks good.
Thank you!

On 13.11.2018 1:35, David Holmes wrote:
> Hi Boris,
> 
> On 12/11/2018 10:18 PM, Boris Ulasevich wrote:
>> Hi David,
>>
>> Ok. Here is the solution exploiting 64-bit & UseCompressedOops == true 
>> check (works ok and ARM64 and makes tests disabled on ARM32):
>> http://cr.openjdk.java.net/~bulasevich/8213410/webrev.03/
> 
> Looks good.
> 
>> I will try to go forward on code-tools-dev and I will correct these 
>> tests later if we get short-circuit boolean evaluation in jtreg.
> 
> Thanks!
> 
> David
> 
>> thank you,
>> Boris
>>
>> On 12.11.2018 0:45, David Holmes wrote:
>>> Hi Boris,
>>>
>>> On 9/11/2018 9:03 PM, Boris Ulasevich wrote:
>>>> Hi David,
>>>>
>>>> Thank you.
>>>>
>>>> Don't you think it is better to learn jtreg to perform short-circuit 
>>>> boolean evaluation?
>>>
>>> Sure but that will take a lot longer to happen. You'll need to file a 
>>> bug in the CODE-TOOLS project and then discuss on code-tools-dev. 
>>> Once the fix is accepted we then have to wait for the next release of 
>>> jtreg to be promoted and then OpenJDK updated to start using it.
>>>
>>> Meanwhile if you want the tests to actually run they will need to be 
>>> modified to work with existing jtreg behaviour.
>>>
>>> Thanks,
>>> David
>>>
>>>> http://cr.openjdk.java.net/~bulasevich/8213410/webrev.02
>>>> http://cr.openjdk.java.net/~bulasevich/8213410/webrev.02_jtreg
>>>>
>>>> By the way, with the given change in jtreg two failed tests from the 
>>>> original list works Ok because they already have 64-bit check on 
>>>> previous lines:
>>>> * @requires (sun.arch.data.model == "64")
>>>> * @requires vm.opt.final.UseCompressedOops
>>>>
>>>> regards,
>>>> Boris
>>>>
>>>> On 09.11.2018 10:39, David Holmes wrote:
>>>>> Hi Boris,
>>>>>
>>>>> On 7/11/2018 6:54 PM, David Holmes wrote:
>>>>>> Hi Boris,
>>>>>>
>>>>>> On 7/11/2018 6:06 PM, Boris Ulasevich wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>>    Yes, at first glance it is weird. We have actually three 
>>>>>>> states for vm.opt.final.UseCompressedOops: true, false and null. 
>>>>>>> Null means "not applicable" - when current VM does not support 
>>>>>>> the option with the given name. Here is another approach for the 
>>>>>>> issue (I am not sure it is a good one):
>>>>>>>    http://cr.openjdk.java.net/~bulasevich/8213410/webrev.01/
>>>>>>
>>>>>> I like that approach! I'm just not sure everyone else will 
>>>>>> necessarily agree :) This proposal might need a wider audience 
>>>>>> than just hotspot-dev.
>>>>>>
>>>>>> Let me make enquiries with jtreg-use at openjdk.java.net and see what 
>>>>>> the general intent was here.
>>>>>
>>>>> That was uninformative as to intent as Jon initially thought both 
>>>>> variants should fail the same way, but on further reflection it is 
>>>>> likely due to the difference between trying to evaluate null as a 
>>>>> boolean - which gives an error - and doing "true".equals(null) , 
>>>>> which is just false.
>>>>>
>>>>> I also see now that this isn't a general facility for using
>>>>>
>>>>> @requires <some VM flag>
>>>>>
>>>>> because we only actually support three directly:
>>>>>
>>>>>   protected void vmOptFinalFlags(Map<String, String> map) {
>>>>>          vmOptFinalFlag(map, "ClassUnloading");
>>>>>          vmOptFinalFlag(map, "UseCompressedOops");
>>>>>          vmOptFinalFlag(map, "EnableJVMCI");
>>>>>    }
>>>>>
>>>>> but that also shows this is not specific to UseCompressedOops 
>>>>> because EnableJVMCI only exists in a VM that was built with JVMCI 
>>>>> support.
>>>>>
>>>>> I still think the correct way for this current case to be handled 
>>>>> in tests is by using:
>>>>>
>>>>> @requires vm.bits == 64 & UseCompressedOops
>>>>>
>>>>> though that requires jtreg to do short-circuit boolean evaluation, 
>>>>> which it appears not to do. :( Though:
>>>>>
>>>>> @requires vm.bits == 64 & UseCompressedOops == true
>>>>>
>>>>> should work (albeit perhaps due to implementation accident).
>>>>>
>>>>> Cheers,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Aside: if the UseCompressOops checks is preceded by a 64-bit check 
>>>>>> does it still fail? (Just wondering whether jtreg does 
>>>>>> short-circuit evaluation for these @require statements.)
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> thank you,
>>>>>>> Boris
>>>>>>>
>>>>>>> On 07.11.2018 1:36, David Holmes wrote:
>>>>>>>> Hi Boris,
>>>>>>>>
>>>>>>>> I'm somewhat perplexed as to what the difference is between:
>>>>>>>>
>>>>>>>> @requires vm.opt.final.UseCompressedOops
>>>>>>>>
>>>>>>>> and
>>>>>>>>
>>>>>>>> @requires vm.opt.final.UseCompressedOops == true
>>>>>>>>
>>>>>>>> if they are indeed different then that seems to be a bug in the 
>>>>>>>> requirement handling in the jtreg support code.
>>>>>>>>
>>>>>>>> I'm also wondering whether any such requirement should always be 
>>>>>>>> proceeded by a check for 64-bits?
>>>>>>>>
>>>>>>>> Otherwise I would expect
>>>>>>>>
>>>>>>>> @requires vm.opt.final.UseCompressedOops
>>>>>>>>
>>>>>>>> to be false in 32-bit, so again a problem with the jtreg support 
>>>>>>>> code.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>> On 7/11/2018 12:43 AM, Boris Ulasevich wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review this patch to fix jtreg @requires 
>>>>>>>>> vm.opt.final.UseCompressedOops flag evaluation fail reproduced 
>>>>>>>>> on ARM32: "invalid boolean value: null for expression 
>>>>>>>>> vm.opt.final.UseCompressedOops".
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~bulasevich/8213410/webrev.00/
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213410
>>>>>>>>>
>>>>>>>>> The fix was checked on ARM32 (tests disabled) and AARCH64 
>>>>>>>>> (works Ok by default, and becomes disabled with 
>>>>>>>>> -javaoptions:"-XX:-UseCompressedOops" jtreg option).
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Boris Ulasevich