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

[David Holmes]

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