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

[David Holmes]

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