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

[Ioi Lam]

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.

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