RFR (XS) 8185160 -XX:DumpLoadedClassList omits graal classes

[Jiangli Zhou]

> On Oct 18, 2017, at 9:50 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 10/18/17 9:35 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>> 
>>> On Oct 18, 2017, at 4:54 PM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>> 
>>> 
>>> 
>>> On 10/17/17 9:04 PM, David Holmes wrote:
>>>> Hi Ioi,
>>>> 
>>>> On 18/10/2017 1:52 PM, Ioi Lam wrote:
>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v01/ <http://cr.openjdk.java.net/%7Eiklam/jdk10/8185160-dump-class-list-omits-graal.v01/> 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8185160 <https://bugs.openjdk.java.net/browse/JDK-8185160>
>>>>> 
>>>>> Summary:
>>>>> 
>>>>> DumpLoadedClassList skips classes that can't be stored into the CDS
>>>>> archive. For boot and platform loaders, only classes from the JRT image
>>>>> are listed.
>>>>> 
>>>>> The test for "is this class from JRT image" was insufficient. It
>>>>> checked only classes whose source ends with "...../modules". For the
>>>>> platform loader, which loads the graal classes, the source is in the
>>>>> form "jrt:/jdk.internal.vm.compiler".
>>>>> 
>>>>> The fix is to also check for a "jrt:" prefix.
>>>> 
>>>> So ./share/classfile/classLoader.hpp:
>>>> 
>>>> static bool is_jrt(const char* name) { return string_ends_with(name, MODULES_IMAGE_NAME); }
>>>> 
>>>> is a badly named function or just plain broken?
>>>> 
>>> 
>>> I've renamed all the is_jrt() functions to is_modules_image(), so it's clear as to what it does.
>>> 
>>> See http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v02/ <http://cr.openjdk.java.net/%7Eiklam/jdk10/8185160-dump-class-list-omits-graal.v02/>
>> 
>> The logic looks puzzling. For both NULL class loader and the PlatformClassLoader:
>> 
>> The first if check at line 5938 excludes any classes not in the jimage.
>> 
>> 5938           if (!ClassLoader::is_modules_image(stream->source()) && strncmp(stream->source(), "jrt:", 4) != 0) {
>> 5939             skip = true;
>> 5940           }
>> The second if check at line 5942 skips any classes from the boot append class path loaded by the NULL class loader. The second check skips a subset of the classes that’s already skipped by the first check. The second check is redundant.
>> 
> You're right. The second check is redundant. I'll remove it.
> 
>> 5942           if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {
>> 5943             // For the boot loader, also skip all classes that are loaded from the appended entries
>> 5944             skip = true;
>> 5945           }
>> What about the classes from -Xbootclasspath/a (not in --patch-module and --upgrade-module-path)?
>> 
> I am not sure what you mean here.

This excludes all classes from the -Xbootclasspath/a. However, classes from -Xbootclasspath/a can be archived and used at runtime properly. 

Thanks,
Jiangli

> 
> Thanks
> - Ioi
>> Thanks,
>> Jiangli
>> 
>>> 
>>> 
>>>>> I also restructured the code to make it easier to read -- the nesting of || and && is hard to follow.
>>>>> 
>>>>> The original code had a subtle bug with the "!" operator:
>>>>> 
>>>>> 5938   if (((class_loader == NULL && !ClassLoader::contains_append_entry(stream->source())) ||
>>>>> 5939 SystemDictionary::is_platform_class_loader(class_loader)) &&
>>>>> 5940       !ClassLoader::is_jrt(stream->source())) {
>>>>> 
>>>>> So if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source()) was true,
>>>>> the class would NOT be skipped.
>>>>> 
>>>>> The new version removes the "!".
>>>> 
>>>> So there should be an updated test to trigger the original bug.
>>>> 
>>> 
>>> I added a test case, but it's in the closed repo for now (since the test case uses utilities in the closed AppCDS tests). This test will be opened as part of JDK-8185996
>>> 
>>> Thanks
>>> - Ioi
>>> 
>>>> Thanks,
>>>> David
>>>> 
>>>>> Thanks
>>>>> - Ioi
>>> 
>> 
>