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

[Ioi Lam]

On 10/19/17 9:47 AM, Jiangli Zhou wrote:
>
>> On Oct 18, 2017, at 9:50 PM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto: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
>>>>>>
>>>>>> 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.
>

Hi Jiangli,

You're right. I misunderstood the original comments about the treatment 
of the bootclasspath/a classes. Here's the updated code that should be 
easier to understand:

         bool skip = false;
         if (class_loader == NULL || 
SystemDictionary::is_platform_class_loader(class_loader)) {
           // For the boot and platform class loaders, skip classes that 
are not found in the
           // java runtime image, such as those found in the 
--patch-module entries.
           // These classes can't be loaded from the archive during runtime.
           if (!ClassLoader::is_modules_image(stream->source()) && 
strncmp(stream->source(), "jrt:", 4) != 0) {
             skip = true;
           }

           if (class_loader == NULL && 
ClassLoader::contains_append_entry(stream->source())) {
             // .. but don't skip the boot classes that are loaded from 
-Xbootclasspath/a
             // as theycan be loaded from the archive during runtime.
             skip = false;
           }
         }

Thanks
- Ioi


> 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
>>>>
>>>
>>
>