RFR (XS) 8205702: assert(UseCompressedClassPointers) failed in universe.hpp

[Calvin Cheung]

Hi Per,

Thanks for coming up with a simpler fix. It looks good. Just one comment 
below.

On 6/27/18, 2:33 AM, Per Liden wrote:
> Actually, that seems a bit too restrictive as 
> vm.cds.archived.java.heap is only true when G1 is enabled.
>
> So, this is probably even better:
>
>  * @requires vm.cds
>  * @requires vm.opt.final.UseCompressedOops
>  * @requires vm.opt.final.UseCompressedClassPointers
I think the @requires vm.cds calls into VMProps.vmCDS() which calls the 
WB_IsCDSIncludedInVmBuild() where it already checks for compressed oops 
and pointers:

     if (!UseCompressedOops || !UseCompressedClassPointers) {
       // On 64-bit VMs, CDS is supported only with compressed oops/pointers
       return false;
     }

Are the last two @requires needed?

thanks,
Calvin
>
> Updated webrev: http://cr.openjdk.java.net/~pliden/8205702/webrev.2
>
> /Per
>
> On 06/27/2018 10:37 AM, Per Liden wrote:
>> Updated webrev, which adjusts the @requires tag, from:
>>
>>    @requires vm.cds & vm.gc != "Z"
>>
>> to:
>>
>>    @requires vm.cds.archived.java.heap
>>
>> which I believe is more correct in this case.
>>
>> http://cr.openjdk.java.net/~pliden/8205702/webrev.1
>>
>> cheers,
>> Per
>>
>>
>> On 06/27/2018 09:15 AM, Per Liden wrote:
>>> Hi Coleen,
>>>
>>> This doesn't look quite right to me. ZGC already disables 
>>> UseCompressedOop and UseCompressedClassPointers, which should be the 
>>> indicators that we can't use CDS. The problem is that CDS checks 
>>> those flags _before_ the heap has had a change to say they it 
>>> supports. So if we just move the call to set_shared_spaces_flags() 
>>> after the call to GCConfig::arguments()->initialize() (which should 
>>> be safe), then we're all good and you'll get the usual:
>>>
>>> $ ./build/fastdebug/images/jdk/bin/java -Xshare:dump 
>>> -XX:+UnlockExperimentalVMOptions -XX:+UseZGC
>>> Error occurred during initialization of VM
>>> Cannot dump shared archive when UseCompressedOops or 
>>> UseCompressedClassPointers is off.
>>>
>>> Here's a proposed patch for this, which also adjusts the appropriate 
>>> tests for this:
>>>
>>> http://cr.openjdk.java.net/~pliden/8205702/webrev.0
>>>
>>> cheers,
>>> Per
>>>
>>> On 06/27/2018 01:09 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Hi Calvin, thank you for reporting the bug and the code review and 
>>>> test code.
>>>>
>>>> On 6/26/18 5:42 PM, Calvin Cheung wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> The code changes look good.
>>>>>
>>>>> Since there's a new error message, I'd suggest adding a test to 
>>>>> runtime/SharedArchiveFile/SharedArchiveFile.java as follows:
>>>>>
>>>>> diff --git 
>>>>> a/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java 
>>>>> b/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>> --- 
>>>>> a/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>> +++ 
>>>>> b/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>> @@ -52,5 +52,13 @@
>>>>>                                "-Xshare:on", "-version");
>>>>>          out = CDSTestUtils.executeAndLog(pb, "SharedArchiveFile");
>>>>>          CDSTestUtils.checkExec(out);
>>>>> +
>>>>> +        // CDS dumping doesn't work with ZGC
>>>>> +        ProcessBuilder pb = 
>>>>> ProcessTools.createJavaProcessBuilder(true,
>>>>> + "-XX:SharedArchiveFile=./SharedArchiveFile.jsa",
>>>>> +                                "-XX:+UseZGC",
>>>>> +                                "-Xshare:dump");
>>>>> +        out = CDSTestUtils.executeAndLog(pb, "SharedArchiveFile");
>>>>> +        CDSTestUtils.checkExecExpectError(out, 1, 
>>>>> "DumpSharedSpaces (-Xshare:dump) is not supported with ZGC.");
>>>>>      }
>>>>>  }
>>>>>
>>>>> (I haven't tested the above)
>>>>
>>>> It needed an -XX:+UnlockExperimentalVMOptions as well, and not 
>>>> reclare pb.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8205702.02/webrev
>>>>
>>>>>
>>>>> Also, I think the new error message should be included in the 
>>>>> release notes.
>>>>>
>>>>
>>>> I added the test case and it passes.  I don't think having a 
>>>> release note for something that nobody would ever do for an 
>>>> experimental option is worth having.   But I can look into the ZGC 
>>>> release notes and see if there's something that says CDS is not 
>>>> supported.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 6/26/18, 2:13 PM, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: Disable CDS with ZGC
>>>>>>
>>>>>> Tested with:
>>>>>> java -XX:+UnlockExperimentalVMOptions -XX:+UseZGC -Xshare:dump
>>>>>> java -XX:+UnlockExperimentalOptions -XX:+UseZGC -Xshare:on -version
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8205702.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8205702
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>