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

[Boris Ulasevich]

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/

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.

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